Merge bitcoin/bitcoin#23706: rpc: getblockfrompeer followups

923312fbf6a89efde1739da0b7209694d4f892ba rpc: use peer_id, block_hash for FetchBlock (Sjors Provoost)
34d5399211eeb61e7e7961c301fb2ddea8aa3f6a rpc: more detailed errors for getblockfrompeer (Sjors Provoost)
60243cac7286e4c4bdda7094bef4cf6d1564b583 rpc: turn already downloaded into error in getblockfrompeer (Sjors Provoost)
809d66bb65aa78048e27c2a878d6f7becaecfe11 rpc: clarify getblockfrompeer behavior when called multiple times (Sjors Provoost)
0e3d7c5ee16d5a4c061ab9a57285bceb7899b512 refactor: drop redundant hash argument from FetchBlock (Sjors Provoost)
8d1a3e6498de6087501969a9d243b0697ca3fe97 rpc: allow empty JSON object result (Sjors Provoost)
bfbf91d0b2004dde358253ac174982f784b43b59 test: fancier Python for getblockfrompeer (Sjors Provoost)

Pull request description:

  Followups from #20295.

ACKs for top commit:
  jonatack:
    ACK 923312fbf6a89efde1739da0b7209694d4f892ba :package:
  fjahr:
    tested ACK 923312fbf6a89efde1739da0b7209694d4f892ba

Tree-SHA512: da9eca76e302e249409c9d7f0d16cca668ed981e2ab6ca2d1743dad0d830b94b1bc5ffb9028a00764b863201945c273cc8f4409a4c9ca3817830007dffa2bc20

1 files changed, 18 insertions, 18 deletions

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index cc23876af5..bd51b0f785 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -320,7 +320,7 @@ public:
     /** 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,