-
Notifications
You must be signed in to change notification settings - Fork 28
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
A0-3958: Refactor extender #406
A0-3958: Refactor extender #406
Conversation
Please make sure the following happened
|
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.
Great work!
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 much more healthy than before.
However, I would strongly suggest running random tests against the previous implementation to make sure it matches the behavior. It's very easy to make a small mistake here that's hard to catch by manual inspection.
For such tests, the simplest approach would be perhaps to adapt
AlephBFT/consensus/src/testing/dag.rs
Line 276 in 039c752
async fn ordering_random_dag_consistency_under_permutations() { |
In the linked unit case, this runs just 4 iterations -- that's to not make unit tests too long. But what I used to do back in the days was to run this for millions of iterations (leave it for ~24h or so) after sensitive changes. This can catch a vast majority of bugs that can occur in practice. In particular -- letting it run on devnet, or even testnet, for lots of time, would not be as effective.
The way this could be adapted is, instead of running on two permutations, somehow run on both versions of Extender, old and new. Such a test would be run just once, no need to add it as a permanent "unit test".
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.
Oh bloody hell, the comments I made yesterday did not save 'cause they are arbitrarily part of a review, what the heck. >.<
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.
Great work!
This mainly separates it into the logic itself and the async service that executes it. Due to this I was able to add a bit more unit tests.
In preparation for adding the performance reports, since they will need to order everything in the DAG, even if slightly behind, and that will require changing this code.
Also fixed some small pieces of documentation here and there.