TxDownloadManager refactor followups #30820

1. 
   glozow commented at 12:53 pm on September 5, 2024: member

   Followups to #30110

   • #30110 (review) + a few more doc fixes
   • #30110 (review)
2. [refactor] add TxDownloadManager wrapping TxOrphanage, TxRequestTracker, and bloom filters

   This module is going to be responsible for managing everything related
   to transaction download, including txrequest, orphan transactions and
   package relay. It will be responsible for managing usage of the
   TxOrphanage and instructing PeerManager:
   - what tx or package-related messages to send to which peer
   - whether a tx or package-related message is allowed or useful
   - what transactions are available to try accepting to mempool
   
   Future commits will consolidate the interface and re-delegate
   interactions from PeerManager to TxDownloadManager.

   ecbbadf0f7
3. [refactor] move ValidationInterface functions to TxDownloadManager

   This is move-only.

   a201bb8a5b
4. [txdownload] add read-only reference to mempool

   This will become necessary in later commits that query mempool. We also
   introduce the TxDownloadOptions in this commit to make the later diff
   easier to review.

   b88b82fc11
5. [refactor] move AlreadyHaveTx to TxDownload

   This is move-only.
   Also delete external RecentConfirmedTransactionsFilter() access since it
   is no longer necessary.

   58d9f83079
6. [refactor] move peer (dis)connection logic to TxDownload

   The information stored in TxDownloadConnectionInfo isn't used until the
   next commit.

   f261f0731a
7. [refactor] rename maybe_add_extra_compact_tx to first_time_failure

   The usage of this bool will increase in scope in the next commit.
   For this commit, the value of this bool is accurate at each
   ProcessInvalidTx callsite:
   - ProcessOrphanTx -> this tx is an orphan i.e. has been rejected before
   - ProcessPackageResult -> 1p1c only, each transaction is either an
     orphan or in m_lazy_recent_rejects_reconsiderable
   - ProcessMessage -> tx was received over p2p and validated for the first
     time

   31b150c915
8. [p2p] don't log tx invs when in IBD

   These invs are ignored anyway, and this allows us to more easily move
   the inv handling to TxDownloadManager in the next commit.

   3bf6d430b1
9. [refactor] move tx inv/getdata handling to txdownload 51717791c7
10. [refactor] move notfound processing to txdownload c40cb85659
11. [refactor] move some definitions

    ProcessInvalidTx will return a PackageToValidate, so it needs to be
    defined afterward.

    c4b94f6b98
12. [refactor] move new orphan handling to ProcessInvalidTx b353f5e4b8
13. move Find1P1CPackage into ProcessInvalidTx 17a9ff4c3e
14. [refactor] ProcessInvalidTx logic to put peerman tasks at the end 5483f1639b
15. [refactor] move Find1P1CPackage to txdownload

    Move-only.

    ec242e7b49
16. [refactor] move valid tx processing to TxDownload e93d7b8012
17. [refactor] move invalid tx processing to TxDownload

    Move-only. Also delete external RecentRejectsFilter() access since it is
    no longer necessary.

    77c5b80374
18. [refactor] move invalid package processing to TxDownload b5b4c863f2
19. [refactor] move new tx logic to txdownload

    Also delete external RecentRejectsReconsiderableFilter() access since it
    is no longer necessary.

    f3cc632279
20. [refactor] make AlreadyHaveTx and Find1P1CPackage private to TxDownloadImpl 2bc5c4fdd6
21. [refactor] wrap {Have,Get}TxToReconsider in txdownload f8b2eda654
22. [refactor] add CheckIsEmpty and remove access to TxDownloadMan internals 715caad450
23. [p2p] don't process orphan if in recent rejects

    This should never happen normally, but just in case.

    d72f66a3fa
24. add TxDownloadOptions bool to make deterministic TxRequestTracker

    Forward this bool to the TxRequestTracker ctor. This is needed for
    stablity in TxDownloadManager fuzzers

    960274bd63
25. [fuzz] txdownloadman and txdownload_impl

    The txdownload_impl is similar but allows us to check specific
    invariants within its implementation. It will also change a lot more
    than the external interface (txdownloadman) will, so we will add more to
    this target later.

    2be76e800f
26. [unit test] MempoolRejectedTx 48bc095998
27. [fuzz] tx download succeeds as long as we have 1 good outbound 1d4e33e7f8
28. [doc] fix comments in txdownloadman_one_honest_peer b39426576e
29. remove unnecessary mutex acquisition in notfound loop

    Previously, we accessed m_txdownloadman within the loop. Now, we are
    just checking and making copies of a local variable.
    
    Co-authored-by: Sebastian Falbesoner <sebastian.falbesoner@gmail.com>

    3edae62886
30. glozow added the label Refactoring on Sep 5, 2024
31. glozow added the label Docs on Sep 5, 2024
32. 
    DrahtBot commented at 12:53 pm on September 5, 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:

    • #30572 (Halt processing of unrequested transactions v2 by ariard)
    • #30538 (Doc: add a comment referencing past vulnerability next to where it was fixed by darosior)
    • #30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
    • #29492 (refactor: Remove redundant definitions by Empact)

    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.

33. DrahtBot added the label CI failed on Sep 7, 2024
34. DrahtBot removed the label CI failed on Sep 8, 2024
35. 
    glozow commented at 0:34 am on September 17, 2024: member

    closing, squashed into #30110. will reopen if followups need to happen again.
36. glozow closed this on Sep 17, 2024

    ---

Contributors
glozow DrahtBot

Labels
Refactoring Docs