txgraph: deterministic optimal transaction order #34257

pull sipa wants to merge 10 commits into bitcoin:master from sipa:202601_det_mempool_order changing 20 files +846 −404
  1. sipa commented at 11:56 pm on January 11, 2026: member

    Part of #30289.

    TxGraph’s fundamental responsibility is deciding the order of transactions in the mempool. It relies on the cluster_linearize.h code to optimize it, but there can and often will be many different orderings that are essentially equivalent from a quality perspective, so we have to pick one. At a high level, the solution will involve one or more of:

    • Deciding based on internal identifiers (Cluster::m_sequence, DepGraphIndex). This is very simple, but risks leaking information about transaction receive order.
    • Deciding randomly, which is private, but may interfere with relay expectations, block propagation, and ability to monitor network behavior.
    • Deciding based on txid, which is private and deterministic, but risks incentivizing grinding to get an edge (though we haven’t really seen such behavior).
    • Deciding based on size (e.g. prefer smaller transactions), which is somewhat related to quality, but not unconditionally (depending on mempool layout, the ideal ordering might call for smaller transactions first, last, or anywhere in between). It’s also not a strong ordering as there can be many identically-sized transactions. However, if it were to encourage grinding behavior, incentivizing smaller transactions is probably not a bad thing.

    As of #32545, the current behavior is primarily picking randomly, though inconsistently, as some code paths also use internal identifiers and size. #33335 sought to change it to use random (preferring size in a few places), with the downsides listed above.

    This PR is an alternative to that, which changes the order to tie-break based on size everywhere possible, and use lowest-txid-first as final fallback. This is fully deterministic: for any given set of mempool transactions, if all linearized optimally, the transaction order exposed by TxGraph is deterministic.

    The transactions within a chunk are sorted according to:

    1. PostLinearize (which improves sub-chunk order), using an initial linearization created using the rules 2-5 below.
    2. Topology (parents before children).
    3. Individual transaction feerate (high to low)
    4. Individual transaction weight (small to large)
    5. Txid (low to high txid)

    The chunks within a cluster are sorted according to:

    1. Topology (chunks after their dependencies)
    2. Chunk feerate (high to low)
    3. Chunk weight (small to large)
    4. Max-txid (chunk with lowest maximum-txid first)

    The chunks across clusters are sorted according to:

    1. Feerate (high to low)
    2. Equal-feerate-chunk-prefix weight (small to large)
    3. Max-txid (chunk with lowest maximum-txid first)

    The equal-feerate-chunk-prefix weight of a chunk C is defined as the sum of the weights of all chunks in the same cluster as C, with the same feerate as C, up to and including C itself, in linearization order (but excluding such chunks that appear after C). This is a well-defined approximation of sorting chunks from small to large across clusters, while remaining consistent with intra-cluster linearization order.

  2. DrahtBot commented at 11:56 pm on January 11, 2026: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34257.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK ajtowns, instagibbs

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34138 (Cluster mempool: more accurate cost model for SFL by sipa)
    • #34023 (Optimized SFL cluster linearization by sipa)

    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 CI failed on Jan 12, 2026
  4. DrahtBot commented at 1:29 am on January 12, 2026: contributor

    🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer: https://github.com/bitcoin/bitcoin/actions/runs/20904012857/job/60054281786 LLM reason (✨ experimental): UndefinedBehaviorSanitizer detected an unsigned integer overflow in the fuzz target cluster_linearize.cpp, causing the CI failure.

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  5. sipa force-pushed on Jan 12, 2026
  6. sipa force-pushed on Jan 12, 2026
  7. DrahtBot removed the label CI failed on Jan 12, 2026
  8. sipa force-pushed on Jan 12, 2026
  9. sipa force-pushed on Jan 13, 2026
  10. sipa force-pushed on Jan 13, 2026
  11. DrahtBot added the label CI failed on Jan 13, 2026
  12. DrahtBot removed the label CI failed on Jan 13, 2026
  13. sipa force-pushed on Jan 14, 2026
  14. sipa commented at 11:00 pm on January 14, 2026: member
    I have a few changes to comments and tests. Most importantly, the optimal linearization unit tests now compare with an exact known-correct optimal linearization. To generate those, I have updated https://github.com/sipa/pyclusterlin to construct the same deterministic linearization order (if optimal).
  15. DrahtBot added the label Needs rebase on Jan 15, 2026
  16. sipa force-pushed on Jan 15, 2026
  17. sipa force-pushed on Jan 15, 2026
  18. sipa force-pushed on Jan 15, 2026
  19. DrahtBot added the label CI failed on Jan 15, 2026
  20. DrahtBot removed the label Needs rebase on Jan 15, 2026
  21. DrahtBot removed the label CI failed on Jan 15, 2026
  22. sipa commented at 3:07 am on January 16, 2026: member

    Both @darosior (here) and @ajtowns (here) suggested preferring larger size over smaller size instead. I agree that this is also fine from an incentives perspective, because nobody is going to deliberately construct larger transactions at the same feerate - that’s just paying more. If that is acceptable to you, you’re better off just paying a higher feerate.

    For ordering chunks across clusters, when combined with a greedy block building algorithm, larger first may have a slight advantage, because skipping a larger chunk still allows the smaller chunk to be considered. This does not work for chunks within a cluster, or transactions within a chunk. I don’t think it matters much, but this could be simulated if we care.

    Practically however, I don’t really see how to achieve this. For ordering chunks across clusters, we need some kind of function that maps chunks to a sorting key, with the property that it must map equal-feerate chunks within a cluster to increasing keys. The cross-cluster chunk ordering then boils down to sorting by increasing key. The PR here currently uses (equal-feerate-chunk-prefix size, maximum txid within cluster), which will approximately prioritize smaller chunks (but it’s a little murky when there are multiple equal-feerate chunks from the same cluster). I guess it’s possible to use a key like sum of inverses of the sizes of all equal-feerate chunks in the same cluster, but that seems extremely ugly and only very approximate anyway. EDIT: this is actually doable, see better idea below.

    For ordering chunks within a cluster, or (if we ever consider a tx-granularity based block building algorithm rather than chunk-granularity) ordering transactions within a chunk, it occurs to me that we really want uniformity - get the chunk/tx boundaries density as uniform as possible, because realistically, every byte position has equal probability of ending up being a block limit, and we want to minimize the gap. This actually calls for something (pseudo)random, or maybe alternating between smallest and largest.

    The only (very weak) argument in favor of smaller first that I know of, is that in a regime where mempools are reliably full, and transactions regularly get dropped/replaced, it may be better to have higher chunk/tx boundary density near the beginning, because the end has a non-negligible probability of never being mined at all, so it may pay off to deprioritize it.

    Overall, it is unlikely to matter I think, and having something is better than nothing. Remember that this is all just tie-breaking, which can’t really guarantee any objective quality - that comes through chunking and linearizations that maximize feerates.


    Re @ajtowns’s comment there about grinding: I think it really boils down to me having a (perhaps irrational) dislike of using txid or any other not-objectively-relevant criterion in the sort order. It feels arbitrary, and possibly brittle if there ever is an incentive for something else equally arbitrary. It’s setting a convention that at least in theory will have winners and losers, unlike feerate which is directly incentive-related, and even unlike size which at least intimately affects block building even if so in a rather chaotic way.

    So I like having a size-based criterion in there first, because it reduces the impact of txid on the order to places where we’re talking about equal-fee and equal-size transactions/chunks. But maybe it doesn’t really matter…

    Thoughts?

  23. gmaxwell commented at 8:21 am on January 16, 2026: contributor

    I also liked having neutral or positive criteria ahead of the arbitrary one– also making your txn smaller (at equal feerate) already increases your odds of being included because you’re more likely to successfully pack, and if doing so as a tie breaker causes people to grind or whatnot to further reduce their transaction sizes then that seems good to me. Those were the points that came to mind when I considered the alternative ‘wouldn’t larger be better’– though mostly in the sense of “prefer the transaction that pays the greater absolute fees, among transactions at the same feerate” which amounts to a prefer larger policy.

    I think a prior comment of mine pointed out that you could stick other criteria in ahead of the arbitrary one– Total bitcoin being spent? the old priority metric? net change in utxo set size? I don’t have strong opinions. These metrics, unlike grinding are kind of naturally self limiting. one only has so much bitcoin to spend, and so on.

  24. sipa commented at 4:24 pm on January 16, 2026: member

    The following approach could be used to get an approximate big-to-small chunk ordering.

    Rather than summing the sizes of all equal-feerate chunks in a given cluster, up to the one under consideration, sum all of them starting from the one under consideration. Then sort by that metric decreasing.

    It also doesn’t need consistency with intra-cluster chunk order, or intra-chunk tx order. Those could still be small-to-large, though the effect of such a suffix-size based inter-chunk order is probably larger (if it is anything at all) if it’s consistent with intra-cluster order.


    Adding additional criteria, after size (whether that’s small first, or large first) is actually not hard with the approach used in this PR, because the fallback tiebreak after feerate and size is abstracted behind a “fallback compare two transactions” function provided by the mempool. Currently, that function compares txids, but it could be changed to compare other things like input amounts, input count, output count, age of inputs, …

  25. sipa commented at 6:20 pm on January 20, 2026: member

    Happy to hear comments on what to do here, but I’m not convinced anything is needed right now.

    All of these things are easily and independently doable, though:

    • Switch from min-size to max size:
      • Use max-size-first inside chunk instead of min-size-first.
      • Use max-chunk-size-first for chunks within a cluster instead of min-chunk-size-first.
      • Use max-equal-feerate-suffix-size-first instead of min-equal-feerate-prefix-size-first for sorting chunks across clusters.
        • EDIT: maybe undesirable, it leads to the ability to “bump” chunks by attaching equal-feerate dependent chunks.
    • Add a tiebreak based on any easily-computable (or compactly-cachable) property of a mempool transaction:
      • Fewer outputs first (very cheap, but unlikely to matter if sizes are already equal)
      • More inputs first (very cheap, but unlike to matter if sizes are already equal)
      • Lowest sigops first (very cheap, unlikely to matter in general)
      • Oldest absolute locktime first (very cheap, unlikely to matter in general)
      • Lowest maximum spent UTXO creation height (would need caching, so adds 4-8 B memory per tx).
        • EDIT: hard to deal with unconfirmed inputs, which would mean transaction order can change when dependencies confirm.
      • Highest sum of UTXO value spent (would need caching, so adds 8 B memory per tx).
      • Lowest value-weighted average spent UTXO creation height (e.g. store 2^20 * sum(txin.height * txin.value) / sum(txin.value), which should fit in a uint64_t, adds 8 B memory per tx).
        • EDIT: hard to deal with unconfirmed inputs, which would mean transaction order can change when dependencies confirm.
  26. darosior commented at 6:50 pm on January 20, 2026: member

    Happy to hear comments on what to do here

    I don’t think there is anything to do here. I also didn’t mean my comment to indicate a preference for a maximum-size tie-breaker. I just read the thread as considering a size-based tie-breaker would necessarily pick smaller first, and couldn’t find a good answer for “why not higher absolute fees (larger size) instead?”. From the little understanding i have of cluster mempool i don’t think it matters and i don’t think it’s necessary to change this PR.

  27. sipa force-pushed on Feb 3, 2026
  28. fanquake commented at 4:08 pm on February 3, 2026: member

    Should fix IWYU (https://github.com/bitcoin/bitcoin/actions/runs/21637603999/job/62367547796?pr=34257):

    0--- a/src/primitives/transaction_identifier.h
    1+++ b/src/primitives/transaction_identifier.h
    2@@ -9,6 +9,7 @@
    3 #include <uint256.h>
    4 #include <util/types.h>
    5 
    6+#include <compare>
    7 #include <cstddef>
    
  29. sipa force-pushed on Feb 3, 2026
  30. DrahtBot added the label CI failed on Feb 3, 2026
  31. DrahtBot commented at 4:12 pm on February 3, 2026: contributor

    🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/21637603999/job/62367547796 LLM reason (✨ experimental): IWYU flagged and fixed an include problem in transaction_identifier.h, causing the CI test script to fail.

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  32. sipa commented at 4:13 pm on February 3, 2026: member

    I considered switching from smallest-equal-feerate-prefix to largest-equal-feerate-suffix, but realized it would mean someone can bump a chunk by attaching an equal-feerate dependent chunk. Not harmful, but it feels unintuitive and strange.

    I also considered adding a value-weighted average input height as tie-break before txid, but realized it’s annoying to deal with transactions with unconfirmed inputs, and the fact that their order might change when their dependencies confirm.

    So, no changes. The above push is just some comment improvements.

  33. sipa force-pushed on Feb 3, 2026
  34. DrahtBot removed the label CI failed on Feb 3, 2026
  35. sipa force-pushed on Feb 3, 2026
  36. fanquake added this to the milestone 31.0 on Feb 5, 2026
  37. instagibbs commented at 5:00 pm on February 5, 2026: member

    The order of chunks within a cluster is tie-broken using:

    Chunk feerate (high to low)

    for here and the inter-cluster context, what does this mean? if a chunk is higher feerate, it’ll necessary already be considered better and not need tiebreaking?

  38. sipa commented at 6:22 pm on February 5, 2026: member

    for here and the inter-cluster context, what does this mean? if a chunk is higher feerate, it’ll necessary already be considered better and not need tiebreaking?

    Yeah, it doesn’t mean anything; it’s arguably not a tie-breaking rule, but inherent to what chunks are. I just added here for completeness, because chunk feerate is the primary thing to sort by (obviously, or not).

  39. in src/txgraph.cpp:184 in 0ac92f06e7 outdated
    181@@ -182,8 +182,11 @@ class Cluster
    182     virtual int GetLevel(const TxGraphImpl& graph) const noexcept = 0;
    183     /** Only called by TxGraphImpl::SwapIndexes. */
    184     virtual void UpdateMapping(DepGraphIndex cluster_idx, GraphIndex graph_idx) noexcept = 0;
    


    instagibbs commented at 3:00 pm on February 6, 2026:

    0ac92f06e774be9c1c506e802baa1e3d57017ca8

    commit message referencing a yet-undefined “maximal-fallback” isn’t very helpful for understanding


    sipa commented at 8:33 pm on February 9, 2026:
    Rewritten to avoid using Language From The Future(tm).
  40. in src/txgraph.cpp:1091 in 0ac92f06e7 outdated
    1027     // OPTIMAL, compute its chunking and store its information in the Entry's m_main_lin_index
    1028     // and m_main_chunk_feerate. These fields are only accessed after making the entire graph
    1029     // ACCEPTABLE, so it is pointless to compute these if we haven't reached that quality level
    1030     // yet.
    1031-    if (level == 0 && IsAcceptable()) {
    1032+    if (level == 0 && (rename || IsAcceptable())) {
    


    instagibbs commented at 3:14 pm on February 6, 2026:

    0ac92f06e774be9c1c506e802baa1e3d57017ca8

    comment above is now out of date?


    sipa commented at 8:34 pm on February 9, 2026:
    I added a phrase to explain why it always runs when rename=true.
  41. in src/txgraph.cpp:2884 in fa86fb6ed2 outdated
    2804-                auto& chunk_data = *entry.m_main_chunkindex_iterator;
    2805-                if (m_done == m_depgraph.Positions() && chunk_pos == 1) {
    2806-                    assert(chunk_data.m_chunk_count == LinearizationIndex(-1));
    2807-                } else {
    2808-                    assert(chunk_data.m_chunk_count == chunk_pos);
    2809+            if (graph.m_main_clusterset.m_to_remove.empty()) {
    


    instagibbs commented at 3:49 pm on February 6, 2026:

    fa86fb6ed2c5f9a551e83bb7562c73dade6e551e

    IIUC now chunk data can be wiped but the actual transactions haven’t been removed from the graph, so now we have to wrap these checks to make sure removals have been applied.


    sipa commented at 7:29 pm on February 9, 2026:
    Right, exactly. We need to deal with a situation where there are to-be-applied removals still, in which case the chunk information may be outdated/meaningless. But this is not a problem either, because in this state the chunk information isn’t observable anyway.
  42. in src/test/fuzz/cluster_linearize.cpp:1104 in a31777c11c outdated
    1096+            // Go over all pairs of transactions. done is the set of transactions seen before pos1.
    1097+            for (unsigned pos1 = chunk_start; pos1 <= chunk_end; ++pos1) {
    1098+                auto tx1 = linearization[pos1];
    1099+                for (unsigned pos2 = pos1 + 1; pos2 <= chunk_end; ++pos2) {
    1100+                    auto tx2 = linearization[pos2];
    1101+                    if ((depgraph.Ancestors(tx2) - done).Count() == 1) {
    


    instagibbs commented at 4:13 pm on February 6, 2026:

    a31777c11c0cbb452d501f314190151c3ba2ad95

    note: done at this point does not include pos1, so it’s a check essentially it doesn’t depend on tx1. I found the accumulation loop a little subtle.


    sipa commented at 8:34 pm on February 9, 2026:
    I added some comments to explain the meaning.
  43. in src/txgraph.cpp:507 in fb6f3026ed outdated
    496@@ -497,6 +497,12 @@ class TxGraphImpl final : public TxGraph
    497         auto feerate_cmp = FeeRateCompare(entry_b.m_main_chunk_feerate, entry_a.m_main_chunk_feerate);
    498         if (feerate_cmp < 0) return std::strong_ordering::less;
    499         if (feerate_cmp > 0) return std::strong_ordering::greater;
    500+        // Compare equal-feerate chunk prefix size for comparing equal chunk feerates. This does two
    501+        // things: it catches equal-feerate chunks within the same cluster (because later ones will
    502+        // always have a higher prefix size), and it may catch equal-feerate chunks across clusters.
    503+        if (entry_a.m_main_equal_feerate_chunk_prefix_size != entry_b.m_main_equal_feerate_chunk_prefix_size) {
    


    instagibbs commented at 4:55 pm on February 6, 2026:

    fb6f3026edddd07e868ed52fe3fe4d77c9d40d1b

    note to self: The final tie-breaker in this function is still useful for intra-chunk calls, such as CompareMainOrder

    update: this was made clear in the final commit of the PR


    sipa commented at 8:34 pm on February 9, 2026:
    I’ve moved the phrasing used in the final commit earlier.
  44. in src/test/fuzz/txgraph.cpp:1194 in 5084095e5a
    1190+            std::shuffle(deps.begin(), deps.end(), rng);
    1191+            for (auto [parent, child] : deps) {
    1192+                real_redo->AddDependency(*txobjects_redo[parent], *txobjects_redo[child]);
    1193+            }
    1194+            // Do work to reach optimality.
    1195+            if (!real_redo->DoWork(300000)) {
    


    ajtowns commented at 10:45 pm on February 8, 2026:
    This test is inverted (and the code within the if isn’t actually executed for any of the fuzz corpus examples in qa-assets).

    sipa commented at 11:07 pm on February 8, 2026:
    Wow, good catch. Fixed, and verified it is executed with the fuzz corpus.
  45. sipa force-pushed on Feb 8, 2026
  46. in src/txgraph.h:78 in 7ad26dffc5 outdated
    76+     *  strictly positive. In all further calls, only Refs passed to AddTransaction() are allowed
    77      *  to be passed to this TxGraph object (or empty Ref objects). Ref objects may outlive the
    78-     *  TxGraph they were created for. */
    79-    [[nodiscard]] virtual Ref AddTransaction(const FeePerWeight& feerate) noexcept = 0;
    80+     *  TxGraph they were added to. */
    81+    virtual void AddTransaction(Ref& arg, const FeePerWeight& feerate) noexcept = 0;
    


    ajtowns commented at 2:06 am on February 9, 2026:
    Commit message for the first commit is missing a verb: “This does not when a Ref is”. “does not work” maybe?

    sipa commented at 8:35 pm on February 9, 2026:

    Oh no, I accidentally a word.

    Fixed.

  47. in src/txgraph.cpp:1731 in a173500850
    1727+        if (idx != m_entries.size() - 1) SwapIndexes(idx, m_entries.size() - 1, affected_main);
    1728         // Drop the entry for idx, now that it is at the end.
    1729         m_entries.pop_back();
    1730     }
    1731+
    1732+    // In a future commit, chunk information will end up containing a GraphIndex of the
    


    ajtowns commented at 2:38 am on February 9, 2026:

    Nit: Feels a bit like this could have been done in the future commit where it’s actually used, by:

    • renaming Updated to UpdatedOrCompacted and adding the bool rename param
    • adding an Updated() { return UpdateOrCompacted(/*rename=*/ false); } inline in the base class
    • updating Compact() to call UpdatedOrCompacted(/*rename=*/ true);

    sipa commented at 8:35 pm on February 9, 2026:
    Agreed, but not going to change it now.
  48. in src/test/fuzz/cluster_linearize.cpp:1086 in 7601196c2b outdated
    1081@@ -1082,6 +1082,53 @@ FUZZ_TARGET(clusterlin_linearize)
    1082         auto read_chunking = ChunkLinearization(depgraph, read);
    1083         auto cmp_read = CompareChunks(chunking, read_chunking);
    1084         assert(cmp_read >= 0);
    1085+
    1086+        // Verify that within every chunk, the transactions are in a valid order. For any pair of
    


    ajtowns commented at 2:56 am on February 9, 2026:
    The commit message here seems backwards: 1. Topology (chunks before chunks they depend on) should be chunks before chunks that depend on them, I think?

    sipa commented at 8:35 pm on February 9, 2026:
    Ha, yes, fixed.
  49. in src/cluster_linearize.h:1277 in 7601196c2b
    1272@@ -1267,16 +1273,35 @@ class SpanningForestState
    1273                 chunk_deps[chl_chunk_rep].first += (par_chunk_rep != chl_chunk_rep);
    1274             }
    1275         }
    1276+        /** Comparison function for the transaction heap. Note that it is a max-heap, so this
    1277+         *  implements the reverse order of what we want in the linearization. */
    


    ajtowns commented at 3:10 am on February 9, 2026:
    A max-heap will give us the max feerate first, if we define an operator< like function, so this doesn’t entirely feel like we’re implementing the reverse order of what we want?

    sipa commented at 8:36 pm on February 9, 2026:
    Ok, rewritten again!
  50. in src/txgraph.cpp:503 in ca0ae9305e outdated
    496@@ -497,6 +497,12 @@ class TxGraphImpl final : public TxGraph
    497         auto feerate_cmp = FeeRateCompare(entry_b.m_main_chunk_feerate, entry_a.m_main_chunk_feerate);
    498         if (feerate_cmp < 0) return std::strong_ordering::less;
    499         if (feerate_cmp > 0) return std::strong_ordering::greater;
    500+        // Compare equal-feerate chunk prefix size for comparing equal chunk feerates. This does two
    


    ajtowns commented at 3:33 am on February 9, 2026:

    It might be good to emphasise (particularly in the commit title) that this is for comparing chunks from different clusters. At least, I had a wtf moment when skimming the commit description.

    FWIW, “equal-feerate chunk prefix” initially read to me as “the total weight of the txs within a chunk that all have the same feerate as the first tx in the chunk’s linearization”. I’m not sure if there’s any better way to write it though, and obviously the actual meaning is already explained.


    sipa commented at 8:36 pm on February 9, 2026:
    I’ve added “distinct-cluster” to the commit title, and in more explanations inside the code.
  51. in src/txgraph.cpp:502 in ca0ae9305e
    496@@ -497,6 +497,12 @@ class TxGraphImpl final : public TxGraph
    497         auto feerate_cmp = FeeRateCompare(entry_b.m_main_chunk_feerate, entry_a.m_main_chunk_feerate);
    498         if (feerate_cmp < 0) return std::strong_ordering::less;
    499         if (feerate_cmp > 0) return std::strong_ordering::greater;
    500+        // Compare equal-feerate chunk prefix size for comparing equal chunk feerates. This does two
    501+        // things: it catches equal-feerate chunks within the same cluster (because later ones will
    502+        // always have a higher prefix size), and it may catch equal-feerate chunks across clusters.
    


    ajtowns commented at 3:37 am on February 9, 2026:
    Might be better to say “it (may) distinguish(es)” rather than “it (may) catch(es)” ?

    sipa commented at 8:36 pm on February 9, 2026:
    Done.
  52. in src/txgraph.cpp:1104 in ca0ae9305e outdated
    1072         // Iterate over the chunks.
    1073         for (unsigned chunk_idx = 0; chunk_idx < chunking.size(); ++chunk_idx) {
    1074             auto& chunk = chunking[chunk_idx];
    1075             auto chunk_count = chunk.transactions.Count();
    1076             Assume(chunk_count > 0);
    1077+            if (chunk.feerate << equal_feerate_chunk_feerate) {
    


    ajtowns commented at 4:00 am on February 9, 2026:

    The operator<< with it’s weird precedence/associativity makes me feel a bit uncomfortable. Did you consider a helper like the following?

     0struct FeeRateCmp {
     1    const FeeFrac& ff;
     2    FeeRateCmp(const FeeFrac& ff) : ff{ff} { }
     3    friend inline std::strong_ordering operator<=>(const FeeRateCmp& a, const FeeRateCmp& b) noexcept
     4    {
     5        auto cross_a = Mul(a.ff.fee, b.ff.size), cross_b = Mul(b.ff.fee, a.ff.size);
     6        return cross_a <=> cross_b;
     7    }
     8};
     9
    10if (FeeRateCmp(chunk.feerate) < equal_feerate_chunk_feerate) {
    

    sipa commented at 8:39 pm on February 9, 2026:

    Yes, I’m doing something like that in my Python implementation.

    I’d like to leave this for a follow-up here, getting rid of the comparison operators on FeeFrac in general, and using explicit comparators or wrappers like you suggest instead.


    sipa commented at 10:40 pm on February 9, 2026:
    Added as idea for a follow-up in #30289.
  53. in src/txgraph.cpp:1109 in ca0ae9305e outdated
    1075             auto chunk_count = chunk.transactions.Count();
    1076             Assume(chunk_count > 0);
    1077+            if (chunk.feerate << equal_feerate_chunk_feerate) {
    1078+                equal_feerate_chunk_feerate = chunk.feerate;
    1079+            } else {
    1080+                equal_feerate_chunk_feerate += chunk.feerate;
    


    ajtowns commented at 4:15 am on February 9, 2026:
    I think it’s a little misleading to call these things feerates: if you have a feerate of 300 sats and 100vb (3 sat/vb) and another of 100 sats and 100vb (1sat/vb), then I think “adding the feerates” would intuitively be expected to give you 4sat/vb, rather than the more useful 400sats/200vb = 2sat/vb. Probably not worth improving though.

    sipa commented at 8:39 pm on February 9, 2026:
    I kept the variable name, but added some comments to explain the fee/size ratio doesn’t change.
  54. in src/test/fuzz/txgraph.cpp:1106 in f3b24a6d46
    1103                 max_chunk_prefix_size = comp_prefix_size;
    1104             }
    1105+
    1106+            // Verify that within each cluster, the internal ordering matches that of the
    1107+            // simulation if that is optimal too, since per-cluster optimal orderings are
    1108+            // deterministic.
    


    instagibbs commented at 5:28 pm on February 9, 2026:

    3749c666fb9513bb81dc2ce72a5690bd3ba0c1a275ae6943984daca4ff991645

    might just mention that PostLinearization was called on both in the comment


    sipa commented at 8:34 pm on February 9, 2026:
    Done.
  55. in src/cluster_linearize.h:1261 in f3b24a6d46 outdated
    1258+     * Specifically, the resulting order consists of:
    1259+     * - The chunks of the current SFL state, sorted by (in decreasing order of priority):
    1260+     *   - topology (parents before children)
    1261+     *   - highest chunk feerate first
    1262+     *   - smallest chunk size first
    1263+     *   - the chunk with the lowest maximum transaction, by fallback_order, first
    


    ajtowns commented at 5:31 pm on February 9, 2026:

    I think this produces a tiny bias towards fewer-tx chunks (assuming a sufficiently adversarial scenario where anyone cares about ordering at this point) – if you have 1 tx, you do K work to reduce the fallback order below T; but if you have N txs, you have to do KN work to get each tx’s fallback order below T. Just having it be “the chunk with the lowest transaction, by fallback_order” seems like it would be fine here. I don’t think it makes the code any simpler though, so unlikely to be worth changing?

    (You can’t add a tx to a chunk to manipulate this rule as adding a tx will change the chunk’s size, which is a higer priority factor to the ordering)


    sipa commented at 8:20 pm on February 9, 2026:

    I wouldn’t say bias directly, but rather a second-order “ability for adversary to cause bias” is harder when there are more transactions in the chunk.

    Agreed that “the chunk with the lowest transaction first, by fallback_order” would be fine, but what is implemented is “the chunk with the highest transaction last, by fallback_order”, which is just as simple in terms of specification/implementation (just switching a < and >, and a min for a max, effectively).

    And I think it’s slightly (if it matters at all) better, though the distinction only matters for many-transaction chunks: it makes it easy to cause a bias to worsen a chunk, but harder to bias it to improve it.

  56. in src/cluster_linearize.h:1302 in f3b24a6d46 outdated
    1297@@ -1273,27 +1298,45 @@ class SpanningForestState
    1298                 chunk_deps[chl_chunk_rep].first += (par_chunk_rep != chl_chunk_rep);
    1299             }
    1300         }
    1301+        /** Function to compute the highest element of a chunk, by fallback_order. */
    1302+        auto max_fallback_fn = [&](TxIdx chunk_rep) noexcept {
    


    ajtowns commented at 5:39 pm on February 9, 2026:
    It’s not being passed as an argument, just called directly, so the _fn seems unnecesary fwiw.

    sipa commented at 8:21 pm on February 9, 2026:

    I like it as a way to signify to readers this is a function object.

    (You may reasonably claim I’m suffering from lingering Stockholm syndrome effects of Hungarian notation…)

  57. in src/test/fuzz/txgraph.cpp:53 in 3df69cd2c3 outdated
    45@@ -42,14 +46,14 @@ struct SimTxGraph
    46     /** The dependency graph (for all transactions in the simulation, regardless of
    47      *  connectivity/clustering). */
    48     DepGraph<SetType> graph;
    49-    /** For each position in graph, which TxGraph::Ref it corresponds with (if any). Use shared_ptr
    50+    /** For each position in graph, which SimTxObject it corresponds with (if any). Use shared_ptr
    


    ajtowns commented at 5:52 pm on February 9, 2026:
    Commit message for “subclass TxGraph::Ref like mempool does” doesn’t need to spell falllback like the coder is actually fallllllling.

    sipa commented at 8:39 pm on February 9, 2026:
    Fixed.
  58. in test/functional/mempool_packages.py:242 in 26b71b3b30
    238@@ -239,9 +239,8 @@ def run_test(self):
    239         self.generate(self.nodes[0], 1)
    240         self.trigger_reorg(fork_blocks, self.nodes[0])
    241 
    242-        # Check if the txs are returned to the mempool (though the transaction ordering may
    243-        # change as it is non-deterministic).
    244-        assert_equal(set(self.nodes[0].getrawmempool()), set(mempool0))
    245+        # Check if the txs are returned to the mempool.
    


    ajtowns commented at 6:17 pm on February 9, 2026:
    “Check that the txs are returned to the mempool, and that transaction ordering is unchanged, as it is deterministic.” ?

    sipa commented at 8:41 pm on February 9, 2026:
    Done. This commit originally just reverted this change from 3efc94d6564deca4e5aaf5219adb71302f522657, which made the order non-deterministic.
  59. in src/txgraph.cpp:191 in dec9934f92 outdated
    186@@ -187,6 +187,8 @@ class Cluster
    187      *  when called from Compact, to recompute after GraphIndexes may have changed; in this case,
    188      *  no chunk index objects are removed or created either. */
    189     virtual void Updated(TxGraphImpl& graph, int level, bool rename) noexcept = 0;
    190+    /** Remove all chunk index entries for this cluster (level=0 only). */
    191+    virtual void RemoveChunkData(TxGraphImpl& graph) noexcept = 0;
    


    ajtowns commented at 6:36 pm on February 9, 2026:
    The commit description says “However, the index itself needs to remain consistent in the mean time, even if not meaningful.” I believe that means “It is better if the index itself remains consistent in the meantime, even if it is not meaningful/useful” – at the end of this PR, if this work wasn’t done, there wouldn’t be a bug, it’s just that it would be harder to understand the live data structures when debugging, and much easier to introduce a bug in future. Please correct me if I’m wrong.

    sipa commented at 8:26 pm on February 9, 2026:

    I believe that’s wrong.

    The problem is that if entries remain inside the chunk index whose comparator-defined ordering is inconsistent with the actually materialized order in the index, it can get corrupted when unrelated chunk index entries are added, because they may end up being inserted in the wrong place.

    The chunk_index must be consistent with its own comparator-defined ordering at all times when modifications to the index are made. And that includes times when this index order is actually not observable, like when there are pending removals.


    ajtowns commented at 9:25 pm on February 9, 2026:
    Perfect, I thought I was missing something.

    instagibbs commented at 2:12 pm on February 10, 2026:

    … specifically because the index can no longer maintain proper ordering because the fallback is not available post-unlink? (saying out loud in case I’m wrong)

    However, the index itself needs to remain consistent in the mean time, even if not meaningful

    maybe s/meaningful/surfaced/ ?


    sipa commented at 3:30 pm on February 10, 2026:

    … specifically because the index can no longer maintain proper ordering because the fallback is not available post-unlink? (saying out loud in case I’m wrong)

    Right. When an element is unlinked, we are no longer able to invoke sort operations on them. If we leave them in the index until the removal is applied, the index will be in a non-functional state, because insertions/removals to it require being able to compare with existing index entries.

  60. in src/txgraph.cpp:2684 in 26b71b3b30 outdated
    2669@@ -2646,6 +2670,10 @@ void TxGraphImpl::CommitStaging() noexcept
    2670     // Staging must exist.
    2671     Assume(m_staging_clusterset.has_value());
    2672     Assume(m_main_chunkindex_observers == 0);
    2673+    // Get rid of removed transactions in staging before moving to main, to avoid the need to add
    2674+    // them to the chunk index (which may be problematic if they were unlinked and thus have no Ref
    2675+    // to use in fallback comparisons).
    2676+    ApplyRemovals(/*up_to_level=*/1);
    


    instagibbs commented at 6:38 pm on February 9, 2026:

    26b71b3b30f1fc0ca54dbb529bb787b094b4e322

    Could the comment be made to seem more urgent? Sounded almost optional to me, when it turns out it’s crucial.


    sipa commented at 8:35 pm on February 9, 2026:
    Made it sound more serious.
  61. instagibbs commented at 6:57 pm on February 9, 2026: member

    concept ACK 26b71b3b30f1fc0ca54dbb529bb787b094b4e322

    code seems to make sense and behavior matches expectations

    IIUC you could have a weird “reverse CPFP” where by someone extends the same-feerate prefix in a cluster to “downgrade it” compared to another same-feerate prefix target in another cluster (that had a worse txid?). I don’t see any meaningful games, I think the set of metrics is fine.

    will do a final pass soon

  62. ajtowns approved
  63. ajtowns commented at 7:05 pm on February 9, 2026: contributor

    ACK 26b71b3b30f1fc0ca54dbb529bb787b094b4e322

    Just nits/questions/suggestions. Overall, I concur with #34257 (comment) – the ordering rationale seems sound to me.

  64. DrahtBot requested review from instagibbs on Feb 9, 2026
  65. sipa force-pushed on Feb 9, 2026
  66. txgraph: initialize Ref in AddTransaction (preparation)
    Instead of returning a TxGraph::Ref from TxGraph::AddTransaction(),
    pass in a TxGraph::Ref& which is updated to refer to the new transaction
    in that graph.
    
    This cleans up the usage somewhat, avoiding the need for dummy Refs in
    CTxMemPoolEntry constructor calls, but the motivation is that a future
    commit will allow a callback to passed to MakeTxGraph to define a
    fallback order on the transaction objects. This does not work when a
    Ref is created separately from the CTxMemPoolEntry it ends up living in,
    as passing the newly-created Ref to the callback would be UB before it's
    emplaced in its final CTxMemPoolEntry.
    3ddafceb9a
  67. txgraph: update chunk index on Compact (preparation)
    This makes TxGraphImpl::Compact() invoke Cluster::Updated() on all
    affected clusters, in case they have internal GraphIndex values stored
    that may have become outdated with the renumbering of GraphIndex values
    that Compact() caused.
    
    No such GraphIndex values are currently stored, but this will change in
    a future commit.
    7427c7d098
  68. txgraph: clear cluster's chunk index in ~Ref (preparation)
    Whenever a TxGraph::Ref is destroyed, if it by then still appears inside
    main-level clusters, wipe the chunk index entries for those clusters, to
    prevent having lingering indexes for transactions without Ref.
    
    This is preparation for enabling a callback being passed to MakeTxGraph
    to define a fallback order on objects. Once the Ref for a transaction is
    gone, it is not possible to invoke the callback anymore. To prevent the
    index becoming inconsistent, we need to immediately get rid of the index
    entries when the Ref disappears.
    
    This is not a problem, because such destructions necessarily will
    trigger a relinearization of the cluster (assuming there are
    transactions in it left) before becoming acceptable again, and the chunk
    ordering is not observable (through CompareMainOrder, or through the
    BlockBuilder interface) until that point. However, the index itself
    needs to remain consistent in the mean time, even if not meaningful.
    6c1bcb2c7c
  69. clusterlin: sort tx in chunk by feerate and size (feature)
    This changes the order of transactions within a chunk to be:
    1. Topology (parents before children)
    2. Individual transaction feerate (high to low)
    3. Individual transaction weight (small to large)
    4. Random tiebreak (will be changed in a future commit)
    
    To do so, use a heap of topology-ready transactions within
    GetLinearization(), sorted by (2), (3), and (4).
    
    This is analogous to the order of chunks within a cluster, which is
    unchanged:
    1. Topology (chunks after chunks they depend on)
    2. Chunk feerate (high to low)
    3. Chunk weight (small to large)
    4. Random tiebreak (will be changed in a future commit)
    e0bc73ba92
  70. txgraph: sort distinct-cluster chunks by equal-feerate-prefix size (feature)
    This makes TxGraph track the equal-feerate-prefix size of all chunks in
    all clusters in the main graph, and uses it to sort chunks coming from
    distinct clusters.
    
    The order of chunks across clusters becomes:
    1. Feerate (high to low)
    2. Equal-feerate-prefix (small to large)
    3. Cluster sequence number (old to new); this will be changed later.
    
    The equal-feerate-prefix size of a chunk C is defined as the sum
    of the weights of all chunks in the same cluster as C, with the same
    feerate as C, up to and including C itself, in linearization order (but
    excluding such chunks that appear after C).
    
    This is an approximation of sorting chunks from small to large across
    clusters, while remaining consistent with intra-cluster linearization
    order.
    8bfbba3207
  71. clusterlin: make optimal linearizations deterministic (feature)
    This allows passing in a fallback order comparator to Linearize(), which
    is used as final tiebreak when deciding the order of chunks and
    transactions within a chunk, rather than a random tiebreak.
    
    The order of transactions within a chunk becomes:
    1. Topology (parents before children)
    2. Individual transaction feerate (high to low)
    3. Weight (small to large)
    4. Fallback (low to high fallback order)
    
    The order of chunks within a cluster becomes:
    1. Topology (chunks after their dependencies)
    2. Feerate (high to low)
    3. Weight (small to large)
    4. Max-fallback (chunk with lowest maximum-fallback-tx first)
    
    For now, txgraph passes a naive comparator to Linearize(), which makes
    the cluster order deterministic when treating the input transactions as
    identified by the DepGraphIndex. However, since DepGraphIndexes are the
    result of possibly-randomized operations inside txgraph, this doesn't
    actually make txgraph's per-cluster ordering deterministic. That will be
    changed in a later commit, by using a txid-based fallback instead.
    39d0052cbf
  72. txgraph test: subclass TxGraph::Ref like mempool does (preparation)
    This is a small change to the txgraph fuzz test to make it used objects
    derived from TxGraph::Ref (SimTxObject) rather than TxGraph::Ref
    directly. This matches how the mempool uses CTxMemPoolEntry, which
    derives from TxGraph::Ref.
    
    This is preparation for a future commit which will introduce simulated
    txids to the transactions in this fuzz test, to be used as fallback
    order.
    941c432a46
  73. txgraph: pass fallback_order to TxGraph (preparation)
    This adds an std::function<strong_ordering(Ref&,Ref&)> argument to the
    MakeTxGraph function, which can be used by the caller (e.g., mempool
    code) to provide a fallback order to TxGraph.
    
    This is just preparation; TxGraph does not yet use this fallback order
    for anything.
    fba004a3df
  74. txgraph: use fallback order when linearizing (feature)
    Add glue to make TxGraph use the fallback order provided to it, in the
    fallback comparator it provides to the cluster linearization code.
    
    The order of chunks within a cluster becomes:
    1. Topology (chunks after their dependencies)
    2. Feerate (high to low)
    3. Weight (small to large)
    4. Max-txid (chunk with lowest maximum-txid first)
    
    The order of transactions within a chunk becomes:
    1. Topology (parents before children)
    2. Individual transaction feerate (high to low)
    3. Weight (small to large)
    4. Txid (low to high txid)
    
    This makes optimal cluster linearization, both the order of chunks
    within a chunk, and the order of transactions within those chunks,
    completely deterministic.
    0a3351947e
  75. txgraph: use fallback order to sort chunks (feature)
    This makes TxGraph also use the fallback order to decide the order of
    chunks from distinct clusters.
    
    The order of chunks across clusters becomes:
    1. Feerate (high to low)
    2. Equal-feerate-chunk-prefix (small to large)
    3. Max-txid (chunk with lowest maximum-txid first)
    
    This makes the full TxGraph ordering fully deterministic as long as all
    clusters in it are optimally linearized.
    6f113cb184
  76. sipa force-pushed on Feb 9, 2026
  77. DrahtBot added the label CI failed on Feb 9, 2026
  78. DrahtBot commented at 8:57 pm on February 9, 2026: contributor

    🚧 At least one of the CI tasks failed. Task previous releases: https://github.com/bitcoin/bitcoin/actions/runs/21839577570/job/63019977418 LLM reason (✨ experimental): Compilation failed in txgraph_tests.cpp due to lambda capture issues (graph not captured) and an incorrect MakeTxGraph call (too few arguments).

    Try to run the tests locally, according to the documentation. However, a CI failure may still 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.

  79. sipa commented at 9:01 pm on February 9, 2026: member

    IIUC you could have a weird “reverse CPFP” where by someone extends the same-feerate prefix in a cluster to “downgrade it” compared to another same-feerate prefix target in another cluster (that had a worse txid?). I don’t see any meaningful games, I think the set of metrics is fine.

    It’s possible, but pretty hard I think. Just attaching equal-feerate transactions isn’t enough, because (assuming optimal/minimal linearization) they’ll end up in a separate, later chunk, after the chunk it’s trying to attack.

    So I think you need a situation where there are already two chunks A<-B, and the attacker can spend outputs of A (but not B) to create a new chunk C, which the same feerate as B, but ordered before it (by being smaller, or having a lower max-txid).

  80. DrahtBot removed the label CI failed on Feb 9, 2026
  81. ajtowns commented at 11:19 pm on February 9, 2026: contributor
    reACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5 it was good before and now it’s better
  82. bitcoin deleted a comment on Feb 10, 2026
  83. instagibbs commented at 2:12 pm on February 10, 2026: member
    ACK 6f113cb1847c6890f1fbd052ff7eb8ea41ccafc5
  84. fanquake requested review from marcofleon on Feb 10, 2026

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: 2026-02-10 21:13 UTC

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