Merge bitcoin#23706: rpc: getblockfrompeer followups

923312f rpc: use peer_id, block_hash for FetchBlock (Sjors Provoost)
34d5399 rpc: more detailed errors for getblockfrompeer (Sjors Provoost)
60243ca rpc: turn already downloaded into error in getblockfrompeer (Sjors Provoost)
809d66b rpc: clarify getblockfrompeer behavior when called multiple times (Sjors Provoost)
0e3d7c5 refactor: drop redundant hash argument from FetchBlock (Sjors Provoost)
8d1a3e6 rpc: allow empty JSON object result (Sjors Provoost)
bfbf91d test: fancier Python for getblockfrompeer (Sjors Provoost)

Pull request description:

  Followups from bitcoin#20295.

ACKs for top commit:
  jonatack:
    ACK 923312f 📦
  fjahr:
    tested ACK 923312f

Tree-SHA512: da9eca76e302e249409c9d7f0d16cca668ed981e2ab6ca2d1743dad0d830b94b1bc5ffb9028a00764b863201945c273cc8f4409a4c9ca3817830007dffa2bc20

Author: MarcoFalke

src/net_processing.cpp

@@ -320,7 +320,7 @@ class PeerManagerImpl final : public PeerManager
     /** Implement PeerManager */
     void StartScheduledTasks(CScheduler& scheduler) override;
     void CheckForStaleTipAndEvictPeers() override;
-    bool FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& index) override;
+    std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override;
     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override;
     bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
     void SendPings() override;
@@ -1460,39 +1460,39 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
            (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
 }
 
-bool PeerManagerImpl::FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& index)
+std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBlockIndex& block_index)
 {
-    if (fImporting || fReindex) return false;
+    if (fImporting) return "Importing...";
+    if (fReindex) return "Reindexing...";
 
     LOCK(cs_main);
     // Ensure this peer exists and hasn't been disconnected
-    CNodeState* state = State(id);
-    if (state == nullptr) return false;
+    CNodeState* state = State(peer_id);
+    if (state == nullptr) return "Peer does not exist";
     // Ignore pre-segwit peers
-    if (!state->fHaveWitness) return false;
+    if (!state->fHaveWitness) return "Pre-SegWit peer";
 
-    // Mark block as in-flight unless it already is
-    if (!BlockRequested(id, index)) return false;
+    // Mark block as in-flight unless it already is (for this peer).
+    // If a block was already in-flight for a different peer, its BLOCKTXN
+    // response will be dropped.
+    if (!BlockRequested(peer_id, block_index)) return "Already requested from this peer";
 
     // Construct message to request the block
+    const uint256& hash{block_index.GetBlockHash()};
     std::vector<CInv> invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, hash)};
 
     // Send block request message to the peer
-    bool success = m_connman.ForNode(id, [this, &invs](CNode* node) {
+    bool success = m_connman.ForNode(peer_id, [this, &invs](CNode* node) {
         const CNetMsgMaker msgMaker(node->GetCommonVersion());
         this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs));
         return true;
     });
 
-    if (success) {
-        LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
-                 hash.ToString(), id);
-    } else {
-        RemoveBlockRequest(hash);
-        LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
-                 hash.ToString(), id);
-    }
-    return success;
+    if (!success) return "Peer not fully connected";
+
+    LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
+                 hash.ToString(), peer_id);
+    return std::nullopt;
 }
 
 std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,

src/net_processing.h

@@ -45,12 +45,11 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
     /**
      * Attempt to manually fetch block from a given peer. We must already have the header.
      *
-     * @param[in]  id       The peer id
-     * @param[in]  hash     The block hash
-     * @param[in]  pindex   The blockindex
-     * @returns             Whether a request was successfully made
+     * @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 bool FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& pindex) = 0;
+    virtual std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;
 
     /** Begin running background tasks, should only be called once */
     virtual void StartScheduledTasks(CScheduler& scheduler) = 0;

src/rpc/blockchain.cpp

@@ -792,15 +792,13 @@ static RPCHelpMan getblockfrompeer()
         "getblockfrompeer",
         "\nAttempt to fetch block from a given peer.\n"
         "\nWe must have the header for this block, e.g. using submitheader.\n"
