feat(block-producer): switch to mempool version#562
Conversation
Initial outline of the block-producer redesign. Includes a new mempool which can track transaction and batch dependencies as a graph. This enables more complex mempool strategies.
Co-authored-by: Bobbin Threadbare <bobbinth@protonmail.com>
This makes `AuthenticatedTransaction` much cheaper to clone and move around.
) `BatchGraph` now uses `DependencyGraph<BatchJobId, TransactionBatch` internally. This allows us to focus test energy on a single graph type instead of replicating tests and edge cases with more specific implementations. This requires adjusting `DependencyGraph` to separate the `insert` method into `insert_pending` and `promote_pending` to support inserting a batch into the graph while still waiting on its proof.
Doing this now in an effort to simplify subsequent work that will be less isolated.
feat(block-producer): merge next into next-block-producer
Replace FIFO with the mempool block-producer rework. This still has known bugs and issues; notably the client's integration tests sometimes hang.
Notably also merges `next` in.
| self.try_build_block().await; | ||
| /// Wrapper around tokio's JoinSet that remains pending if the set is empty, | ||
| /// instead of returning None. | ||
| struct WorkerPool { |
There was a problem hiding this comment.
Is there any specific reason why the worker count is not a field in sthis struct rather than having to be managed in the BatchBuilder? Seems at first sight that it could be managed here, alongside asking for vacancy, etc. unless there's something I'm missing (not that it makes a big difference here). Then maybe BatchBuilder could just be BatchBuilder { batch_interval: Duration, worker_pool: WorkerPool }
There was a problem hiding this comment.
Its possible to do it that way sure. My thinking was that BatchBuilder contains the configuration options which includes the worker count. Right now these always just default, but I imagine we would want to expose all these options as cli args e.g.
miden-node start block-producer --batch.interval ... --batch.workers 10 --batch.failure-rate 0.1
which would then be passed in here. If we move this around then its no longer pure configuration values, and instead also contains state which means you can no longer do things like trivially overriding defaults and instead need a bunch of fn new-like functions.
I'll move the worker count into the worker pool regardless; since that still works and probably makes more sense.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! Not a very deep review from my end (as I've reviewed most of this code before), but I left some small comments/questions throughout.
I would probably merge as is (i.e., without squashing). |
|
Note: |
tomyrd
left a comment
tomyrd
left a comment
There was a problem hiding this comment.
I'm still a little unfamiliar with the node's codebase but I looked through the whole PR and I haven't found any big issues. Left some suggestions/comments, but overall it looks good. It's well explained, I like the graph figures showing the different states/transitions.
I tested this version of the node with the miden-client in the next branch (integration tests and manual tests with the CLI) and I haven't found any issues.
Mirko-von-Leipzig
force-pushed
the
next-block-producer
branch
from
d504085 to
25160ea
Compare
5 participants
This PR replaces the FIFO based
block-producerwith a dependency graph mempool variant.This upgrade has largely already been reviewed via PRs into the
next-block-producerfeature branch, however this PR will provide the first holistic overview of the completed feature. As such, it would be helpful if those unfamiliar (I guess everyone excluding @bobbinth) would at minimum do a single somewhat thorough pass.This dependency graph is a pre-requisite for adding fee support, and creating transaction/batch selection strategies.
Outstanding work
This feature is not 100% complete, outstanding tasks are tracked in #514. The new
block-producershould however be functionally equivalent to the FIFO queue. Getting this work back into the development branch will make future additions much less painful.Overview
We introduce a
Mempoolwhich acts as the co-ordination point between incoming transactions, batch and block producers.Inflight state
Incoming transactions are validated against the inflight state i.e. is account state correct, and are the notes valid. The inflight state also adds transaction dependency information e.g. the new transaction becomes the child of the last transaction to update the account, or produce the note.
Dependency graphs
These parent - child dependencies between transactions are tracked in
TransactionGraph. In addition we have a batch graph which acts as a sort of overlay on-top of the transaction graph. This complicates matters a bit because batch dependencies connect transactions which previously were not connected (which is why its a separate graph).Concerns
I am not 100% happy with the mempool implementation. It feels a bit brittle in the sense that one must be careful when making changes. It easy to forget to cleanup a graph node, or remove state in one of the methods.
Its also possible that this shape is a poor match for various selection strategies i.e. one might want access to all meta-data at all stages. We'll have to see and adapt when we get there.
Meta
I'm unsure if I should squash merge this; or keep the various commits as is (which are effectively the various PRs already). Currently I'm leaning towards the latter..