rpc: getblockfrompeer followups #23706

pull Sjors wants to merge 7 commits into bitcoin:master from Sjors:2021/12/getblockfrompeer_followups changing 7 files +48 −58
  1. Sjors commented at 1:28 pm on December 8, 2021: member
    Followups from #20295.
  2. DrahtBot added the label P2P on Dec 8, 2021
  3. DrahtBot added the label RPC/REST/ZMQ on Dec 8, 2021
  4. in test/functional/rpc_getblockfrompeer.py:59 in 8e12436823 outdated 
    55@@ -60,7 +56,7 @@ def run_test(self):
56         assert_raises_rpc_error(-1, "Block header missing", self.nodes[0].getblockfrompeer, "00" * 32, 0)
57 
58         self.log.info("Non-existent peer generates error")
59-        assert_raises_rpc_error(-1, f"Peer nodeid {peer_0_peer_1_id + 1} does not exist", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
60+        assert_raises_rpc_error(-1, f"Peer does not exist", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)
    MarcoFalke commented at 3:46 pm on December 8, 2021: 
    0        assert_raises_rpc_error(-1, "Peer does not exist", self.nodes[0].getblockfrompeer, short_tip, peer_0_peer_1_id + 1)

    typo in the last commit?

  5. in src/rpc/blockchain.cpp:784 in 8e12436823 outdated 
    780@@ -781,7 +781,8 @@ static RPCHelpMan getblockfrompeer()
781         "getblockfrompeer",
782         "\nAttempt to fetch block from a given peer.\n"
783         "\nWe must have the header for this block, e.g. using submitheader.\n"
784-        "\nReturns {} if a block-request was successfully scheduled\n",
785+        "Calling this again for the same block and a new peer, will cause the response from the previous peer to be ignored.\n"
    jonatack commented at 3:49 pm on December 8, 2021:

    4c5505be omit the comma: s/peer,/peer/

    maybe:

    0        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
  6. in src/rpc/blockchain.cpp:804 in 8e12436823 outdated 
    808-
809-    // Check that the peer with nodeid exists
810-    if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
811-        throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
812-    }
813+    const uint256 hash(ParseHashV(request.params[0], "hash"));
    jonatack commented at 3:53 pm on December 8, 2021:

    737cee7

    0    const uint256& hash{ParseHashV(request.params[0], "hash")};
    fjahr commented at 9:38 pm on January 7, 2022:
    note to other reviewers: This is resolved in a later commit, could have been done here already probably.
  7. in src/rpc/blockchain.cpp:785 in 8e12436823 outdated 
    780@@ -781,7 +781,8 @@ static RPCHelpMan getblockfrompeer()
781         "getblockfrompeer",
782         "\nAttempt to fetch block from a given peer.\n"
783         "\nWe must have the header for this block, e.g. using submitheader.\n"
784-        "\nReturns {} if a block-request was successfully scheduled\n",
785+        "Calling this again for the same block and a new peer, will cause the response from the previous peer to be ignored.\n"
786+        "\nReturns {} if a block-request was successfully scheduled.\n",
    jonatack commented at 3:57 pm on December 8, 2021:

    4c5505be1438a8e0473adc7c96b5f681a9f81b02 maybe

    0        "\nReturns an empty JSON object if a block-request was successfully scheduled.\n",
  8. jonatack commented at 3:57 pm on December 8, 2021: member
    Approach ACK
  9. Sjors force-pushed on Dec 9, 2021
  10. Sjors force-pushed on Dec 9, 2021
  11. Sjors commented at 2:48 am on December 9, 2021: member
    Rebased and addresses @MarcoFalke’s and @jonatack’s comments.
  12. test: fancier Python for getblockfrompeer 
    Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    bfbf91d0b2
  13. in src/rpc/blockchain.cpp:785 in 49c18dacfd outdated 
    780@@ -781,7 +781,8 @@ static RPCHelpMan getblockfrompeer()
