From b23d94b14f3f92da644f179ca08f035ade751d5f Mon Sep 17 00:00:00 2001 From: Kittywhiskers Van Gogh <63189531+kwvg@users.noreply.github.com> Date: Sun, 23 Jun 2024 20:46:29 +0000 Subject: [PATCH] partial bitcoin#23706: getblockfrompeer followups excludes: - 8d1a3e6498de6087501969a9d243b0697ca3fe97 - 809d66bb65aa78048e27c2a878d6f7becaecfe11 - 60243cac7286e4c4bdda7094bef4cf6d1564b583 --- src/net_processing.cpp | 34 ++++++++++++------------- src/net_processing.h | 9 +++---- src/rpc/blockchain.cpp | 23 ++++++----------- src/rpc/client.cpp | 2 +- test/functional/rpc_getblockfrompeer.py | 10 +++----- 5 files changed, 33 insertions(+), 45 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d36d0c472c..0694c92fe2 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -398,7 +398,7 @@ public: /** Implement PeerManager */ void CheckForStaleTipAndEvictPeers() override; - bool FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& index) override; + std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override; bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);; @@ -1863,37 +1863,37 @@ 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 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"; - // Mark block as in-flight unless it already is - if (!MarkBlockAsInFlight(id, index.GetBlockHash(), &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. + const uint256& hash{block_index.GetBlockHash()}; + if (!MarkBlockAsInFlight(peer_id, hash, &block_index)) return "Already requested from this peer"; // Construct message to request the block std::vector invs{CInv(MSG_BLOCK, 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 { - MarkBlockAsReceived(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::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman, diff --git a/src/net_processing.h b/src/net_processing.h index 20e11f98cf..4edf1f371e 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -69,12 +69,11 @@ public: /** * 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 FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0; /** Get statistics from node state */ virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0; diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 75846c7a74..3b2874b910 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -775,8 +775,8 @@ static RPCHelpMan getblockfrompeer() "\nWe must have the header for this block, e.g. using submitheader.\n" "\nReturns {} if a block-request was successfully scheduled\n", { - {"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, "", "", { @@ -791,18 +791,11 @@ 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 uint256& block_hash{ParseHashV(request.params[0], "block_hash")}; + const NodeId peer_id{request.params[1].get_int64()}; - const NodeId nodeid = static_cast(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);); + 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"); @@ -812,8 +805,8 @@ static RPCHelpMan getblockfrompeer() 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"); + } else if (const auto err{peerman.FetchBlock(peer_id, *index)}) { + throw JSONRPCError(RPC_MISC_ERROR, err.value()); } return result; }, @@ -3053,7 +3046,7 @@ static const CRPCCommand commands[] = { "blockchain", "getbestchainlock", &getbestchainlock, {} }, { "blockchain", "getblockcount", &getblockcount, {} }, { "blockchain", "getblock", &getblock, {"blockhash","verbosity|verbose"} }, - { "blockchain", "getblockfrompeer", &getblockfrompeer, {"blockhash", "nodeid"}}, + { "blockchain", "getblockfrompeer", &getblockfrompeer, {"block_hash", "peer_id"}}, { "blockchain", "getblockhashes", &getblockhashes, {"high","low"} }, { "blockchain", "getblockhash", &getblockhash, {"height"} }, { "blockchain", "getblockheader", &getblockheader, {"blockhash","verbose"} }, diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 754b024d2d..863fbfd6d8 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -66,7 +66,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "getbalance", 2, "addlocked" }, { "getbalance", 3, "include_watchonly" }, { "getbalance", 4, "avoid_reuse" }, - { "getblockfrompeer", 1, "nodeid" }, + { "getblockfrompeer", 1, "peer_id" }, { "getchaintips", 0, "count" }, { "getchaintips", 1, "branchlen" }, { "getblockhash", 0, "height" }, diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py index a5d02cbbf2..71ed87293e 100755 --- a/test/functional/rpc_getblockfrompeer.py +++ b/test/functional/rpc_getblockfrompeer.py @@ -40,12 +40,8 @@ class GetBlockFromPeerTest(BitcoinTestFramework): 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,7 +56,7 @@ class GetBlockFromPeerTest(BitcoinTestFramework): 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)