-
Notifications
You must be signed in to change notification settings - Fork 214
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
[Merged by Bors] - block certifyer: handle multiple certificates and equivocation #3736
[Merged by Bors] - block certifyer: handle multiple certificates and equivocation #3736
Conversation
bors try |
tryBuild succeeded: |
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.
LGTM, just added a comment for a possible performance improvement.
sql/certificates/certs.go
Outdated
return nil | ||
} | ||
|
||
// GetHareOutput returns the block that's valid as hare output for the spcified layer. |
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.
Typo:
// GetHareOutput returns the block that's valid as hare output for the spcified layer. | |
// GetHareOutput returns the block that's valid as hare output for the specified layer. |
sql/certificates/certs.go
Outdated
// if there are more than one valid blocks, return types.EmptyBlockID. | ||
func GetHareOutput(db sql.Executor, lid types.LayerID) (types.BlockID, error) { | ||
var ( | ||
result []types.BlockID |
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.
result can be just a types.BlockID
if rows > 1 then it can return types.EmptyBlockID
mesh/mesh.go
Outdated
if err != nil { | ||
logger.With().Warning("failed to get hare output", layerID, log.Err(err)) | ||
return nil, fmt.Errorf("%w: get hare output %v", errMissingHareOutput, err.Error()) | ||
} | ||
// hare output an empty layer | ||
// hare output an empty layer, or the network equivocated with multiple valid certificate |
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.
turns out there is a case when multiple valid certificates can exist even without equivocation, so maybe equivocation can be omitted from this description
800 is an expected number of eligibility (so it can be 802 for example), and then both sides of 50/50 split can succeed in forming valid certs
Co-authored-by: Matthias Fasching <5011972+fasmat@users.noreply.github.com>
bors merge |
## Motivation <!-- Please mention the issue fixed by this PR or detailed motivation --> Closes #3467 <!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged --> ## Changes <!-- Please describe in detail the changes made --> - sql: add table `certificates` to hold certificates. the reason why the certificate is not a property of a block is that the network will certify a empty layer (using `types.EmptyBlockID` in certificate) when the hare output an empty set. - mesh, tortoise: while multiple valid certificates are stored in the table, when requesting the block to apply/vote for, `certificates.GetHareOutput` will return `types.EmptyBlockID` as hare output when there are multiple valid certificates - sync, fetcher: only asked peers for certificate when the nodes doesn't have any. only serve a certificate to peers when there is just one
Build failed: |
bors merge |
## Motivation <!-- Please mention the issue fixed by this PR or detailed motivation --> Closes #3467 <!-- `Closes #XXXX, closes #XXXX, ...` links mentioned issues to this PR and automatically closes them when this it's merged --> ## Changes <!-- Please describe in detail the changes made --> - sql: add table `certificates` to hold certificates. the reason why the certificate is not a property of a block is that the network will certify a empty layer (using `types.EmptyBlockID` in certificate) when the hare output an empty set. - mesh, tortoise: while multiple valid certificates are stored in the table, when requesting the block to apply/vote for, `certificates.GetHareOutput` will return `types.EmptyBlockID` as hare output when there are multiple valid certificates - sync, fetcher: only asked peers for certificate when the nodes doesn't have any. only serve a certificate to peers when there is just one
Pull request successfully merged into develop. Build succeeded: |
Motivation
Closes #3467
Changes
sql: add table
certificates
to hold certificates. the reason why the certificate is not a property of a block is that the network will certify a empty layer (usingtypes.EmptyBlockID
in certificate) when the hare output an empty set.mesh, tortoise: while multiple valid certificates are stored in the table, when requesting the block to apply/vote for,
certificates.GetHareOutput
will returntypes.EmptyBlockID
as hare output when there are multiple valid certificatessync, fetcher: only asked peers for certificate when the nodes doesn't have any. only serve a certificate to peers when there is just one