feat(block-producer): refactor batch graph to use dependency graph #533
Conversation
This file contains 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
bobbinth
approved these changes
Oct 30, 2024
Comment on lines
+310
to
+312
| if self.pending.contains(&key) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Why is it not an error to try to make a pending node into a root?
There was a problem hiding this comment.
This method is private and called from a few locations. Effectively either callers have to perform this check, or this method could enforce it. I thought it safer to do it here in one place.
More specifically, this is called when
- A pending node is promoted (so this check isn't necessary)
- A root node is processed, so we check all of its children. We would have to filter children here.
- Subgraphs are reverted, where we have to check and filter a bunch of nodes
Mirko-von-Leipzig
added a commit
that referenced
this pull request
Nov 3, 2024
) `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.
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 ports
BatchGraphto useDependencyGraphas its foundation.I implemented this by extending
DependencyGraphto have a newpendingstage, where the node exists but does not yet have an associated value. For batches this value is its proof i.e.TransactionBatch.Effectively nodes now have a pending state, and must be promoted by passing in a value for it. I'm not 100% happy with the naming of things. Pending, promotions, processed, committed.
This model does not quite match the
TransactionGraphlifecycle, however this was easy to adjust by havingTransacionGraph::insertinvoke both actions.The main benefit of this is reducing the test surface.