-
Notifications
You must be signed in to change notification settings - Fork 62
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
Fix DoubleProposalProof
s
#2983
base: albatross
Are you sure you want to change the base?
Fix DoubleProposalProof
s
#2983
Conversation
@@ -405,13 +419,27 @@ impl DoubleProposalProof { | |||
|
|||
if self.header1.block_number != self.header2.block_number | |||
|| self.header1.round != self.header2.round |
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 check still correct or should it go away?
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.
I would say this was not correct before and it should be removed. The round in the header only denotes when a proposal was created, not when it was proposed. Thus it should not be checked here.
mut justification1: SchnorrSignature, | ||
mut header2: MacroHeader, | ||
mut round2: u32, | ||
mut valid_round2: Option<u32>, |
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.
Should I merge the two round
parameters into one, given that they have to be the same?
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.
I would prefer these be TendermintProposal
, making the signature much smaller. If that turns out to be annoying or to require unnecessary cloning, then I think one round could be removed yes.
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 to me outside of the MacroHeader.round
check being incorrect (existed prior to this PR). So that should be removed. Other comments are purely cosmetic .
header1: MacroHeader, | ||
/// Header number 2. | ||
header2: MacroHeader, | ||
round1: u32, | ||
round2: u32, | ||
valid_round1: Option<u32>, | ||
valid_round2: Option<u32>, |
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.
I would prefer
header1: MacroHeader, | |
/// Header number 2. | |
header2: MacroHeader, | |
round1: u32, | |
round2: u32, | |
valid_round1: Option<u32>, | |
valid_round2: Option<u32>, | |
proposal1: TendermintProposal, | |
/// Proposal number 2. | |
proposal2: TendermintProposal, |
mut justification1: SchnorrSignature, | ||
mut header2: MacroHeader, | ||
mut round2: u32, | ||
mut valid_round2: Option<u32>, |
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.
I would prefer these be TendermintProposal
, making the signature much smaller. If that turns out to be annoying or to require unnecessary cloning, then I think one round could be removed yes.
@@ -405,13 +419,27 @@ impl DoubleProposalProof { | |||
|
|||
if self.header1.block_number != self.header2.block_number | |||
|| self.header1.round != self.header2.round |
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.
I would say this was not correct before and it should be removed. The round in the header only denotes when a proposal was created, not when it was proposed. Thus it should not be checked here.
263c16e
to
117207b
Compare
They required data unrelated to the signed proposals. It now uses the same signing infrastructure by factoring it out to `TendermintProposal` in a common crate. Fixes #2981.
The round in the proposal message is the relevant one.
117207b
to
4f21297
Compare
They required data unrelated to the signed proposals. It now uses the same signing infrastructure by factoring it out to
TendermintProposal
in a common crate.Fixes #2981.