rpc: getblockfrompeer followups #23706

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

Followups from #20295.

test/functional/rpc_getblockfrompeer.py

jonatack

Approach ACK

src/rpc/blockchain.cpp

Sjors · 2021-12-09T02:48:22Z

Rebased and addresses @MarcoFalke's and @jonatack's comments.

src/rpc/blockchain.cpp

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

jonatack

Lightly tested ACK 00e23db610d3ed206b8d34cf296540b594264732

$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0
{
  "warnings": "Block already downloaded"
}

$ ./src/bitcoin-cli -signet help getblockfrompeer
getblockfrompeer "blockhash" nodeid

Attempt to fetch block from a given peer.

We must have the header for this block, e.g. using submitheader.
Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.

Returns an empty JSON object if a block-request was successfully scheduled.

Arguments:
1. blockhash    (string, required) The block hash
2. nodeid       (numeric, required) The node ID (see getpeerinfo for node IDs)

Result:
{                        (json object)
  "warnings" : "str"     (string, optional) any warnings
}

Examples:
> bitcoin-cli getblockfrompeer "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09" 0
> curl --user myusername --data-binary '{"jsonrpc": "1.0", "id": "curltest", "method": "getblockfrompeer", "params": ["00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09" 0]}' -H 'content-type: text/plain;' http://127.0.0.1:8332/

src/net_processing.cpp

src/rpc/blockchain.cpp

maflcko · 2021-12-10T13:50:35Z

AssertionError: RPC client conversion table (/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-i686-pc-linux-gnu/src/rpc/client.cpp) and RPC server named arguments mismatch! {('getblockfrompeer', 1, 'peer_id'), ('getblockfrompeer', 1, 'nodeid')}

jonatack · 2021-12-11T12:19:56Z

ACK 009ccf8d3b173b44b05a7784bb4d053f770b8619

$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0
{
  "warnings": "Block already downloaded"
}

$ ./src/bitcoin-cli -signet help getblockfrompeer 
getblockfrompeer "blockhash" peer_id

Attempt to fetch block from a given peer.

We must have the header for this block, e.g. using submitheader.
Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.

Returns an empty JSON object if a block-request was successfully scheduled.

Arguments:
1. blockhash    (string, required) The block hash
2. peer_id      (numeric, required) The peer ID (see getpeerinfo for peer IDs)

Result:
{                        (json object)
  "warnings" : "str"     (string, optional) any warnings
}

If you have to retouch, maybe s/a block-request/the request/ in the help and snakecase naming for the blockhash param, e.g. block_hash like peer_id (sorry for the incremental review, iteration works for me :D)

DrahtBot · 2021-12-11T12:21:22Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23927 (rpc: Pruning nodes can not fetch blocks before syncing past their height by fjahr)
#23813 (Add test and docs for getblockfrompeer by fjahr)
#22932 (Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main by jonatack)

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.

luke-jr · 2021-12-12T17:18:52Z

src/rpc/blockchain.cpp

Why are you removing this? Where is the peer id checked now?

It's checked in FetchBlock:
https://github.com/Sjors/bitcoin/blob/009ccf8d3b173b44b05a7784bb4d053f770b8619/src/net_processing.cpp#L1437-L1439

As suggested here:
#20295 (comment)

But this check was for the case where FetchBlock is not called (block already downloaded)

I see. But we only return one error, and it seems to me that the "block already downloaded" error is more relevant anyway.

That's currently just a warning. Maybe it should be an error?

Good point, let's just make it an error, so it's easier to reason about.

my previous test:

$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0 { "warnings": "Block already downloaded" }

now

$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0 error code: -1 error message: Block already downloaded

luke-jr · 2021-12-12T17:25:05Z

src/rpc/blockchain.cpp

Nit: This change appears to be part of the wrong commit

Sjors · 2021-12-13T02:54:51Z

src/rpc/blockchain.cpp

@jonatack I also fixed the ParseHashV call while renaming

Sjors · 2021-12-13T06:36:06Z

@MarcoFalke I added a commit to allow the RPC result to be an empty JSON object (rather than null or no result).

maflcko · 2021-12-13T08:43:41Z

src/rpc/util.cpp

why does this need a new case? Why can't the above case be re-used?

Because of CHECK_NONFATAL(!m_inner.empty());

The rest of the Type::OBJ code block doesn't seem to do anything useful for an empty object either, so it seemed more readable to just add a new case. But I could use an if statement instead.

I found a more compact solution...

Reverting back to using a separate case. The string manipulation code Type:OBJ does not behave nicely if the object is empty, see #23706 (review)

Github-Pull: bitcoin#23706 Partially-Rebased-From: 9d70f38f757fd540edf10eb392028867927856bf

…e changing to peer_id Github-Pull: bitcoin#23706 Partially-Rebased-From: 009ccf8d3b173b44b05a7784bb4d053f770b8619

jonatack · 2021-12-16T10:38:31Z

src/rpc/blockchain.cpp

Is the help now missing a brace?

previous reviews

Result: { (json object) "warnings" : "str" (string, optional) any warnings }

now

Result: (json object) }

maybe this would be nice

Result: { (empty json object) }

2158b9f (nit, seems new since my last review) format for clang-tidy verification of this named arg in the RPCResult constructor

Suggested change

RPCResult{RPCResult::Type::EMPTY, "", /* optional */ false, "", {}},

RPCResult{RPCResult::Type::EMPTY, "", /*optional=*/ false, "", {}},

I'll bring back the braces and fix the nit

src/rpc/blockchain.cpp

jonatack

Lightly tested diff-review ACK a7bd4aaf17fdbfc2cbc5080467b3d17977b8b6b5 per git diff 86c9863 a7bd4aa

$ ./src/bitcoin-cli -signet help getblockfrompeer 
getblockfrompeer "block_hash" peer_id

Attempt to fetch block from a given peer.

We must have the header for this block, e.g. using submitheader.
Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.

Returns an empty JSON object if the request was successfully scheduled.

Arguments:
1. block_hash    (string, required) The block hash to try to fetch
2. peer_id       (numeric, required) The peer to fetch it from (see getpeerinfo for peer IDs)

Result:
{}    (empty JSON object)

$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0
error code: -1
error message:
Block already downloaded

jonatack · 2021-12-16T20:02:57Z

src/rpc/util.h

pico-nit, wonder if EMPTY_OBJ would be clearer, as there are OBJ and OBJ_DYN enums (feel free to ignore)

Suggested change

EMPTY, //!< Special type to allow empty OBJ

EMPTY_OBJ, //!< Special type to allow empty JSON object

Maybe, @MarcoFalke?

OBJ_EMPTY 🗳️

Also, if you retouch, pls put it next to OBJ_DYN in the list

OBJ_EMPTY it is!

jonatack

ACK 923312f 📦

fjahr

tested ACK 923312f

Reviewed code, built and tested the rpc locally.

src/rpc/blockchain.cpp

maflcko · 2022-01-25T19:26:24Z

This broke the rpc docs: #24155 (comment)

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

