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. Sjors cross-referenced this on Dec 8, 2021 from issue rpc: getblockfrompeer by Sjors
  3. Sjors cross-referenced this on Dec 8, 2021 from issue doc: Add missing optional to getblockfrompeer by MarcoFalke
  4. DrahtBot added the label P2P on Dec 8, 2021
  5. DrahtBot added the label RPC/REST/ZMQ on Dec 8, 2021
  6. 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: 
            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?

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

            "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n"
  8. 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

        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.

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

            "\nReturns an empty JSON object if a block-request was successfully scheduled.\n",
  10. jonatack commented at 3:57 PM on December 8, 2021: contributor

    Approach ACK

  11. Sjors force-pushed on Dec 9, 2021
  12. Sjors force-pushed on Dec 9, 2021
  13. Sjors commented at 2:48 AM on December 9, 2021: member

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

  14. test: fancier Python for getblockfrompeer
    Co-authored-by: MarcoFalke <falke.marco@gmail.com>
    bfbf91d0b2
  15. 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: 
            "\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.

  16. Sjors force-pushed on Dec 9, 2021
  17. 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: 
        if (!success) return "Peer not fully connected";

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

  18. jonatack commented at 6:10 PM on December 9, 2021: contributor

    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/
  19. 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

                {"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.

  20. 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')}

  21. Sjors force-pushed on Dec 11, 2021
  22. jonatack commented at 12:19 PM on December 11, 2021: contributor

    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)

  23. DrahtBot commented at 12:21 PM on December 11, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  24. DrahtBot cross-referenced this on Dec 11, 2021 from issue rpc/wallet: add optional transaction(s) to getbalances by kallewoof
  25. 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:

    $ ./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
  26. 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.

  27. Sjors force-pushed on Dec 13, 2021
  28. 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

  29. Sjors force-pushed on Dec 13, 2021
  30. 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).

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

  32. Sjors force-pushed on Dec 13, 2021
  33. luke-jr referenced this in commit b0f2ed40e6 on Dec 14, 2021
  34. luke-jr referenced this in commit 721ecb5841 on Dec 14, 2021
  35. 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

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

    now

    Result:
     (json object)
}

    maybe this would be nice

    Result:
{
     (empty json object)
}
    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

            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

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

    -            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash"},
-            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer ID (see getpeerinfo for peer IDs)"},
+            {"block_hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"},
+            {"peer_id", RPCArg::Type::NUM, RPCArg::Optional::NO, "The peer to fetch it from (see getpeerinfo for peer IDs)"},
  37. Sjors force-pushed on Dec 16, 2021
  38. 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)

            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!

  39. jonatack commented at 8:04 PM on December 16, 2021: contributor

    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
  40. DrahtBot cross-referenced this on Dec 16, 2021 from issue Add CBlockIndex lock annotations, guard nStatus/nFile/nDataPos/nUndoPos by cs_main by jonatack
  41. DrahtBot cross-referenced this on Dec 18, 2021 from issue Add test and docs for getblockfrompeer with pruning by fjahr
  42. MarcoFalke added this to the milestone 23.0 on Dec 24, 2021
  43. rpc: allow empty JSON object result 8d1a3e6498
  44. refactor: drop redundant hash argument from FetchBlock 0e3d7c5ee1
  45. rpc: clarify getblockfrompeer behavior when called multiple times 809d66bb65
  46. rpc: turn already downloaded into error in getblockfrompeer 60243cac72
  47. rpc: more detailed errors for getblockfrompeer 34d5399211
  48. rpc: use peer_id, block_hash for FetchBlock 923312fbf6
  49. Sjors force-pushed on Dec 24, 2021
  50. DrahtBot cross-referenced this on Jan 1, 2022 from issue rpc: Pruning nodes can not fetch blocks before syncing past their height by fjahr
  51. jonatack approved
  52. jonatack commented at 2:52 PM on January 7, 2022: contributor

    ACK 923312fbf6a89efde1739da0b7209694d4f892ba :package:

  53. fjahr commented at 9:58 PM on January 7, 2022: contributor

    tested ACK 923312fbf6a89efde1739da0b7209694d4f892ba

    Reviewed code, built and tested the rpc locally.

  54. MarcoFalke merged this on Jan 25, 2022
  55. MarcoFalke closed this on Jan 25, 2022
  56. MarcoFalke commented at 7:26 PM on January 25, 2022: member

    This broke the rpc docs: #24155 (comment)

  57. Sjors deleted the branch on Jan 25, 2022
  58. sidhujag referenced this in commit eb07514e5a on Jan 28, 2022
  59. luke-jr cross-referenced this on Feb 9, 2022 from issue RPC: Switch getblockfrompeer back to standard param names blockhash and nodeid by luke-jr
  60. Fabcien referenced this in commit 93ae2058ec on Dec 3, 2022
  61. Fabcien referenced this in commit 392507352a on Dec 3, 2022
  62. Fabcien referenced this in commit 46ff54fbde on Dec 3, 2022
  63. Fabcien referenced this in commit 9132c4dcfd on Dec 3, 2022
  64. bitcoin locked this on Jan 25, 2023
Contributors
SjorsMarcoFalkejonatackfjahrDrahtBotluke-jr
Labels
RPC/REST/ZMQP2P

Milestone
23.0

Linked (view graph)
#20295 rpc: getblockfrompeer#23702 doc: Add missing optional to getblockfrompeer#23714 doc: Normalize RPC description whitespace#24155 doc: Fix rpc docs#24294 RPC: Switch getblockfrompeer back to standard param names blockhash and nodeid
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: 2026-05-20 08:53 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me