Merge rpc_getblockfrompeer_wo_header

src/net_processing.cpp

@@ -515,7 +515,7 @@ public:
     /** Implement PeerManager */
     void StartScheduledTasks(CScheduler& scheduler) override;
     void CheckForStaleTipAndEvictPeers() override;
-    std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
+    std::optional<std::string> 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<TxOrphanage::OrphanTxBase> 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<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBlockIndex& block_index)
+std::optional<std::string> 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<std::string> 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<CInv> invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, hash)};
@@ -2117,6 +2118,12 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
     return std::nullopt;
 }
 
+std::optional<std::string> 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> PeerManager::make(CConnman& connman, AddrMan& addrman,
                                                BanMan* banman, ChainstateManager& chainman,
                                                CTxMemPool& pool, node::Warnings& warnings, Options opts)

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<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;
+    std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index);
+    virtual std::optional<std::string> 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;

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;

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")
+        self.log.info("We can request blocks for which we do not have the header")
-        assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0)
+        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())