Skip to content

Commit b23d94b

Browse files
kwvgkwvg
committed
partial bitcoin#23706: getblockfrompeer followups
excludes: - 8d1a3e6 - 809d66b - 60243ca
1 parent 7e5cc5e commit b23d94b

File tree

5 files changed

+33
-45
lines changed

5 files changed

+33
-45
lines changed

‎src/net_processing.cpp‎

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -398,7 +398,7 @@ class PeerManagerImpl final : public PeerManager
398398

399399
/** Implement PeerManager */
400400
void CheckForStaleTipAndEvictPeers() override;
401-
bool FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& index) override;
401+
std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override;
402402
bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
403403
bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
404404
void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);;
@@ -1863,37 +1863,37 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
18631863
(GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
18641864
}
18651865

1866-
bool PeerManagerImpl::FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& index)
1866+
std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBlockIndex& block_index)
18671867
{
1868-
if (fImporting || fReindex) return false;
1868+
if (fImporting) return "Importing...";
1869+
if (fReindex) return "Reindexing...";
18691870

18701871
LOCK(cs_main);
18711872
// Ensure this peer exists and hasn't been disconnected
1872-
CNodeState* state = State(id);
1873-
if (state == nullptr) return false;
1873+
CNodeState* state = State(peer_id);
1874+
if (state == nullptr) return "Peer does not exist";
18741875

1875-
// Mark block as in-flight unless it already is
1876-
if (!MarkBlockAsInFlight(id, index.GetBlockHash(), &index)) return false;
1876+
// Mark block as in-flight unless it already is (for this peer).
1877+
// If a block was already in-flight for a different peer, its BLOCKTXN
1878+
// response will be dropped.
1879+
const uint256& hash{block_index.GetBlockHash()};
1880+
if (!MarkBlockAsInFlight(peer_id, hash, &block_index)) return "Already requested from this peer";
18771881

18781882
// Construct message to request the block
18791883
std::vector<CInv> invs{CInv(MSG_BLOCK, hash)};
18801884

18811885
// Send block request message to the peer
1882-
bool success = m_connman.ForNode(id, [this, &invs](CNode* node) {
1886+
bool success = m_connman.ForNode(peer_id, [this, &invs](CNode* node) {
18831887
const CNetMsgMaker msgMaker(node->GetCommonVersion());
18841888
this->m_connman.PushMessage(node, msgMaker.Make(NetMsgType::GETDATA, invs));
18851889
return true;
18861890
});
18871891

1888-
if (success) {
1889-
LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
1890-
hash.ToString(), id);
1891-
} else {
1892-
MarkBlockAsReceived(hash);
1893-
LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
1894-
hash.ToString(), id);
1895-
}
1896-
return success;
1892+
if (!success) return "Peer not fully connected";
1893+
1894+
LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
1895+
hash.ToString(), peer_id);
1896+
return std::nullopt;
18971897
}
18981898

18991899
std::unique_ptr<PeerManager> PeerManager::make(const CChainParams& chainparams, CConnman& connman, AddrMan& addrman, BanMan* banman,

‎src/net_processing.h‎

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -69,12 +69,11 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
6969
/**
7070
* Attempt to manually fetch block from a given peer. We must already have the header.
7171
*
72-
* @param[in] id The peer id
73-
* @param[in] hash The block hash
74-
* @param[in] pindex The blockindex
75-
* @returns Whether a request was successfully made
72+
* @param[in] peer_id The peer id
73+
* @param[in] block_index The blockindex
74+
* @returns std::nullopt if a request was successfully made, otherwise an error message
7675
*/
77-
virtual bool FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& pindex) = 0;
76+
virtual std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) = 0;
7877

7978
/** Get statistics from node state */
8079
virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0;

‎src/rpc/blockchain.cpp‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -775,8 +775,8 @@ static RPCHelpMan getblockfrompeer()
775775
"\nWe must have the header for this block, e.g. using submitheader.\n"
776776
"\nReturns {} if a block-request was successfully scheduled\n",
777777
{
778-
{"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
779-
{"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"},
778+
{"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
779+
{"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"},
780780
},
781781
RPCResult{RPCResult::Type::OBJ, "", "",
782782
{
@@ -791,18 +791,11 @@ static RPCHelpMan getblockfrompeer()
791791
const NodeContext& node = EnsureAnyNodeContext(request.context);
792792
ChainstateManager& chainman = EnsureChainman(node);
793793
PeerManager& peerman = EnsurePeerman(node);
794-
CConnman& connman = EnsureConnman(node);
795-
796-
uint256 hash(ParseHashV(request.params[0], "hash"));
797794

798-
const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
799-
800-
// Check that the peer with nodeid exists
801-
if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
802-
throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
803-
}
795+
const uint256& block_hash{ParseHashV(request.params[0], "block_hash")};
796+
const NodeId peer_id{request.params[1].get_int64()};
804797

805-
const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
798+
const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(block_hash););
806799

807800
if (!index) {
808801
throw JSONRPCError(RPC_MISC_ERROR, "Block header missing");
@@ -812,8 +805,8 @@ static RPCHelpMan getblockfrompeer()
812805

813806
if (index->nStatus & BLOCK_HAVE_DATA) {
814807
result.pushKV("warnings", "Block already downloaded");
815-
} else if (!peerman.FetchBlock(nodeid, hash, *index)) {
816-
throw JSONRPCError(RPC_MISC_ERROR, "Failed to fetch block from peer");
808+
} else if (const auto err{peerman.FetchBlock(peer_id, *index)}) {
809+
throw JSONRPCError(RPC_MISC_ERROR, err.value());
817810
}
818811
return result;
819812
},
@@ -3053,7 +3046,7 @@ static const CRPCCommand commands[] =
30533046
{ "blockchain", "getbestchainlock", &getbestchainlock, {} },
30543047
{ "blockchain", "getblockcount", &getblockcount, {} },
30553048
{ "blockchain", "getblock", &getblock, {"blockhash","verbosity|verbose"} },
3056-
{ "blockchain", "getblockfrompeer", &getblockfrompeer, {"blockhash", "nodeid"}},
3049+
{ "blockchain", "getblockfrompeer", &getblockfrompeer, {"block_hash", "peer_id"}},
30573050
{ "blockchain", "getblockhashes", &getblockhashes, {"high","low"} },
30583051
{ "blockchain", "getblockhash", &getblockhash, {"height"} },
30593052
{ "blockchain", "getblockheader", &getblockheader, {"blockhash","verbose"} },

‎src/rpc/client.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -66,7 +66,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
6666
{ "getbalance", 2, "addlocked" },
6767
{ "getbalance", 3, "include_watchonly" },
6868
{ "getbalance", 4, "avoid_reuse" },
69-
{ "getblockfrompeer", 1, "nodeid" },
69+
{ "getblockfrompeer", 1, "peer_id" },
7070
{ "getchaintips", 0, "count" },
7171
{ "getchaintips", 1, "branchlen" },
7272
{ "getblockhash", 0, "height" },

‎test/functional/rpc_getblockfrompeer.py‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -40,12 +40,8 @@ def run_test(self):
4040
self.sync_blocks()
4141

4242
self.log.info("Node 0 should only have the header for node 1's block 3")
43-
for x in self.nodes[0].getchaintips():
44-
if x['hash'] == short_tip:
45-
assert_equal(x['status'], "headers-only")
46-
break
47-
else:
48-
raise AssertionError("short tip not synced")
43+
x = next(filter(lambda x: x['hash'] == short_tip, self.nodes[0].getchaintips()))
44+
assert_equal(x['status'], "headers-only")
4945
assert_raises_rpc_error(-1, "Block not found on disk", self.nodes[0].getblock, short_tip)
5046

5147
self.log.info("Fetch block from node 1")
@@ -60,7 +56,7 @@ def run_test(self):
6056
assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0)
6157

6258
self.log.info("Non-existent peer generates error")
63-
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)
59+
assert_raises_rpc_error(-1, "Peer does not exist", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
6460

6561
self.log.info("Successful fetch")
6662
result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id)

0 commit comments

Comments
 (0)