Cluster mempool: introduce TxGraph (mempool)

https://github.com/bitcoin/bitcoin/pull/31363

Host: glozow  -  PR author: sipa

The PR branch HEAD was 3c7732bbbe735d1883cdd1737ec200cab8caa049 at the time of this review club meeting.

Notes

Cluster Mempool Motivation and Background

Skip this section if you are just interested in reviewing the code and don’t need to hear the pitch for why.

The primary goal of Cluster Mempool (tracked here) is to enable the mempool to more intelligently assess which transactions would be most incentive compatible to keep. In simple terms: “which transactions would earn a miner the most in fees?” The complexity of incentive compatibility assessment comes from dependencies between transactions, i.e. where a transaction spends the output of another.

If the motivations for this project are unclear to you, try to spend up to 60min on these reading materials:

  • “But aren’t fees a trivially small portion of block rewards? Why do we care so much?”
    • One of the biggest problems we want to solve is the asymmetry between how we decide what to evict and what goes into blocks: it’s possible that the first transaction we would evict is also the first transaction we would mine.
    • An original motivation for trying to come up with a metric for “incentive compatibility” was within the context of RBF pinning. Our current feerate-based approach causes us to reject transactions that would be more incentive compatible, creating censorship problems.
  • “Why is cluster-tracking the solution for incentive compatibility?”
    • This Delving post explains why incentive compatibility is complicated and why tracking and limiting clusters is necessary for a complete solution.
    • We talked about clusters in previous review club meetings about auto-bumping unconfirmed UTXOs in coin selection (which added CTxMmemPool::GatherClusters) and Topologically Restricted Until Confirmation transactions. As we discussed, one of the benefits of TRUC is to have an effective cluster limit within the existing framework of ancestor/descendant limits (limit of cluster count 2 is the equivalent of limiting ancestor and descendant counts to 2). However, a limit of 2 is quite prohibitive.

Mempool transactions and their dependency relationships can naturally be represented as a directed graph where nodes represent transactions and edges represent UTXO spending relationships. This graph may contain multiple connected components, each called a cluster. A transaction’s cluster includes all of the transactions it is connected to, which can be defined recursively: all parents and children (nodes with an edge to or from this one), each of their parents and children, etc.

Cluster Mempool includes some large work components, including (1) adding cluster linearization algorithms which are used to order (and thus compare) transactions by incentive compatibility, (2) changing the mempool’s internal structure to enable cluster tracking and always maintain a linearization order, and (3) upgrading RBF, eviction, and mining to use the improved incentive compatibility assessments. This PR is pretty much only relevant to the second component; you can review it without knowing anything about the cluster linearization algorithm or specific RBF rules. The code for the first component has been merged (see PR #30126 and its followups), and work-in-progress code for the third component can be found at PR #28676).

Here is everything you need to know about linearization for this PR:

  • A linearization is topologically valid if it ensures that a parent transaction appears somewhere before its child. There are often multiple valid linearizations for the same cluster, and linearization L1 is better than L2 if taking the first N bytes of L1 would yield higher fees than taking the first N bytes of L2. The optimal linearization of a cluster is better or as good as all the others.
  • Computation needed for linearization scales with the size of the cluster, which is why their size needs to be limited.
  • A cluster requires re-linearization each time a transaction is added or removed.

If you have more time to spend on background concepts or need more convincing, here is additional optional reading material.

  • BIP 431 provides an overview of why RBF pinning is problematic and some of the other pieces that come together to address its issues.
  • This Delving post provides an overview of the algorithms used in cluster linearization.
  • This Delving post discusses how to evaluate replacements using feerate diagrams, which are enabled with cluster mempool.

Background on Mempool Architecture and Interfaces

Skip this section if you are already familiar with current mempool code.

“The Mempool” usually refers to the class CTxMemPool. This class holds a map of CTxMemPoolEntrys and other supporting data structures. It is responsible for keeping itself internally consistent. Much of the code is dedicated to dealing with dependency relationships.

Outside of the CTxMemPool class, here are the major pieces of mempool functionality to think about while reviewing cluster mempool:

  • Mempool validation decides what unconfirmed transactions to keep in memory. This code also enforces transaction relay policies.
  • Block connection removes mempool transactions that would be inconsistent with chainstate.
  • Block disconnection (aka reorganization) resubmits the transactions from the previous chain that are no longer in the new chain.
  • Block assembly (aka mining) fills a block template using mempool transactions, optimizing for fee revenue.
  • Mempool eviction is an internal process that decides what transactions to delete when the memory usage exceeds configured limits. It is triggered in multiple places, including Mempool validation and Block disconnection.

On master, Bitcoin Core’s mempool does not track clusters, but it does track ancestor and descendant sets. CTxMemPoolEntry stores references to its parents and children and tracks the {count, total vsize, total fees} of its ancestor and descendant sets. Each time a transaction is added or removed, CTxMemPool uses the references to traverse ancestors and/or descendants recursively, updating the cached ancestor and descendant information.

Design and Usage with Existing Mempool

While the current (on master) CTxMemPool class does not contain a data structure dedicated to storing the graph representation itself, “the graph already exists” in the form of CTxMemPoolEntry’s parent/child references and cached ancestor/descendant information.

The responsibility of keeping this graph up-to-date and within its specified limits largely belongs to CTxMemPool, but occasionally bleeds out to other areas. For example, in mempool validation, MemPoolAccept queries CTxMemPool for the ancestors of a transaction in order to:

  • Calculate whether this transaction would exceed the ancestor/descendant limits. However, sometimes, when evaluating a replacement, we apply an RBF Carve Out, literally increasing the limit by 1, to avoid double-counting the replacement and to-be-replaced transactions. Yuck!
  • Check whether a TRUC transaction meets its additional topology restrictions. However we first need to aggregate the in-package and in-mempool ancestors.
  • When evaluating a package, the precise ancestor/descendant counts of each package transaction and connected mempool transactions are not calculated before submission. Instead, the aggregate package count and size are used. This approximation avoids complexity, but can result in overestimation for certain topologies.

These examples also illustrate current accuracy problems with enforcing topology limits.

Design and Usage with TxGraph

PR #31363 adds a TxGraph class, used by CTxMemPool to encapsulate the transaction graph. The PR description provides an overview of its interface and implementation. TxGraph supports “main” and “staged” changes: transaction additions and removals can be staged and later discarded or applied. Additionally, removals and additions are applied lazily, allowing them to be batched.

You can view the full cluster mempool implementation PR to see the usage of TxGraph and how these new functions are used in the surrounding logic and contrast them with the existing code:

  • This commit makes CTxMemPoolentry derive from TxGraph::Ref.
  • Mempool validation stages the addition of the new transaction(s) to a changeset, which stages changes to the mempool TxGraph. To check cluster limits, it just queries whether it is oversized. It no longer needs to know what the ancestor/descendant limits are or apply carveouts in a replacement.
  • RBF incentive compatibility simply entails asking the TxGraph if the staged subgraph’s feerate diagram improves upon the original feerate diagram.
  • PrioritiseTransaction now just calls TxGraph::SetTransactionFee() and expects it to handle everything from there. Before, it needed to iterate through all ancestors and descendants to update their cached fees.
  • Additionally, prior to being queried for oversize or feerate diagrams, no cluster calculation and linearization is done.

Taking a look at PR #28676 can help illustrate some of the benefits of having TxGraph, including:

  • The mechanical details of updating the dependency graph and its impact on incentive compatibility are abstracted away from higher level mempool logic.
    • CTxMemPool’s code is less cluttered with graph-wrangling logic. Lots of code is removed, and the much of the remaining ancestor/descendant tracking can be cleaned up in the future.
    • TxGraph doesn’t know that it represents a mempool. It doesn’t know what a transaction is, and doesn’t care whether elements are being added and removed for replacements or block (dis)connection. It has a smaller scope and well-specified behavior.
  • We now have the ability to reason accurately about the addition of multiple transactions and removal of their collective replacements.
  • Batched updates and linearization improve the overall performance of mempool operations.
  • Deferred linearization also can allow for slower linearization improvements to be applied later, e.g. in the background, giving us the benefits of optimal linearizations without sacrificing performance.

Implementation

The rest of the notes are about reviewing the PR itself. Advice on review approach:

  • The author has written recommendations for reviewing the code in this comment.
  • The PR is largely split into feature and optimization commits. The features are introduced iteratively, so it may be easiest to review the PR in chunks of commits for each feature if the overall changes seem intimidating.
  • Another approach is to first read through the overall changes to TxGraph and TxGraphImpl’s respective interfaces, and then review implementation. You could also review the interface, then the fuzzer implementation, then the real implementation.

These notes are split by feature to make it easier to review the PR iteratively.

Basic TxGraph, Lazy and Oversized

This section includes code up to “txgraph: (optimization) avoid per-group vectors for clusters & dependencies”.

A TxGraph has a vector of Entrys (where each entry is a single transaction), and a vector of Clusters (which represent clusters as defined earlier). Clients can AddTransaction, RemoveTransaction, AddDependency, and SetTransactionFee. When a transaction is added, it starts in a cluster of its own. When AddDependency is called for two transactions that are not already in the same cluster, their clusters are merged.

It also has queues of “work” to do which may or may not need to be completed in order to perform certain tasks or answer external queries. Some tasks have other tasks as prerequisites.

This is the order in which things have to be resolved:

  1. ApplyRemovals: Apply all of the queued removals.
  2. SplitAll: Split up clusters if they contain components that are no longer connected to each other.
  3. GroupClusters: Calculate what clusters would need to be merged if dependencies are applied. However, these changes are not yet applied.
  4. ApplyDependencies: Apply new dependencies and merge clusters as computed in the previous step.
  5. Relinearize: Relinearize clusters.

Much of the work is done lazily. For example, calling RemoveTransaction does not actually mean its etry is deleted, just that the index has been added to m_to_remove. However, TxGraph cannot answer a call to Exists without first calling ApplyRemovals.

An Oversized TxGraph is one that has at least one cluster that violates the cluster count or size limits.

  • A TxGraph only knows if it is oversized after it has completed steps 1-3.
  • When a TxGraph is oversized, mutation functions (like RemoveTransaction and AddDependency) and GetIndividualFeerate can still be called, but inspection functions like GetChunkFeerate are unavailable because we won’t merge or linearize clusters that are oversized. (GetChunkFeerate becomes GetMainChunkFeerate in a later commit).
  • Additionally, step 4 will not proceed if the TxGraph is oversized; step 5 is thus also impossible.

The GroupClusters function implements union-find, a favorite amongst software engineering interviewers.

Staging Support

This section includes code up to “txgraph: (optimization) cache oversizedness of graphs”.

This feature allows users of TxGraph to stage changes and then apply or discard them, much like git.

Much of the TxGraph members are grouped together as a ClusterSet. This refactor is done so that a TxGraph can have multiple subgraphs or levels represented as ClusterSets: “main” (level 0) and “staged” (level 1).

By default, mutations like AddTransaction, RemoveTransaction, AddDependency are applied directly to main, but clients of TxGraph can create a staging session:

  • To StartStaging, a new subgraph is added containing copies of information from the main graph. It starts with no clusters and no transactions.
  • Additions, removals, and new dependencies are automatically applied on the highest level: staged if it exists, main otherwise.
  • To CommitStaging, everything is copied from staged to main after finding conflicting clusters and deleting them from main. Conflicting clusters include those belonging to all staged removals and any transactions the subgraphs have in common. Then, clusters, work queues, and statistics are copied from staged to main.
  • AbortStaging largely consists of simply deleting the staged subgraph.

Specialized Functionality

This section includes the rest of PR #31363, but also PR #31444 and PR #31553.

There are various functions added to TxGraph to obtain specialized information. It may be useful to look at the code surrounding mempool to understand why:

  • CountDistinctClusters and GetMainStagingDiagram are used in RBF logic?
  • GetBlockBuilder is used in block assembly?
  • GetWorstMainChunk is used in mempool eviction?
  • Trim is used in reorgs? It instructs TxGraph to delete transactions until it is no longer oversized, and allows it to decide which transactions to evict.

Questions

Conceptual

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

  2. What is the mempool “graph” and to what extent does it exist in the mempool code on master?

  3. What are the benefits of having a TxGraph, in your own words? Can you think of drawbacks?

  4. Can TxGraph help to more accurately calculate (and thus enforce) topology limits, or is it a just refactor?

Basic TxGraph, Lazy and Oversized

  1. What is the difference between TxGraph and Cluster?

  2. What does it mean for a TxGraph to be oversized? Is that the same as the mempool being full?

  3. If a TxGraph is oversized, which functions can still be called, and which ones can’t?

  4. Under what circumstances could GetIndividualFeerate and GetChunkFeerate return an empty FeeFrac? Is it possible that one returns an empty FeeFrac and the other doesn’t? How? (Note we are talking about pre-staging support code).

  5. What are the 4 mutation functions? Which ones necessitate relinearization?

  6. After calling TxGraph::RemoveTransaction, has the transaction been removed from m_entries? If not, what happens instead, and when are transactions removed from m_entries?

  7. Why can’t ApplyDependencies continue if the TxGraph is oversized?

  8. Why isn’t Exists a const function like one would expect?

  9. What is the benefit of processing removals, dependencies, merges, and linearizations lazily?

  10. Why is AddTransaction annotated with [[nodiscard]]?

  11. For the QualityLevels NEEDS_RELINEARIZE, ACCEPTABLE, and OPTIMAL, what can / can’t {TxGraph internally, a client of TxGraph} do with a cluster that is linearized to that quality?

Staging Support

  1. What is the purpose of TxGraph supporting the staging of update operations? In what situation would we use it, and do you think the benefits warrant the extra code complexity?

  2. What are the TxGraph functions analogous to git add, git commit, and git stash? In the other direction, what is analogous to AddTransaction, RemoveTransaction, and AddDependency?

  3. What does PullIn do, and when is it necessary? How many times can the while (level < to_level) loop run?

  4. In the commit to “cache oversizedness,” why is it ok that the m_oversized value for staged is set to that of main? Why is calling SplitAll() necessary?

  5. How many levels are possible?

  6. Which of the four combinations are possible {main, staged} {oversized, not oversized}?

Specialized Functionality

  1. Why are these specialized functions (CountDistinctClusters, GetMainStagingDiagram, GetBlockBuilder, GetWorstMainChunk, Trim) necessary? Can you think of a slimmer interface to serve these purposes?

  2. What are all of the places where the mempool may need to evict transactions in order to stay within cluster limits?

  3. Should TxGraph be allowed to decide which transactions to evict when it is oversized? Why or why not - does it depend on the situation?

Meeting Log

  117:00 <glozow> #startmeeting
  217:00 <stickies-v> hi
  317:00 <glozow> Welcome to PR review club! Today is TXGRAPH DAY https://bitcoincore.reviews/31363
  417:00 <glozow> If you say hi now you'll be in the logs ;)
  517:00 <schmidty> hi
  617:00 <marcofleon> hi!!!
  717:01 <glozow> Did anybody get a chance to review the PR or look at the notes?
  817:01 <kevkevin> hi
  917:01 <stringintech> Hi!
 1017:01 <hernanmarino> Hi !
 1117:01 <alfonsoromanz> hi
 1217:01 <Polaris> hi
 1317:01 <jaruonic> hi
 1417:01 <Guest273> Yes, quickly went through the notes.
 1517:01 <kevkevin> I was able to look at the PR briefly to get a general idea of what it is trying to do
 1617:01 <hernanmarino> just the notes today, it looks interesting
 1717:01 <sipa> hi
 1817:01 <stickies-v> i looked at the notes and linked resources, mostly!
 1917:01 <marcofleon> notes and a bit of review of the pr
 2017:01 <instagibbs> read the notes, played with the interface, started seriously looking through the commits today
 2117:01 <stringintech> Tried to figure out the overall structure through the PR description and header files.
 2217:02 <glozow> Great! hopefully we can get further today, as ultimately we want to be looking at the code
 2317:02 <glozow> Does anybody have any general cluster mempool questions before we dive in?
 2417:03 <abubakarsadiq> hi
 2517:03 <glozow> Ok. Feel free to ask questions whenever, since this is a text meeting
 2617:03 <glozow> What is the mempool “graph” and to what extent does it exist in the mempool code on master?
 2717:04 <Murch[m]> Hi
 2817:04 <marcofleon> in CTxMempoolEntry tx parents and descendants are kept track of... but there's no explicit graph
 2917:05 <jasan> to know the fees in order to make a heavy block template
 3017:05 <stringintech> Currently It exists as the ancestor-dependant connections and no such thing as clusters.
 3117:05 <stringintech> descendant*
 3217:06 <sipa> marcofleon: well, arguably txgraph doesn't change that... there is still just the set of ancestors and descendants for each transaction there, but it's better encapsulated, and uses more efficient data structures
 3317:06 <kevkevin> What I understand is its the mempool entries and their parents and descendants
 3417:06 <abubakarsadiq> list of all the transactions where connected ones form a single graph where a node in the graph represent a tx
 3517:06 <stickies-v> the nodes of the graph are the `CTXMemPoolEntry` objects, and the edges are are all their ancestor/dependant connection
 3617:06 <glozow> Right. When we need to know the ancestors of a transaction, what do we do? Bonus points if you can link me a code example
 3717:07 <sipa> glozow: in the current master code, i assume you mean?
 3817:07 <marcofleon> right, that makes sense
 3917:07 <glozow> yes on master
 4017:08 <glozow> (will also happily accept an answer that is for calculating the descendant set)
 4117:09 <stringintech> Have not check out the code but recursively look up parents of all the parents of the tx?
 4217:09 <abubakarsadiq> you have to recursively find parent of the transactions parents
 4317:09 <CosmikDebris> https://github.com/bitcoin/bitcoin/blob/a43f08c4ae3235f2fa460dd17a7f5f9f9842efd9/src/txmempool.cpp#L243 invoke `CalculateMemPoolAncestors`
 4417:09 <stickies-v> recursively use `GetMemPoolParents` and `GetMemPoolChildren`?
 4517:09 <Guest273>     virtual std::vector<Ref*> GetAncestors(const Ref& arg, bool main_only = false) noexcept = 0; ?
 4617:10 <sipa> Guest273: that's in the PR; the question is how it works in master today
 4717:10 <kevkevin> Do we calculate using this function? CTXMemPool::CalculateMemPoolAncestors()
 4817:10 <marcofleon> CTxMemPool::CalculateMemPoolAncestors gotta be involved
 4917:10 <glozow> stringintech: abubakarsadiq: stickies-v: yep!
 5017:10 <marcofleon> haha seems to be a lot
 5117:10 <glozow> CMPA yep
 5217:10 <glozow> https://github.com/bitcoin/bitcoin/blob/a43f08c4ae3235f2fa460dd17a7f5f9f9842efd9/src/txmempool.cpp#L172-L200 here's the loop
 5317:11 <glozow> and descendants: https://github.com/bitcoin/bitcoin/blob/a43f08c4ae3235f2fa460dd17a7f5f9f9842efd9/src/txmempool.cpp#L571-L593
 5417:11 <glozow> What are the benefits of having a TxGraph, in your own words? Can you think of drawbacks?
 5517:12 <glozow> These should be pros that we *don't* already have today
 5617:12 <marcofleon> Just more structured and better encapsulated as sipa said previously. Makes it easier to reason about. Also the atomic operations with staging seems like a good approach. I'm not sure if we do something like that today?
 5717:12 <stringintech> First thing I noticed when looking at the code was how relatively easy it was to understand things. Even not knowing the previous logic.
 5817:13 <stringintech> As a first-timer.
 5917:13 <abubakarsadiq> many benefits, we now group connected transactions into clusters, we maintain the list of this clusters, which make it easy for getting ancestors of transaction, linearization, maintaining cluster size, mining and eviction
 6017:13 <kevkevin> ya I'd say it makes it easier to reason about the the structure of the graph
 6117:14 <sipa> as for comparison with *today*, i think the answer is primarily that it brings in all of the cluster mempool goodness: chunk feerates, linearizations, the ability to observe a full ordering of transactions in the mempool
 6217:14 <instagibbs> marcofleon currently we have no great way of "simualting" entry and testing, so you get weird things in the code in mayn places
 6317:14 <marcofleon> got it
 6417:14 <sipa> as for comparison with a theoretical alternative where cluster mempool was brought it, without an intermediary layer that models the transaction graph: encapsulation, and the fact that it's fully specified (almost...) which significantly increases the power of tests for it
 6517:15 <instagibbs> marcofleon https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L951 f.e.
 6617:15 <glozow> I really like that it abstracts away all the topological details so e.g. validation isn't trying to figure out how to not double-count replacements and combine package/mempool ancestors etc
 6717:15 <glozow> Also it can do that accurately, instead of doing ugly carveouts to try to guess
 6817:16 <glozow> biggest offender imo https://github.com/bitcoin/bitcoin/blob/a43f08c4ae3235f2fa460dd17a7f5f9f9842efd9/src/validation.cpp#L950-L984
 6917:16 <marcofleon> The carveouts are indeed ugly
 7017:17 <glozow> sipa: ooh, making a mental note to ask "which parts are not specified?" later
 7117:17 <sipa> though to be fair, getting rid of the carveout isn't a benefit of this approach
 7217:17 <glozow> why not?
 7317:17 <sipa> it's a choice we make, because it is a policy rule which is incompatible with the approach we want
 7417:17 <sipa> we could do it without this PR
 7517:18 <glozow> sipa: you seem to be thinking of cpfp carve out
 7617:18 <glozow> I'm talking about RBF carve out?
 7717:18 <sipa> oooh, sorry, i didn't check the link!
 7817:18 <instagibbs> there are multiple carveouts :(
 7917:18 <sipa> you are right
 8017:18 <sipa> i didn't know that was also called carvout
 8117:19 <instagibbs> very long comment in the code too, gives confidence to quality of design
 8217:19 <glozow> can anybody think of any drawbacks? :)
 8317:20 <sipa> we have to do annoying review clubs about it? ;)
 8417:20 <instagibbs> drawbacks to txgraph?
 8517:20 <glozow> so, complexity? haha
 8617:20 <glozow> but encapsulated complexity!
 8717:20 <sipa> i guess that's the most obvious answer... it's a very non-trivial amount of code that needs review
 8817:21 <marcofleon> 0 drawbacks for sure... no i guess some overhead with all the additional data structures? but I think the encapsulation and the fact that its much easier to test makes it worth it
 8917:21 <jasan> what about tx prioritization, is is retained with the cluster look at mempool?
 9017:22 <jasan> I mean RPC prioritizetransaction
 9117:22 <sipa> jasan: yes, it is independent, as that lives on a higher level
 9217:22 <abubakarsadiq> @marcofleon the additional data structures are mostly not drawbacks IIUC some are fixing current issues
 9317:22 <instagibbs> jasan SetTransactionFee see that
 9417:22 <monlovesmango> it might me more overhead to just get direct ancestor/child?
 9517:22 <glozow> jasan: yes, that's what `SetTransactionFee` is for
 9617:22 <schmidty> jasan: TxGraph::SetTransactionFee()
 9717:22 <stickies-v> wrt drawbacks: I suppose encapsulation sometimes doesn't allow you to make certain (hacky) performance optimizations without changing the interface?
 9817:22 <jasan> prioritisetransaction
 9917:23 <jasan> sipa: thanks
10017:23 <abubakarsadiq> like `FeeFrac` it fixes the rounding error of `CFeeRate`
10117:23 <sipa> jasan: in the PR description i state that TxGraph doesn't know about prioritization; that doesn't mean it's incompatible with it - the point is just that it doesn't *care* why the mempool things a particular transaction has a particular fee, but it's perfectly possible for the mempool to apply prioritization before handing fees to txgraph (and will)
10217:24 <marcofleon> abubakarsadiq: nice yeah that makes sense. I still need to understand a lot of the lower level pros
10317:24 <glozow> the only thing I can think of it is that it's sometimes nice for validation to get to dictate what topology limits are per-tx, e.g. with TRUC and other imagined policies https://delvingbitcoin.org/t/v3-and-some-possible-futures/523
10417:25 <sipa> monlovesmango: there is indeed some overhead, though it's tiny (and probably well-compensated by using more efficient data structures internally)... the fact that transaction references always need to be translated from/to TxGraph::Ref* pointers is a runtime cost throughout the design
10517:25 <instagibbs> glozow I kind of imagine we'd be able to clean up TRUC logic with txgraph though, even without txgraph driving the logic itself
10617:26 <glozow> instagibbs: how?
10717:26 <instagibbs> maybe after the hour 👍
10817:26 <sipa> glozow: no reason why functionality for testing for particular topologies can't be added to txgraph, but the abstraction does mean there is an extra cost
10917:26 <glozow> was thinking about some of the whiteboarding we did on limiting direct parents/children
11017:27 <glozow> fosho doable. I'm just trying to come up with something to put on this side of the chart 😂
11117:27 <glozow> Ok let's look at implementation
11217:27 <glozow> What is the difference between `TxGraph` and `Cluster`?
11317:28 <CosmikDebris> clusters are connected components, the graph is a DAG
11417:28 <marcofleon> txgraph is the full set of transactions and their relationships. cluster is a group of connected txs within the txgraph
11517:28 <monlovesmango> TxGraph holds all transaction refs, clusters are assigned to each transaction
11617:29 <sipa> bonus questions: how many Clusters can an individual transaction be part of, within a TxGraph?
11717:29 <monlovesmango> one
11817:29 <monlovesmango> ?
11917:30 <schmidty> 2
12017:30 <sipa> monlovesmango: depends which commit we're looking at, but initially, indeed 1
12117:30 <instagibbs> monlovesmango Refs are held by the user of TxGraph, I think "Entry" is the terminlogy?
12217:30 <stringintech> maybe 2 considering the staging changes?
12317:30 <sipa> stringintech, schmidty: eventually, indeed 2
12417:30 <marcofleon> 2 i think yeah. with staging graph and main graph?
12517:31 <abubakarsadiq> 1 to externals, 2 within
12617:31 <sipa> abubakarsadiq: well, Cluster is entirely within
12717:31 <monlovesmango> if the question is within a single TxGraph it should be 1 right? staging and main are different TxGraphs?
12817:31 <sipa> monlovesmango: incorrect, TxGraph encapsulates both the main graph, and optionally a staging graph
12917:31 <monlovesmango> ok thank you!
13017:32 <glozow> is it possible for a transaction to never be in a singleton Cluster?
13117:32 <sipa> glozow: nice one
13217:32 <glozow> (singleton Cluster means there is only 1 tx in the cluster)
13317:32 <instagibbs> I don't think so
13417:33 <abubakarsadiq> I think no, each transaction has to be added individually first
13517:33 <instagibbs> AddTransaction says: "/ Construct a new singleton Cluster (which is necessarily optimally linearized)."
13617:33 <stickies-v> aren't operations batched?
13717:33 <monlovesmango> how would anchor txs be processed?
13817:33 <monlovesmango> *ephemeral anchor txs
13917:33 <glozow> stickies-v: good point! let's look at `AddTransaction` https://github.com/bitcoin-core-review-club/bitcoin/blob/3c7732bbbe735d1883cdd1737ec200cab8caa049/src/txgraph.cpp#L1305
14017:34 <sipa> stickies-v: they are, but transaction additions are not (only removals and dependency additions are batched)
14117:34 <glozow> Looks like we immediately make a cluster for it, so batching doesn't change this
14217:34 <glozow> So every entry has to be a singleton at some point
14317:34 <instagibbs> monlovesmango TxGraph is unaware of what an anchor is, it just knows there are at least two transactions, one depending on another
14417:34 <instagibbs> at given feerates
14517:35 <monlovesmango> got it, thanks :)
14617:35 <glozow> `TxGraph` doesn't even know what a transaction is
14717:35 <glozow> What does it mean for a TxGraph to be oversized? Is that the same as the mempool being full?
14817:35 <CosmikDebris> it means at least one of its clusters exceeds size limits
14917:35 <abubakarsadiq> @monlovesmango You add the parent, then add the child, then add the dependency between them
15017:36 <marcofleon> its not the same as mempool being full. txgraph being oversized means that at least one cluster exceeds max_cluster_count_limit
15117:36 <monlovesmango> it means at least one cluster is exceeding the size limits
15217:36 <marcofleon> also txgraph not even knowing what a transaction is is cool
15317:36 <glozow> marcofleon: monlovesmango: yep exactly. can be oversized and not full. Can be full and not oversized.
15417:37 <abubakarsadiq> unclear to me what next to do txgraph is oversized? what are cant you do at that state
15517:38 <instagibbs> monlovesmango size(weight/vbytes) and count (number of txns)
15617:38 <glozow> abubakarsadiq: what's the question? are you asking what a client should do if they see that it's oversized?
15717:38 <hernanmarino> marcofleon: +1
15817:38 <kevkevin> There is a function TxGraphImpl::IsOversized to determine if the txgraph is oversized
15917:38 <marcofleon> correct if i'm wrong but I feel Abubakar is asking the. next q in the notes essentially?
16017:39 <marcofleon> like what can and can't be done
16117:39 <sipa> the interface in txgraph.h should be properly documented with which things you can do while oversized and which you cannot
16217:39 <glozow> If a TxGraph is oversized, which functions can still be called, and which ones can’t?
16317:40 <schmidty> Functions that query cannot be called, presumably for performance, or?
16417:40 <marcofleon> all 4 mutations can be called still
16517:40 <CosmikDebris> you can't call stuff that depends on linearizing clusters, like computing chunk feerates
16617:40 <sipa> schmidty: no, because oversized clusters internally just cannot be _represented_, so if there are batched operations present which would require an oversized Cluster to be created, they just don't get applied, and instead the TxGraph goes into "oversized" state
16717:40 <kevkevin> I see that ApplyDependencies cannot be called if oversized
16817:40 <sipa> CosmikDebris: not just linearization!
16917:41 <instagibbs> anything that involves larger than O(1)-ish work, I think
17017:41 <instagibbs> (can't be)
17117:41 <sipa> schmidty: so we solve that by outlawing any operations that could require actually materializing an oversized cluster
17217:41 <glozow> to linearize, first you must merge clusters. to merge clusters, you must first ensure you will not be creating a cluster that is too big
17317:41 <instagibbs> can't even ask for vector of Refs from cluster
17417:41 <stringintech> I had a relevant question. What causes the TxGraph to be not oversized anymore? Only tx removal/evict?
17517:41 <schmidty> sipa: thanks!
17617:42 <sipa> stringintech: bingo, or if the oversizedness is restricted to a staging graph, AbortStaging() will fix it too (by just throwing it away)
17717:42 <glozow> stringintech: yes, you can `RemoveTransaction`. Or you can `AbortStaging` ("I've decided not to accept this tx to mempool")
17817:42 <glozow> can `main` be oversized?
17917:42 <sipa> instagibbs: i think the threshold is anything that requires O(n^2) work or more (which includes computing the ancestors/descendants of a transaction)
18017:43 <sipa> instagibbs: the last PR's TxGraph::Trim function is roughly O(n log n), and can be used in oversized state
18117:43 <glozow> Under what circumstances could GetIndividualFeerate and GetChunkFeerate return an empty FeeFrac? Is it possible that one returns an empty FeeFrac and the other doesn’t?
18217:44 <stringintech> sipa: glozow: Thanks!
18317:44 <instagibbs> sipa fair enough
18417:44 <glozow> marcofleon: you mentioned "4 mutations", what are they?
18517:45 <monlovesmango> both return empty if tx doesn't exist in txgraph. chunk can return empty when individual does not when oversized
18617:45 <marcofleon> AddTransaction, RemoveTransaction, AddDependency, and SetTransactionFee
18717:45 <kevkevin> glozow arent they AddTransaction RemoveTransaction AddDepndency and SetTransactionFeeRate?
18817:46 <sipa> monlovesmango: GetChunkFeerate is just *not allowed* to be called when oversized, so talking about its correctness or what it would return isn't really well-defined
18917:46 <monlovesmango> sipa: oh right!
19017:47 <glozow> kevkevin: marcofleon: great. and why can we keep calling them when oversized?
19117:48 <marcofleon> could chunkfeerate return empty beacuse MakeAcceptable fails while individualfeerate would be fine?
19217:48 <marcofleon> was just trying to answer the second part there but not quite sure
19317:48 <sipa> monlovesmango: MakeAcceptable cannot fail
19417:48 <glozow> What do you have to do before you know chunk feerate?
19517:48 <sipa> eh, marcofleon
19617:48 <stringintech> glozow: I guess after the mutations it could be not oversized anymore
19717:48 <marcofleon> thanks, need to look at that more
19817:49 <sipa> marcofleon: or rather, if it might fail, you're just not allowed to call anything that needs MakeAcceptable (because that implies you're oversized)
19917:50 <kevkevin> stringintech: if we're using the AddTransaction mutation would it not still be oversized?
20017:50 <sipa> yeah, only RemoveTransaction can result in changing oversized -> not oversized
20117:51 <sipa> adding transactions or adding dependencies can only result in going the other way
20217:51 <glozow> the answer I was looking for is essentially: txgraph is oversized but also lazy (wow that's kind of mean to say) and none of the mutation functions synchronously do cluster merging or linearization
20317:51 <sipa> exactly ^
20417:51 <glozow> For example: After calling `TxGraph::RemoveTransaction`, has the transaction been removed from `m_entries`?
20517:51 <hernanmarino> glozow: :))
20617:52 <instagibbs> added to m_to_remove
20717:53 <stringintech> Earlier glozow: asked if `main` can be oversized too which became my question too :)) I guess it was not answered or I missed the answer.
20817:53 <glozow> yep: https://github.com/bitcoin-core-review-club/bitcoin/blob/3c7732bbbe735d1883cdd1737ec200cab8caa049/src/txgraph.cpp#L1327
20917:53 <stringintech> kevkevin: right.
21017:53 <glozow> what about `AddDependency`, what happens instead of merging clusters? https://github.com/bitcoin-core-review-club/bitcoin/blob/3c7732bbbe735d1883cdd1737ec200cab8caa049/src/txgraph.cpp#L1344
21117:53 <sipa> stringintech: what do you think the answer is? can main be oversized?
21217:54 <glozow> :)
21317:54 <stringintech> As far as I saw in the code (not deep enough though), operations were happening on the upmost clusterset. So if we do not have staging the main could be oversized.
21417:55 <marcofleon> Is the tx actually removed when Compact is called?
21517:55 <sipa> marcofleon: define "tx actually removed"
21617:55 <sipa> the Ref? the Entry? the state of the Locator?
21717:56 <marcofleon> hmm yeah... i guess removed from m_entries
21817:56 <sipa> marcofleon: right, what happens to the Ref then?
21917:57 <glozow> stringintech: yes, it depends on usage. Intended usage is to first stage changes and discard them if they result in an oversized txgraph. But you can also apply things directly to main and make it oversized
22017:57 <stringintech> glozow: Thank you
22117:57 <glozow> `TxGraph`, at least, does not stop you from doing this
22217:58 <kevkevin> I think it can be oversized if we don't have the staging txgraph? since then m_clustersets.size() - 1 = 0
22317:58 <kevkevin> the main txgraph
22417:58 <marcofleon> The Ref stays i believe...
22517:58 <sipa> stringintech: in practice (after follow-up PRs to make the mempool code actually use txgraph), staging will be used for rbf evaluations, while main will be used directly from blocks... and a reorg can add transactions back to the mempool, which could temporarily cause main to be oversized
22617:58 <abubakarsadiq> sipa: I think the ref was removed already
22717:58 <sipa> abubakarsadiq: bingo
22817:58 <marcofleon> oh. Hmm okay
22917:59 <glozow> does `CommitStaging` stop you from committing changes that would make `main` oversized?
23017:59 <sipa> marcofleon: sorry, it was a trick question (actually, i just forgot that had changed recently), Entrys are not removed from m_entries until the Ref is destructed
23117:59 <stringintech> sipa: Hmmm. Thanks!
23217:59 <monlovesmango> glozow: no doesn't look like it
23317:59 <glozow> https://github.com/bitcoin-core-review-club/bitcoin/blob/3c7732bbbe735d1883cdd1737ec200cab8caa049/src/txgraph.cpp#L1600
23417:59 <marcofleon> Got it thanks. Trick questions are how we learn!
23517:59 <glozow> Ah we are out of time!
23618:00 <jasan> Thank you all!
23718:00 <kevkevin> Thank you guys!
23818:00 <monlovesmango> thank you glozow, sipa, and all!!
23918:00 <glozow> Do people want to do another meeting? I am not free at this time tomorrow, but if somebody else wants to run it...?
24018:00 <abubakarsadiq> thanks for hosting and the nice docs @glozow
24118:00 <stringintech> Thanks for making life easier for first-timers :))
24218:00 <sipa> thanks glozow for hosting, and thanks everyone for taking some time to look at my PRs
24318:00 <jasan> https://alt.signetfaucet.com/ - TRUC related
24418:00 <marcofleon> Thanks glozow and sipa! Good stuff
24518:00 <monlovesmango> learned a lot :)
24618:00 <hernanmarino> thanks !
24718:00 <algo2043> thanks!
24818:01 <glozow> We could also meet again next week
24918:01 <stickies-v> thank you for hosting and your awesome prep work glozow !
25018:01 <hernanmarino> next week is better i think
25118:01 <glozow> #endmeeting