Summary: > rpc: allow empty JSON object result bitcoin/bitcoin@8d1a3e6 > rpc: turn already downloaded into error in getblockfrompeer bitcoin/bitcoin@60243ca This is a partial backport of [[bitcoin/bitcoin#23706 | core#23706]] Depends on D12717 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12719

Summary: This is a partial backport of [[bitcoin/bitcoin#23706 | core#23706]] bitcoin/bitcoin@34d5399 Depends on D12719 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D12720

Summary: This concludes backport of [[bitcoin/bitcoin#23706 | core#23706]] and [[bitcoin/bitcoin#24806 | core#24806]] bitcoin/bitcoin@923312f Depends on D12720 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D12721

This was referenced Dec 8, 2021

rpc: getblockfrompeer #20295

Merged

doc: Add missing optional to getblockfrompeer #23702

Merged

DrahtBot added P2P RPC/REST/ZMQ labels Dec 8, 2021

maflcko reviewed Dec 8, 2021

View reviewed changes

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

jonatack reviewed Dec 8, 2021

View reviewed changes

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

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

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

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch 2 times, most recently from c0d3124 to 49c18da Compare December 9, 2021 02:48

maflcko reviewed Dec 9, 2021

View reviewed changes

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

test: fancier Python for getblockfrompeer

bfbf91d

Co-authored-by: MarcoFalke <falke.marco@gmail.com>

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from 49c18da to 00e23db Compare December 9, 2021 08:53

jonatack reviewed Dec 9, 2021

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

jonatack reviewed Dec 10, 2021

View reviewed changes

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

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from 0510ada to 009ccf8 Compare December 11, 2021 02:30

DrahtBot mentioned this pull request Dec 11, 2021

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

Closed

luke-jr reviewed Dec 12, 2021

View reviewed changes

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from 009ccf8 to 45ed85e Compare December 13, 2021 02:53

Sjors commented Dec 13, 2021

View reviewed changes

src/rpc/blockchain.cpp Outdated

Copy link

Member Author

Sjors Dec 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonatack I also fixed the ParseHashV call while renaming

jonatack reacted with thumbs up emoji

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from 45ed85e to d631b1d Compare December 13, 2021 06:35

maflcko reviewed Dec 13, 2021

View reviewed changes

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from d631b1d to 86c9863 Compare December 13, 2021 12:02

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021

rpc: clarify getblockfrompeer behavior when called multiple times

b0f2ed4

Github-Pull: bitcoin#23706 Partially-Rebased-From: 9d70f38f757fd540edf10eb392028867927856bf

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 14, 2021

RPC/blockchain: Forward compatibility with getblockfrompeer param nam…

721ecb5

…e changing to peer_id Github-Pull: bitcoin#23706 Partially-Rebased-From: 009ccf8d3b173b44b05a7784bb4d053f770b8619

jonatack reviewed Dec 16, 2021

View reviewed changes

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

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from 86c9863 to a7bd4aa Compare December 16, 2021 11:33

jonatack reviewed Dec 16, 2021

View reviewed changes

This was referenced Dec 16, 2021

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

Merged

Add test and docs for getblockfrompeer with pruning #23813

Merged

maflcko added this to the 23.0 milestone Dec 24, 2021

Sjors added 6 commits December 24, 2021 16:28

rpc: allow empty JSON object result

8d1a3e6

refactor: drop redundant hash argument from FetchBlock

0e3d7c5

rpc: clarify getblockfrompeer behavior when called multiple times

809d66b

rpc: turn already downloaded into error in getblockfrompeer

60243ca

rpc: more detailed errors for getblockfrompeer

34d5399

rpc: use peer_id, block_hash for FetchBlock

923312f

Sjors force-pushed the 2021/12/getblockfrompeer_followups branch from a7bd4aa to 923312f Compare December 24, 2021 15:29

DrahtBot mentioned this pull request Jan 1, 2022

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

Merged

jonatack approved these changes Jan 7, 2022

View reviewed changes

fjahr reviewed Jan 7, 2022

View reviewed changes

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

maflcko merged commit 39d9bbe into bitcoin:master Jan 25, 2022

Sjors deleted the 2021/12/getblockfrompeer_followups branch January 25, 2022 20:08

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jan 28, 2022

Merge bitcoin#23706: rpc: getblockfrompeer followups

eb07514

luke-jr mentioned this pull request Feb 9, 2022

RPC: Switch getblockfrompeer back to standard param names blockhash and nodeid #24294

Closed

bitcoin locked and limited conversation to collaborators Jan 25, 2023

	RPCResult{RPCResult::Type::EMPTY, "", /* optional */ false, "", {}},
	RPCResult{RPCResult::Type::EMPTY, "", /optional=/ false, "", {}},

	EMPTY, //!< Special type to allow empty OBJ
	EMPTY_OBJ, //!< Special type to allow empty JSON object

rpc: getblockfrompeer followups #23706

rpc: getblockfrompeer followups #23706

Uh oh!

Conversation

Sjors commented Dec 8, 2021

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors commented Dec 9, 2021

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

maflcko commented Dec 10, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Dec 13, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Sjors Dec 13, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Dec 13, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors Dec 13, 2021 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack Dec 16, 2021 • 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!

jonatack commented Dec 11, 2021 •

edited

Loading

DrahtBot commented Dec 11, 2021 •

edited

Loading

Sjors Dec 13, 2021 •

edited

Loading

Sjors Dec 13, 2021 •

edited

Loading

Sjors Dec 13, 2021 •

edited

Loading

jonatack Dec 16, 2021 •

edited

Loading

maflcko Dec 24, 2021 •

edited

Loading