-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(block-producer): common graph type #525
refactor(block-producer): common graph type #525
Conversation
Need more tests and switching graph types to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thank you! I left some comments inline - but they are mostly nits.
} | ||
} | ||
|
||
Ok(pruned) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pruned
and keys
parameter should contain the same data, right? We should probably describe the returned value in the doc comments for this function.
But also, why do we need to return the same data as was passed in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pruned
andkeys
parameter should contain the same data, right? We should probably describe the returned value in the doc comments for this function.
I'll document this; though I'm inclined to make the change below instead which will nullify the need for this return value. Would do this in a separate PR ofc.
But also, why do we need to return the same data as was passed in?
The method returns the value V
for each key K
passed in. The usage of this is in Mempool::block_committed
and flows something like:
- Commit batches -> returns
[V: TransactionId]
, which is used to - commit transactions -> returns
[V: AuthenticatedTransaction]
, which is used to - commit state in
InflightState
which uses the tx data to create the state deltas
It might make more sense to have InflightState
keep a mapping of TransactionId -> Delta
internally, then committing it only requires the IDs and not the full data. This makes sense to me from the perspective of data ownership, as it means InflightState
doesn't depend on the caller to maintain the data.
} | ||
} | ||
|
||
/// Set of nodes that are ready for processing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I would start this with an action verb - e.g., "Returns a set of nodes ... ".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. As an aside I believe there should be a way of connecting roots()
and process_root
via the type system. Something similar to a guard type for locks, which lets you choose a root(s) from the set and call .process()
on it.
Not super critical but might be nice if its short code.
Won't look into this now; but when I'm feeling like type-golf sometime I'll pick at it.
This PR introduces a single
DependencyGraph<K, V>
structure which aims to be the common graph foundation for both transaction and batch dependency graphs. This PR only includes the migration ofTransactionGraph<V>
to this new type.A follow-up PR will migrate the batch graph to the new type. This will be a bit more involved but still less than what we have currently. Essentially we need to differentiate between batches being-proven before they may be submitted for consideration for blocks as part of the dependency graph. i.e. we need a mini-graph before the actual graph :) The analog for these batches under-going proving are incoming transactions -- but they arrive proven and validated so this stage does not exist.
The layout of
DependencyGraph
does differ to the current graph implementations. A more "classical" definition of vertices and edges is used instead of the node traversal required before. Methods returnResult
instead of panic'ing on misuse. Methods are also made atomic. This allows the error handling to move up a layer or two which makes more sense to me from a business logic perspective.I find the new structure easier to read and test. This may be a bit slower as it requires more key lookups, but that would be required regardless if we want atomicity/results. I don't foresee this being an issue for a long while.
TransactionGraph::select_batch
demonstrates how to create a selection strategy. I think this will scale well; though we might need to amend theprocess_root
function to take in a set instead of 1-by-1.Existing tests were ported and extended. I would appreciate thoughts on missing test cases for the graph. We should also consider adding property testing.