rpc: add getorphantxs #30793

pull tdb3 wants to merge 6 commits into bitcoin:master from tdb3:rpc_getorphantxs changing 14 files +294 −20
  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.

    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:

    • #29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
    • #28710 (Remove the legacy wallet and BDB dependency by achow101)

    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. tdb3 force-pushed on Sep 6, 2024
  28. DrahtBot added the label CI failed on Sep 6, 2024
  29. 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.

  30. tdb3 commented at 10:47 pm on September 6, 2024: contributor
    The push adds verbosity levels and more detailed information about the orphans.
  31. theStack commented at 3:16 am on September 7, 2024: contributor
    Concept ACK
  32. 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?

  33. DrahtBot removed the label CI failed on Sep 9, 2024
  34. 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
  35. brunoerg commented at 9:43 pm on September 9, 2024: contributor
    Concept ACK
  36. 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.
  37. 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.
  38. 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

  39. 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.
  40. in test/functional/rpc_getorphantxs.py:100 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.
  41. in test/functional/rpc_getorphantxs.py:106 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.
  42. tdb3 force-pushed on Sep 18, 2024
  43. 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).
  44. in src/rpc/mempool.cpp:885 in f83a4f6929 outdated
    880+        RPCExamples{
    881+            HelpExampleCli("getorphantxs", "2")
    882+            + HelpExampleRpc("getorphantxs", "2")
    883+        },
    884+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
    885+{
    


    hodlinator commented at 7:58 pm on September 26, 2024:
    My guess is that this weird indentation style is an artifact of refactorings not wanting to re-indent old lines. Should not apply to new code.

    tdb3 commented at 1:41 pm on September 29, 2024:

    I’m open to adjusting it. Are you thinking something like the style of the following? https://github.com/bitcoin/bitcoin/blob/f83a4f6929c2287bfd26756a03b096da9c974dfd/src/rpc/mempool.cpp#L971-L973

    The updates for other comments are queued up locally. Will push with changes for this comment


    hodlinator commented at 2:24 pm on October 1, 2024:
    Exactly like that, thanks!
  45. in test/functional/test_framework/test_node.py:850 in f83a4f6929 outdated
    843@@ -841,6 +844,10 @@ def bumpmocktime(self, seconds):
    844     def wait_until(self, test_function, timeout=60):
    845         return wait_until_helper_internal(test_function, timeout=timeout, timeout_factor=self.timeout_factor)
    846 
    847+    def tx_in_orphanage(self, tx: CTransaction) -> bool:
    848+        """Returns true if the transaction is in the orphanage."""
    849+        found = [o for o in self.getorphantxs(verbosity=1) if o["txid"] == tx.rehash() and o["wtxid"] == tx.getwtxid()]
    850+        return len(found) == 1
    


    hodlinator commented at 8:14 pm on September 26, 2024:
    Feels like TestNode itself shouldn’t have tx-level code. Maybe /test/functional/test_framework/blocktools.py would be slightly better? It has create_witness_tx(node, ...) and send_to_witness(use_p2wsh, node, utxo, ...).

    tdb3 commented at 4:03 pm on September 29, 2024:

    The original thought/rationale was that the function pertains to the orphanage, which is part of the node (although a node argument to the function could enable obtaining the same information). No file stands out as a perfect fit. Do you feel test_framework/mempool_util.py would be reasonable? It seems appropriate since the orphanage and mempool both deal with unconfirmed transactions and are somewhat related.

    Let me know and I can update either way.


    hodlinator commented at 2:30 pm on October 1, 2024:

    Ah, yeah, mempool_util.py looks like a better alternative than blocktools.py.

    test_node.py feels more like the high-level P2P stuff to me at least + lower level dynamic RPC invocation.


    tdb3 commented at 0:00 am on October 2, 2024:
    Moved to mempool_util.py
  46. in src/rpc/rawtransaction.cpp:341 in f83a4f6929 outdated
    345-            verbosity = request.params[1].get_bool();
    346-        } else {
    347-            verbosity = request.params[1].getInt<int>();
    348-        }
    349-    }
    350+    int verbosity{ParseVerbosity(request.params[1])};
    


    hodlinator commented at 8:21 pm on September 26, 2024:
    Could be applied to getblock in src/rpc/blockchain.cpp as well.

    tdb3 commented at 2:08 pm on September 29, 2024:
    Thanks! Will update

    tdb3 commented at 0:01 am on October 2, 2024:
    Updated
  47. in src/txorphanage.h:77 in f83a4f6929 outdated
    71@@ -72,11 +72,19 @@ class TxOrphanage {
    72         return m_orphans.size();
    73     }
    74 
    75-protected:
    76-    struct OrphanTx {
    77+    /**
    78+     * Allows providing orphan information externally
    79+     */
    


    hodlinator commented at 8:29 pm on September 26, 2024:

    nit: The other 15 comments in the file do not occupy the same ratio of vertical space.

    0    /** Allows providing orphan information externally */
    

    tdb3 commented at 3:06 pm on September 29, 2024:
    Yes, will update the comment to be more consistent
  48. in test/functional/rpc_getorphantxs.py:75 in f83a4f6929 outdated
    65+        assert tx_child_2["txid"] in raw_mempool
    66+        self.log.info("Check that the orphanage is empty")
    67+        assert_equal(len(node.getorphantxs()), 0)
    68+
    69+        self.log.info("Confirm the transactions (clears mempool)")
    70+        self.generate(node, 1)
    


    hodlinator commented at 9:01 pm on September 26, 2024:
    nit: Might be nice to check the mempool size is zero as a post-condition, even though it should be covered by other tests.

    tdb3 commented at 3:20 pm on September 29, 2024:
    Agreed, that’s more thorough. Will update

    tdb3 commented at 0:01 am on October 2, 2024:
    Added check
  49. hodlinator commented at 9:17 pm on September 26, 2024: contributor

    Concept ACK

    Feels like p2p_orphan_handling.py/test_same_txid_orphan() might benefit from some augmented checking using this new getorphantxs. It could verify that same-txids-differing-wtxid orphans are handled in the expected way, with the bogus one being removed after the one with the valid witness gets adopted.

  50. 0xB10C commented at 4:47 pm on September 27, 2024: contributor

    Played around with the getorphantxs 1 output a bit regarding visualization. You can now plug your own getorphantxs.json (verbosity >=1) into this demo tool https://observablehq.com/d/a481f4ced64b7975. Produces something like this based on the size and from-peers (color) of the orphans.

    image

  51. tdb3 commented at 1:36 pm on September 29, 2024: contributor

    Concept ACK

    Feels like p2p_orphan_handling.py/test_same_txid_orphan() might benefit from some augmented checking using this new getorphantxs. It could verify that same-txids-differing-wtxid orphans are handled in the expected way, with the bogus one being removed after the one with the valid witness gets adopted.

    Great idea. Agreed. A branch was started (https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2327703573) to capture changes to p2p_orphan_handling.py. Updates to p2p_orphan_handling.py are planned for a follow-up PR to keep this one scoped and easier to review.

  52. tdb3 commented at 4:06 pm on September 29, 2024: contributor
    Thanks for the review @hodlinator! Left some updates.
  53. tdb3 commented at 4:09 pm on September 29, 2024: contributor

    Played around with the getorphantxs 1 output a bit regarding visualization. You can now plug your own getorphantxs.json (verbosity >=1) into this demo tool https://observablehq.com/d/a481f4ced64b7975. Produces something like this based on the size and from-peers (color) of the orphans.

    This looks great!

  54. in src/txorphanage.h:84 in f83a4f6929 outdated
    81         CTransactionRef tx;
    82         NodeId fromPeer;
    83         NodeSeconds nTimeExpire;
    84+    };
    85+
    86+    std::vector<OrphanTxBase> GetOrphanTransactions();
    


    itornaza commented at 6:29 pm on September 29, 2024:
    Do you think defining this as a const function std::vector<OrphanTxBase> GetOrphanTransactions() const; makes sense? I do not think it changes any members of the TxOrphanage.

    tdb3 commented at 7:17 pm on September 29, 2024:
    Thanks! Will include this in next push.

    tdb3 commented at 0:02 am on October 2, 2024:
    Updated
  55. in src/txorphanage.cpp:281 in f83a4f6929 outdated
    276@@ -277,3 +277,13 @@ std::vector<std::pair<CTransactionRef, NodeId>> TxOrphanage::GetChildrenFromDiff
    277     }
    278     return children_found;
    279 }
    280+
    281+std::vector<TxOrphanage::OrphanTxBase> TxOrphanage::GetOrphanTransactions()
    


    itornaza commented at 6:32 pm on September 29, 2024:
    My comment in the function declaration at the txorphanage.h:84 applies here as well.

    tdb3 commented at 0:02 am on October 2, 2024:
    Updated
  56. itornaza approved
  57. itornaza commented at 6:39 pm on September 29, 2024: contributor

    Concept ACK.

    The changes look good to me and the added functionality looks even greater! Built and run the extended test harness and everything checks out. Left more like a question rather than a nit.

  58. tdb3 force-pushed on Oct 2, 2024
  59. tdb3 commented at 0:04 am on October 2, 2024: contributor
    Updated to e6853592361341c27103ed74b25470ac1e098d6d to address comments from @hodlinator and @itornaza
  60. refactor: add OrphanTxBase for external use
    Enables external entities to obtain orphan
    information
    91b65adff2
  61. net: add GetOrphanTransactions() to PeerManager
    Updates PeerManager (and Impl) to provide
    orphans with metadata
    532491faf1
  62. tdb3 force-pushed on Oct 2, 2024
  63. tdb3 commented at 2:17 am on October 2, 2024: contributor

    Pushed b6368fc285bf00b3033061dcd4e29298b227c6df (rebases on top of fc642c33ef28829eda0119a0fe39fd9bc4b84051)

    This seems like it could be related to PR #30684?

     0node0 2024-10-02T00:15:45.783638Z [init] [src/noui.cpp:57] [noui_InitMessage] init message: Verifying wallet(s) 
     1Error:  node0 2024-10-02T00:15:45.783653Z [init] [src/noui.cpp:31] [noui_ThreadSafeMessageBox] [error] Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets 
     2 node0 2024-10-02T00:15:45.783737Z [init] [src/httpserver.cpp:510] [InterruptHTTPServer] [http] Interrupting HTTP server 
     3 node0 2024-10-02T00:15:45.783748Z [init] [src/httprpc.cpp:379] [InterruptHTTPRPC] [rpc] Interrupting HTTP RPC server 
     4 node0 2024-10-02T00:15:45.783756Z [init] [src/rpc/server.cpp:291] [operator()] [rpc] Interrupting RPC 
     5 node0 2024-10-02T00:15:45.783770Z [init] [src/init.cpp:277] [Shutdown] Shutdown: In progress... 
     6 node0 2024-10-02T00:15:45.783782Z [shutoff] [src/httprpc.cpp:384] [StopHTTPRPC] [rpc] Stopping HTTP RPC server 
     7 node0 2024-10-02T00:15:45.783794Z [shutoff] [src/httpserver.cpp:772] [UnregisterHTTPHandler] [http] Unregistering HTTP handler for / (exactmatch 1) 
     8 node0 2024-10-02T00:15:45.783804Z [shutoff] [src/httpserver.cpp:772] [UnregisterHTTPHandler] [http] Unregistering HTTP handler for /wallet/ (exactmatch 0) 
     9 node0 2024-10-02T00:15:45.783810Z [shutoff] [src/rpc/server.cpp:303] [operator()] [rpc] Stopping RPC 
    10 node0 2024-10-02T00:15:45.783892Z [shutoff] [src/rpc/server.cpp:309] [operator()] [rpc] RPC stopped. 
    11 node0 2024-10-02T00:15:45.783904Z [shutoff] [src/httpserver.cpp:522] [StopHTTPServer] [http] Stopping HTTP server 
    12 node0 2024-10-02T00:15:45.783911Z [shutoff] [src/httpserver.cpp:524] [StopHTTPServer] [http] Waiting for HTTP worker threads to exit 
    13 node0 2024-10-02T00:15:45.784048Z [http] [src/httpserver.cpp:354] [ThreadHTTP] [http] Entering http event loop 
    14 node0 2024-10-02T00:15:45.784129Z [http] [src/httpserver.cpp:357] [ThreadHTTP] [http] Exited http event loop 
    15 test  2024-10-02T00:55:45.674000Z TestFramework (ERROR): Assertion failed 
    16                                   Traceback (most recent call last):
    17                                     File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 665, in assert_start_raises_init_error
    18                                       ret = self.process.wait(timeout=self.rpc_timeout)
    19                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    20                                     File "/opt/homebrew/Cellar/python@3.12/3.12.6/Frameworks/Python.framework/Versions/3.12/lib/python3.12/subprocess.py", line 1264, in wait
    21                                       return self._wait(timeout=timeout)
    22                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    23                                     File "/opt/homebrew/Cellar/python@3.12/3.12.6/Frameworks/Python.framework/Versions/3.12/lib/python3.12/subprocess.py", line 2045, in _wait
    24                                       raise TimeoutExpired(self.args, timeout)
    25                                   subprocess.TimeoutExpired: Command '['/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/src/bitcoind', '-datadir=/Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner__🏃_20241002_001101/feature_settings_11/node0', '-logtimemicros', '-debug', '-debugexclude=libevent', '-debugexclude=leveldb', '-debugexclude=rand', '-uacomment=testnode0', '-logthreadnames', '-logsourcelocations', '-loglevel=trace', '-v2transport=0', '-settings=/Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner__🏃_20241002_001101/feature_settings_11/node0/regtest/settings.json']' timed out after 2400 seconds
    26                                   During handling of the above exception, another exception occurred:
    27                                   Traceback (most recent call last):
    28                                     File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
    29                                       self.run_test()
    30                                     File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/feature_settings.py", line 110, in run_test
    31                                       self.test_wallet_settings(settings)
    32                                     File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/feature_settings.py", line 39, in test_wallet_settings
    33                                       node.assert_start_raises_init_error(expected_msg="Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets",
    34                                     File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 695, in assert_start_raises_init_error
    35                                       self._raise_assertion_error(assert_msg)
    36                                     File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_node.py", line 197, in _raise_assertion_error
    37                                       raise AssertionError(self._node_msg(msg))
    38                                   AssertionError: [node 0] bitcoind should have exited within 2400s with expected error Error: Invalid value detected for '-wallet' or '-nowallet'. '-wallet' requires a string value, while '-nowallet' accepts only '1' to disable all wallets
    39 test  2024-10-02T00:55:45.690000Z TestFramework (DEBUG): Closing down network thread 
    40 test  2024-10-02T00:55:45.894000Z TestFramework (INFO): Stopping nodes 
    41 test  2024-10-02T00:55:45.894000Z TestFramework (WARNING): Not cleaning up dir /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner__🏃_20241002_001101/feature_settings_11 
    42 test  2024-10-02T00:55:45.895000Z TestFramework (ERROR): Test failed. Test logging available at /Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner__🏃_20241002_001101/feature_settings_11/test_framework.log 
    43 test  2024-10-02T00:55:45.895000Z TestFramework (ERROR): 
    44 test  2024-10-02T00:55:45.896000Z TestFramework (ERROR): Hint: Call /Users/runner/work/bitcoin/bitcoin/test/functional/combine_logs.py '/Users/runner/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20241002_001101/feature_settings_11' to consolidate all logs 
    45 test  2024-10-02T00:55:45.897000Z TestFramework (ERROR): 
    46 test  2024-10-02T00:55:45.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 
    47 test  2024-10-02T00:55:45.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 
    48 test  2024-10-02T00:55:45.897000Z TestFramework (ERROR): 
    

    Diff for last comments addressed can be viewed with: git range-diff a74bdeea1b8e27b2335f0f7da78006e87ecfb235..f83a4f6929c2287bfd26756a03b096da9c974dfd fc642c33ef28829eda0119a0fe39fd9bc4b84051..b6368fc285bf00b3033061dcd4e29298b227c6df

  64. luisschwab commented at 2:59 am on October 2, 2024: contributor
    Concept ACK
  65. maflcko commented at 1:46 pm on October 2, 2024: member

    Please report the CI failure. Otherwise, it will be harder to track and fix it. This is explained in the log:

    0 test  2024-10-02T00:55:45.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 
    1 test  2024-10-02T00:55:45.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues 
    
  66. tdb3 commented at 4:16 pm on October 2, 2024: contributor

    Please report the CI failure. Issue #31019 opened

  67. in src/rpc/mempool.cpp:896 in b6368fc285 outdated
    891+
    892+            UniValue ret(UniValue::VARR);
    893+
    894+            if (verbosity <= 0) {
    895+                for (auto const& orphan : orphanage) {
    896+                    ret.push_back(orphan.tx->GetHash().ToString()); // txid
    


    glozow commented at 4:20 pm on October 2, 2024:
    nit: comment is unnecessary. Also my personal preference is to not put them on the same line

    tdb3 commented at 10:35 pm on October 2, 2024:
    Updated
  68. in src/rpc/util.h:110 in 7b64d2608f outdated
    105+ *
    106+ * @param[in] arg The verbosity argument as a bool (true) or int (0, 1, 2,...)
    107+ * @param[in] default_verbosity The value to return if verbosity argument is null
    108+ * @returns An integer describing the verbosity level (e.g. 0, 1, 2, etc.)
    109+ */
    110+int ParseVerbosity(const UniValue& arg, int default_verbosity = 0);
    


    glozow commented at 4:23 pm on October 2, 2024:

    I find default arguments to be pretty footgunny and prefer to require explicit. Since you’re introducing this function for the first time, I suggest no default arg.

    0int ParseVerbosity(const UniValue& arg, int default_verbosity);
    

    hodlinator commented at 8:20 pm on October 2, 2024:
    Also have a slight preference against default args in new code in general.

    tdb3 commented at 10:35 pm on October 2, 2024:
    Good point. Updated.
  69. in src/rpc/blockchain.cpp:769 in 7b64d2608f outdated
    772-            verbosity = request.params[1].get_bool() ? 1 : 0;
    773-        } else {
    774-            verbosity = request.params[1].getInt<int>();
    775-        }
    776-    }
    777+    int verbosity{ParseVerbosity(request.params[1], 1)};
    


    glozow commented at 4:24 pm on October 2, 2024:

    an annotation can help here

    0   int verbosity{ParseVerbosity(request.params[1], /*default_verbosity=*/1)};
    

    tdb3 commented at 10:35 pm on October 2, 2024:
    Agreed. Updated
  70. in src/rpc/mempool.cpp:894 in b6368fc285 outdated
    889+
    890+            int verbosity{ParseVerbosity(request.params[0])};
    891+
    892+            UniValue ret(UniValue::VARR);
    893+
    894+            if (verbosity <= 0) {
    


    glozow commented at 4:32 pm on October 2, 2024:
    It is a bit funky that a negative verbosity turns into 0! I was kind of expecting a JSONRPCError. I don’t mind and it looks like it’s the same for getblock, so not asking for a change, but could be worth a test case?

    tdb3 commented at 10:35 pm on October 2, 2024:
    Good point. Test added.
  71. glozow commented at 4:37 pm on October 2, 2024: member

    ACK b6368fc285b. I have some picky review comments if you’re interested, but I don’t feel that strongly on anything except the default argument one.

    I tested with @0xB10C’s visualizer as well, very cool!

  72. DrahtBot requested review from theStack on Oct 2, 2024
  73. DrahtBot requested review from itornaza on Oct 2, 2024
  74. DrahtBot requested review from 0xB10C on Oct 2, 2024
  75. DrahtBot requested review from brunoerg on Oct 2, 2024
  76. DrahtBot requested review from hodlinator on Oct 2, 2024
  77. hodlinator approved
  78. hodlinator commented at 8:24 pm on October 2, 2024: contributor

    ACK b6368fc285bf00b3033061dcd4e29298b227c6df

    Reviewed git range-diff master f83a4f6 b6368fc + “add tx_in_orphanage” commit + GitHub Files Changed.

    Thanks for addressing the latest feedback!

    build/test/functional/rpc_getorphantxs.py passed locally. Ran Doxygen and inspected ParseVerbosity docs to confirm [in]-parameters were handled well etc.

  79. refactor: move verbosity parsing to rpc/util
    Provides a common way for rpcs to obtain
    verbosity from an rpc parameter
    f511ff3654
  80. rpc: add getorphantxs
    Adds an rpc to obtain data about the
    transactions currently in the orphanage.
    Hidden and marked as experimental
    34a9c10e8c
  81. test: add tx_in_orphanage()
    Allows tests to check if a transaction
    is contained within the orphanage
    93f48fceb7
  82. test: add getorphantxs tests
    Adds functional tests for getorphantxs
    98c1536852
  83. tdb3 force-pushed on Oct 2, 2024
  84. tdb3 commented at 10:38 pm on October 2, 2024: contributor

    Updated to address comments from @glozow

    Removed default argument from ParseVerbosity(), cleaned up comments, and updated the functional test to check negative verbosity.

    0git range-diff fc642c33ef28829eda0119a0fe39fd9bc4b84051..b6368fc285bf00b3033061dcd4e29298b227c6df fc642c33ef28829eda0119a0fe39fd9bc4b84051..98c1536852d1de9a978b11046e7414e79ed40b46
    
  85. tdb3 commented at 10:39 pm on October 2, 2024: contributor

    Ran Doxygen and inspected ParseVerbosity docs to confirm [in]-parameters were handled well etc.

    Much appreciated

  86. glozow commented at 1:56 am on October 3, 2024: member
    reACK 98c1536852d
  87. DrahtBot requested review from hodlinator on Oct 3, 2024
  88. hodlinator approved
  89. hodlinator commented at 7:23 am on October 3, 2024: contributor

    re-ACK 98c1536852d1de9a978b11046e7414e79ed40b46

    git range-diff master b6368fc:

    • Removed default parameter value in ParseVerbosity and added /*default_verbosity=*/ at callsites.
    • Added test for verbosity=-1.
    • Removed “txid”-comment.
  90. in test/functional/rpc_getorphantxs.py:41 in 98c1536852
    36+
    37+        self.log.info("Check that neither parent is in the mempool")
    38+        assert_equal(node.getmempoolinfo()["size"], 0)
    39+
    40+        self.log.info("Check that both children are in the orphanage")
    41+
    


    danielabrozzoni commented at 11:54 am on October 3, 2024:
    nit: this blank line seems unnecessary

    tdb3 commented at 7:37 pm on October 3, 2024:
    True. I’ll leave this for a potential follow-up
  91. in test/functional/rpc_getorphantxs.py:45 in 98c1536852
    40+        self.log.info("Check that both children are in the orphanage")
    41+
    42+        orphanage = node.getorphantxs(verbosity=0)
    43+        self.log.info("Check the size of the orphanage")
    44+        assert_equal(len(orphanage), 2)
    45+        self.log.info("Check that negative verbosity is treated as 0")
    


    danielabrozzoni commented at 11:55 am on October 3, 2024:
    nit: we could also test that verbosity > 2 is treated as 2

    tdb3 commented at 7:38 pm on October 3, 2024:
    I’ll leave this one for follow-up as well.
  92. in src/rpc/mempool.cpp:1131 in 98c1536852
    1127@@ -1027,6 +1128,7 @@ void RegisterMempoolRPCCommands(CRPCTable& t)
    1128         {"blockchain", &getrawmempool},
    1129         {"blockchain", &importmempool},
    1130         {"blockchain", &savemempool},
    1131+        {"hidden", &getorphantxs},
    


    danielabrozzoni commented at 12:07 pm on October 3, 2024:
    nit: we could test that it’s hidden in the functional test, similarly to addpeeraddress/getrawaddrman (https://github.com/bitcoin/bitcoin/blob/cfb59da4b3bb34afae467691a3e901f2b5a186f3/test/functional/rpc_net.py#L338-L341)

    tdb3 commented at 7:47 pm on October 3, 2024:

    We seem to be limited/selective in testing for hidden RPCs in functional tests (i.e. there are more hidden RPCs than addpeeraddress and getrawaddrman). Maybe that sort of check would/could be a larger scope than just getorphantxs?

    I’m on the fence about testing that RPCs are hidden. Changing from hidden to another category would be noticed in a PR review, and having a functional test for it means that more code needs to be maintained.


    instagibbs commented at 2:09 pm on October 4, 2024:

    Maybe that sort of check would/could be a larger scope than just getorphantxs?

    Please don’t invalidate the ACKs for this, but personally find it useful as a reviewer to just enforce expected behavior in tests, even minor ones like this. It’s also documentation to the future reader as well; the functional tests are often the first place I look for intention of RPCs, without having to dig into git(hub) history to discover it.

  93. danielabrozzoni commented at 12:14 pm on October 3, 2024: contributor

    ACK 98c1536852d1de9a978b11046e7414e79ed40b46

    Code looks good to me, I have a few nits, but feel free to ignore them.

  94. pablomartin4btc commented at 6:09 pm on October 3, 2024: member

    tACK 98c1536852d1de9a978b11046e7414e79ed40b46

    Missed the club but went thru all of it, very interesting and useful. Please set “Review Club” label on the PR so other ppl can go there and learn more about it.

    Tested on mainnet. Not sure if would be use cases to add other arguments on a follow-up (count, filter by node id), in the meantime I used these queries:

    • ./build/src/bitcoin-cli -datadir=${AU_DATADIR} getorphantxs 2 | jq '.[] | select(.from | index(43))
    • ./build/src/bitcoin-cli -datadir=${AU_DATADIR} getorphantxs | jq 'length' 95
    • ./build/src/bitcoin-cli -datadir=${AU_DATADIR} getorphantxs 1 | jq '[.[] | {peer_id: .from[0]}] | group_by(.peer_id) | map({peer_id: .[0].peer_id, count: length})'
      0[
      1  {
      2    "peer_id": 43,
      3    "count": 95
      4  }
      5]
      

    The code looks very clean and neat.

    Also tested the treemap made by @0xB10C.

  95. tdb3 commented at 7:51 pm on October 3, 2024: contributor

    Thanks for the review @danielabrozzoni and @pablomartin4btc!

    Tested on mainnet. Not sure if would be use cases to add other arguments on a follow-up (count, filter by node id)

    Yes, those sound interesting for a follow-up or something like getorphanageinfo.

  96. itornaza approved
  97. itornaza commented at 8:23 am on October 4, 2024: contributor
    reACK 98c1536852d1de9a978b11046e7414e79ed40b46
  98. in src/rpc/mempool.cpp:856 in 34a9c10e8c outdated
    851+    return RPCHelpMan{"getorphantxs",
    852+        "\nShows transactions in the tx orphanage.\n"
    853+        "\nEXPERIMENTAL warning: this call may be changed in future releases.\n",
    854+        {
    855+            {"verbosity|verbose", RPCArg::Type::NUM, RPCArg::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",
    856+             RPCArgOptions{.skip_type_check = true}},
    


    davidgumberg commented at 8:24 pm on October 4, 2024:
    non-blocking out-of-scope: Might be worth it to either deprecate true/false values for verbosity arguments or introduce an argument type specific to verbosity arguments to avoid skipping type checks for them.

    tdb3 commented at 8:27 pm on October 4, 2024:
    I would support either, with the latter being my preference (at least initially/in transition)

    maflcko commented at 3:58 pm on October 7, 2024:
    Indeed, for new code, it would be good to not use the ambiguous bool fallback. Especially, given that it is undocumented?
  99. glozow added the label Review club on Oct 4, 2024
  100. glozow merged this on Oct 5, 2024
  101. glozow closed this on Oct 5, 2024

  102. rkrux commented at 9:39 am on October 22, 2024: none

    Post Merge tACK 98c1536852d1de9a978b11046e7414e79ed40b46

    Successful make and functional tests.

    I tried to test this RPC in my local by connecting 2 nodes in the regtest env. However, I couldn’t get an orphan transaction generated. I tried 2 ways:

    1. Set mempoolexpiry to 1hr in one node and broadcast the child transaction 15mins after the parent transaction from the other node. Initially I expected the parent transaction to get dropped after an hour but now it didn’t. My guess it’s because of the presence of atleast 1 descendant.
    2. Propagated both the transactions from the other node and deleted the mempool.dat file of the first node. Restarted the first node and broadcast the child transaction from the second node but the parent transaction was also implicitly broadcast to the first node.

    Is there any other easy way to generate orphans?


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-12-03 15:12 UTC

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