p2p: Allow 1P1C to fetch parent from compact block extra_txn #30032

pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:2024-05-opportunistic-extratxn changing 4 files +80 −22
  1. instagibbs commented at 4:04 pm on May 3, 2024: member

    One relatively common pattern of 1P1C relay is receiving a just-below-minfee parent, dropping it and marking it as reconsiderable, then fetching it again from the peer once the child is added to the orphanage.

    A cache of dropped parents would be useful, and it turns out we’re already opportunistically storing transactions like this for compact blocks as “extra transactions”. Use this size-limited cache to potentially fetch a reconsiderable parent from memory, and submit for validation.

  2. DrahtBot commented at 4:04 pm on May 3, 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. A summary of reviews will appear here.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #15218 (validation: sync chainstate to disk after syncing to tip by andrewtoth)

    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 P2P on May 3, 2024
  4. instagibbs force-pushed on May 3, 2024
  5. p2p: Allow 1P1C to fetch parent from compact block extra_txn
    One relatively common pattern of 1P1C relay is receiving
    a just-below-minfee parent, dropping it and marking it
    as reconsiderable, then fetching it again from the peer
    once the child is added to the orphanage.
    
    A cache of dropped parents would be useful, and it turns
    out we're already opportunistically storing transactions
    like this for compact blocks as "extra transactions".
    Use this size-limited cache to potentially fetch a
    reconsiderable parent, and submit for validation.
    4fa5543c33
  6. instagibbs force-pushed on May 3, 2024
  7. DrahtBot added the label CI failed on May 3, 2024
  8. DrahtBot commented at 5:29 pm on May 3, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is 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.

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

    Debug: https://github.com/bitcoin/bitcoin/runs/24563145250

  9. in src/net_processing.cpp:1089 in 4fa5543c33
    1085 
    1086     /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction.
    1087      *  The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of
    1088      *  these are kept in a ring buffer */
    1089     std::vector<CTransactionRef> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
    1090+    std::vector<NodeId> m_extra_txn_senders GUARDED_BY(g_msgproc_mutex);
    


    glozow commented at 6:26 pm on May 3, 2024:
    needs comment
  10. in src/net_processing.cpp:1910 in 4fa5543c33
    1907+    for (size_t i = 0; i < vExtraTxnForCompact.size(); i++) {
    1908+        const auto& maybe_parent = vExtraTxnForCompact[i];
    1909+        // We stopped checking it earlier due to fee reasons, try again?
    1910+        if (maybe_parent && m_recent_rejects_reconsiderable.contains(maybe_parent->GetWitnessHash().ToUint256())) {
    1911+            Package maybe_cpfp_package{maybe_parent, orphan_child};
    1912+            if (IsChildWithParents(maybe_cpfp_package) && !m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
    


    glozow commented at 6:30 pm on May 3, 2024:
    Feels inefficient/expensive to construct a package + call these checks for every tx every time there’s a missing input. Maybe parameterize by the txid of the missing parent (i.e. just 1 exists) and first check that the txid matches?
  11. in test/functional/p2p_opportunistic_1p1c.py:63 in 4fa5543c33
    62-        ]]
    63+        if self.options.noextratxn:
    64+            self.extra_args = [[
    65+                "-datacarriersize=100000",
    66+                "-maxmempool=5",
    67+                "-blockreconstructionextratxn=0", # require fetching parent even if cached
    


    glozow commented at 6:30 pm on May 3, 2024:
    nice :brain:
  12. glozow commented at 6:30 pm on May 3, 2024: member
    Nice. Have you tried running this to see how often it hits?
  13. instagibbs commented at 6:34 pm on May 3, 2024: member

    Nice. Have you tried running this to see how often it hits?

    I’ll do some logging :+1: I suspect it’ll be quite above non-0 because from my log-reading just-below-minfee is quite commonly a reason for missing the parent.

    edit: this isn’t ready for prime-time, have to more carefully think about handling the results. This new case is triggered even if we would consider continuing on failure in various cases, so I likely have bugs here.

  14. instagibbs marked this as a draft on May 3, 2024
  15. DrahtBot removed the label CI failed on May 3, 2024
  16. instagibbs commented at 11:44 am on May 6, 2024: member
    from my non-listening node, looks like ~3% of the time of package evaluation is being considered it triggers
  17. instagibbs commented at 3:43 pm on May 6, 2024: member
    Don’t think I find these results compelling vs the complexity in how we would need to handle the packages
  18. instagibbs closed this on May 6, 2024


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

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