diff --git a/src/net_processing.cpp b/src/net_processing.cpp index edace2a3b7..10aada2b15 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -515,7 +515,7 @@ public: /** Implement PeerManager */ void StartScheduledTasks(CScheduler& scheduler) override; void CheckForStaleTipAndEvictPeers() override; - std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override + std::optional FetchBlock(NodeId peer_id, const uint256& hash, const CBlockIndex* block_index) 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); @@ -2078,7 +2078,7 @@ 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(NodeId peer_id, const CBlockIndex& block_index) +std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const uint256& hash, const CBlockIndex* block_index) { if (m_chainman.m_blockman.LoadingBlocks()) return "Loading blocks ..."; @@ -2089,17 +2089,18 @@ 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"; + // Mark block as in-flight unless we don't have the header. + if (block_index != nullptr) { // Forget about all prior requests RemoveBlockRequest(hash, std::nullopt); // Mark block as in-flight - Assume(BlockRequested(peer_id, block_index)); + Assume(BlockRequested(peer_id, *block_index)); + } // Construct message to request the block std::vector invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, hash)}; @@ -2117,6 +2118,12 @@ std::optional PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl return std::nullopt; } +std::optional PeerManager::FetchBlock(NodeId peer_id, const CBlockIndex& block_index) +{ + const uint256& hash{block_index.GetBlockHash()}; + return FetchBlock(peer_id, hash, &block_index); +} + std::unique_ptr PeerManager::make(CConnman& connman, AddrMan& addrman, BanMan* banman, ChainstateManager& chainman, CTxMemPool& pool, node::Warnings& warnings, Options opts) diff --git a/src/net_processing.h b/src/net_processing.h index adb65edd78..a7cb983996 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -80,13 +80,14 @@ public: virtual ~PeerManager() = default; /** - * Attempt to manually fetch block from a given peer. We must already have the header. + * Attempt to manually fetch block from a given peer. * * @param[in] peer_id The peer id * @param[in] block_index The blockindex * @returns std::nullopt if a request was successfully made, otherwise an error message */ - virtual std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0; + std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index); + virtual std::optional FetchBlock(NodeId peer_id, const uint256& hash, const CBlockIndex* block_index) = 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 456abdad2f..0507db8503 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -453,7 +453,6 @@ static RPCHelpMan getblockfrompeer() return RPCHelpMan{ "getblockfrompeer", "Attempt to fetch block from a given peer.\n\n" - "We must have the header for this block, e.g. using submitheader.\n" "The block will not have any undo data which can limit the usage of the block data in a context where the undo data is needed.\n" "Subsequent calls for the same block may cause the response from the previous peer to be ignored.\n" "Peers generally ignore requests for a stale block that they never fully verified, or one that is more than a month old.\n" @@ -480,10 +479,6 @@ static RPCHelpMan getblockfrompeer() const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(block_hash);); - if (!index) { - throw JSONRPCError(RPC_MISC_ERROR, "Block header missing"); - } - #if 0 // Fetching blocks before the node has syncing past their height can prevent block files from // being pruned, so we avoid it if the node is in prune mode. @@ -492,12 +487,12 @@ static RPCHelpMan getblockfrompeer() } #endif - const bool block_has_data = WITH_LOCK(::cs_main, return index->nStatus & BLOCK_HAVE_DATA); + const bool block_has_data = index && WITH_LOCK(::cs_main, return index->nStatus & BLOCK_HAVE_DATA); if (block_has_data) { 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, block_hash, index)}) { throw JSONRPCError(RPC_MISC_ERROR, err.value()); } return UniValue::VOBJ; diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py index 635a68eff1..43fe094185 100755 --- a/test/functional/rpc_getblockfrompeer.py +++ b/test/functional/rpc_getblockfrompeer.py @@ -8,6 +8,7 @@ from test_framework.authproxy import JSONRPCException from test_framework.messages import ( CBlock, from_hex, + msg_block, msg_headers, NODE_WITNESS, ) @@ -70,8 +71,8 @@ class GetBlockFromPeerTest(BitcoinTestFramework): assert_raises_rpc_error(-3, "JSON value of type number is not of expected type string", self.nodes[0].getblockfrompeer, 1234, peer_0_peer_1_id) assert_raises_rpc_error(-3, "JSON value of type string is not of expected type number", self.nodes[0].getblockfrompeer, short_tip, "0") - self.log.info("We must already have the header") - assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0) + self.log.info("We can request blocks for which we do not have the header") + self.nodes[0].getblockfrompeer("11" * 32, 0) self.log.info("Non-existent peer generates error") for peer_id in [-1, peer_0_peer_1_id + 1]: @@ -103,7 +104,7 @@ class GetBlockFromPeerTest(BitcoinTestFramework): 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("Do fetch blocks even if the node has not synced past it yet") + self.log.info("Do fetch blocks even if the node has not seen the header 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. self.restart_node(1, ["-prune=550"]) @@ -113,6 +114,27 @@ class GetBlockFromPeerTest(BitcoinTestFramework): block_hex = self.nodes[0].getblock(blockhash=blockhash, verbosity=0) block = from_hex(CBlock(), block_hex) + # Connect a P2PInterface to the pruning node + p2p_i = P2PInterface() + node1_interface = self.nodes[1].add_p2p_connection(p2p_i) + + node1_peers = self.nodes[1].getpeerinfo() + assert_equal(len(node1_peers), 1) + node1_interface_id = node1_peers[0]["id"] + assert_equal(self.nodes[1].getblockfrompeer(blockhash, node1_interface_id), {}) + block.calc_sha256() + p2p_i.wait_for_getdata([block.sha256]) + p2p_i.send_and_ping(msg_block(block)) + assert_equal(block_hex, self.nodes[1].getblock(blockhash, 0)) + self.nodes[1].disconnectnode(nodeid=node1_interface_id) + + self.log.info("Do fetch blocks even if the node has not synced past it yet") + + # Generate a block on the disconnected node that the pruning node is not connected to + blockhash = self.generate(self.nodes[0], 1, sync_fun=self.no_op)[0] + block_hex = self.nodes[0].getblock(blockhash=blockhash, verbosity=0) + block = from_hex(CBlock(), block_hex) + # Connect a P2PInterface to the pruning node and have it submit only the header of the # block that the pruning node has not seen node1_interface = self.nodes[1].add_p2p_connection(P2PInterface())