From b18c72cfcd6f27070928bc0ba16db6897960f211 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 1 Aug 2023 18:27:14 -0300 Subject: [PATCH] rpc: getblockfrompeer, add one-shot block requests capability Allowing what we had before, a single block request with no automatic retry nor tracking mechanism. --- src/net_processing.cpp | 9 ++++--- src/net_processing.h | 2 +- src/rpc/blockchain.cpp | 6 ++++- src/rpc/client.cpp | 1 + test/functional/rpc_getblockfrompeer.py | 34 ++++++++++++++++++++++++- 5 files changed, 45 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a4e06f73a5f..c70b648fa6b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -513,7 +513,7 @@ public: /** Implement PeerManager */ void StartScheduledTasks(CScheduler& scheduler) override; void CheckForStaleTipAndEvictPeers() override; - std::optional FetchBlock(std::optional op_peer_id, const CBlockIndex& block_index) override + std::optional FetchBlock(std::optional op_peer_id, const CBlockIndex& block_index, bool retry) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); @@ -2072,13 +2072,14 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) (GetBlockProofEquivalentTime(*m_chainman.m_best_header, *pindex, *m_chainman.m_best_header, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT); } -std::optional PeerManagerImpl::FetchBlock(std::optional op_peer_id, const CBlockIndex& block_index) +std::optional PeerManagerImpl::FetchBlock(std::optional op_peer_id, const CBlockIndex& block_index, bool retry) { if (m_chainman.m_blockman.LoadingBlocks()) return "Loading blocks ..."; // If no peer id was specified, track block. The internal block sync process will // be in charge of downloading the block. if (!op_peer_id) { + if (!retry) return "'retry' disabled, cannot perform single block request"; // future: enable one-try requests. if (!m_block_tracker.track(block_index.GetBlockHash())) return "Already tracked block"; LogDebug(BCLog::NET, "Block added to the tracking list, hash %s\n", block_index.GetBlockHash().ToString()); return std::nullopt; @@ -2101,8 +2102,8 @@ std::optional PeerManagerImpl::FetchBlock(std::optional op_ // Mark block as in-flight if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer"; - // Track block request - m_block_tracker.track(block_index.GetBlockHash(), peer_id); + // Track block request (only if requested) + if (retry) m_block_tracker.track(block_index.GetBlockHash(), peer_id); // Construct message to request the block const uint256& hash{block_index.GetBlockHash()}; diff --git a/src/net_processing.h b/src/net_processing.h index 2591d0bb271..f1dc8c8b804 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -92,7 +92,7 @@ public: * @param[in] block_index The blockindex * @returns std::nullopt if a request was successfully made, otherwise an error message */ - virtual std::optional FetchBlock(std::optional op_peer_id, const CBlockIndex& block_index) = 0; + virtual std::optional FetchBlock(std::optional op_peer_id, const CBlockIndex& block_index, bool retry) = 0; /** Begin running background tasks, should only be called once */ virtual void StartScheduledTasks(CScheduler& scheduler) = 0; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index e7c200f7f22..ed6ddc108c5 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -461,6 +461,8 @@ static RPCHelpMan getblockfrompeer() {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"}, {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "The peer to fetch it from (see getpeerinfo for peer IDs). " "If omitted, the node will fetch the block from any available peer."}, + {"retry", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Whether to automatically retry to download the block from different peers if the initial request fails or not. " + "If omitted, the node will continuously attempt to download the block from various peers until it succeeds."}, }, RPCResult{RPCResult::Type::OBJ, "", /*optional=*/false, "", {}}, RPCExamples{ @@ -475,6 +477,8 @@ static RPCHelpMan getblockfrompeer() const uint256& block_hash{ParseHashV(request.params[0], "blockhash")}; const std::optional peer_id = request.params[1].isNull() ? std::nullopt : std::make_optional(request.params[1].getInt()); + const bool retry = !request.params[2].isNull() ? request.params[2].get_bool() : true; // default true + if (!retry && !peer_id) throw JSONRPCError(RPC_INVALID_PARAMETER, "Cannot disable the 'retry' process without specifying a peer from which the block will be downloaded ('peer_id')"); const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(block_hash);); @@ -493,7 +497,7 @@ static RPCHelpMan getblockfrompeer() throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded"); } - if (const auto err{peerman.FetchBlock(peer_id, *index)}) { + if (const auto err{peerman.FetchBlock(peer_id, *index, retry)}) { throw JSONRPCError(RPC_MISC_ERROR, err.value()); } return UniValue::VOBJ; diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 601e4fa7bf5..097cf13cdcb 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -65,6 +65,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "getbalance", 2, "include_watchonly" }, { "getbalance", 3, "avoid_reuse" }, { "getblockfrompeer", 1, "peer_id" }, + { "getblockfrompeer", 2, "retry" }, { "getblockhash", 0, "height" }, { "waitforblockheight", 0, "height" }, { "waitforblockheight", 1, "timeout" }, diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py index 4568d545d84..4b5043a3c6b 100755 --- a/test/functional/rpc_getblockfrompeer.py +++ b/test/functional/rpc_getblockfrompeer.py @@ -195,9 +195,10 @@ class GetBlockFromPeerTest(BitcoinTestFramework): self.connect_nodes(1, 2) # Move clock above the block request timeout and assert the initial block fetching failed + current_time = current_time + 610 with pruned_node.assert_debug_log([f"Timeout downloading block {pruned_block_15} from peer={not_responding_peer_id}"], timeout=5): for node in self.nodes: - node.setmocktime(current_time + 610) + node.setmocktime(current_time) # Now verify that the block was requested and received from another peer after the initial failure self.wait_until(lambda: self.check_for_block(node=2, hash=pruned_block_15), timeout=3) @@ -219,6 +220,37 @@ class GetBlockFromPeerTest(BitcoinTestFramework): self.connect_nodes(0, 2) self.wait_until(lambda: self.check_for_block(node=2, hash=pruned_block_10), timeout=5) + ############################################################################## + # Try to fetch block from certain peer only once, no automatic retry process # + ############################################################################## + + self.log.info("Try to fetch block from certain peer only once") + + # Advance peers time so every peer stay responsive + current_time = current_time + 60 + for node in self.nodes: + node.setmocktime(current_time) + + # Connect peer that can serve blocks but will not answer to the getdata requests + pruned_node.add_p2p_connection(P2PInterface()) + not_responding_peer_id = pruned_node.getpeerinfo()[-1]["id"] # last connection + # Also connect full node that can serve the block + self.connect_nodes(1, 2) + + # Request block with 'retry=false' + pruned_block_9 = self.nodes[0].getblockhash(9) + result = pruned_node.getblockfrompeer(pruned_block_9, not_responding_peer_id, retry=False) + assert_equal(result, {}) + + # Move clock above the block request timeout and assert the initial block fetching failed + with pruned_node.assert_debug_log([f"Timeout downloading block {pruned_block_9} from peer={not_responding_peer_id}"]): + for node in self.nodes: + node.setmocktime(current_time + 610) + + # Sleep for a bit and verify that the block was not requested to any other peer + time.sleep(3) + self.wait_until(lambda: not self.check_for_block(node=2, hash=pruned_block_9), timeout=3) + if __name__ == '__main__': GetBlockFromPeerTest(__file__).main()