-        "\nReturns {} if a block-request was successfully scheduled\n",
+        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
+        "\nReturns an empty JSON object if the request was successfully scheduled.",
         {
-            {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
-            {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"},
+            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
+            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"},
         },
-        RPCResult{RPCResult::Type::OBJ, "", "",
-        {
-            {RPCResult::Type::STR, "warnings", /*optional=*/true, "any warnings"},
-        }},
+        RPCResult{RPCResult::Type::OBJ_EMPTY, "", /*optional=*/ false, "", {}},
         RPCExamples{
             HelpExampleCli("getblockfrompeer", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\" 0")
             + HelpExampleRpc("getblockfrompeer", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\" 0")
@@ -810,31 +808,24 @@ static RPCHelpMan getblockfrompeer()
     const NodeContext& node = EnsureAnyNodeContext(request.context);
     ChainstateManager& chainman = EnsureChainman(node);
     PeerManager& peerman = EnsurePeerman(node);
-    CConnman& connman = EnsureConnman(node);
-
-    uint256 hash(ParseHashV(request.params[0], "hash"));
-
-    const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
 
-    // Check that the peer with nodeid exists
-    if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
-        throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
-    }
+    const uint256& block_hash{ParseHashV(request.params[0], "block_hash")};
+    const NodeId peer_id{request.params[1].get_int64()};
 
-    const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
+    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");
     }
 
-    UniValue result = UniValue::VOBJ;
-
     if (index->nStatus & BLOCK_HAVE_DATA) {
-        result.pushKV("warnings", "Block already downloaded");
-    } else if (!peerman.FetchBlock(nodeid, hash, *index)) {
-        throw JSONRPCError(RPC_MISC_ERROR, "Failed to fetch block from peer");
+        throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
     }
-    return result;
+
+    if (const auto err{peerman.FetchBlock(peer_id, *index)}) {
+        throw JSONRPCError(RPC_MISC_ERROR, err.value());
+    }
+    return UniValue::VOBJ;
 },
     };
 }

src/rpc/client.cpp

@@ -60,7 +60,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "getbalance", 1, "minconf" },
     { "getbalance", 2, "include_watchonly" },
     { "getbalance", 3, "avoid_reuse" },
-    { "getblockfrompeer", 1, "nodeid" },
+    { "getblockfrompeer", 1, "peer_id" },
     { "getblockhash", 0, "height" },
     { "waitforblockheight", 0, "height" },
     { "waitforblockheight", 1, "timeout" },

src/rpc/util.cpp

@@ -830,6 +830,10 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
         return;
     }
     case Type::OBJ_DYN:
+    case Type::OBJ_EMPTY: {
+        sections.PushSection({indent + maybe_key + "{}", Description("empty JSON object")});
+        return;
+    }
     case Type::OBJ: {
         sections.PushSection({indent + maybe_key + "{", Description("json object")});
         for (const auto& i : m_inner) {
@@ -879,6 +883,7 @@ bool RPCResult::MatchesType(const UniValue& result) const
         return UniValue::VARR == result.getType();
     }
     case Type::OBJ_DYN:
+    case Type::OBJ_EMPTY:
     case Type::OBJ: {
         return UniValue::VOBJ == result.getType();
     }

src/rpc/util.h

@@ -240,6 +240,7 @@ struct RPCResult {
         STR_AMOUNT, //!< Special string to represent a floating point amount
         STR_HEX,    //!< Special string with only hex chars
         OBJ_DYN,    //!< Special dictionary with keys that are not literals
+        OBJ_EMPTY,  //!< Special type to allow empty OBJ
         ARR_FIXED,  //!< Special array that has a fixed number of entries
         NUM_TIME,   //!< Special numeric to denote unix epoch time
         ELISION,    //!< Special type to denote elision (...)

test/functional/rpc_getblockfrompeer.py

@@ -40,12 +40,8 @@ def run_test(self):
         self.sync_blocks()
 
         self.log.info("Node 0 should only have the header for node 1's block 3")
-        for x in self.nodes[0].getchaintips():
-            if x['hash'] == short_tip:
-                assert_equal(x['status'], "headers-only")
-                break
-        else:
-            raise AssertionError("short tip not synced")
+        x = next(filter(lambda x: x['hash'] == short_tip, self.nodes[0].getchaintips()))
+        assert_equal(x['status'], "headers-only")
         assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, short_tip)
 
         self.log.info("Fetch block from node 1")
@@ -60,17 +56,15 @@ def run_test(self):
         assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0)
 
         self.log.info("Non-existent peer generates error")
-        assert_raises_rpc_error(-1, f"Peer nodeid {peer_0_peer_1_id + 1} does not exist", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
+        assert_raises_rpc_error(-1, "Peer does not exist", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
 
         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(short_tip), timeout=1)
-        assert(not "warnings" in result)
+        assert_equal(result, {})
 
         self.log.info("Don't fetch blocks we already have")
-        result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id)
-        assert("warnings" in result)
-        assert_equal(result["warnings"], "Block already downloaded")
+        assert_raises_rpc_error(-1, "Block already downloaded", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id)
 
 if __name__ == '__main__':
     GetBlockFromPeerTest().main()