Audit #10
Audit #10
Conversation
|
|
||
| round1ProposalMsg *proto.Message | ||
|
|
||
| round0Proposer = []byte("round 0 proposer") |
There was a problem hiding this comment.
This is the only value that is shared in the test (checked against the proposal), the rest (below) are only used to initialize fields.
Please inline them where they are used
| backend mockBackend | ||
| transport mockTransport | ||
| msgs mockMessages | ||
|
|
There was a problem hiding this comment.
Why not define the handlers here in-line?
It's much cleaner, and more in line with our other tests
| return bytes.Equal(maxRoundProposalHash, messages.ExtractProposalHash(msg)) | ||
| } | ||
|
|
||
| func (i *IBFT) validateRoundChangeCertificate( |
There was a problem hiding this comment.
Why return an error from this method, if you're only using it as a boolean value anyways?
Not to mention that the error is not checked anywhere (typed) specifically
There was a problem hiding this comment.
Added a log line to display the error in question when validating an RCC
| if len(certificate.RoundChangeMessages) < int(i.backend.Quorum(height)) { | ||
| return false | ||
| if len(certificate.RoundChangeMessages) < int(i.backend.Quorum(view.Height)) { | ||
| return fmt.Errorf("no quorum round change messages in certificate: expected=%d actual=%d", |
There was a problem hiding this comment.
Where are these errors checked?
core/ibft_test.go
Outdated
| if bytes.Equal(from, []byte("unique node")) { | ||
| return true | ||
| } | ||
|
|
||
| return false |
There was a problem hiding this comment.
You can simplify this to
return bytes.Equal(from, []byte("unique node"))
core/ibft_test.go
Outdated
| //nolint | ||
| msg.Payload.(*proto.Message_RoundChangeData).RoundChangeData.LastPreparedProposedBlock = &proto.ProposedBlock{ | ||
| Block: lastPreparedProposedBlock, | ||
| Round: 0, | ||
| } | ||
| //nolint |
There was a problem hiding this comment.
Please resolve these linting errors instead of specifying //nolint
core/ibft_test.go
Outdated
| //nolint | ||
| msg.Payload.(*proto.Message_RoundChangeData).RoundChangeData.LastPreparedProposedBlock = &proto.ProposedBlock{ | ||
| Block: round1PreparedProposedBlock, | ||
| Round: 1, | ||
| } | ||
| //nolint |
There was a problem hiding this comment.
Please resolve these linting errors instead of specifying //nolint
core/ibft_test.go
Outdated
| //nolint | ||
| someRCMsg.Payload.(*proto.Message_RoundChangeData).RoundChangeData.LastPreparedProposedBlock = &proto.ProposedBlock{ | ||
| Block: round1PreparedProposedBlock, | ||
| Round: 1, | ||
| } | ||
| //nolint |
There was a problem hiding this comment.
Please resolve these linting errors instead of specifying //nolint
messages/proto/messages.proto
Outdated
| @@ -46,7 +46,7 @@ message Message { | |||
| // PrePrepareMessage is the message for the PREPREPARE phase | |||
| message PrePrepareMessage { | |||
| // proposal is the actual data being proposed for consensus | |||
| bytes proposal = 1; | |||
| ProposedBlock proposal = 1; | |||
There was a problem hiding this comment.
Why not change the Backend interface to also accept the round number, or the new proto type?
This concern is outlined in Issue G, and it can be resolved by this.
This is the method I'm talking about:
// InsertBlock inserts a proposal with the specified committed seals
InsertBlock(proposal []byte, committedSeals []*messages.CommittedSeal)There was a problem hiding this comment.
Refactored
| @@ -623,7 +626,7 @@ func (i *IBFT) validateProposal0(msg *proto.Message, view *proto.View) bool { | |||
| ) | |||
|
|
|||
| // proposal must be for round 0 | |||
| if msg.View.Round != 0 { | |||
| if messages.ExtractProposal(msg).Round != 0 { | |||
There was a problem hiding this comment.
Unrelated to this check here, but wanted to point out an additional check is missing (both from the pseudocode and the implementation), and it is referenced as part of Issue N:
There is no check that the r_pp from the round change message matches the round from the proposal message (r_EB), and this check is required for this to be safe.
Please implement this, along with a coverage test
There was a problem hiding this comment.
Added the check in a03cc81
The check is already triggered with existing tests effectively bypassing subsequent checks triggered by the other cases (mostly due to how messages are generated with test helpers)
The testing suite has been greatly improved in v2 branch, so I won't be amending the cases present on this one
Description
Fixed (most) issues reported by the audit.
Skipped issues: I, J, K, L, M
Changes include
Breaking changes
Certain proto types changed field type from
[]bytetoProposedBlockChecklist