781         "getblockfrompeer",
782         "\nAttempt to fetch block from a given peer.\n"
783         "\nWe must have the header for this block, e.g. using submitheader.\n"
784-        "\nReturns {} if a block-request was successfully scheduled\n",
785+        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
786+        "\nReturns an empty JSON object if a block-request was successfully scheduled.\n",
    MarcoFalke commented at 8:22 am on December 9, 2021: 
    0        "\nReturns an empty JSON object if a block-request was successfully scheduled.",

    ;) after https://github.com/bitcoin/bitcoin/pull/23714

    Sjors commented at 8:53 am on December 9, 2021:
    Done, and rebased.
  14. Sjors force-pushed on Dec 9, 2021
  15. in src/net_processing.cpp:1459 in 00e23db610 outdated 
    1460-        LogPrint(BCLog::NET, "Requesting block %s from peer=%d\n",
1461-                 hash.ToString(), id);
1462-    } else {
1463-        RemoveBlockRequest(hash);
1464-        LogPrint(BCLog::NET, "Failed to request block %s from peer=%d\n",
1465+    if (!success) return "Node not fully connected";
    jonatack commented at 6:07 pm on December 9, 2021:
    Maybe add a test assertion for this in the functional test.
    Sjors commented at 2:23 am on December 10, 2021:
    I have no idea how to produce that scenario; IIUC there’s only a very brief interval between when a peer id is assigned and the handshake is done.
    jonatack commented at 9:21 am on December 10, 2021: 
    0    if (!success) return "Peer not fully connected";

    (a peer is more specific, as it is a node that is not our own)

  16. jonatack commented at 6:10 pm on December 9, 2021: member

    Lightly tested ACK 00e23db610d3ed206b8d34cf296540b594264732

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

     0$ ./src/bitcoin-cli -signet help getblockfrompeer
 1getblockfrompeer "blockhash" nodeid
 2
 3Attempt to fetch block from a given peer.
 4
 5We must have the header for this block, e.g. using submitheader.
 6Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.
 7
 8Returns an empty JSON object if a block-request was successfully scheduled.
 9
10Arguments:
111. blockhash    (string, required) The block hash
122. nodeid       (numeric, required) The node ID (see getpeerinfo for node IDs)
13
14Result:
15{                        (json object)
16  "warnings" : "str"     (string, optional) any warnings
17}
18
19Examples:
20> bitcoin-cli getblockfrompeer "00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09" 0
21> 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/
  17. in src/rpc/blockchain.cpp:788 in 00e23db610 outdated 
    780@@ -781,7 +781,8 @@ static RPCHelpMan getblockfrompeer()
781         "getblockfrompeer",
782         "\nAttempt to fetch block from a given peer.\n"
783         "\nWe must have the header for this block, e.g. using submitheader.\n"
784-        "\nReturns {} if a block-request was successfully scheduled\n",
785+        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
786+        "\nReturns an empty JSON object if a block-request was successfully scheduled.",
787         {
788             {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
789             {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"},
    jonatack commented at 9:20 am on December 10, 2021:

    maybe s/nodeid/peer_id/? the call is named getblockfrompeer, the help description uses “peer”, and snakecase is current rpc args naming style (doc/developer-notes.md::L1163), and a peer is more specific, as it is a node that is not our own

    0            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer ID (see getpeerinfo for node IDs)"},
    Sjors commented at 1:12 pm on December 10, 2021:
    Done, and also changed the variable names.
    jonatack commented at 1:26 pm on December 10, 2021:
    Nice. May need to update client.cpp as well: { "getblockfrompeer", 1, "nodeid" },
    Sjors commented at 2:30 am on December 11, 2021:
    Oops, fixed.
  18. MarcoFalke commented at 1:50 pm on December 10, 2021: member
    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')}
  19. Sjors force-pushed on Dec 11, 2021
  20. jonatack commented at 12:19 pm on December 11, 2021: member

    ACK 009ccf8d3b173b44b05a7784bb4d053f770b8619

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

     0$ ./src/bitcoin-cli -signet help getblockfrompeer 
 1getblockfrompeer "blockhash" peer_id
 2
 3Attempt to fetch block from a given peer.
 4
 5We must have the header for this block, e.g. using submitheader.
 6Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.
 7
 8Returns an empty JSON object if a block-request was successfully scheduled.
 9
10Arguments:
111. blockhash    (string, required) The block hash
122. peer_id      (numeric, required) The peer ID (see getpeerinfo for peer IDs)
13
14Result:
15{                        (json object)
16  "warnings" : "str"     (string, optional) any warnings
17}

    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)

  21. DrahtBot commented at 12:21 pm on December 11, 2021: member

    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.

  22. in src/rpc/blockchain.cpp:811 in 009ccf8d3b outdated 
    807-    const NodeId nodeid = static_cast<NodeId>(request.params[1].get_int64());
808-
809-    // Check that the peer with nodeid exists
810-    if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
811-        throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
812-    }
    luke-jr commented at 5:18 pm on December 12, 2021:
    Why are you removing this? Where is the peer id checked now?
    Sjors commented at 2:41 am on December 13, 2021:

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

    As suggested here: #20295 (review)

    luke-jr commented at 3:24 am on December 13, 2021:
    But this check was for the case where FetchBlock is not called (block already downloaded)
    Sjors commented at 4:10 am on December 13, 2021:
    I see. But we only return one error, and it seems to me that the “block already downloaded” error is more relevant anyway.
    luke-jr commented at 4:12 am on December 13, 2021:
    That’s currently just a warning. Maybe it should be an error?
    Sjors commented at 5:39 am on December 13, 2021:
    Good point, let’s just make it an error, so it’s easier to reason about.
    jonatack commented at 10:40 am on December 16, 2021:

    my previous test:

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

    now

    0$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0
1error code: -1
2error message:
3Block already downloaded
  23. in src/rpc/blockchain.cpp:784 in 009ccf8d3b outdated 
    780@@ -781,10 +781,11 @@ static RPCHelpMan getblockfrompeer()
781         "getblockfrompeer",
782         "\nAttempt to fetch block from a given peer.\n"
783         "\nWe must have the header for this block, e.g. using submitheader.\n"
784-        "\nReturns {} if a block-request was successfully scheduled\n",
785+        "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
    luke-jr commented at 5:25 pm on December 12, 2021:
    Nit: This change appears to be part of the wrong commit
    Sjors commented at 2:47 am on December 13, 2021:
    Moved.
  24. Sjors force-pushed on Dec 13, 2021
  25. in src/rpc/blockchain.cpp:801 in 45ed85e9a8 outdated 
    808-
809-    // Check that the peer with nodeid exists
810-    if (!connman.ForNode(nodeid, [](CNode* node) {return true;})) {
811-        throw JSONRPCError(RPC_MISC_ERROR, strprintf("Peer nodeid %d does not exist", nodeid));
812-    }
813+    const uint256& block_hash{ParseHashV(request.params[0], "block_hash")};
    Sjors commented at 2:54 am on December 13, 2021:
    @jonatack I also fixed the ParseHashV call while renaming
  26. Sjors force-pushed on Dec 13, 2021
  27. Sjors commented at 6:36 am on December 13, 2021: member
    @MarcoFalke I added a commit to allow the RPC result to be an empty JSON object (rather than null or no result).
  28. in src/rpc/util.cpp:850 in f76065f9bd outdated 
    845@@ -846,6 +846,10 @@ void RPCResult::ToSections(Sections& sections, const OuterType outer_type, const
846         sections.PushSection({indent + "}" + maybe_separator, ""});
847         return;
848     }
849+    case Type::EMPTY: {
850+        sections.PushSection({indent + maybe_key + "{}", Description("empty JSON object")});
    MarcoFalke commented at 8:43 am on December 13, 2021:
    why does this need a new case? Why can’t the above case be re-used?
    Sjors commented at 11:58 am on December 13, 2021:

    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.

    Sjors commented at 12:01 pm on December 13, 2021:
    I found a more compact solution…
    Sjors commented at 11:28 am on December 16, 2021:
    Reverting back to using a separate case. The string manipulation code Type:OBJ does not behave nicely if the object is empty, see #23706#pullrequestreview-833917359
  29. Sjors force-pushed on Dec 13, 2021
  30. luke-jr referenced this in commit b0f2ed40e6 on Dec 14, 2021
  31. luke-jr referenced this in commit 721ecb5841 on Dec 14, 2021
  32. in src/rpc/blockchain.cpp:790 in 86c9863a07 outdated 
    792         },
793-        RPCResult{RPCResult::Type::OBJ, "", "",
794-        {
795-            {RPCResult::Type::STR, "warnings", /*optional=*/true, "any warnings"},
796-        }},
797+        RPCResult{RPCResult::Type::EMPTY, "", /* optional */ false, "", {}},
    jonatack commented at 10:38 am on December 16, 2021:

    Is the help now missing a brace?

    previous reviews

    0Result:
1{                        (json object)
2  "warnings" : "str"     (string, optional) any warnings
3}

    now

    0Result:
1     (json object)
2}

    maybe this would be nice

    0Result:
1{
2     (empty json object)
3}
    jonatack commented at 10:38 am on December 16, 2021:

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

    0        RPCResult{RPCResult::Type::EMPTY, "", /*optional=*/ false, "", {}},
    Sjors commented at 11:02 am on December 16, 2021:
    I’ll bring back the braces and fix the nit
  33. in src/rpc/blockchain.cpp:788 in 86c9863a07 outdated 
    786+        "\nReturns an empty JSON object if the request was successfully scheduled.",
787         {
788-            {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
789-            {"nodeid", RPCArg::Type::NUM, RPCArg::Optional::NO, "The node ID (see getpeerinfo for node IDs)"},
790+            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
791+            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer ID (see getpeerinfo for peer IDs)"},
    jonatack commented at 10:39 am on December 16, 2021:

    2158b9f perhaps better descriptions

    0-            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
1-            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer ID (see getpeerinfo for peer IDs)"},
2+            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
3+            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"},
  34. Sjors force-pushed on Dec 16, 2021
  35. in src/rpc/util.h:246 in a7bd4aaf17 outdated 
    242@@ -243,6 +243,7 @@ struct RPCResult {
243         ARR_FIXED,  //!< Special array that has a fixed number of entries
244         NUM_TIME,   //!< Special numeric to denote unix epoch time
245         ELISION,    //!< Special type to denote elision (...)
246+        EMPTY,      //!< Special type to allow empty OBJ
    jonatack commented at 8:02 pm on December 16, 2021:

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

    0        EMPTY_OBJ,  //!< Special type to allow empty JSON object
    Sjors commented at 2:39 am on December 23, 2021:
    Maybe, @MarcoFalke?
    MarcoFalke commented at 11:20 am on December 24, 2021:

    OBJ_EMPTY :ballot_box:

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

    Sjors commented at 3:30 pm on December 24, 2021:
    OBJ_EMPTY it is!
  36. jonatack commented at 8:04 pm on December 16, 2021: member

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

     0$ ./src/bitcoin-cli -signet help getblockfrompeer 
 1getblockfrompeer "block_hash" peer_id
 2
 3Attempt to fetch block from a given peer.
 4
 5We must have the header for this block, e.g. using submitheader.
 6Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.
 7
 8Returns an empty JSON object if the request was successfully scheduled.
 9
10Arguments:
111. block_hash    (string, required) The block hash to try to fetch
122. peer_id       (numeric, required) The peer to fetch it from (see getpeerinfo for peer IDs)
13
14Result:
15{}    (empty JSON object)

    0$ ./src/bitcoin-cli -signet getblockfrompeer 00000152031b708d8b29013d55aedd24e8db90af7fcf7f1e51881ad97cf0e9a4 0
1error code: -1
2error message:
3Block already downloaded
  37. MarcoFalke added this to the milestone 23.0 on Dec 24, 2021
  38. rpc: allow empty JSON object result 8d1a3e6498
  39. refactor: drop redundant hash argument from FetchBlock 0e3d7c5ee1
  40. rpc: clarify getblockfrompeer behavior when called multiple times 809d66bb65
  41. rpc: turn already downloaded into error in getblockfrompeer 60243cac72
  42. rpc: more detailed errors for getblockfrompeer 34d5399211
  43. rpc: use peer_id, block_hash for FetchBlock 923312fbf6
  44. Sjors force-pushed on Dec 24, 2021
  45. jonatack approved
  46. jonatack commented at 2:52 pm on January 7, 2022: member
    ACK 923312fbf6a89efde1739da0b7209694d4f892ba :package:
  47. fjahr commented at 9:58 pm on January 7, 2022: member

    tested ACK 923312fbf6a89efde1739da0b7209694d4f892ba

    Reviewed code, built and tested the rpc locally.

  48. MarcoFalke merged this on Jan 25, 2022
  49. MarcoFalke closed this on Jan 25, 2022
  50. MarcoFalke commented at 7:26 pm on January 25, 2022: member
    This broke the rpc docs: #24155 (comment)
  51. Sjors deleted the branch on Jan 25, 2022
  52. sidhujag referenced this in commit eb07514e5a on Jan 28, 2022
  53. Fabcien referenced this in commit 93ae2058ec on Dec 3, 2022
  54. Fabcien referenced this in commit 392507352a on Dec 3, 2022
  55. Fabcien referenced this in commit 46ff54fbde on Dec 3, 2022
  56. Fabcien referenced this in commit 9132c4dcfd on Dec 3, 2022
  57. DrahtBot locked this on Jan 25, 2023


Sjors MarcoFalke jonatack fjahr DrahtBot luke-jr

Labels
RPC/REST/ZMQ P2P

Milestone
23.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me