partial bitcoin#23706: getblockfrompeer followups

excludes:
- 8d1a3e6498de6087501969a9d243b0697ca3fe97
- 809d66bb65aa78048e27c2a878d6f7becaecfe11
- 60243cac7286e4c4bdda7094bef4cf6d1564b583
This commit is contained in:
Kittywhiskers Van Gogh 2024-06-23 20:46:29 +00:00
parent 7e5cc5e375
commit b23d94b14f
No known key found for this signature in database
GPG Key ID: 30CD0C065E5C4AAD
5 changed files with 33 additions and 45 deletions

View File

@ -398,7 +398,7 @@ public:
/** Implement PeerManager */ /** Implement PeerManager */
void CheckForStaleTipAndEvictPeers() 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 EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; } bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);; 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); (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); LOCK(cs_main);
// Ensure this peer exists and hasn't been disconnected // Ensure this peer exists and hasn't been disconnected
CNodeState* state = State(id); CNodeState* state = State(peer_id);
if (state == nullptr) return false; if (state == nullptr) return "Peer does not exist";
// Mark block as in-flight unless it already is // Mark block as in-flight unless it already is (for this peer).
if (!MarkBlockAsInFlight(id, index.GetBlockHash(), &index)) return false; // 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 // Construct message to request the block
std::vector<CInv> invs{CInv(MSG_BLOCK, hash)}; std::vector<CInv> invs{CInv(MSG_BLOCK, hash)};
// Send block request message to the peer // 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()); const CNetMsgMaker msgMaker(node->GetCommonVersion());
this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs)); this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs));
return true; return true;
}); });
if (success) { if (!success) return "Peer not fully connected";
LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
hash.ToString(), id); LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
} else { hash.ToString(), peer_id);
MarkBlockAsReceived(hash); return std::nullopt;
LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
hash.ToString(), id);
}
return success;
} }
std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman, std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman,

View File

@ -69,12 +69,11 @@ public:
/** /**
* Attempt to manually fetch block from a given peer. We must already have the header. * Attempt to manually fetch block from a given peer. We must already have the header.
* *
* @param[in] id The peer id * @param[in] peer_id The peer id
* @param[in] hash The block hash * @param[in] block_index The blockindex
* @param[in] pindex The blockindex * @returns std::nullopt if a request was successfully made, otherwise an error message
* @returns Whether a request was successfully made
*/ */
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;
/** Get statistics from node state */ /** Get statistics from node state */
virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0; virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0;

View File

@ -775,8 +775,8 @@ static RPCHelpMan getblockfrompeer()
"\nWe must have the header for this block, e.g. using submitheader.\n" "\nWe must have the header for this block, e.g. using submitheader.\n"
"\nReturns {} if a block-request was successfully scheduled\n", "\nReturns {} if a block-request was successfully scheduled\n",
{ {
{"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"}, {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
{"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"}, {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"},
}, },
RPCResult{RPCResult::Type::OBJ, "", "", RPCResult{RPCResult::Type::OBJ, "", "",
{ {
@ -791,18 +791,11 @@ static RPCHelpMan getblockfrompeer()
const NodeContext& node = EnsureAnyNodeContext(request.context); const NodeContext& node = EnsureAnyNodeContext(request.context);
ChainstateManager& chainman = EnsureChainman(node); ChainstateManager& chainman = EnsureChainman(node);
PeerManager& peerman = EnsurePeerman(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<NodeId>(request.params[1].get_int64()); const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(block_hash););
// 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) { if (!index) {
throw JSONRPCError(RPC_MISC_ERROR, "Block header missing"); throw JSONRPCError(RPC_MISC_ERROR, "Block header missing");
@ -812,8 +805,8 @@ static RPCHelpMan getblockfrompeer()
if (index->nStatus & BLOCK_HAVE_DATA) { if (index->nStatus & BLOCK_HAVE_DATA) {
result.pushKV("warnings", "Block already downloaded"); result.pushKV("warnings", "Block already downloaded");
} else if (!peerman.FetchBlock(nodeid, hash, *index)) { } else if (const auto err{peerman.FetchBlock(peer_id, *index)}) {
throw JSONRPCError(RPC_MISC_ERROR, "Failed to fetch block from peer"); throw JSONRPCError(RPC_MISC_ERROR, err.value());
} }
return result; return result;
}, },
@ -3053,7 +3046,7 @@ static const CRPCCommand commands[] =
{ "blockchain", "getbestchainlock", &getbestchainlock, {} }, { "blockchain", "getbestchainlock", &getbestchainlock, {} },
{ "blockchain", "getblockcount", &getblockcount, {} }, { "blockchain", "getblockcount", &getblockcount, {} },
{ "blockchain", "getblock", &getblock, {"blockhash","verbosity|verbose"} }, { "blockchain", "getblock", &getblock, {"blockhash","verbosity|verbose"} },
{ "blockchain", "getblockfrompeer", &getblockfrompeer, {"blockhash", "nodeid"}}, { "blockchain", "getblockfrompeer", &getblockfrompeer, {"block_hash", "peer_id"}},
{ "blockchain", "getblockhashes", &getblockhashes, {"high","low"} }, { "blockchain", "getblockhashes", &getblockhashes, {"high","low"} },
{ "blockchain", "getblockhash", &getblockhash, {"height"} }, { "blockchain", "getblockhash", &getblockhash, {"height"} },
{ "blockchain", "getblockheader", &getblockheader, {"blockhash","verbose"} }, { "blockchain", "getblockheader", &getblockheader, {"blockhash","verbose"} },

View File

@ -66,7 +66,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "getbalance", 2, "addlocked" }, { "getbalance", 2, "addlocked" },
{ "getbalance", 3, "include_watchonly" }, { "getbalance", 3, "include_watchonly" },
{ "getbalance", 4, "avoid_reuse" }, { "getbalance", 4, "avoid_reuse" },
{ "getblockfrompeer", 1, "nodeid" }, { "getblockfrompeer", 1, "peer_id" },
{ "getchaintips", 0, "count" }, { "getchaintips", 0, "count" },
{ "getchaintips", 1, "branchlen" }, { "getchaintips", 1, "branchlen" },
{ "getblockhash", 0, "height" }, { "getblockhash", 0, "height" },

View File

@ -40,12 +40,8 @@ class GetBlockFromPeerTest(BitcoinTestFramework):
self.sync_blocks() self.sync_blocks()
self.log.info("Node 0 should only have the header for node 1's block 3") self.log.info("Node 0 should only have the header for node 1's block 3")
for x in self.nodes[0].getchaintips(): x = next(filter(lambda x: x['hash'] == short_tip, self.nodes[0].getchaintips()))
if x['hash'] == short_tip: assert_equal(x['status'], "headers-only")
assert_equal(x['status'], "headers-only")
break
else:
raise AssertionError("short tip not synced")
assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, short_tip) assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, short_tip)
self.log.info("Fetch block from node 1") 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) assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0)
self.log.info("Non-existent peer generates error") 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") self.log.info("Successful fetch")
result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id) result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id)