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 Suggestion 5: Unnecessary checks increase complexity #355

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

Conversation

MatheusFranco99
Copy link
Contributor

@MatheusFranco99 MatheusFranco99 commented Jan 31, 2024

Overview

We changed the isProposalJustification function according to the QBFT formal specification (Audit suggestion 5).

To compare the changes, follow up the QBFT formal spec isProposalJustification and validRoundChange functions.

Basically, we used to:

  • Verify all Round-Change messages and their nested prepare messages.
  • Take one of the highest prepared Round-Change messages and compare its data to the attached prepare messages

Now, we:

  • Verify all Round-Change messages without verifying their justifications.
  • Check if one of the highest-prepared Round-Change messages is in accordance with the attached prepare messages. To be "in accordance" means to have the same proposed data.

Dependency

This PR is built upon the Fix - Misaligned terms PR's branch.

Discussion topics

  • If a proposal has no prepared round-change messages but includes a justification of prepare messages, it will be accepted. This is due to the fact that, if there's no prepared round-change, the validation will return nil without checking the prepare messages. Should we add a check to ensure that the prepare messages should be empty in this case?
  • A correctly signed prepared round-change message without justification is:
    • valid if it's included in the justification of a proposal msg
    • invalid as a single RC message.
      We could add a quorum check in the validation of the proposal justification, but what would be the point if we don't inspect the contents of the RC justification?

Performance implications

All times are in milliseconds.

Max processing time for the Proposal message

Round 1 Round 2 Round 3
Old 1.5 18 18
New 1.5 8 8

Max processing time for a consensus round

1st round 2nd round 3rd round
Old 13 64 64
New 13 44 44

The difference is 20ms (not 10ms as one could expect) because, once a quorum of Round-Change messages is received, the isReceivedProposalJustification function is called.

Namely, look at the break-down times of each phase for round 2:

Round-Changes Proposal Prepares Commits
Old 4, 9, 21 18 1, 1, 1, 1 1, 1, 1, 1
New 4, 9, 11 8 1, 1, 1, 1 1, 1, 1, 1

Tests

In the following, we list existing and added tests for the proposal phase.

The existing tests are listed for one to check the test coverage.

Valid

Existing:

  • Valid proposal for rounds 1 and 2 (with prepared data)
  • Valid proposal for round 5 with no preparation
  • Proposal for a future round
  • Proposal for a future round not prepared
  • Proposal for the first round

Proposal fields

Message{
		MsgType(ProposalMsgType),
		Height,
		Round,
		Identifier,
		Root,
		RoundChangeJustification,
		ProposalJustification,
	}
SignedMessage{
		Signature,
		Signers([]types.OperatorID{state.Share.OperatorID}),
		Message,
		FullData,
	}

Existing:

  • Wrong height
  • Old round
  • Old round with no preparation
  • Round 15
  • Invalid identifier
  • Wrong signature
  • Non-leader
  • Unknown signer (not in committee)
  • Multi-signers
  • Full-data: nil
  • Full-data: doesn't pass the value check

Added:

  • No Root
  • Wrong full data

Message time or multiplicity

Existing:

  • A second proposal with the same data
  • A second proposal with different data
  • A second proposal with the same data
  • A second proposal after the preparation phase
  • A second proposal after the commit phase
  • A proposal after force-stop

Justification - Quorum checks

Existing:

  • No quorum of prepare msgs (2 messages)
  • No quorum of RC msgs (2 messages)
  • Duplicate RC msg in 4 msgs -> Valid
  • Duplicate RC msg in 3 msgs -> No quorum
  • Duplicate Prepare msg in 4 msgs -> Valid
  • Duplicate Prepare msg in 3 msgs -> No quorum
  • No quorum of RC msgs (3 messages, one with wrong signer -> no quorum)
  • No quorum of RC msgs (3 messages, one with wrong signer -> no quorum)
  • Duplicate RC msg (no quorum)
  • No quorum of RC msgs (2 messages)

Justification - Logic

Existing:

  • Invalid proposal with highest prepared RC with diff data round than prepare msg justification
  • Invalid proposal with a prepare msg with a root value that differs from others
  • Invalid proposal with a prepare msg with a round value that differs from others

Added:

  • One RC message with different round
  • All RC messages with different round
  • Prepare messages with round > highest prepared RC round
  • Prepare messages but no prepared RC
  • Single prepared RC with data != prepare msgs data
  • Single prepared RC with data == prepare msgs data
  • All RC prepared but all with data != prepare msg data
  • All RC prepared, only one with data == prepare msg data
  • RC messages with invalid justification (should pass since it's not checked anymore)
  • RC messages with empty justification (should pass since it's not checked anymore)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants