This adds an RPC method to fetch a block directly from a peer. This can used to fetch stale blocks with lower proof of work that are normally ignored by the node (headers-only in getchaintips).
Usage:
0bitcoin-cli getblockfrompeer HASH peer_n
Closes #20155
Limitations:
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
@Sjors Please note that I’m very new to C++ and I’m here only to learn.
Concept AKN, IMO can also be useful if the user need info about a pruned block.
Not sure if getblockfrompeer should persist the block, maybe just print it. For consistency the interface of getblock (verbosity 0, 1, 2) could be reused.
Maybe getblockfrompeer could just be a flag of getblock.
Also, IMO GetBlockChecked should notify the user to use getblockfrompeer for block that are not on disk.
Check if the node is connected
Checking for connectivity could be done with connman->GetNodeCount as in mining.cpp.
Mark the block as in flight
Instead of mark the block as in flight and persist the block, I would add 2 fields to the public interface of CNode the first to notify ProcessMessage that a block with a certain hash is expected, the second to check (in a loop) for the requested block.
@Fi3 being able to (temporarily) retrieve pruned blocks is useful too indeed.
It can be difficult to find nodes that still have a stale block, so I’d rather hold on to it. That also allows inspection using the regular methods, getblock, getchaintips and getrawtransaction (e.g. to learn about a specific doublespend). Pruned nodes will toss it automatically after a while (IIUC).
I also want to be able to fully validate the block. It’s very time consuming, but it works: call invalidateblock on the main chain block for the same height, so reorgs to the block you just downloaded and verified it. Then call reconsiderblock to sync to the tip again.
Concept ACK
This would be useful for https://forkmonitor.info/ to assess the validity of shorter chains
Concept ACK
This would be very useful for testing
Concept -1
I think it would be better to just do the right thing if you getblock a block you don’t have… Don’t make users figure out which peer has it…?
@luke-jr my thinking was to expand it later, to try all peers: getblockfrompeer HASH *. Overloading getblock in a way that could make it unresponsive would seem controversial. Adding a fetch_from_peer argument to getblock could make sense as an alternative.
In practice none of the existing peers might have it, so you’ll have to:
This could get complicated, so a fresh RPC seemed like a more future proof approach.
Linter:
A new circular dependency in the form of “rpc/blockchain -> rpc/net -> rpc/blockchain” appears to have been introduced. @MarcoFalke any thoughts on how to avoid that? I could move this new RPC method to
rpc/net, or maybe theEnsurePeermanandEnsureConnmanhelpers should be in their own file?
Ensure* helpers to rpc/util (or a new module)?
245@@ -246,6 +246,7 @@ class PeerManagerImpl final : public PeerManager
246
247 /** Implement PeerManager */
248 void CheckForStaleTipAndEvictPeers() override;
249+ bool FetchBlock(const NodeId nodeid, const CBlockIndex* pindex) override;
No need to make the pass-by-value NodeId nodeid const.
Style guide is to use snake_case and not hungarian notation for parameter names:
0 bool FetchBlock(const NodeId id, const CBlockIndex* index) override;
1252@@ -1252,6 +1253,33 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(const NodeId nodeid, const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
I don’t think this EXCLUSIVE_LOCKS_REQUIRED(cs_main) is doing anything. This function is being called without cs_main (by getblockfrompeer()).
I think it’s better to remove this annotation and lock cs_main within this function.
1252@@ -1252,6 +1253,33 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(const NodeId nodeid, const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1257+{
1258+ PeerRef peer = GetPeerRef(nodeid);
1259+ if (peer == nullptr) return false;
1260+ uint32_t nFetchFlags = 0;
Move this down to where it’s first used, and make const:
0 const uint32_t nFetchFlags = state->fHaveWitness ? MSG_WITNESS_FLAG : 0;
nFetchFlags around…
1268+ }
1269+ if (state->fHaveWitness) {
1270+ nFetchFlags |= MSG_WITNESS_FLAG;
1271+ }
1272+ std::vector<CInv> vInv(1);
1273+ vInv[0] = CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash());
Use initializer list ctor:
0 std::vector<CInv> vInv{CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash())};
1252@@ -1252,6 +1253,33 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(const NodeId nodeid, const CBlockIndex* pindex) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1257+{
block message will be ignored)
1264+ }
1265+ const int node_sync_height = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
1266+ if (pindex->nHeight > node_sync_height) {
1267+ return false;
1268+ }
1269+ if (state->fHaveWitness) {
MSG_WITNESS_FLAG. It’s 2021. Receiving a block without witnesses isn’t going to be useful.
1274+ bool success = m_connman.ForNode(nodeid, [this, vInv](CNode* pnode) {
1275+ const CNetMsgMaker msgMaker(pnode->GetCommonVersion());
1276+ this->m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::GETDATA, vInv));
1277+ return true;
1278+ });
1279+ MarkBlockAsInFlight(nodeid, pindex->GetBlockHash(), pindex);
ForNode() call and don’t send the getdata if it returns false, since that indicates that the block is already in flight. We don’t want a faulty RPC client causing us to spam a peer with multiple requests for the same block.
1269+ if (state->fHaveWitness) {
1270+ nFetchFlags |= MSG_WITNESS_FLAG;
1271+ }
1272+ std::vector<CInv> vInv(1);
1273+ vInv[0] = CInv(MSG_BLOCK | nFetchFlags, pindex->GetBlockHash());
1274+ bool success = m_connman.ForNode(nodeid, [this, vInv](CNode* pnode) {
1267+ return false;
1268+ }
1269+ if (state->fHaveWitness) {
1270+ nFetchFlags |= MSG_WITNESS_FLAG;
1271+ }
1272+ std::vector<CInv> vInv(1);
style guide:
0 std::vector<CInv> invs(1);
785+ uint256 hash(ParseHashV(request.params[0], "hash"));
786+
787+ const CBlockIndex* pblockindex;
788+ {
789+ LOCK(cs_main);
790+ pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
EnsureChainman()
795+ }
796+
797+ const UniValue &id_arg = request.params[1];
798+ const NodeId nodeid = (NodeId) id_arg.get_int64();
799+ const NodeContext& node = EnsureAnyNodeContext(request.context);
800+ PeerManager& peerman = EnsurePeerman(node);
786+
787+ const CBlockIndex* pblockindex;
788+ {
789+ LOCK(cs_main);
790+ pblockindex = g_chainman.m_blockman.LookupBlockIndex(hash);
791+ }
0 const CBlockIndex* const pblockindex = WITH_LOCK(cs_main, return g_chainman.m_blockman.LookupBlockIndex(hash););
773+ "\nAttempts to fetch block from a given peer.\n",
774+ {
775+ {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
776+ {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"},
777+ },
778+ RPCResult{RPCResult::Type::NONE, "", ""},
766@@ -764,6 +767,46 @@ static RPCHelpMan getmempoolentry()
767 };
768 }
769
770+static RPCHelpMan getblockfrompeer()
771+{
772+ return RPCHelpMan{"getblockfrompeer",
773+ "\nAttempts to fetch block from a given peer.\n",
Imperative mood:
0 "\nAttempt to fetch block from a given peer.\n",
util.h and addressed the above comments.
730+ + HelpExampleRpc("getblockfrompeer", "\"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09\" 0")
731+ },
732+ [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
733+{
734+ const UniValue &id_arg = request.params[1];
735+ const NodeId nodeid = (NodeId) id_arg.get_int64();
Prefer a named cast to c-style casts (https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast)
0 const NodeId nodeid = static_cast<NodeId>(id_arg.get_int64());
Perhaps join these two lines. No point in having a id_arg var that just gets used once and thrown away:
0 const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
735+ const NodeId nodeid = (NodeId) id_arg.get_int64();
736+ const NodeContext& node = EnsureAnyNodeContext(request.context);
737+ ChainstateManager& chainman = EnsureChainman(node);
738+ PeerManager& peerman = EnsurePeerman(node);
739+
740+ uint256 hash(ParseHashV(request.params[0], "hash"));
This ordering seems weird to me (read param 1, then state assumptions, then read param 0). I’d suggest:
0 const NodeContext& node = EnsureAnyNodeContext(request.context);
1 ChainstateManager& chainman = EnsureChainman(node);
2 PeerManager& peerman = EnsurePeerman(node);
3
4 uint256 hash(ParseHashV(request.params[0], "hash"));
5
6 const UniValue &id_arg = request.params[1];
7 const NodeId nodeid = (NodeId) id_arg.get_int64();
50+ChainstateManager& EnsureChainman(const NodeContext& node)
51+{
52+ if (!node.chainman) {
53+ throw JSONRPCError(RPC_INTERNAL_ERROR, "Node chainman not found");
54+ }
55+ WITH_LOCK(::cs_main, CHECK_NONFATAL(std::addressof(g_chainman) == std::addressof(*node.chainman)));
This causes a link failure for libbitcoin_common.
Perhaps move these Ensure* functions to a new src/rpc/server_util.cpp which is part of libbitcoin_server. src/rpc/util is for utility functions that are shared between server and client.
server_util.cpp. This seems to make the linker happy.
203 rpc/protocol.h \
204 rpc/rawtransaction_util.h \
205 rpc/register.h \
206 rpc/request.h \
207 rpc/server.h \
208+ rpc/server_util.h \
344@@ -345,6 +345,7 @@ libbitcoin_server_a_SOURCES = \
345 rpc/net.cpp \
346 rpc/rawtransaction.cpp \
347 rpc/server.cpp \
348+ rpc/server_util.cpp \
534@@ -534,7 +535,7 @@ libbitcoin_common_a_SOURCES = \
535 psbt.cpp \
536 rpc/rawtransaction_util.cpp \
537 rpc/external_signer.cpp \
538- rpc/util.cpp \
539+ rpc/util.cpp \
14+struct NodeContext;
15+class PeerManager;
16+
17+/**
18+ * Ensure* helpers
19+ */
Ensure.
The includes could be improved to include what’s needed and not include what’s not needed:
0diff --git a/src/rpc/server_util.cpp b/src/rpc/server_util.cpp
1index dfe07f5c3d..046a630b51 100644
2--- a/src/rpc/server_util.cpp
3+++ b/src/rpc/server_util.cpp
4@@ -2,12 +2,18 @@
5 // Distributed under the MIT software license, see the accompanying
6 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
7
8+#include <rpc/server_util.h>
9+
10 #include <net_processing.h>
11 #include <node/context.h>
12-#include <rpc/server_util.h>
13+#include <policy/fees.h>
14+#include <rpc/protocol.h>
15+#include <rpc/request.h>
16+#include <txmempool.h>
17 #include <util/system.h>
18 #include <validation.h>
19-#include <validationinterface.h>
20+
21+#include <any>
22
23 NodeContext& EnsureAnyNodeContext(const std::any& context)
24 {
25diff --git a/src/rpc/server_util.h b/src/rpc/server_util.h
26index c330efd0e6..dd8e8a6042 100644
27--- a/src/rpc/server_util.h
28+++ b/src/rpc/server_util.h
29@@ -5,11 +5,11 @@
30 #ifndef BITCOIN_RPC_SERVER_UTIL_H
31 #define BITCOIN_RPC_SERVER_UTIL_H
32
33-#include <rpc/protocol.h>
34-#include <rpc/request.h>
35+#include <any>
36
37 class CBlockPolicyEstimator;
38 class CConnman;
39+class ChainstateManager;
40 class CTxMemPool;
41 struct NodeContext;
42 class PeerManager;
1252@@ -1252,6 +1253,39 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex* index)
If you’re taking a pointer, you should check that it’s non-null before dereferencing it. Even better, pass by reference to communicate that this must be called with a valid CBlockIndex:
0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
1index a822960dc2..f552aa36b5 100644
2--- a/src/net_processing.cpp
3+++ b/src/net_processing.cpp
4@@ -246,7 +246,7 @@ public:
5
6 /** Implement PeerManager */
7 void CheckForStaleTipAndEvictPeers() override;
8- bool FetchBlock(NodeId id, const CBlockIndex* index) override;
9+ bool FetchBlock(NodeId id, const CBlockIndex& index) override;
10 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override;
11 bool IgnoresIncomingTxs() override { return m_ignore_incoming_txs; }
12 void SendPings() override;
13@@ -1253,7 +1253,7 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
14 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
15 }
16
17-bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex* index)
18+bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex& index)
19 {
20 PeerRef peer = GetPeerRef(id);
21 if (peer == nullptr) return false;
22@@ -1263,12 +1263,12 @@ bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex* index)
23 if (state == nullptr) return false;
24 if (!state->fHaveWitness) return false;
25 const int node_sync_height = state->pindexBestKnownBlock ? state->pindexBestKnownBlock->nHeight : -1;
26- if (index->nHeight > node_sync_height) {
27+ if (index.nHeight > node_sync_height) {
28 return false;
29 }
30- std::vector<CInv> invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, index->GetBlockHash())};
31+ std::vector<CInv> invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, index.GetBlockHash())};
32 // Mark block as in-flight unless it already is
33- if (!MarkBlockAsInFlight(id, index->GetBlockHash(), index)) return false;
34+ if (!MarkBlockAsInFlight(id, index.GetBlockHash(), &index)) return false;
35 bool success = m_connman.ForNode(id, [this, invs](CNode* pnode) {
36 const CNetMsgMaker msgMaker(pnode->GetCommonVersion());
37 this->m_connman.PushMessage(pnode, msgMaker.Make(NetMsgType::GETDATA, invs));
38@@ -1276,12 +1276,12 @@ bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex* index)
39 });
40 if (success) {
41 LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
42- index->GetBlockHash().ToString(), id);
43+ index.GetBlockHash().ToString(), id);
44 } else {
45 // Remove block from in-flight
46- MarkBlockAsReceived(index->GetBlockHash());
47+ MarkBlockAsReceived(index.GetBlockHash());
48 LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
49- index->GetBlockHash().ToString(), id);
50+ index.GetBlockHash().ToString(), id);
51 }
52 return success;
53 }
54diff --git a/src/net_processing.h b/src/net_processing.h
55index 577c925562..659504397f 100644
56--- a/src/net_processing.h
57+++ b/src/net_processing.h
58@@ -43,7 +43,7 @@ public:
59 virtual ~PeerManager() { }
60
61 /** Attempt to manually fetch block from a given node. */
62- virtual bool FetchBlock(const NodeId nodeid, const CBlockIndex* pindex) = 0;
63+ virtual bool FetchBlock(const NodeId nodeid, const CBlockIndex& pindex) = 0;
64
65 /** Get statistics from node state */
66 virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0;
67diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
68index aa6b216ab1..cb3f2f3abc 100644
69--- a/src/rpc/blockchain.cpp
70+++ b/src/rpc/blockchain.cpp
71@@ -746,7 +746,7 @@ static RPCHelpMan getblockfrompeer()
72 throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found");
73 }
74
75- if (!peerman.FetchBlock(nodeid, pblockindex)) {
76+ if (!peerman.FetchBlock(nodeid, *pblockindex)) {
77 throw JSONRPCError(RPC_MISC_ERROR, "Failed to fetch block from peer");
78 }
79 return UniValue::VOBJ;
1252@@ -1252,6 +1253,39 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex* index)
1257+{
1252@@ -1252,6 +1253,47 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex& index)
1257+{
1258+ PeerRef peer = GetPeerRef(id);
1259+ if (peer == nullptr) return false;
peer isn’t used. You can remove it.
CNodeState exists, which can only happen if the peer isn’t disconnected, which makes this check redundant.
1252@@ -1252,6 +1253,47 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1253 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1254 }
1255
1256+bool PeerManagerImpl::FetchBlock(NodeId id, const CBlockIndex& index)
1257+{
1258+ PeerRef peer = GetPeerRef(id);
1259+ if (peer == nullptr) return false;
1260+ if (fImporting && !fReindex) return false;
Why not match the logic in ProcessMessage() when receiving a block:
0 if (fImporting || fReindex) return false;
41@@ -42,6 +42,9 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
42 CTxMemPool& pool, bool ignore_incoming_txs);
43 virtual ~PeerManager() { }
44
45+ /** Attempt to manually fetch block from a given node. */
46+ virtual bool FetchBlock(const NodeId nodeid, const CBlockIndex& pindex) = 0;
Remove const from the pass-by-value argument:
0 virtual bool FetchBlock(NodeId nodeid, const CBlockIndex& pindex) = 0;
1274+
1275+ // Mark block as in-flight unless it already is
1276+ if (!MarkBlockAsInFlight(id, index.GetBlockHash(), &index)) return false;
1277+
1278+ // Send block request message to the peer
1279+ bool success = m_connman.ForNode(id, [this, invs](CNode* pnode) {
Prefer to capture invs by reference than making a copy:
0 bool success = m_connman.ForNode(id, [this, &invs](CNode* pnode) {
Also maybe rename pnode to node if you’re touching this part again.
8+from test_framework.util import (
9+ assert_equal,
10+ assert_raises_rpc_error,
11+)
12+
13+import time
20+ def setup_network(self):
21+ self.setup_nodes()
22+
23+ def run_test(self):
24+ self.log.info("Mine 4 blocks on Node 0")
25+ self.nodes[0].generatetoaddress(4, self.nodes[0].get_deterministic_priv_key().address)
0 self.nodes[0].generate(4)
Same below.
30+ assert_equal(self.nodes[1].getblockcount(), 203)
31+ short_tip = self.nodes[1].getbestblockhash()
32+
33+ self.log.info("Connect nodes to sync headers")
34+ self.connect_nodes(0, 1)
35+ self.sync_blocks(self.nodes[0:2])
0 self.sync_blocks()
47+ peers = self.nodes[0].getpeerinfo()
48+ assert_equal(len(peers), 1)
49+ peer_0_peer_1_id = peers[0]["id"]
50+ self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id)
51+ time.sleep(1)
52+ self.nodes[0].getblock(short_tip)
wait_until rather than a sleep (sleep will take needlessly long on a fast system and may time out on a slow system).
1282+ return true;
1283+ });
1284+
1285+ if (success) {
1286+ LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
1287+ index.GetBlockHash().ToString(), id);
0 index.GetBlockHash().ToString(), id);
1287+ index.GetBlockHash().ToString(), id);
1288+ } else {
1289+ // Remove block from in-flight
1290+ MarkBlockAsReceived(index.GetBlockHash());
1291+ LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
1292+ index.GetBlockHash().ToString(), id);
0 index.GetBlockHash().ToString(), id);
I think it would be better to just do the right thing if you
getblocka block you don’t have… Don’t make users figure out which peer has it…?
I agree with @luke-jr
Or this RPC can make sense if all peers would reject such request by default but if someone is okay with it, can save something like allowgetblockfrompeer=1 in bitcoin.conf
It’s not that simple. Do we fetch the block from all peers at once? That’s a huge bandwidth waste if you have 100 connection. Do we ask them one by one in sequence? What’s the timeout? Do we do in parallel? Hence my suggestion to leave that to a followup.
allowgetblockfrompeer=1
Peers will have no idea someone used this RPC call so that’s not a concern. Nodes request blocks from each other all the time. This PR does not introduce a new p2p message.
0@@ -0,0 +1,62 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2020 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+"""Test the getblockfrompeer RPC."""
6+
7+from test_framework.test_framework import BitcoinTestFramework
8+from test_framework.authproxy import JSONRPCException
744@@ -792,6 +745,43 @@ static RPCHelpMan getmempoolentry()
745 };
746 }
747
748+static RPCHelpMan getblockfrompeer()
749+{
750+ return RPCHelpMan{"getblockfrompeer",
751+ "\nAttempt to fetch block from a given peer.\n",
MarkBlockAsInFlight can handle a nullprtr index. It might indeed be simpler to drop this requirement, as well as the index.nHeight > node_sync_height check.
GetPeerRef, dropped requirement for other node to be at the right height.
246@@ -247,6 +247,7 @@ class PeerManagerImpl final : public PeerManager
247
248 /** Implement PeerManager */
249 void CheckForStaleTipAndEvictPeers() override;
250+ bool FetchBlock(NodeId id, uint256 hash, const CBlockIndex* index) override;
uint256 by const reference (to avoid unnecessary copying, generally only basic types should be passed by value).
index may be null here?
utACK 803d2f568f7f24557543f61bf9d5a73952caedb5.
Changes look safe.
@sipa the current block fetching logic is buried deep inside net_processing. A MSG_BLOCK is generated when processing received headers:
Or when receiving a block: https://github.com/bitcoin/bitcoin/blob/4741aec1dd28829f45abcc529cddaa0ff04d07a0/src/net_processing.cpp#L1733-L1739
There doesn’t seem to be a queue of blocks to fetch that’s distributed over peers and checked in a background process.
In practice I’m already able to use this PR to fetch stale blocks by simply looping over my peers and “spamming” them all. I then disconnect if the block doesn’t show up, connect to random new peers and then try again. But those are behaviors we probably shouldn’t implement here.
So although the current incarnation is not ideal, it’s already useful. A revamp of ProcessHeadersMessage with a separate block download manager sounds doesn’t sound easy to me. It also needs different timeout rules than IBD, because most of the time a peer won’t have the block.
The use case of re-fetching pruned blocks is more similar to IBD, since you can expect every non-pruned node with sufficiently high pindexLastCommonBlock to have it.
Yes, I imagine it’d use the “background’ fetching logic in SendMessages, not the direct fetch in response to an announcement one.
Why would it need different timeout, or why would most blocks not have it? We know which peers are pruned.
Oh, I see. I missed that fetching non-main-chain blocks was a goal here.
Why would you want that?
That’s indeed the main reason I wrote this, though I can see how it’s useful for redownloading pruned blocks too.
I’m using this for ForkMonitor to fetch stale blocks and compare the transactions, to see if there was a double-spend: https://forkmonitor.info/stale/btc/679823 (we haven’t found a proper one in the past few months)
Rebased after some Ensure* methods were renamed in #21391.
In addition since #22221, BlockRequested no longer takes a pointer. Which means we can’t call it for blocks for which we don’t have a header. Paging @MarcoFalke to check if that was indeed the intention.
requestblock RPC? There’s been too much refactoring to simply cherry-pick it, but it could contain inspiration for improvement.
770+
771+ uint256 hash(ParseHashV(request.params[0], "hash"));
772+
773+ const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
774+
775+ const CBlockIndex* const pblockindex = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
0 const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
(we no longer use hungarian notation to show what type the variable is)
In addition since #22221, BlockRequested no longer takes a pointer. Which means we can’t call it for blocks for which we don’t have a header.
What do you think about restricting this RPC to where the node already has the header, by returning in getblockfrompeer() if LookupBlockIndex() fails? That’d make the functionality in net_processing more closely mirror our normal block download, where we’ll only ever request a block if we already have its header. If the node doesn’t already have the header, the user could first call submitheader with the serialized block header, followed by getblockfrompeer().
775+ const CBlockIndex* const pblockindex = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
776+
777+ if (!peerman.FetchBlock(nodeid, hash, pblockindex)) {
778+ throw JSONRPCError(RPC_MISC_ERROR, "Failed to fetch block from peer");
779+ }
780+ return UniValue::VOBJ;
NullUniValue here? Returning UniValue::VOBJ causes bitcoin-cli to print empty JSON object; returning NullUniValue would print nothing (as many other RPCs do). Is it to make this more backward-compatible if we later make it return something, like the block?
success boolean, but that didn’t make sense because we don’t know if we actually get the block. So I guess I made the response an empty VOBJ. I’ll switch to Null.
43+ short_tip_synced = False
44+ for x in self.nodes[0].getchaintips():
45+ if x['hash'] == short_tip:
46+ assert_equal(x['status'], "headers-only")
47+ short_tip_synced = True
48+ assert short_tip_synced
0 for x in self.nodes[0].getchaintips():
1 if x['hash'] == short_tip:
2 assert_equal(x['status'], "headers-only")
3 break
4 else:
5 raise AssertionError("short tip not synced")
51+ self.log.info("Fetch block from node 1")
52+ peers = self.nodes[0].getpeerinfo()
53+ assert_equal(len(peers), 1)
54+ peer_0_peer_1_id = peers[0]["id"]
55+ self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id)
56+ self.wait_until(lambda: self.check_for_block(short_tip), timeout=1)
Suggestions for additional tests:
0 self.log.info("No error if block is not found in the index")
1 assert_equal(self.nodes[0].getblockfrompeer("12" * 32, peer_0_peer_1_id), None)
2 self.log.info("Non-existent peer generates error")
3 assert_raises_rpc_error(-1, "Failed to fetch block from peer", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
What do you think about restricting this RPC to where the node already has the header, by returning in
getblockfrompeer()ifLookupBlockIndex()fails?
I agree that this would make sense. My understanding is that if we’d request a block for which we don’t have the header and which has less work than our tip, then getblockfrompeer would currently signal success, we might also get the block delivered by the peer, but then AcceptBlock() would never store it because fRequested and fHasMoreOrSameWork would be false (code)
But if we cannot accept the block, I don’t think it makes sense to request it in the first place (and we should instead report an RPC error).
Yesterdays PR Review Club covered this PR, thanks everyone! https://bitcoincore.reviews/20295
Rebased and addressed feedback.
@jnewbery in the use case I have in mind the node already has the header. We could always add a getheaderfrompeer RPC method if anyone needs it, though that seems a bit tedious to use. Alternative we can later expand getblockfrompeer to be smart about fetching headers first. For now I’ll add the check you suggested so the user gets a clear error when requesting a block for which we don’t have a header.
I’ll ponder this a bit, but for now I just added a line to the TODO warning about potentially breaking getblockfrompeer.
I reverted back to returning an empty object as per the developer notes “Try to make the RPC response a JSON object”.
Also made FetchBlock take CBlockIndex as a reference rather than a pointer, since it’s now mandatory.
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")
nit:
0 x = next(x for x in self.nodes[0].getchaintips() if x['hash'] == short_tip)
1 assert_equal(x['status'], "headers-only")
17@@ -18,6 +18,8 @@
18 #include <hash.h>
19 #include <index/blockfilterindex.h>
20 #include <index/coinstatsindex.h>
21+#include <net.h> // For NodeId
// For NodeId comment here. I think comments like these tend to be overlooked when adding something else that needs net.h and thus out-dating the comment.
749@@ -748,6 +750,44 @@ static RPCHelpMan getmempoolentry()
750 };
751 }
752
753+static RPCHelpMan getblockfrompeer()
754+{
755+ return RPCHelpMan{"getblockfrompeer",
756+ "\nAttempt to fetch block from a given peer.\n"
{} once the block-request has been successfully scheduled and that I have to manually check if my node now has the block.
777+ const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
778+
779+ if (!index) {
780+ throw JSONRPCError(RPC_MISC_ERROR, "Block header missing");
781+ }
782+
getblockfrompeer.
I’ve manually tested getblockfrompeer against two peers on a custom signet. Both peers share the chain tip, but both know about different (now stale) branches. I purposefully black-box tested the RPC without look at the code and tests added in this PR.
Tests:
Block header missing. This behavior is documented and expected :heavy_check_mark:.{} and a getblock RPC call returns with Block not found on disk. This behavior is as expected as peer can’t send the block if it does not know about it :heavy_check_mark:.getchaintips status is valid-headers which is documented as All blocks are available for this branch, but they were never fully validated): Returns with {} and a getblock RPC call returns with Block not found on disk. This behavior is fine I guess. In normal operation, we probably don’t want to serve unvalidated blocks to peers as a DoS mitigation. The RPC likely inherits this behavior :question:.getchaintips status is ‘headers-only’): Returns with {} and a getblock RPC call returns with Block not found on disk. This behavior is as expected as peer can’t send the block if it only knows about the header :heavy_check_mark:.getchaintips status is valid-fork): Returns with {} and a getblock RPC call returns the block :heavy_check_mark:.{} and a getblock RPC call fails. The peer does not send the block likely to a similar reason as in test 3 :question:.Notes:
Requesting an unavailable block from a peer A multiple times in a row fails with the error message Failed to fetch block from peer starting with the second request. A getdata message is only send to the peer on the first attempt. The error message is not displayed when requesting from a peer B in between the requests to peer A.
Re-requesting already available blocks resends a getdata message and the peer responds with the block again. Maybe check if a block is available before requesting it?
My node disconnected from a peer at some point because a getdata timed out. Timeout downloading block 00000268... from peer=0, disconnecting. This block was in a branch marked as valid-headers by peer 0. My node directly reconnected to peer 0 as peer 2 as it’s manually configured with addnode. This could be an issue when using this in the real world.
Requesting blocks that would be an orphan to us (the previous header is unknown) is not possible as submitheader requires the previous header to be known. I’m not sure there is another way to have orphan (orphan, not stale) blocks or headers in the block tree.
Some code review comments below too.
RE my own comment:
Requesting an unavailable block from a peer
Amultiple times in a row fails with the error messageFailed to fetch block from peerstarting with the second request. Agetdatamessage is only send to the peer on the first attempt. The error message is not displayed when requesting from a peerBin between the requests to peerA.
Looked this behavior up in the code: PeerManagerImpl::BlockRequested() returns false if the block is already requested. This causes the RPC to fail too. If the block is requested from another peer, the request is dropped.
Doesn’t this cause us to drop a request to peer A if A is slow and we instantly request it from B too? Could it cause issues if if A then sends us the block or do we just accept it?
Generally seems like doing getblockfrompeer for many peers without a delay between peers is not the ideal use of this RPC.
Thanks for the thorough testing @0xB10C.
Regarding (3) and (6): if a peer doesn’t give us the block, there’s nothing we can do. Since the call is not asynchronous, we can’t give the user much useful feedback either. Changing p2p behavior for peers to give us a block before it’s evaluated, or one it considers invalid, is beyond the scope of this PR.
Requesting an unavailable block from a peer
Amultiple times in a row fails with the error messageFailed to fetch block from peerstarting with the second request. Agetdatamessage is only send to the peer on the first attempt.
Interesting. I’d rather not make the RPC aware of too much internal magic of netmanager. If the user calls an RPC again after an error message, I guess they’re on their own :-)
The error message is not displayed when requesting from a peer
Bin between the requests to peerA.
The only reason I can think of for requesting a block from multiple peers, is that the user is happy as long as at least one succeeds. So this is probably not a huge deal, but still strange. I could add a recommendation to the help that it’s safer to call this method in sequence with a few second delay?
PeerManager::make directly under the new FetchBlock, though I’m disappointed Git couldn’t figure that out).
trivial reACK 9181e2e217
Confirmed with git rangediff that the only conflict was for touching lines.
1437+ if (state == nullptr) return false;
1438+ // Ignore pre-segwit peers
1439+ if (!state->fHaveWitness) return false;
1440+
1441+ // Construct message to request the block
1442+ std::vector<CInv> invs{CInv(MSG_BLOCK | MSG_WITNESS_FLAG, hash)};
m_connman.ForNode() call below?
785+ }
786+
787+ UniValue result = UniValue::VOBJ;
788+
789+ if (index->nStatus & BLOCK_HAVE_DATA) {
790+ result.pushKV("warnings", "Block already downloaded");
Is it intentional that this succeeds even with an invalid nodeid?
Perhaps the tests should check for the expected behaviour.
You could use CConnman::ForNode() to determine whether the peer with that nodeid exists. Here’s the diff (with the required changes to make the tests run on master):
0diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
1index 9fc586c122..fdbfe55aaf 100644
2--- a/src/rpc/blockchain.cpp
3+++ b/src/rpc/blockchain.cpp
4@@ -780,11 +780,17 @@ static RPCHelpMan getblockfrompeer()
5 const NodeContext& node = EnsureAnyNodeContext(request.context);
6 ChainstateManager& chainman = EnsureChainman(node);
7 PeerManager& peerman = EnsurePeerman(node);
8+ CConnman& connman = EnsureConnman(node);
9
10 uint256 hash(ParseHashV(request.params[0], "hash"));
11
12 const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
13
14+ // Check that the peer with nodeid exists
15+ if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
16+ throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
17+ }
18+
19 const CBlockIndex* const index = WITH_LOCK(cs_main, return chainman.m_blockman.LookupBlockIndex(hash););
20
21 if (!index) {
22diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py
23index ec562c94f9..e841cba70e 100755
24--- a/test/functional/rpc_getblockfrompeer.py
25+++ b/test/functional/rpc_getblockfrompeer.py
26@@ -27,11 +27,11 @@ class GetBlockFromPeerTest(BitcoinTestFramework):
27
28 def run_test(self):
29 self.log.info("Mine 4 blocks on Node 0")
30- self.nodes[0].generate(4)
31+ self.generate(self.nodes[0], 4, sync_fun=self.no_op)
32 assert_equal(self.nodes[0].getblockcount(), 204)
33
34 self.log.info("Mine competing 3 blocks on Node 1")
35- self.nodes[1].generate(3)
36+ self.generate(self.nodes[1], 3, sync_fun=self.no_op)
37 assert_equal(self.nodes[1].getblockcount(), 203)
38 short_tip = self.nodes[1].getbestblockhash()
39
40@@ -60,7 +60,7 @@ class GetBlockFromPeerTest(BitcoinTestFramework):
41 assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0)
42
43 self.log.info("Non-existent peer generates error")
44- assert_raises_rpc_error(-1, "Failed to fetch block from peer", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
45+ 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)
46
47 self.log.info("Successful fetch")
48 result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id)
sync_fun=self.no_op bit, as it results in: AttributeError: 'GetBlockFromPeerTest' object has no attribute 'no_op'
sync_fun=self.no_op bit before merge.
749@@ -748,6 +750,52 @@ static RPCHelpMan getmempoolentry()
750 };
751 }
752
753+static RPCHelpMan getblockfrompeer()
754+{
755+ return RPCHelpMan{"getblockfrompeer",
756+ "\nAttempt to fetch block from a given peer.\n"
757+ "\nWe must have the header for this block, e.g. using submitheader.\n"
758+ "\nReturns {} if a block-request was successfully scheduled\n",
Note: On a pruned node this block will be pruned again when the pruneheight surpasses the blockheight at the time of fetching the block.
tested ACK 9181e2e217955fff5f726b0e011afb78fb90af8f
I think this is ready to be merged as-is and my comments below can be addressed in a follow-up.
The documentation should be improved on how long users can expect this block to be around on a pruned node. If someone uses this on a prune=550 node, for example, this may become confusing for certain use cases. I made a suggestion in my comment but I can also propose this in a follow-up PR since it might require further discussion.
I also wrote an additional test for the pruned node use case here. Feel free to add it here if you have to retouch, otherwise, I will open a follow-up PR with it.
Co-authored-by: John Newbery <john@johnnewbery.com>
1427@@ -1427,6 +1428,41 @@ bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
1428 (GetBlockProofEquivalentTime(*pindexBestHeader, *pindex, *pindexBestHeader, m_chainparams.GetConsensus()) < STALE_RELAY_AGE_LIMIT);
1429 }
1430
1431+bool PeerManagerImpl::FetchBlock(NodeId id, const uint256& hash, const CBlockIndex& index)
What about returning the error string on failure std::optional<std::string>?
The caller could then
0if(const auto err{Fetch()}){
1 throw err.value();
2}
This will make users and tests more happy
1438+ if (state == nullptr) return false;
1439+ // Ignore pre-segwit peers
1440+ if (!state->fHaveWitness) return false;
1441+
1442+ // Mark block as in-flight unless it already is
1443+ if (!BlockRequested(id, index)) return false;
BLOCKTXN are ignored entirely if they’re no longer on the in flight list for a given peer.
1455+ if (success) {
1456+ LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
1457+ hash.ToString(), id);
1458+ } else {
1459+ RemoveBlockRequest(hash);
1460+ LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
41@@ -42,6 +42,16 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
42 CTxMemPool& pool, bool ignore_incoming_txs);
43 virtual ~PeerManager() { }
44
45+ /**
46+ * Attempt to manually fetch block from a given peer. We must already have the header.
47+ *
48+ * @param[in] id The peer id
49+ * @param[in] hash The block hash
50+ * @param[in] pindex The blockindex
why are you passing the hash twice? the blockindex already holds its own hash, no?
Also, pindex is not a pointer, so should be called block_index.
767+ {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
768+ {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"},
769+ },
770+ RPCResult{RPCResult::Type::OBJ, "", "",
771+ {
772+ {RPCResult::Type::STR, "warnings", "any warnings"}
780+ const NodeContext& node = EnsureAnyNodeContext(request.context);
781+ ChainstateManager& chainman = EnsureChainman(node);
782+ PeerManager& peerman = EnsurePeerman(node);
783+ CConnman& connman = EnsureConnman(node);
784+
785+ uint256 hash(ParseHashV(request.params[0], "hash"));
782+ PeerManager& peerman = EnsurePeerman(node);
783+ CConnman& connman = EnsureConnman(node);
784+
785+ uint256 hash(ParseHashV(request.params[0], "hash"));
786+
787+ const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
any reason to use a narrowing cast and not the safe alternative?
0 const NodeId nodeid{request.params[1].get_int64()};
786+
787+ const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
788+
789+ // Check that the peer with nodeid exists
790+ if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
791+ throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
756@@ -803,6 +757,58 @@ static RPCHelpMan getmempoolentry()
757 };
758 }
759
760+static RPCHelpMan getblockfrompeer()
761+{
762+ return RPCHelpMan{"getblockfrompeer",
763+ "\nAttempt to fetch block from a given peer.\n"
