rpc: getblockfrompeer #20295

Sjors · 2020-11-03T13:39:24Z

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

DrahtBot · 2020-11-03T17:47:09Z

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

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.

Fi3 · 2020-11-06T18:20:08Z

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

Sjors · 2020-11-11T11:06:48Z

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

jonathanbier · 2020-11-11T13:59:42Z

Concept ACK

This would be useful for https://forkmonitor.info/ to assess the validity of shorter chains

benthecarman · 2020-11-12T00:30:20Z

Concept ACK

This would be very useful for testing

Fi3 · 2020-11-13T12:37:46Z

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

luke-jr · 2020-11-13T18:09:54Z

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

Sjors · 2020-11-14T12:31:18Z

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

Sjors · 2020-12-09T16:15:12Z

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

src/rpc/blockchain.cpp

fjahr

tested ACK 9181e2e

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.

fjahr · 2021-11-27T20:19:31Z

src/rpc/blockchain.cpp

+    return RPCHelpMan{"getblockfrompeer",
+                "\nAttempt to fetch block from a given peer.\n"
+                "\nWe must have the header for this block, e.g. using submitheader.\n"
+                "\nReturns {} if a block-request was successfully scheduled\n",


I think it would be good to add more information for pruned node users of how long the block will be around. If my understanding is correct, something like this: Note: On a pruned node this block will be pruned again when the pruneheight surpasses the blockheight at the time of fetching the block.

Even that is not guaranteed at the moment (albeit very likely to hold in practice).

Combined with #19463, the caller could set a prune lock to ensure it doesn't get re-pruned before the caller is done with it.

So far I wasn't able to figure out in which cases this could not be guaranteed. I opened a follow-up PR in #23813. Please let me know if you have suggestions on improving the text there. Sure #19463 can be helpful but my aim with this for now is to just make sure that users don't get wrong expectations, like that the block would be available forever for example.

Co-authored-by: John Newbery <john@johnnewbery.com>

jnewbery · 2021-12-07T12:38:13Z

ACK dce8c4c

fjahr · 2021-12-07T21:05:57Z

re-ACK dce8c4c

test/functional/rpc_getblockfrompeer.py

maflcko · 2021-12-08T09:55:04Z

src/net_processing.cpp

@@ -1427,6 +1428,41 @@ 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)


What about returning the error string on failure std::optional<std::string>?

The caller could then

if(const auto err{Fetch()}){ throw err.value(); }

This will make users and tests more happy

maflcko · 2021-12-08T10:04:46Z

src/net_processing.cpp

+    if (!state->fHaveWitness) return false;
+
+    // Mark block as in-flight unless it already is
+    if (!BlockRequested(id, index)) return false;


maybe add a comment that this will re-assign the block if it is in flight from another peer?

I'll also update the RPC doc, because it looks like BLOCKTXN are ignored entirely if they're no longer on the in flight list for a given peer.

maflcko · 2021-12-08T10:07:38Z

src/net_processing.cpp

