-
Notifications
You must be signed in to change notification settings - Fork 51
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
Add remaining *2 types #3932
Add remaining *2 types #3932
Conversation
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.
Nothing major, just a few comments.
crates/task-impls/src/da.rs
Outdated
payload_commit: payload_commitment, | ||
epoch: self.cur_epoch, |
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.
It would be good to use the epoch from the proposal. Something like 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.
agreed! change: 2fed4ef
crates/task-impls/src/da.rs
Outdated
encoded_transactions: Arc::clone(encoded_transactions), | ||
metadata: metadata.clone(), | ||
// Upon entering a new view we want to send a DA Proposal for the next view -> Is it always the case that this is cur_view + 1? | ||
view_number, | ||
epoch_number: self.cur_epoch, |
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.
This is fine here because we're proposing a new block and we don't know its block number yet. We need to rely on the task keeping track of the current epoch.
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.
your comment led me to look at this again, and I think I fixed it "properly": 3ef6a5b
this now takes its epoch number directly from the ViewChange
event that triggered the transaction request
crates/task-impls/src/network.rs
Outdated
@@ -115,18 +114,34 @@ impl<TYPES: NodeType> NetworkMessageTaskState<TYPES> { | |||
HotShotEvent::UpgradeVoteRecv(message) | |||
} | |||
GeneralConsensusMessage::HighQc(qc) => HotShotEvent::HighQcRecv(qc, sender), | |||
GeneralConsensusMessage::Proposal2(proposal) => { | |||
HotShotEvent::QuorumProposalRecv(convert_proposal(proposal), sender) |
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 this conversion here necessary?
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.
oops. removed: 18a6c10
Adds the remaining new types. This includes logic for
DaData2
and its associated vote/certificate, but not for any timeout or view sync types. Those require some additional changes, which are being made in #3893.Key places to review:
The changes in
task-impls
are the most relevant things to review; the remaining changes are mostly boilerplate for the added types.