Skip to content
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

Audit #10

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Audit #10

wants to merge 22 commits into from

Conversation

dbrajovic
Copy link
Contributor

@dbrajovic dbrajovic commented Jul 4, 2023

Description

Fixed (most) issues reported by the audit.

Skipped issues: I, J, K, L, M

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Breaking changes

Certain proto types changed field type from []byte to ProposedBlock

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have added sufficient documentation in code

@dbrajovic dbrajovic self-assigned this Aug 6, 2023
@dbrajovic dbrajovic added the bug Something isn't working label Aug 6, 2023
@dbrajovic dbrajovic marked this pull request as ready for review August 14, 2023 09:05
Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for going through the audit and remedying the implementation 🙏

There are some points which need to be resolved before we proceed with merging, and issues that need to be implemented (Issue N), otherwise it looks good 💯


round1ProposalMsg *proto.Message

round0Proposer = []byte("round 0 proposer")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define the handlers here in-line?
It's much cleaner, and more in line with our other tests

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inlined

024d328

return bytes.Equal(maxRoundProposalHash, messages.ExtractProposalHash(msg))
}

func (i *IBFT) validateRoundChangeCertificate(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a log line to display the error in question when validating an RCC

1e0b261

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",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are these errors checked?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above

Comment on lines 2539 to 2543
if bytes.Equal(from, []byte("unique node")) {
return true
}

return false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify this to

return bytes.Equal(from, []byte("unique node"))

Comment on lines 497 to 502
//nolint
msg.Payload.(*proto.Message_RoundChangeData).RoundChangeData.LastPreparedProposedBlock = &proto.ProposedBlock{
Block: lastPreparedProposedBlock,
Round: 0,
}
//nolint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve these linting errors instead of specifying //nolint

Comment on lines 724 to 729
//nolint
msg.Payload.(*proto.Message_RoundChangeData).RoundChangeData.LastPreparedProposedBlock = &proto.ProposedBlock{
Block: round1PreparedProposedBlock,
Round: 1,
}
//nolint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve these linting errors instead of specifying //nolint

Comment on lines 785 to 790
//nolint
someRCMsg.Payload.(*proto.Message_RoundChangeData).RoundChangeData.LastPreparedProposedBlock = &proto.ProposedBlock{
Block: round1PreparedProposedBlock,
Round: 1,
}
//nolint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please resolve these linting errors instead of specifying //nolint

@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored

310e6ad

@@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants