refactor(mempool): revert to separate DAGs#1820
Open
Mirko-von-Leipzig wants to merge 40 commits intonextfrom
Open
refactor(mempool): revert to separate DAGs#1820Mirko-von-Leipzig wants to merge 40 commits intonextfrom
Mirko-von-Leipzig wants to merge 40 commits intonextfrom
Conversation
44f240d to
8bba7a8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR effectively rewrites the mempool implementation (again). The result is a combination of the first implementation and the current implementation, hopefully resulting in a best of both.
Background
The former was simple -- separate graphs for txs and batches, with nodes moving from one to the next as they are selected. It also had a singular inflight state which was checked when txs were added, but never after again. This meant it was had complex internal transitions which were never checked as nodes moved between graphs. However, within a single graph things were simple, and the graph impl could be shared.
The current implementation has a single graph, with transaction nodes being folded into batch nodes, which fold into block nodes. This graph impl is much more complex, in particular the fold and unfold functions are difficult to grok and test, in part due to pass-through nodes causing DAG properties to no longer hold. The positive is that state cannot really be corrupted since there aren't multiple graphs to make mistakes between.
Why
The driving motivation was to do something about
foldandunfold. I couldn't convince myself that the implementations were ever 100% correct, nor that they would survive changes over time.Eventually I realised that folding and unfolding could be achieved by first reverting all the node subgraphs, and then re-inserting with the new replacement nodes. Reverting and inserting are simple (relatively speaking), and we can lean on the existing insertion and reverting checks as additional assertions during this process.
I then realised there wasn't much need for a single graph anymore, since the original motivation was to support folding and unfolding. However we can use the above to add additional safety to the underlying graph by enforcing that nodes can only ever be reverted if they're leaves, and pruned if they're roots aka either no parents or no children remain in the graph.
This PR
A single graph type is created, which holds its state, and edges based on that state. Nodes can only be appended, reverted (removed from the top/leaves), or pruned (removed from the bottom/roots). Nodes can be marked as selected (to mark a tx/batch as included in batch/block).
Transaction and batch graphs wrap this underlying graph type. This is very similar to the original implementation, with some improvements and additional state checks due to the lessons learnt from the current implementation.
The main
Mempoolfunctions should be much easier to read (imo). This implementation is a pure refactor - user batches, and tx reverting improvement strategies will be in follow-up PRs so they can be reviewed without this huge diff.How to review
There is nearly no current code that applies ito graph. They're also in separate files (which is a positive imo).
I suggest:
mempool; since this business level logic shouldn't changeCloses #1439, closes #1253, closes #537