rpc: warn that nodes ignore requests for old stale blocks

Sjors commented at 1:47 pm on February 1, 2022: member

Adds warning to RPC help that getblockfrompeer is of little use for stale blocks that are more than a month old.

This is an anti-fingerprinting measure. See BlockRequestAllowed in net_processing.

It’s been in Bitcoin Core since 2014, introduced in #2910 and later improved to not rely on checkpoints. Older and alternative clients might still serve these blocks, so not throwing an error.

Allowing whitelisted nodes to fetch these blocks anyway might be nice.

Sjors commented at 1:48 pm on February 1, 2022: member

(I noticed this when reviewing #24178)

in src/rpc/blockchain.cpp:796 in a40e2c4cc6 outdated

791@@ -792,7 +792,8 @@ static RPCHelpMan getblockfrompeer()
792         "getblockfrompeer",
793         "Attempt to fetch block from a given peer.\n\n"
794         "We must have the header for this block, e.g. using submitheader.\n"
795-        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n\n"
796+        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
797+        "Nodes generally ignore requests for a stale block that they never fully verified, or one that is more than a month old.\n\n"

maflcko commented at 2:19 pm on February 1, 2022:

This will also disconnect the peer, no? #20295 (review)

If so, I still think it should be mentioned.

maflcko commented at 2:19 pm on February 1, 2022:

0        "Peers generally ignore requests for a stale block that they never fully verified, or one that is more than a month old.\n\n"

Sjors commented at 2:22 pm on February 1, 2022:

No, it’s only ignored: https://github.com/bitcoin/bitcoin/blob/133f73e86bd7c3114263500be2fb5090dd76b4bc/src/net_processing.cpp#L1848-L1851

Sjors commented at 2:25 pm on February 1, 2022:

Oh you mean we’ll disconnect on our end. Let me see, that would be annoying…

maflcko commented at 2:29 pm on February 1, 2022:

Yes, when I checked at the time, we’d disconnect. But I haven’t tested or re-checked since then.

Sjors commented at 7:12 pm on February 1, 2022:

Afaik there’s only one place where we track which blocks we requested: mapBlocksInFlight. And afaik that map doesn’t get vacuum cleaned. So how would a non-response from a peer lead us to disconnect?

Disclaimer: net_processing is very hard to follow, so I probably missed something.

maflcko commented at 8:26 am on February 2, 2022:

0                LogPrintf("Timeout downloading block %s from peer=%d, disconnecting\n", queuedBlock.pindex->GetBlockHash().ToString(), pto->GetId());

Sjors commented at 10:07 am on February 2, 2022:

Ah yes, we’re also updating state->vBlocksInFlight, and that does get checked. I’ll take a closer look.

maflcko commented at 10:32 am on February 22, 2022:

Are you still working on this?

Sjors commented at 10:23 am on July 18, 2022:

Took a closer look. It indeed appears this would disconnect. Updated the doc, but haven’t tested it..

Sjors force-pushed on Feb 1, 2022

maflcko added the label Docs on Feb 1, 2022

DrahtBot commented at 10:36 am on February 6, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	fjahr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23813 (Add test and docs for getblockfrompeer with pruning by fjahr)

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.

laanwj commented at 8:39 am on April 15, 2022: member

I’m not sure about the state of this PR. What are the plans here? Is this a (sufficient) improvement to the documentation, should it be merged as-is, or do you want to do something else instead.

laanwj added the label Waiting for author on Apr 15, 2022

Sjors commented at 9:25 am on April 19, 2022: member

This fell of my radar. I still need to investigate this: #24226 (review)

Could use some hints from p2p gurus like @jnewbery. The “Waiting for author” tag is fine for now.

ajtowns commented at 8:38 pm on June 1, 2022: contributor

Maybe get the doc improved, and open an issue for the disconnection and fix that later?

Sjors commented at 10:16 am on July 18, 2022: member

I added the disconnect behavior to the RPC doc. Also added a more detailed explanation in net_processing.cpp.

Sjors force-pushed on Jul 18, 2022

maflcko removed the label Waiting for author on Aug 1, 2022

rpc: warn that nodes ignore requests for old stale blocks

This is an anti-fingerprinting measure. See BlockRequestAllowed in net_processing.

It has been around since 2014, but alternative clients might still serve these blocks.

See also: d8b4b49667f3eaf5ac16c218aaba2136ece907d8, 85da07a5a001a563488382435202b74a3e3e964a, a2be3b66b56bccc01dfa2fb992515ae56bbedd49, 3788a8479b4efd481f3e91419bcf347113375112

f39d9269eb

Sjors commented at 9:26 am on September 6, 2022: member

Rebased just in case.

Sjors force-pushed on Sep 6, 2022

Sjors commented at 11:56 am on November 29, 2022: member

@fjahr @luke-jr can you take a look?

fjahr commented at 3:42 pm on December 4, 2022: contributor

Code review ACK f39d9269ebbcffe70d02674c67c4f53783b63005

The improvement of the docs make sense and I agree that, if there is a way to solve this issue, it should happen in a follow-up since it will probably require more discussion.

CI failure looks unrelated, maybe re-run the task @Sjors ?

maflcko merged this on Dec 5, 2022

maflcko closed this on Dec 5, 2022

Sjors deleted the branch on Dec 5, 2022

sidhujag referenced this in commit 27034e80c6 on Dec 5, 2022

bitcoin locked this on Dec 5, 2023

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

Reviews

Conflicts