-
Notifications
You must be signed in to change notification settings - Fork 33
Notify rpc of new root and supermajority roots #70
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
Conversation
core/src/replay_stage.rs
Outdated
| .then_some(r_bank_forks.get(maybe_new_root).unwrap()) | ||
| }; | ||
| if let Some(new_root_bank) = maybe_new_root_bank { | ||
| let new_root = new_root_bank.slot(); |
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: new_root-> new_root_slot ?
core/src/replay_stage.rs
Outdated
| rpc_subscriptions.notify_subscribers(CommitmentSlots { | ||
| slot: new_root, | ||
| root: new_root, | ||
| highest_confirmed_slot: new_root, |
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.
Is there any value advertising slots who got notarize cert but not finalization cert? I'm mostly worried about the case where we would do async execution later, so that finalization may be several slots away from notarization, but I guess for now this is fine.
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.
notarization doesn't guarantee finality, so it's not safe to advertise it under OC/root levels of finality
We can introduce another finality level in the future for this if desired
core/src/replay_stage.rs
Outdated
| return; | ||
| } | ||
| rpc_subscriptions.notify_subscribers(CommitmentSlots { | ||
| slot: new_root, |
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.
slot should be the current slot right?
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.
In today's commitment_service, it's equivalent to the slot we just voted on to make the new root:
let mut new_block_commitment = BlockCommitmentCache::new(
block_commitment,
aggregation_data.total_stake,
CommitmentSlots {
slot: aggregation_data.bank.slot(),
root: aggregation_data.root,
highest_confirmed_slot: aggregation_data.root,
highest_super_majority_root,
},
);
which in this case, we voted on the the new root
Problem
Alpenglow doesn't need to rely on
AggregateCommitmentServiceto notify rpc of new rootsSummary of Changes
Fixes #