rpc: getblockfrompeer #20295

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:

bitcoin-cli getblockfrompeer HASH peer_n

Closes #20155

Limitations:

• you have to specify which peer to fetch the block from
• the node must already have the header

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23497 (Add src/node/ and src/wallet/ code to node:: and wallet:: namespaces by ryanofsky)
• #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)

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

@Sjors

It can be difficult to find nodes that still have a stale block, so I'd rather hold on to it.

I agree, didn't consider it.

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:

• manually connect to new peers and retry; or
• add an argument with an array of IPs to try (but not keep as a peer); or
• go through peers.dat entries

This could get complicated, so a fresh RPC seemed like a more future proof approach.

I moved FetchBlock to the PeerManager while rebasing on #19910. Not sure if that's the right home for it, cc @jnewbery.

Does this work with the pruned node too? If yes, then this implements option 2) of #19312

It does, however I'm not sure how quickly the node re-prunes it.

Rebased. No longer needs to haul the mempool around.

Rebased after #21719

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 the EnsurePeerman and EnsureConnman helpers should be in their own file?

I guess by moving the Ensure* helpers to rpc/util (or a new module)?

I moved FetchBlock to the PeerManager while rebasing on #19910. Not sure if that's the right home for it, cc @jnewbery.

It's exactly the right place for it. PeerManager is the node's interface to net_processing.

Thanks for the feedback! I moved the Ensure* helpers to util.h and addressed the above comments.

Looking good (apart from the linker failure :slightly_smiling_face: )

I added server_util.cpp. This seems to make the linker happy.

Comments addressed.

The includes could be improved to include what's needed and not include what's not needed:

diff --git a/src/rpc/server_util.cpp b/src/rpc/server_util.cpp
index dfe07f5c3d..046a630b51 100644
--- a/src/rpc/server_util.cpp
+++ b/src/rpc/server_util.cpp
@@ -2,12 +2,18 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <rpc/server_util.h>
+
 #include <net_processing.h>
 #include <node/context.h>
-#include <rpc/server_util.h>
+#include <policy/fees.h>
+#include <rpc/protocol.h>
+#include <rpc/request.h>
+#include <txmempool.h>
 #include <util/system.h>
 #include <validation.h>
-#include <validationinterface.h>
+
+#include <any>
 
 NodeContext& EnsureAnyNodeContext(const std::any& context)
 {
diff --git a/src/rpc/server_util.h b/src/rpc/server_util.h
index c330efd0e6..dd8e8a6042 100644
--- a/src/rpc/server_util.h
+++ b/src/rpc/server_util.h
@@ -5,11 +5,11 @@
 #ifndef BITCOIN_RPC_SERVER_UTIL_H
 #define BITCOIN_RPC_SERVER_UTIL_H
 
-#include <rpc/protocol.h>
-#include <rpc/request.h>
+#include <any>
 
 class CBlockPolicyEstimator;
 class CConnman;
+class ChainstateManager;
 class CTxMemPool;
 struct NodeContext;
 class PeerManager;

Also done.

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...?

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.

Rebased, dropped requirement for local node to have the header, dropped redundant GetPeerRef, dropped requirement for other node to be at the right height.

utACK 803d2f568f7f24557543f61bf9d5a73952caedb5.

Changes look safe.

Is it really necessary to make this specify which peer? It doesn't sound too hard to make the existing block fetching logic support extra requested blocks, I think.

@sipa the current block fetching logic is buried deep inside net_processing. A MSG_BLOCK is generated when processing received headers:

https://github.com/bitcoin/bitcoin/blob/4741aec1dd28829f45abcc529cddaa0ff04d07a0/src/net_processing.cpp#L2011-L2021

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.

@Sjors Sure, but that doesn't sound too hard to add. And doing it that way sounds a lot more useful.

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.

@sipa we don't fetch a block if we already have (a valid) one for that height. So if you're trying retrieve the losing block from a race, most likely your peers also saw it later and didn't fetch it.

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)

Error: RPC command "getblockfrompeer" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp.

Rebased and marked safe for fuzzing (but I haven't tried that).

ACK 1df0fd9aceaeaf8b45cb0474ce44135418c7d5b7

Rebased due to silent merge conflict with #22141, which I randomly caught while looking at IRC notifications :-) But it's a welcome simplification.

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.

See also #10794

@MarcoFalke specifically its requestblock RPC? There's been too much refactoring to simply cherry-pick it, but it could contain inspiration for improvement.

Yes, just left the comment to remind myself that this has been worked on in the past already. No need to change anything here, but if someone wants to read some related threads, it might be helpful.

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().

ACK 30e2ba6c4cef9248c53542faeb8ec768079fc5fb Suggestions optional

What do you think about restricting this RPC to where the node already has the header, by returning in getblockfrompeer() if LookupBlockIndex() 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. <img width="1104" alt="Schermafbeelding 2021-08-12 om 13 49 18" src="https://user-images.githubusercontent.com/10217/129199510-c8bdfb75-95c5-43de-a2a6-d63a9b39b067.png">

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.

reACK 29263ae14a81d4cc55de61f51db981edf54399d0

Concept ACK

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:

1. Requesting a block without having the header: returns the error message Block header missing. This behavior is documented and expected :heavy_check_mark:.
2. Requesting a block from a peer that does not know about the block: 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 does not know about it :heavy_check_mark:.
3. Requesting a block from a peer that hasn't validated the block (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:.
4. Requesting a block from a peer that has only the block header (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:.
5. Requesting a block from a peer that has validated the block but it's not in the active chain (getchaintips status is valid-fork): Returns with {} and a getblock RPC call returns the block :heavy_check_mark:.
6. Requesting a block from a peer that has marked as 'invalid': Returns with {} 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 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.

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.

https://github.com/bitcoin/bitcoin/blob/29263ae14a81d4cc55de61f51db981edf54399d0/src/net_processing.cpp#L867-L868

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 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.

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 B in between the requests to peer A.

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?

Rebased after #22650 (trivial conflict with first commit).

reACK 4096e6fb013aa3fbe3463d24b70310ad8517da5f

Rebased once more (dd8f7f250095e58bbf4cd4effb481b52143bd1ed touched 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.

I'd love to see this merged in. 👍

ACK 9181e2e217955fff5f726b0e011afb78fb90af8f

Concept ACK, will review in the next few days.

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.

ACK dce8c4c38111556ca480aa0e63c46b71f66b508f

re-ACK dce8c4c38111556ca480aa0e63c46b71f66b508f

@MarcoFalke thanks, working on a followup PR...

could make sense to base on #23702 to avoid conflicts

Followups in #23706