-
Notifications
You must be signed in to change notification settings - Fork 65
apollo_consensus_orchestrator: split consensus config into static and dynamic configs #10963
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
base: main-v0.14.1-committer
Are you sure you want to change the base?
apollo_consensus_orchestrator: split consensus config into static and dynamic configs #10963
Conversation
ShahakShama
left a comment
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.
@ShahakShama reviewed 10 files and made 1 comment.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @guy-starkware).
a discussion (no related file):
I assume this isn't the full feature. What happens if this PR is merged and the next PRs aren't merged and someone changes the dynamic config?
If the answer is that nothing will happen, I think we should consider entering all PRs as one PR (we can have a stack of PRs, but merge it from the top)
guy-starkware
left a comment
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.
@guy-starkware made 1 comment.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ShahakShama).
a discussion (no related file):
Previously, ShahakShama wrote…
I assume this isn't the full feature. What happens if this PR is merged and the next PRs aren't merged and someone changes the dynamic config?
If the answer is that nothing will happen, I think we should consider entering all PRs as one PR (we can have a stack of PRs, but merge it from the top)
I don't know if merging from the top will make sense or not. But certainly we'd want to merge them very close together (as in, after the whole feature is reviewed)
dafnamatsry
left a comment
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.
@dafnamatsry reviewed 10 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guy-starkware and @ShahakShama).
crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs line 757 at r1 (raw file):
.config .dynamic_config .override_l2_gas_price_fri
Was it a bug that we didn't take the override_ value here?
crates/apollo_consensus_orchestrator_config/src/config.rs line 136 at r1 (raw file):
/// block hash to be available. The remaining time is used to build the proposal. /// for the retrospective block hash to be available. The remaining time is used to build the /// proposal.
Suggestion:
/// The fraction (0.0 - 1.0) of the total build time allocated to waiting
/// for the retrospective block hash to be available. The remaining time is used to build the
/// proposal.
guy-starkware
left a comment
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.
@guy-starkware made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @ShahakShama).
crates/apollo_consensus_orchestrator/src/sequencer_consensus_context.rs line 757 at r1 (raw file):
Previously, dafnamatsry wrote…
Was it a bug that we didn't take the
override_value here?
I think it might be.
guy-starkware
left a comment
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.
@guy-starkware made 1 comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dafnamatsry and @ShahakShama).
crates/apollo_consensus_orchestrator_config/src/config.rs line 136 at r1 (raw file):
/// block hash to be available. The remaining time is used to build the proposal. /// for the retrospective block hash to be available. The remaining time is used to build the /// proposal.
Done.
8eae748 to
1fd82a4
Compare
dafnamatsry
left a comment
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.
@dafnamatsry reviewed 1 file and resolved 2 discussions.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @ShahakShama).
dafnamatsry
left a comment
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.
@dafnamatsry reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ShahakShama).
1fd82a4 to
0665010
Compare
ShahakShama
left a comment
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.
@ShahakShama reviewed 1 file and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware).
a discussion (no related file):
Previously, guy-starkware wrote…
I don't know if merging from the top will make sense or not. But certainly we'd want to merge them very close together (as in, after the whole feature is reviewed)
IMO we should merge from the top
a5474a0 to
1a86558
Compare
0b45708 to
f0e1ebd
Compare
guy-starkware
left a comment
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.
@guy-starkware reviewed 18 files and all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware).
f0e1ebd to
e467c22
Compare

No description provided.