rpc: more detailed errors for getblockfrompeer

maflcko · Dec 24, 2021 · 34d5399 · 34d5399
1 parent 60243ca
commit 34d5399
Show file tree

Hide file tree

Showing 4 changed files with 17 additions and 26 deletions.
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -312,7 +312,7 @@ class PeerManagerImpl final : public PeerManager
     /** Implement PeerManager */
     void StartScheduledTasks(CScheduler& scheduler) override;
     void CheckForStaleTipAndEvictPeers() override;
-    bool FetchBlock(NodeId id, const CBlockIndex& block_index) override;
+    std::optional<std::string> FetchBlock(NodeId 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;
@@ -1428,21 +1428,22 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
            (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
 }
 
-bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex& block_index)
+std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId 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;
+    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 (for this peer).
     // If a block was already in-flight for a different peer, its BLOCKTXN
     // response will be dropped.
-    if (!BlockRequested(id, block_index)) return false;
+    if (!BlockRequested(id, block_index)) return "Already requested from this peer";
 
     // Construct message to request the block
     const uint256& hash{block_index.GetBlockHash()};
@@ -1455,15 +1456,11 @@ bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex& block_index)
         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",
+    if (!success) return "Node not fully connected";
+
+    LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
                  hash.ToString(), id);
-    }
-    return success;
+    return std::nullopt;
 }
 
 std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman,

diff --git a/src/net_processing.h b/src/net_processing.h
@@ -47,9 +47,9 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
      *
      * @param[in]  id           The peer id
      * @param[in]  block_index  The blockindex
-     * @returns                 Whether a request was successfully made
+     * @returns std::nullopt if a request was successfully made, otherwise an error message
      */
-    virtual bool FetchBlock(NodeId id, const CBlockIndex& block_index) = 0;
+    virtual std::optional<std::string> FetchBlock(NodeId id, 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
@@ -797,16 +797,10 @@ static RPCHelpMan getblockfrompeer()
     const NodeContext& node = EnsureAnyNodeContext(request.context);
     ChainstateManager& chainman = EnsureChainman(node);
     PeerManager& peerman = EnsurePeerman(node);
-    CConnman& connman = EnsureConnman(node);
 
-    const uint256 hash(ParseHashV(request.params[0], "hash"));
+    const uint256& hash{ParseHashV(request.params[0], "hash")};
     const NodeId 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 CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
 
     if (!index) {
@@ -817,8 +811,8 @@ static RPCHelpMan getblockfrompeer()
         throw JSONRPCError(RPC_MISC_ERROR, "Block already downloaded");
     }
 
-    if (!peerman.FetchBlock(nodeid, *index)) {
-        throw JSONRPCError(RPC_MISC_ERROR, "Failed to fetch block from peer");
+    if (const auto err{peerman.FetchBlock(nodeid, *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
@@ -56,7 +56,7 @@ 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)