p2p: stop announcing txs with ancestors below fee filter #28533

1. 
   glozow commented at 9:56 am on September 26, 2023: member

   Our mempool may receive a CPFP of a transaction that is below our peer’s mempool minimum feerate. There are many possible reasons, but the most common is when our peer’s mempool has a smaller capacity and so their mempool minimum feerate is higher than ours. In the future, another likely reason is that we accept packages but they don’t (this change was originally part of package relay but seems beneficial today).

   Today, we broadcast this CPFP transaction to our peers (assuming its individual feerate is above the fee filter). Assuming they’re running Bitcoin Core, this happens:

   • Peer requests the child
   • We send the child
   • The peer sees this child as an orphan, stores it in orphanage, and requests the parent
   • We send the parent
   • The parent is rejected for being too low feerate

   This is a waste of bandwidth and computation. It also wastes the peer’s orphanage slots - instagibbs has been monitoring orphanage usage recently and noted that this is happening pretty frequently.

   When announcing a transaction, if it contains a mempool ancestor below the peer’s fee filter, drop it.

2. [test] announcement of CPFPs to feefiltered peers 1b03c05497
3. [txmempool] add GetMinimumAncestorFeerate and info_for_announcement

   Independent function and std::pair because we don't want to bloat the
   memory used for infoAll() and info() in other places that aren't
   interested in feerates of ancestor.

   f0eb8d8fbb
4. [p2p] don't inv tx with ancestor below feefilter

   Nodes waste bandwidth, computation, and orphanage space on transactions
   that CPFP below-min-feerate parents. Without package relay, this can
   arise when nodes have different mempool capacities.

   add174260d
5. glozow added the label P2P on Sep 26, 2023
6. 
   DrahtBot commented at 9:56 am on September 26, 2023: contributor

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

   Reviews

   See the guideline for information on the review process. A summary of reviews will appear here.

7. 
   ajtowns commented at 10:20 am on September 26, 2023: contributor

   There’s the case:

   • A was accepted into the mempool when fees were lower
   • B, spending A’s first output, has a higher fee, and CPFPs A sufficiently to keep it in the mempool
   • mempool minfee rises above A’s fee rate, but due to B, A is not evicted from the mempool
   • C, spending A’s second output, comes along CPFPing A at a fee rate high enough to get into the next block

   with this change, C won’t be relayed any longer, as I understand it. Orphan slots can already be wasted pretty trivially by an attacker, so is this really doing much good?

8. 
   glozow commented at 10:25 am on September 26, 2023: member

   Hm good point. Maybe we drop if there’s an ancestor below fee filter + not in their m_tx_inventory_known_filter?
9. DrahtBot added the label CI failed on Sep 26, 2023
10. 
    ajtowns commented at 1:14 pm on September 26, 2023: contributor

    Hm good point. Maybe we drop if there’s an ancestor below fee filter + not in their m_tx_inventory_known_filter?

    We could reply NOTFOUND if they are requesting a tx below their feefilter via GETDATA (rather than GETPKGTXNS)?

    I don’t really understand the point of worrying about special casing this, vs “just” getting package relay deployed?

11. 
    glozow commented at 1:29 pm on September 26, 2023: member

    Just the same rationale for why we have fee filters in the first place - I thought it’d be nice to not waste our/their bandwidth if they’ve told us they’re going to reject this kind of stuff anyway :shrug:. I don’t think it’s something to worry about, just thought an easy drop condition could be worthwhile. Sounds like no, so closing this.
12. glozow closed this on Sep 26, 2023

    ---

13. bitcoin locked this on Sep 25, 2024

Contributors
glozow DrahtBot ajtowns

Labels
P2P CI failed