Add test and docs for getblockfrompeer with pruning #23813

fjahr · 2021-12-18T11:55:45Z

These are additions to getblockfrompeer that I already suggested on the original PR.

The two commits do the following:

Add a test for getblockfrompeer usage on pruned nodes. This is important because many use-cases for getblockfrompeer are in a context of a pruned node.
Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely.

DrahtBot · 2021-12-18T16:24:48Z

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	stratospher, Sjors, brunoerg, MarcoFalke
Concept ACK	theStack
Stale ACK	jonatack

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

Conflicts

No conflicts as of last run.

theStack · 2021-12-18T17:31:42Z

Concept ACK

Sjors · 2021-12-20T02:39:23Z

Can you rebase this on #23706? Otherwise it might get confusing...

fjahr · 2021-12-20T21:16:05Z

Can you rebase this on #23706? Otherwise it might get confusing...

Done

test/functional/rpc_getblockfrompeer.py

brunoerg

tACK 809b2af

func logs

➜  bitcoin git:(fjahr) ✗ ./test/functional/rpc_getblockfrompeer.py
2021-12-22T10:51:49.578000Z TestFramework (INFO): Initializing test directory /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test__5i5gur3
2021-12-22T10:51:50.430000Z TestFramework (INFO): Mine 4 blocks on Node 0
2021-12-22T10:51:50.434000Z TestFramework (INFO): Mine competing 3 blocks on Node 1
2021-12-22T10:51:50.438000Z TestFramework (INFO): Connect nodes to sync headers
2021-12-22T10:51:50.499000Z TestFramework (INFO): Node 0 should only have the header for node 1's block 3
2021-12-22T10:51:50.500000Z TestFramework (INFO): Fetch block from node 1
2021-12-22T10:51:50.501000Z TestFramework (INFO): Arguments must be sensible
2021-12-22T10:51:50.501000Z TestFramework (INFO): We must already have the header
2021-12-22T10:51:50.502000Z TestFramework (INFO): Non-existent peer generates error
2021-12-22T10:51:50.503000Z TestFramework (INFO): Successful fetch
2021-12-22T10:51:50.504000Z TestFramework (INFO): Don't fetch blocks we already have
2021-12-22T10:51:50.505000Z TestFramework (INFO): Connect pruned node
2021-12-22T10:51:50.967000Z TestFramework (INFO): Fetch pruned block
2021-12-22T10:51:51.024000Z TestFramework (INFO): Stopping nodes
2021-12-22T10:51:51.515000Z TestFramework (INFO): Cleaning up /var/folders/7j/m0yjzmhj4ys9jgl353v2mqph0000gn/T/bitcoin_func_test__5i5gur3 on exit
2021-12-22T10:51:51.515000Z TestFramework (INFO): Tests successful

fjahr · 2022-01-25T19:56:58Z

Rebased since #23706 was merged

Sjors

6946327 looks good.

Do you want to expand the test to show how the fetched block disappears again at the next prune event?

Sjors · 2022-01-26T13:17:32Z

src/rpc/blockchain.cpp

@@ -793,7 +793,8 @@ static RPCHelpMan getblockfrompeer()
        "\nAttempt to fetch block from a given peer.\n"
        "\nWe must have the header for this block, e.g. using submitheader.\n"
        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
-        "\nReturns an empty JSON object if the request was successfully scheduled.",
+        "\nReturns an empty JSON object if the request was successfully scheduled."
+        "\nNote: On a pruned node this block might be pruned again as soon as the pruneheight surpasses the blockheight at the time of fetching the block.\n",


What block does "the block" refer to? The most recent block that's being fetched as part of the regular activity?

Maybe this can be a bit shorter: "Previously pruned blocks are only preserved until the next pruning event." (afaik this is true as long as the user doesn't change -prune)

"block" both times refers to the block being fetched with getblockfrompeer. I see how it was not super clear so I revised the wording a bit.

I think your shorter version is not necessarily correct. The block might be pruned with the next pruning event or not, it depends on the prune height requested if it is in a file that is otherwise ready to be pruned. So the block with the highest height in the same file is the deciding factor as far as I understand. Since the fetched blocked is usually appended in the file with the current tip, the next pruning event will often not allow that file to be pruned. The expanded test that you requested actually shows this well already and so I structured it in a way that this behavior is made explicit.

Ah I see. Then I think there should be a warning that this can temporarily cause pruning to be deferred? I.e. disk space may exceed -prune? Is there an upper limit of required extra free disk space that we can suggest?

Yes, if the node has not synced up to the fetched block yet, that can happen. That is the issue I am addressing with #23927 . I could amend the comment here now and then change it again if that PR is merged. But I think I would rather keep it separate since I also don't know what might be merged first. If the fix in #23927 is not getting in, I would change that PR to at least document the issue better for the user instead.

fjahr · 2022-01-29T19:12:15Z

Do you want to expand the test to show how the fetched block disappears again at the next prune event?

Expanded the test. See my comment for some more related info.

jonatack

ACK 6946327

Couple nits, happy to re-ACK if take.

src/rpc/blockchain.cpp

test/functional/rpc_getblockfrompeer.py

fjahr · 2022-02-06T00:27:27Z

Rebased and took @jonatack 's suggestions.

test/functional/rpc_getblockfrompeer.py

Sjors · 2022-07-18T09:51:04Z

Somewhat ominous CI failure.

fjahr · 2022-07-22T16:22:03Z

Somewhat ominous CI failure.

Seems to have been the CI choking but I also saw that there was a silent merge conflict after I re-ran, so rebased and fixed that.

Sjors

utACK da1bd8e

test/functional/rpc_getblockfrompeer.py

fjahr · 2022-11-13T19:21:01Z

rebased

src/rpc/blockchain.cpp

fjahr · 2022-12-22T19:02:55Z

Failed to address @Sjors ' comments on the previous rebase, done now.

stratospher

ACK fe329dc.

Went through the test and observed how:

200 + 4 + 400 = 1..604 blocks present in blk0.dat, blk1.dat, blk2.dat
pruneblockchain(300) removes blk0.dat and now 249..604 blocks in blk1.dat, blk2.dat
then a previously pruned block is stored in blk2.dat (using getblockfrompeer RPC)
604 + 250 = 249..854 blocks in blk1.dat, blk2.dat, blk3.dat (+ prev pruned block in blk2.dat)
pruneblockchain(700) removes blk1.dat. we still have blk2.dat, blk3.dat and so prev pruned block also.
854 + 250 = 500..1104 blocks in blk2.dat, blk3.dat, blk4.dat (+ prev pruned block in blk2.dat)
pruneblockchain(1000) removes blk2.dat and prev pruned block is no longer there

Sjors · 2023-03-09T17:10:54Z

re-utACK fe329dc

brunoerg

re-ACK fe329dc

maflcko · 2023-03-10T12:32:39Z

review ACK fe329dc 🍉

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK fe329dc936d1e02da406345e4223e11d1fa6fb38 🍉
u3CI1IY3BfxGVCvywNIp4h0mrUP/3/VdSmvbxT7Lm+Zq1O8vpmSbEHTjPOu+KM/2fiovMCWbbc44ZC0cYHe+BA==

fe329dc test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr) cd761e6 rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr) Pull request description: These are additions to `getblockfrompeer` that I already [suggested on the original PR](bitcoin#20295 (review)). The two commits do the following: 1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node. 2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely. ACKs for top commit: Sjors: re-utACK fe329dc MarcoFalke: review ACK fe329dc 🍉 stratospher: ACK fe329dc. brunoerg: re-ACK fe329dc Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac

fjahr mentioned this pull request Dec 18, 2021

rpc: getblockfrompeer #20295

Merged

laanwj added Docs RPC/REST/ZMQ labels Dec 18, 2021

DrahtBot mentioned this pull request Dec 18, 2021

rpc: getblockfrompeer followups #23706

Merged

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from fa03845 to 5ed8de7 Compare December 20, 2021 21:14

Sjors reviewed Dec 21, 2021

View reviewed changes

test/functional/rpc_getblockfrompeer.py Outdated Show resolved Hide resolved

This was referenced Dec 21, 2021

Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main #22932

Merged

rpc/wallet: add optional transaction(s) to getbalances #22776

Closed

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from 5ed8de7 to 809b2af Compare December 21, 2021 20:45

brunoerg approved these changes Dec 22, 2021

View reviewed changes

DrahtBot mentioned this pull request Jan 1, 2022

rpc: Pruning nodes can not fetch blocks before syncing past their height #23927

Merged

DrahtBot added the Needs rebase label Jan 25, 2022

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from 809b2af to 6946327 Compare January 25, 2022 19:56

DrahtBot removed the Needs rebase label Jan 25, 2022

maflcko changed the title ~~Add test and docs for getblockfrompeer~~ Add test and docs for getblockfrompeer with pruning Jan 26, 2022

Sjors reviewed Jan 26, 2022

View reviewed changes

DrahtBot mentioned this pull request Jan 30, 2022

doc: Fix typos pointed out by lint-spelling #24203

Merged

jonatack reviewed Jan 30, 2022

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

test/functional/rpc_getblockfrompeer.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Jan 31, 2022

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from 6946327 to 476f63a Compare February 6, 2022 00:27

DrahtBot removed the Needs rebase label Feb 6, 2022

DrahtBot mentioned this pull request Feb 6, 2022

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

Merged

theStack reviewed Mar 1, 2022

View reviewed changes

test/functional/rpc_getblockfrompeer.py Outdated Show resolved Hide resolved

DrahtBot removed the Needs rebase label Jun 6, 2022

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from 06d1947 to da1bd8e Compare July 22, 2022 09:08

Sjors approved these changes Sep 13, 2022

View reviewed changes

test/functional/rpc_getblockfrompeer.py Outdated Show resolved Hide resolved

test/functional/rpc_getblockfrompeer.py Outdated Show resolved Hide resolved

DrahtBot added the Needs rebase label Oct 26, 2022

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from da1bd8e to 23ea3e0 Compare November 13, 2022 18:39

DrahtBot removed the Needs rebase label Nov 13, 2022

luke-jr suggested changes Nov 15, 2022

View reviewed changes

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from 23ea3e0 to c9d2f75 Compare November 27, 2022 12:25

DrahtBot added the Needs rebase label Dec 5, 2022

rpc: Add note on guarantees to getblockfrompeer

cd761e6

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from c9d2f75 to 3679ae5 Compare December 19, 2022 22:24

DrahtBot removed the Needs rebase label Dec 20, 2022

test: Add test for getblockfrompeer on pruned nodes

fe329dc

fjahr force-pushed the 2021-12-getblockfrompeer-follow-up branch from 3679ae5 to fe329dc Compare December 22, 2022 19:02

fanquake requested a review from Sjors January 12, 2023 13:47

stratospher reviewed Mar 8, 2023

View reviewed changes

DrahtBot requested review from brunoerg and jonatack March 8, 2023 18:13

DrahtBot removed the request for review from Sjors March 9, 2023 17:11

brunoerg approved these changes Mar 10, 2023

View reviewed changes

fanquake merged commit 6e662a8 into bitcoin:master Mar 10, 2023

fjahr mentioned this pull request May 10, 2023

assumeutxo (2) #27596

Merged

bitcoin locked and limited conversation to collaborators Mar 9, 2024

Add test and docs for getblockfrompeer with pruning #23813

Add test and docs for getblockfrompeer with pruning #23813

Uh oh!

Conversation

fjahr commented Dec 18, 2021

Uh oh!

DrahtBot commented Dec 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

theStack commented Dec 18, 2021

Uh oh!

Sjors commented Dec 20, 2021

Uh oh!

fjahr commented Dec 20, 2021

Uh oh!

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jan 25, 2022

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 26, 2022

Choose a reason for hiding this comment

Uh oh!

fjahr Jan 29, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors Jan 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjahr Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

fjahr commented Jan 29, 2022

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjahr commented Feb 6, 2022

Uh oh!

Uh oh!

Sjors commented Jul 18, 2022

Uh oh!

fjahr commented Jul 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fjahr commented Nov 13, 2022

Uh oh!

Uh oh!

fjahr commented Dec 22, 2022

Uh oh!

stratospher left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Mar 9, 2023

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Mar 10, 2023

Uh oh!

Uh oh!

DrahtBot commented Dec 18, 2021 •

edited

Loading

Sjors Jan 30, 2022 •

edited

Loading

fjahr commented Jul 22, 2022 •

edited

Loading