rpc: add getorphantxs #30793

pull tdb3 wants to merge 6 commits into bitcoin:master from tdb3:rpc_getorphantxs changing 13 files +290 −13
  1. tdb3 commented at 10:52 pm on September 2, 2024: contributor

    This PR adds a new hidden rpc, getorphantxs, that provides the caller with a list of orphan transactions. This rpc may be helpful when checking orphan behavior/scenarios (e.g. in tests like p2p_orphan_handling) or providing additional data for statistics/visualization.

     0getorphantxs ( verbosity )
     1
     2Shows transactions in the tx orphanage.
     3
     4EXPERIMENTAL warning: this call may be changed in future releases.
     5
     6Arguments:
     71. verbosity    (numeric, optional, default=0) 0 for an array of txids (may contain duplicates), 1 for an array of objects with tx details, and 2 for details from (1) and tx hex
     8
     9Result (for verbose = 0):
    10[           (json array)
    11  "hex",    (string) The transaction hash in hex
    12  ...
    13]
    14
    15Result (for verbose = 1):
    16[                          (json array)
    17  {                        (json object)
    18    "txid" : "hex",        (string) The transaction hash in hex
    19    "wtxid" : "hex",       (string) The transaction witness hash in hex
    20    "bytes" : n,           (numeric) The serialized transaction size in bytes
    21    "vsize" : n,           (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
    22    "weight" : n,          (numeric) The transaction weight as defined in BIP 141.
    23    "expiration" : xxx,    (numeric) The orphan expiration time expressed in UNIX epoch time
    24    "from" : [             (json array)
    25      n,                   (numeric) Peer ID
    26      ...
    27    ]
    28  },
    29  ...
    30]
    31
    32Result (for verbose = 2):
    33[                          (json array)
    34  {                        (json object)
    35    "txid" : "hex",        (string) The transaction hash in hex
    36    "wtxid" : "hex",       (string) The transaction witness hash in hex
    37    "bytes" : n,           (numeric) The serialized transaction size in bytes
    38    "vsize" : n,           (numeric) The virtual transaction size as defined in BIP 141. This is different from actual serialized size for witness transactions as witness data is discounted.
    39    "weight" : n,          (numeric) The transaction weight as defined in BIP 141.
    40    "expiration" : xxx,    (numeric) The orphan expiration time expressed in UNIX epoch time
    41    "from" : [             (json array)
    42      n,                   (numeric) Peer ID
    43      ...
    44    ],
    45    "hex" : "hex"          (string) The serialized, hex-encoded transaction data
    46  },
    47  ...
    48]
    49
    50Examples:
    51> bitcoin-cli getorphantxs 2
    52> curl --user myusername --data-binary '{"jsonrpc": "2.0", "id": "curltest", "method": "getorphantxs", "params": [2]}' -H 'content-type: application/json' http://127.0.0.1:8332/
    
     0$ build/src/bitcoin-cli getorphantxs 2
     1[
     2  {
     3    "txid": "50128aac5deab548228d74d846675ad4def91cd92453d81a2daa778df12a63f2",
     4    "wtxid": "bb61659336f59fcf23acb47c05dc4bbea63ab533a98c412f3a12cb813308d52c",
     5    "bytes": 133,
     6    "vsize": 104,
     7    "weight": 415,
     8    "expiration": 1725663854,
     9    "from": [
    10      1
    11    ],
    12    "hex": "020000000001010b992959eaa2018bbf31a4a3f9aa30896a8144dbd5cfaf263bf07c0845a3a6620000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
    13  },
    14  {
    15    "txid": "330bb7f701604a40ade20aa129e9a3eb8a7bf024e599084ca1026d3222b9f8a1",
    16    "wtxid": "b7651f7d4c1a40c4d01f6a1e43a121967091fa0f56bb460146c1c5c068e824f6",
    17    "bytes": 133,
    18    "vsize": 104,
    19    "weight": 415,
    20    "expiration": 1725663854,
    21    "from": [
    22      2
    23    ],
    24    "hex": "020000000001013600adfe41e0ebd2454838963d270916d2b47239c9eebb93a992b720d3589a080000000000000000000140fe042a010000002251202913b252fe537830f843bfdc5fa7d20ba48639a87c86ff837b92d083c55ad7c102015121c0000000000000000000000000000000000000000000000000000000000000000100000000"
    25  }
    26]
    
  2. DrahtBot commented at 10:52 pm on September 2, 2024: contributor

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

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK glozow, 0xB10C, theStack, brunoerg

    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:

    • #30893 (test: Introduce ensure helper by fjahr)
    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)
    • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    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.

  3. DrahtBot added the label RPC/REST/ZMQ on Sep 2, 2024
  4. tdb3 force-pushed on Sep 2, 2024
  5. DrahtBot added the label CI failed on Sep 2, 2024
  6. DrahtBot commented at 11:14 pm on September 2, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29583420190

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  7. andrewtoth commented at 11:14 pm on September 2, 2024: contributor
    Can you expand on the motivation behind exposing this please? Why would someone want to use such an RPC?
  8. tdb3 force-pushed on Sep 2, 2024
  9. tdb3 force-pushed on Sep 3, 2024
  10. tdb3 closed this on Sep 3, 2024

  11. in src/rpc/mempool.cpp:825 in 3544e291ba outdated
    820+        {},
    821+        {
    822+            RPCResult{"orphans",
    823+                RPCResult::Type::ARR, "", "",
    824+                {
    825+                    {RPCResult::Type::STR_HEX, "", "The transaction id"},
    


    glozow commented at 2:57 pm on September 3, 2024:
    The orphanage can contain duplicates by txid, so adding wtxid as well could be helpful. This could be a UniValue::VOBJ instead, with both fields? That would allow adding more fields in the future (perhaps with different verbosity levels), e.g. telling us the size or originator.

    tdb3 commented at 4:31 pm on September 3, 2024:

    Great suggestion, thanks. Updated to an array of objects, with each object containing txid and wtixd. Some fields below that might make sense to add (either now or in a follow up). Any others?

    • Expiration time
    • Size info
    • Fee info
    • Originator (what sort of info would we like to see here? address?)

    Alternatively, maybe could replace the wrapping array with an object, with each orphan being keyed by its wtixd?

    Verbosity options also make sense to me. Similar to getrawmempool, could show just txid/wtxid with low verbosity, and additional tx details for higher verbosity.


    glozow commented at 12:28 pm on September 4, 2024:

    For originator, I was thinking fromPeer (the NodeId). Currently each orphan tracks 1 sender, but hoping to have multiple in the future (#28031 if you’re curious).

    For this PR, I think it would suffice to take an approach similar to getrawmempool but with an integer verbosity. verbose=0: list of txids (it’s ok if there are duplicates, it should be rare anyway) verbose=1: list of objects with txid, wtxid, size, senders (a list of NodeIds which as of now has length 1), expiration

    I think a further verbose=2 could contain the transaction hex as well. There are more orphanage changes coming, which could add more fields. However I don’t want to hold things up with bikeshedding, so feel free to decide what you think the scope of this PR should be.


    tdb3 commented at 10:27 pm on September 6, 2024:
    Thanks. Updated with three verbosity levels (0, 1, and 2).
  12. in src/net_processing.cpp:1924 in 3544e291ba outdated
    1920@@ -1920,6 +1921,18 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c
    1921     return true;
    1922 }
    1923 
    1924+std::vector<CTransaction> PeerManagerImpl::GetOrphans()
    


    glozow commented at 2:58 pm on September 3, 2024:
    What’s wrong with a std::vector<CTransactionRef>? Doesn’t this need to make copies of the CTransaction objects?

    tdb3 commented at 4:27 pm on September 3, 2024:
    Right. Updated. Initially, the thought was to break clean from PeerManager/TxOrphanage, but CTransactionRef is a shared_ptr so could access the CTransaction even if it was dropped in TxOrphanage.
  13. glozow commented at 3:00 pm on September 3, 2024: member

    Concept ACK

    I once talked with @0xB10C about adding this, but never got around to it. I think it would be helpful for visualizing / trying out different orphan protection methods.

  14. tdb3 reopened this on Sep 3, 2024

  15. tdb3 force-pushed on Sep 3, 2024
  16. in src/rpc/mempool.cpp:819 in c6634e2ff8 outdated
    812@@ -812,6 +813,46 @@ static RPCHelpMan savemempool()
    813     };
    814 }
    815 
    816+static RPCHelpMan getorphantxs()
    817+{
    818+    return RPCHelpMan{"getorphantxs",
    819+        "\nShows transactions from the tx orphanage.\n",
    


    maflcko commented at 4:31 pm on September 3, 2024:
    Maybe this can be marked Experimental, so that the interface can easily be changed, given that the goal would mostly only be for testing/debugging? Also, the RPC could be hidden, like other similar RPC, for example estimaterawfee?

    tdb3 commented at 4:34 pm on September 3, 2024:
    Thank you. Both of those suggestions are reasonable to me. Will update soon.

    tdb3 commented at 0:53 am on September 4, 2024:
    Updated
  17. instagibbs commented at 8:48 pm on September 3, 2024: member
    this PR might be a good time to enhance test/functional/p2p_orphan_handling.py, reducing our reliance on log messages at least
  18. DrahtBot removed the label CI failed on Sep 3, 2024
  19. tdb3 force-pushed on Sep 4, 2024
  20. tdb3 commented at 0:53 am on September 4, 2024: contributor

    this PR might be a good time to enhance test/functional/p2p_orphan_handling.py, reducing our reliance on log messages at least

    Great idea. For now, a child branch was started (https://github.com/tdb3/bitcoin/tree/enhance_p2p_orphan_handling) that adds a commit (https://github.com/tdb3/bitcoin/commit/344ab30487235a2096d878894dc6f4e2d0d1ef65) to update p2p_orphan_handling.

    Since additional enhancements could be made on that branch (e.g. to check the orphanage when the debug log is silent), maybe we keep this PR scoped to the new RPC, then open a followup PR using the enhance_p2p_orphan_handling branch? This could simplify review of both PRs.

    Alternatively, we could add just the commit handling assert_debug_log reduction here, and the followup PR would further enhance p2p_orphan_handling.

  21. tdb3 commented at 0:56 am on September 4, 2024: contributor
    Updated to include an experimental warning and make the RPC hidden. Also added a convenience function for checking the orphanage in tests.
  22. 0xB10C commented at 8:43 am on September 4, 2024: contributor

    Could also look into adding more transaction details (e.g. orphan expiration time, and potentially other tx information similar to what’s provided in getrawmempool or getrawtransaction).

    I think it would be helpful for visualizing / trying out different orphan protection methods.

    Concept ACK

    I agree that this would be interesting. The the orphan pool is mostly a black box to me (and probably others too). To discuss improvements/changes, having a better visual and mental model would be helpful. Maybe even similar to getrawaddrman and https://addrman.observer.

  23. tdb3 commented at 11:29 am on September 4, 2024: contributor

    Concept ACK

    I agree that this would be interesting. The the orphan pool is mostly a black box to me (and probably others too). To discuss improvements/changes, having a better visual and mental model would be helpful. Maybe even similar to getrawaddrman and https://addrman.observer.

    Thanks. That web app is gorgeous! Any particular tx details you’d like to see added (e.g. orphan expiration, etc.)?

  24. tdb3 marked this as ready for review on Sep 4, 2024
  25. in src/rpc/mempool.cpp:847 in 64458e788a outdated
    842+    std::vector<CTransactionRef> orphanage = peerman.GetOrphans();
    843+
    844+    UniValue ret(UniValue::VARR);
    845+    for (auto const& orphan : orphanage) {
    846+        UniValue o(UniValue::VOBJ);
    847+        o.pushKV("txid", orphan->GetHash().ToString());
    


    instagibbs commented at 12:16 pm on September 4, 2024:

    I feel like the ability to fetch the whole transaction (set) optionally is useful, otherwise we just have a couple hashes of the transactions, no ability to estimate how many vbytes the orphanage is using, do any analysis on the data given?

    Maybe have an optional argument via txid/wtxid that fetches the full serialized transaction in hex?


    tdb3 commented at 12:19 pm on September 4, 2024:
    Agreed, will refine

    instagibbs commented at 12:26 pm on September 4, 2024:

    Follow-up PRs can probably add more interesting fields as well, such as entry time, announcer peerid, etc.

    I’ll try not to scope creep this PR too bad


    glozow commented at 12:32 pm on September 4, 2024:

    ability to estimate how many vbytes the orphanage is using

    A getorphanageinfo can be added for overall stats, perhaps after #28031 which adds size tracking.

  26. tdb3 force-pushed on Sep 6, 2024
  27. refactor: add OrphanTxBase for external use
    Enables external entities to obtain orphan
    information
    2f13462a73
  28. tdb3 force-pushed on Sep 6, 2024
  29. DrahtBot added the label CI failed on Sep 6, 2024
  30. DrahtBot commented at 10:35 pm on September 6, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29806202290

    Make sure to run all tests locally, according to the documentation.

    The failure may happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  31. tdb3 commented at 10:47 pm on September 6, 2024: contributor
    The push adds verbosity levels and more detailed information about the orphans.
  32. theStack commented at 3:16 am on September 7, 2024: contributor
    Concept ACK
  33. tdb3 commented at 5:51 pm on September 7, 2024: contributor

    Not sure if the previous releases CI failure (timeout) is related (https://cirrus-ci.com/task/6175438823751680). Nothing sticks out immediately to me, except for possibly the following (but maybe this is just a symptom of overall CI machine load?):

    0+ podman container rm --force --all
    1time="2024-09-07T09:55:54Z" level=warning msg="StopSignal SIGTERM failed to stop container ci_native_previous_releases in 10 seconds, resorting to SIGKILL"
    2ef7ef7197494aa565138bc0560b4937dd11a79ade74357aa89bbf111955959e0
    

    Also, the 32-bit CentOS CI barely passed (https://cirrus-ci.com/task/5310638300332032). I clocked compiling finishing in 85 minutes, with unit tests completing about 15 minutes later and functionals passing just shy of the 120-minute mark.

    Thoughts @maflcko?

  34. DrahtBot removed the label CI failed on Sep 9, 2024
  35. maflcko commented at 6:53 am on September 9, 2024: member
    It could be a ccache issue, but another look is needed, see https://github.com/bitcoin/bitcoin/issues/30851
  36. brunoerg commented at 9:43 pm on September 9, 2024: contributor
    Concept ACK
  37. in src/txorphanage.h:88 in 17a64c4e1c outdated
    80@@ -81,6 +81,13 @@ class TxOrphanage {
    81         NodeSeconds nTimeExpire;
    82     };
    83 
    84+    /**
    85+     * Provides orphans
    86+     *
    87+     * @returns Orphans with metadata
    88+     */
    


    glozow commented at 0:06 am on September 18, 2024:
    Maybe self-explanatory :sweat_smile:

    tdb3 commented at 4:34 pm on September 18, 2024:
    Removed.
  38. in src/net_processing.cpp:522 in 17a64c4e1c outdated
    518@@ -519,6 +519,7 @@ class PeerManagerImpl final : public PeerManager
    519     std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
    520         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    521     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
    522+    std::vector<TxOrphanage::OrphanTxBase> GetOrphans() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
    


    glozow commented at 0:07 am on September 18, 2024:
    Perhaps GetOrphanTransactions, since this is on the peerman and orphan blocks can exist too

    tdb3 commented at 4:34 pm on September 18, 2024:
    Agreed. Updated.
  39. in src/rpc/mempool.cpp:823 in 4e7d2e4afa outdated
    814@@ -812,6 +815,104 @@ static RPCHelpMan savemempool()
    815     };
    816 }
    817 
    818+static std::vector<RPCResult> OrphanDescription()
    819+{
    820+    return {
    821+        RPCResult{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
    822+        RPCResult{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
    823+        RPCResult{RPCResult::Type::NUM, "size", "The serialized transaction size"},
    


    glozow commented at 0:09 am on September 18, 2024:
    maybe “bytes” instead? to make it hard to confuse with vsize

    tdb3 commented at 4:34 pm on September 18, 2024:

    “bytes” seems clearer to me as well. Updated.

    Originally “size” was chosen because it aligns with getrawtransaction. If there’s friction with using “bytes”, I can change it back to “size”.

    Argument and field naming: please consider whether there is already a naming style or spelling convention in the API for the type of object in question (blockhash, for example), and if so, try to use that

    https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#rpc-interface-guidelines

  40. in src/rpc/mempool.cpp:825 in 4e7d2e4afa outdated
    820+    return {
    821+        RPCResult{RPCResult::Type::STR_HEX, "txid", "The transaction hash in hex"},
    822+        RPCResult{RPCResult::Type::STR_HEX, "wtxid", "The transaction witness hash in hex"},
    823+        RPCResult{RPCResult::Type::NUM, "size", "The serialized transaction size"},
    824+        RPCResult{RPCResult::Type::NUM, "vsize", "The virtual transaction size (differs from size for witness transactions)"},
    825+        RPCResult{RPCResult::Type::NUM, "weight", "The transaction weight"},
    


    glozow commented at 0:09 am on September 18, 2024:
    mention BIP 141?

    tdb3 commented at 4:38 pm on September 18, 2024:
    Thanks. Increases clarity. Updated.
  41. in test/functional/rpc_getorphantxs.py:94 in f6dee50f85 outdated
    89+
    90+        self.log.info("Check that orphan 1 and 2 were from different peers")
    91+        assert orphanage[0]["from"][0] != orphanage[1]["from"][0]
    92+
    93+        self.log.info("Unorphan child 2")
    94+        peer_2.send_and_ping(msg_tx(tx_parent_2["tx"]))
    


    glozow commented at 0:19 am on September 18, 2024:
    Maybe should have a method for testing that a tx isn’t in orphanage so we can test that here

    tdb3 commented at 4:34 pm on September 18, 2024:
    Seemed reasonable to add a check assert not node.tx_in_orphanage(tx_child_2["tx"]) rather than a new method, but happy to be convinced otherwise.
  42. in test/functional/rpc_getorphantxs.py:100 in f6dee50f85 outdated
    93+        self.log.info("Unorphan child 2")
    94+        peer_2.send_and_ping(msg_tx(tx_parent_2["tx"]))
    95+
    96+        self.log.info("Checking orphan details (verbosity 1)")
    97+        orphanage = node.getorphantxs(1)
    98+        orphan_1 = orphanage[0]
    


    glozow commented at 0:21 am on September 18, 2024:
    I was briefly confused about ordering stability, and then realized this has length 1

    tdb3 commented at 4:34 pm on September 18, 2024:
    Thanks, yeah it’s a bit too implicit so I added assert_equal(len(node.getorphantxs()), 1) to make it clearer and more explicit.
  43. net: add GetOrphanTransactions() to PeerManager
    Updates PeerManager (and Impl) to provide
    orphans with metadata
    c409001d60
  44. refactor: move verbosity parsing to rpc/util
    Provides a common way for rpcs to obtain
    verbosity from an rpc parameter
    97578ba49a
  45. rpc: add getorphantxs
    Adds an rpc to obtain data about the
    transactions currently in the orphanage.
    Hidden and marked as experimental
    8ec094959d
  46. test: add tx_in_orphanage()
    Allows tests to check if a transaction
    is contained within the orphanage
    4c1f91748d
  47. test: add getorphantxs tests
    Adds functional tests for getorphantxs
    f83a4f6929
  48. tdb3 force-pushed on Sep 18, 2024
  49. tdb3 commented at 4:39 pm on September 18, 2024: contributor
    Thanks for the review @glozow! Updated to address comments, and made some clarity tweaks (e.g. use named argument in tests).

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-09-20 01:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me