From 2032b166208fcdbccb4433c63f67b3552fd4da18 Mon Sep 17 00:00:00 2001 From: Chris Stewart Date: Fri, 21 Jul 2023 17:08:01 -0500 Subject: [PATCH] Fix bug where we just need to `awaitSyncAndIBD()` rather than attempt to `sync()` (#5158) * Fix bug where we just ned to awaitAllSync() rather than attempt to sync() * Use awaitSyncAndIBD, mv awaitSyncAndIBD to NodeTestUtil * Replace more NodeUnitTest.syncNeutrinoNode() with NodeTestUtil.awaitSyncAndIBD * Replace more NodeUnitTest.syncNeutrinoNode() with NodeTestUtil.awaitSyncAndIBD --- .../org/bitcoins/node/NeutrinoNodeTest.scala | 16 ++++------- .../scala/org/bitcoins/node/PeerManager.scala | 2 +- .../bitcoins/testkit/node/NodeTestUtil.scala | 26 +++++++++++++++++ .../bitcoins/testkit/node/NodeUnitTest.scala | 28 +------------------ 4 files changed, 34 insertions(+), 38 deletions(-) diff --git a/node-test/src/test/scala/org/bitcoins/node/NeutrinoNodeTest.scala b/node-test/src/test/scala/org/bitcoins/node/NeutrinoNodeTest.scala index 254a1eab44..356b72bf49 100644 --- a/node-test/src/test/scala/org/bitcoins/node/NeutrinoNodeTest.scala +++ b/node-test/src/test/scala/org/bitcoins/node/NeutrinoNodeTest.scala @@ -10,11 +10,7 @@ import org.bitcoins.node.models.{PeerDAO, PeerDb} import org.bitcoins.server.BitcoinSAppConfig import org.bitcoins.testkit.BitcoinSTestAppConfig import org.bitcoins.testkit.node.fixture.NeutrinoNodeConnectedWithBitcoinds -import org.bitcoins.testkit.node.{ - NodeTestUtil, - NodeTestWithCachedBitcoindPair, - NodeUnitTest -} +import org.bitcoins.testkit.node.{NodeTestUtil, NodeTestWithCachedBitcoindPair} import org.bitcoins.testkit.util.{AkkaUtil, TorUtil} import org.scalatest.{Assertion, FutureOutcome, Outcome} @@ -69,7 +65,7 @@ class NeutrinoNodeTest extends NodeTestWithCachedBitcoindPair { for { _ <- connAndInit - _ <- NodeUnitTest.syncNeutrinoNode(node, bitcoinds.head) + _ <- NodeTestUtil.awaitSyncAndIBD(node, bitcoinds.head) } yield { succeed } @@ -187,10 +183,10 @@ class NeutrinoNodeTest extends NodeTestWithCachedBitcoindPair { .retryUntilSatisfied(peers.size == 2, interval = 1.second, maxTries = 30) - _ <- NodeUnitTest.syncNeutrinoNode(node, bitcoind) _ <- Future .sequence(peers.map(peerManager.isConnected)) .flatMap(p => assert(p.forall(_ == true))) + _ <- NodeTestUtil.awaitSyncAndIBD(node, bitcoind) res <- Future .sequence(peers.map(peerManager.isConnected)) .flatMap(p => assert(p.forall(_ == true))) @@ -221,7 +217,7 @@ class NeutrinoNodeTest extends NodeTestWithCachedBitcoindPair { //itself out of IBD. bitcoind will not sendheaders //when it believes itself, or it's peer is in IBD val gen1F = for { - _ <- NodeUnitTest.syncNeutrinoNode(node, bitcoind) + _ <- NodeTestUtil.awaitSyncAndIBD(node, bitcoind) x <- bitcoind.generate(1) } yield x @@ -269,7 +265,7 @@ class NeutrinoNodeTest extends NodeTestWithCachedBitcoindPair { val bitcoind = nodeConnectedWithBitcoind.bitcoinds(0) for { - _ <- NodeUnitTest.syncNeutrinoNode(node, bitcoind) + _ <- NodeTestUtil.awaitSyncAndIBD(node, bitcoind) _ <- AkkaUtil.nonBlockingSleep(3.seconds) //have to generate the block headers independent of one another //rather than just calling generateToAddress(2,junkAddress) @@ -291,7 +287,7 @@ class NeutrinoNodeTest extends NodeTestWithCachedBitcoindPair { val bitcoind = nodeConnectedWithBitcoind.bitcoinds(0) for { - _ <- NodeUnitTest.syncNeutrinoNode(node, bitcoind) + _ <- NodeTestUtil.awaitSyncAndIBD(node, bitcoind) _ <- node.stop() //drop all compact filter headers / filters _ <- CompactFilterHeaderDAO()(executionContext, node.chainConfig) diff --git a/node/src/main/scala/org/bitcoins/node/PeerManager.scala b/node/src/main/scala/org/bitcoins/node/PeerManager.scala index 96742ebeb4..97f5106238 100644 --- a/node/src/main/scala/org/bitcoins/node/PeerManager.scala +++ b/node/src/main/scala/org/bitcoins/node/PeerManager.scala @@ -958,8 +958,8 @@ case class PeerManager( val chainApi: ChainApi = ChainHandler.fromDatabase() val headerF = chainApi.getBestBlockHeader() for { - _ <- getHeaderSyncHelper(syncPeerOpt) _ <- chainApi.setSyncing(true) + _ <- getHeaderSyncHelper(syncPeerOpt) cancellable = createFilterSyncJob(chainApi, syncPeerOpt) _ = { syncFilterCancellableOpt = Some(cancellable) diff --git a/testkit/src/main/scala/org/bitcoins/testkit/node/NodeTestUtil.scala b/testkit/src/main/scala/org/bitcoins/testkit/node/NodeTestUtil.scala index 97ec7fa554..953bc6f70f 100644 --- a/testkit/src/main/scala/org/bitcoins/testkit/node/NodeTestUtil.scala +++ b/testkit/src/main/scala/org/bitcoins/testkit/node/NodeTestUtil.scala @@ -1,6 +1,7 @@ package org.bitcoins.testkit.node import akka.actor.ActorSystem +import org.bitcoins.asyncutil.AsyncUtil import org.bitcoins.core.api.node.Peer import org.bitcoins.core.api.tor.Socks5ProxyParams import org.bitcoins.crypto.DoubleSha256DigestBE @@ -178,6 +179,31 @@ abstract class NodeTestUtil extends P2PLogger { } yield () } + def awaitSyncAndIBD(node: NeutrinoNode, bitcoind: BitcoindRpcClient)(implicit + system: ActorSystem): Future[Unit] = { + import system.dispatcher + + for { + _ <- NodeTestUtil.awaitSync(node, bitcoind) + _ <- AsyncUtil.retryUntilSatisfiedF( + () => { + val chainApi = node.chainApiFromDb() + val syncingF = chainApi.flatMap(_.isSyncing()) + val isIBDF = chainApi.flatMap(_.isIBD()) + for { + syncing <- syncingF + isIBD <- isIBDF + } yield { + !syncing && !isIBD + } + }, + interval = 1.second, + maxTries = 5 + ) + } yield () + + } + /** get our neutrino node's uri from a test bitcoind instance to send rpc commands for our node. * The peer must be initialized by the node. */ diff --git a/testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala b/testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala index 152c917f35..61b9e5f42d 100644 --- a/testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala +++ b/testkit/src/main/scala/org/bitcoins/testkit/node/NodeUnitTest.scala @@ -21,7 +21,6 @@ import org.scalatest.FutureOutcome import java.net.InetSocketAddress import java.time.Instant -import scala.concurrent.duration.DurationInt import scala.concurrent.{ExecutionContext, Future} trait NodeUnitTest extends BaseNodeTest { @@ -470,7 +469,7 @@ object NodeUnitTest extends P2PLogger { //do nothing as we are already syncing logger.info( s"Node is already syncing, skipping initiating a new sync.") - awaitSyncAndIBD(node, bitcoind).map(_ => node) + NodeTestUtil.awaitSyncAndIBD(node, bitcoind).map(_ => node) } else { neutrinoNodeSyncHelper(node, bitcoind) } @@ -494,31 +493,6 @@ object NodeUnitTest extends P2PLogger { } yield node } - private def awaitSyncAndIBD(node: NeutrinoNode, bitcoind: BitcoindRpcClient)( - implicit system: ActorSystem): Future[Unit] = { - import system.dispatcher - - for { - _ <- NodeTestUtil.awaitSync(node, bitcoind) - _ <- AsyncUtil.retryUntilSatisfiedF( - () => { - val chainApi = node.chainApiFromDb() - val syncingF = chainApi.flatMap(_.isSyncing()) - val isIBDF = chainApi.flatMap(_.isIBD()) - for { - syncing <- syncingF - isIBD <- isIBDF - } yield { - !syncing && !isIBD - } - }, - interval = 1.second, - maxTries = 5 - ) - } yield () - - } - /** This is needed for postgres, we do not drop tables in between individual tests with postgres * rather an entire test suite shares the same postgres database. * therefore, we need to clean the database after each test, so that migrations can be applied during