+                 hash.ToString(), id);
+    } else {
+        RemoveBlockRequest(hash);
+        LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",


Seems odd to log a failure to the debug log on an rpc call. I'd expect the error to go to the user instead. Also, shouldn't this say "peer not found"?

We already check that the peer exists above, so the failure would be "not fully connected"; changed...

src/net_processing.h

src/rpc/blockchain.cpp

maflcko · 2021-12-08T10:14:54Z

src/rpc/blockchain.cpp

+
+    // 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));


Since the FetchBlock call has to deal with this anyway, might be better to remove this code and let FetchBlock deal with it.

maflcko · 2021-12-08T10:28:38Z

src/rpc/blockchain.cpp

+static RPCHelpMan getblockfrompeer()
+{
+    return RPCHelpMan{"getblockfrompeer",
+                "\nAttempt to fetch block from a given peer.\n"


Should also mention that this will disconnect the peer if the peer pretends to not have the block

That seems a bit too deep in the net_processing weeds?

Sjors · 2021-12-08T11:29:11Z

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

maflcko · 2021-12-08T11:34:03Z

could make sense to base on #23702 to avoid conflicts

Sjors · 2021-12-08T13:28:16Z

Followups in #23706

36b9fe1 rpc: getblockfrompeer (Sjors Provoost) 4aa9484 rpc: move Ensure* helpers to server_util.h (Sjors Provoost) Pull request description: 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 ACKs for top commit: jnewbery: ACK 36b9fe1 fjahr: re-ACK 36b9fe1 Tree-SHA512: 843ba2b7a308f640770d624d0aa3265fdc5c6ea48e8db32269b96a082b7420f7953d1d8d1ef2e6529392c7172dded9d15639fbc9c24e7bfa5cfb79e13a5498c8

923312f rpc: use peer_id, block_hash for FetchBlock (Sjors Provoost) 34d5399 rpc: more detailed errors for getblockfrompeer (Sjors Provoost) 60243ca rpc: turn already downloaded into error in getblockfrompeer (Sjors Provoost) 809d66b rpc: clarify getblockfrompeer behavior when called multiple times (Sjors Provoost) 0e3d7c5 refactor: drop redundant hash argument from FetchBlock (Sjors Provoost) 8d1a3e6 rpc: allow empty JSON object result (Sjors Provoost) bfbf91d test: fancier Python for getblockfrompeer (Sjors Provoost) Pull request description: Followups from bitcoin#20295. ACKs for top commit: jonatack: ACK 923312f 📦 fjahr: tested ACK 923312f Tree-SHA512: da9eca76e302e249409c9d7f0d16cca668ed981e2ab6ca2d1743dad0d830b94b1bc5ffb9028a00764b863201945c273cc8f4409a4c9ca3817830007dffa2bc20

…he block header Github-Pull: bitcoin#20295 Rebased-From: d0b5374

Summary: This is a partial backport of [[bitcoin/bitcoin#20295 | core#20295]] bitcoin/bitcoin@b884aba Depends on D12700 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12701

Summary: 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`). Limitations: - you have to specify which peer to fetch the block from - the node must already have the header Co-authored-by: John Newbery <john@johnnewbery.com> This is a backport of [[bitcoin/bitcoin#20295 | core#20295]], [[bitcoin/bitcoin#23702 | core#23702]] and [[bitcoin/bitcoin#23706 | core#23706]] (partial) bitcoin/bitcoin@dce8c4c bitcoin/bitcoin@aaaa34e bitcoin/bitcoin@bfbf91d bitcoin/bitcoin@809d66b bitcoin/bitcoin@0e3d7c5 The main commit is the first one. The other commits are minor style & comments improvements, and one commit removing an unnecessary argument in the newly added `FetchBlock` function. Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12716

fanquake added RPC/REST/ZMQ P2P labels Nov 3, 2020

This was referenced Nov 3, 2020

net: compat.h related cleanup #20221

Merged

Per-Peer Message Capture #19509

Merged

net: Add NAT-PMP port forwarding support #18077

Merged

DONALDSULLENDER approved these changes Nov 3, 2020

View reviewed changes

Sjors force-pushed the 2020/11/getblockfrompeer branch from 1a192c4 to f74c19e Compare November 11, 2020 13:39

Sjors marked this pull request as ready for review November 11, 2020 13:53

DrahtBot mentioned this pull request Nov 12, 2020

net processing: Move peer_map to PeerManager #19910

Merged

bitcoin deleted a comment Nov 14, 2020

This was referenced Nov 16, 2020

Erlay: bandwidth-efficient transaction relay protocol #18261

Closed

rpc: Remove duplicate name and argNames from CRPCCommand #20012

Merged

DrahtBot mentioned this pull request Nov 26, 2020

Coinstats Index #19521

Merged

DrahtBot added the Needs rebase label Dec 9, 2020

Sjors force-pushed the 2020/11/getblockfrompeer branch from f74c19e to 513e38f Compare December 9, 2020 15:59

DrahtBot removed the Needs rebase label Dec 9, 2020

DrahtBot mentioned this pull request Dec 23, 2020

net-processing refactoring -- lose globals, move implementation details from .h to .cpp #20758

Closed

Sjors mentioned this pull request Dec 30, 2020

Add scanblocks RPC call #20664

Closed

DrahtBot mentioned this pull request Dec 31, 2020

refactor: move net_processing implementation details out of header #20811

Merged

DrahtBot added the Needs rebase label Jan 13, 2021

luke-jr reviewed Nov 22, 2021

View reviewed changes

src/rpc/blockchain.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 27, 2021

[WIP] DRAFT NOMERGE Tidy up RPCTxSerializationFlags #23599

Closed

fjahr reviewed Nov 27, 2021

View reviewed changes

Sjors force-pushed the 2020/11/getblockfrompeer branch 2 times, most recently from 434dbbc to 4edaf67 Compare December 2, 2021 06:05

Sjors and others added 2 commits December 2, 2021 13:15

rpc: move Ensure* helpers to server_util.h

b884aba

rpc: getblockfrompeer

dce8c4c

Co-authored-by: John Newbery <john@johnnewbery.com>

Sjors force-pushed the 2020/11/getblockfrompeer branch from 4edaf67 to dce8c4c Compare December 2, 2021 06:16

DrahtBot mentioned this pull request Dec 5, 2021

refactor: Nuke policy/fees->mempool circular dependencies #17786

Merged

maflcko merged commit f601326 into bitcoin:master Dec 8, 2021

Sjors deleted the 2020/11/getblockfrompeer branch December 8, 2021 09:45

maflcko reviewed Dec 8, 2021

View reviewed changes

Sjors mentioned this pull request Dec 8, 2021

rpc: getblockfrompeer followups #23706

Merged

fjahr mentioned this pull request Dec 18, 2021

Add test and docs for getblockfrompeer with pruning #23813

Merged

prusnak mentioned this pull request Jan 25, 2022

Feature: work with pruned node romanz/electrs#673

Open

maflcko mentioned this pull request Feb 1, 2022

rpc: warn that nodes ignore requests for old stale blocks #24226

Merged

prusnak mentioned this pull request Feb 7, 2022

pruneblockchain should be able to increase the size of pruned blockchain #24286

Closed

aureleoules mentioned this pull request Apr 29, 2022

rpc: Make pruneblockchain fetch old blocks if height is lower than pruned height #25026

Closed

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

RPC/blockchain: Allow using getblockfrompeer without already having t…

a01f191

…he block header Github-Pull: bitcoin#20295 Rebased-From: d0b5374

bitcoin locked and limited conversation to collaborators Jan 15, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpc: getblockfrompeer #20295

rpc: getblockfrompeer #20295

Sjors commented Nov 3, 2020 •

edited

Loading

DrahtBot commented Nov 3, 2020 •

edited

Loading

Fi3 commented Nov 6, 2020

Sjors commented Nov 11, 2020

jonathanbier commented Nov 11, 2020

benthecarman commented Nov 12, 2020

Fi3 commented Nov 13, 2020

luke-jr commented Nov 13, 2020

Sjors commented Nov 14, 2020

Sjors commented Dec 9, 2020

fjahr left a comment •

edited

Loading

fjahr Nov 27, 2021 •

edited

Loading

luke-jr Nov 27, 2021

fjahr Dec 18, 2021

jnewbery commented Dec 7, 2021

fjahr commented Dec 7, 2021

maflcko Dec 8, 2021

maflcko Dec 8, 2021

Sjors Dec 8, 2021

maflcko Dec 8, 2021

Sjors Dec 8, 2021

maflcko Dec 8, 2021

maflcko Dec 8, 2021

Sjors Dec 8, 2021

Sjors commented Dec 8, 2021

maflcko commented Dec 8, 2021

Sjors commented Dec 8, 2021

rpc: getblockfrompeer #20295

rpc: getblockfrompeer #20295

Conversation

Sjors commented Nov 3, 2020 • edited Loading

DrahtBot commented Nov 3, 2020 • edited Loading

Conflicts

Fi3 commented Nov 6, 2020

Sjors commented Nov 11, 2020

jonathanbier commented Nov 11, 2020

benthecarman commented Nov 12, 2020

Fi3 commented Nov 13, 2020

luke-jr commented Nov 13, 2020

Sjors commented Nov 14, 2020

Sjors commented Dec 9, 2020

fjahr left a comment • edited Loading

Choose a reason for hiding this comment

fjahr Nov 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnewbery commented Dec 7, 2021

fjahr commented Dec 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sjors commented Dec 8, 2021

maflcko commented Dec 8, 2021

Sjors commented Dec 8, 2021

Sjors commented Nov 3, 2020 •

edited

Loading

DrahtBot commented Nov 3, 2020 •

edited

Loading

fjahr left a comment •

edited

Loading

fjahr Nov 27, 2021 •

edited

Loading