From 2b67ea465c72461eca7d07897b6476f583bf9a19 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sun, 9 Jul 2023 01:05:30 +0000 Subject: [PATCH 1/3] Bugfix: net_processing: Restore "Already requested" error for FetchBlock --- src/net_processing.cpp | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8da2c701d3..7e5b64d464 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -878,6 +878,9 @@ private: /** Have we requested this block from an outbound peer */ bool IsBlockRequestedFromOutbound(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** Have we requested this block from a specific peer */ + bool IsBlockRequestedFromPeer(const uint256& hash, NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + /** Remove this block from our tracked requested blocks. Called if: * - the block has been received from a peer * - the request for the block has timed out @@ -1132,6 +1135,16 @@ bool PeerManagerImpl::IsBlockRequestedFromOutbound(const uint256& hash) return false; } +bool PeerManagerImpl::IsBlockRequestedFromPeer(const uint256& hash, NodeId peer) +{ + for (auto range = mapBlocksInFlight.equal_range(hash); range.first != range.second; range.first++) { + auto [nodeid, block_it] = range.first->second; + if (nodeid == peer) return true; + } + + return false; +} + void PeerManagerImpl::RemoveBlockRequest(const uint256& hash, std::optional from_peer) { auto range = mapBlocksInFlight.equal_range(hash); @@ -1781,16 +1794,19 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl // Ignore pre-segwit peers if (!CanServeWitnesses(*peer)) return "Pre-SegWit peer"; + const uint256& hash{block_index.GetBlockHash()}; + LOCK(cs_main); + if (IsBlockRequestedFromPeer(hash, peer_id)) return "Already requested from this peer"; + // Forget about all prior requests - RemoveBlockRequest(block_index.GetBlockHash(), std::nullopt); + RemoveBlockRequest(hash, std::nullopt); // Mark block as in-flight - if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer"; + Assume(BlockRequested(peer_id, block_index)); // Construct message to request the block - const uint256& hash{block_index.GetBlockHash()}; std::vector invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, hash)}; // Send block request message to the peer From d7e08d408c34b9ffbd5475d3c351fdec3087cc5a Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Mon, 22 Nov 2021 02:21:49 +0000 Subject: [PATCH 2/3] QA: rpc_getblockfrompeer: Test that a non-existent peer generates an error, even if we already have the block --- test/functional/rpc_getblockfrompeer.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py index 1ab1023cf1..8feb9ab007 100755 --- a/test/functional/rpc_getblockfrompeer.py +++ b/test/functional/rpc_getblockfrompeer.py @@ -92,6 +92,9 @@ class GetBlockFromPeerTest(BitcoinTestFramework): self.log.info("Don't fetch blocks we already have") assert_raises_rpc_error(-1, "Block already downloaded", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id) + self.log.info("Non-existent peer generates error, even if we already have the block") + assert_raises_rpc_error(-1, "Block already downloaded", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1) + self.log.info("Don't fetch blocks while the node has not synced past it yet") # For this test we need node 1 in prune mode and as a side effect this also disconnects # the nodes which is also necessary for the rest of the test. From 017ab85cecc7252bc2dc8e6bc6c7456ecef83389 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sun, 9 Jul 2023 00:41:44 +0000 Subject: [PATCH 3/3] QA: rpc_getblockfrompeer: Test specific error for trying to fetch from peer twice --- test/functional/rpc_getblockfrompeer.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py index 8feb9ab007..884a672178 100755 --- a/test/functional/rpc_getblockfrompeer.py +++ b/test/functional/rpc_getblockfrompeer.py @@ -84,6 +84,14 @@ class GetBlockFromPeerTest(BitcoinTestFramework): presegwit_peer_id = peers[1]["id"] assert_raises_rpc_error(-1, "Pre-SegWit peer", self.nodes[0].getblockfrompeer, short_tip, presegwit_peer_id) + self.log.info("Fetching from same peer twice generates error") + self.nodes[0].add_p2p_connection(P2PInterface()) + peers = self.nodes[0].getpeerinfo() + assert_equal(len(peers), 3) + slow_peer_id = peers[2]["id"] + assert_equal(self.nodes[0].getblockfrompeer(short_tip, slow_peer_id), {}) + assert_raises_rpc_error(-1, "Already requested from this peer", self.nodes[0].getblockfrompeer, short_tip, slow_peer_id) + self.log.info("Successful fetch") result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id) self.wait_until(lambda: self.check_for_block(node=0, hash=short_tip), timeout=1)