Adds Prepared Certificates to ensure Istanbul liveness #366

trianglesphere · 2019-07-30T18:42:46Z

Description

This PR removes the locking mechanism in IBFT and adds:

Optional PREPARED certificates to ROUND CHANGE messages, which are included when the node has seen 2F+1 PREPARE/COMMIT messages or a PREPARED certificate for this sequence.
ROUND CHANGE certificates to PREPREPARE messages, which are required for rounds > 0. If any of the messages in this certificate contain a PREPARED certificate, the PREPREPARE must propose the PREPARED block with the highest round number.

In addition, this overhauls the round change logic:

On timeout or f+1 round change messages, transition to StateWaitingForNewRound and if the new desired round (one past the timeout round or the RC message view), it will set the new desired round, send a round change message for the desired round, set a timeout for the desired round. (All these actions done in core.waitForDesiredRound)
On quorum RC messages or a round change certificate where the round associated with the messages is >= desired round, start a new round at the desired round and proceed with the normal consensus protocol.

This addresses a bug in which F nodes could lock onto block A, while 2F nodes could lock onto block B, causing a permanent loss of liveness.

This PR also changes how the randomness source is verified to fix an issues with validators correctly re-proposing blocks.

Tested

Unit tests in consensus pass.
Testnets for old versions were working, going to spin up a testnet with latest.

Testing Changes:

Changing the round change message payload is needed to match what validators
actually do.
errIgnored -> nil exists b/c I no longer return errIgnored from handleRoundChange
Check for consistent commits in TestCommitsBlockAfterRoundChange and TestPreparedCertificatePersistsThroughRoundChange because prepared certificates are generated, but no single node has committed, so it is fine to commit a different block than the original block from validator 0 as long as there is not a fork.

Other changes

Verify the CommittedSeal included in COMMIT messages
Adds create empty block with empty randomness. Used for a null value when creating a prepared certificate.
Lots of testing changes, logging + match other changes
Removes src instanbul.Validator from being passed around endlessly
Namespaces istanbul types
BLS changes merged in (some slight changes in verifying commit seal)
More logging. Testing only shows core.current is nil in startNewRound and not in the message handlers, but I still included nil checks there.

Related issues

Fixes Devs SBAT be sure of Istanbul liveness #116

Backwards compatibility

No.

Edge case where prepared view could be round 0, but the prepared certificate would never be recorded as the largest prepared certificate seen in the round change certificate.

.circleci/config.yml

consensus/istanbul/core/backlog.go

consensus/istanbul/core/commit.go

consensus/istanbul/core/core.go

asaj · 2019-09-06T01:52:18Z

consensus/istanbul/core/request.go

@@ -33,8 +41,9 @@ func (c *core) handleRequest(request *istanbul.Request) error {
 	logger.Trace("handleRequest", "number", request.Proposal.Number(), "hash", request.Proposal.Hash())

 	c.current.pendingRequest = request
-	if c.state == StateAcceptRequest {
-		c.sendPreprepare(request)
+	// Must go through startNewRound to send a proposal that matches the round change certificate if not on the first round.


In that case, maybe change the comment to Must go through startNewRound to send proposals for round > 0?

asaj · 2019-09-06T01:54:16Z

consensus/istanbul/core/roundchange.go

+		}
+
+		// Verify ROUND CHANGE message is for a proper view
+		// TODO(joshua): May be able to relax to the proposal is >= than the round change message.


I still don't understand. Wouldn't the proposal need to be for the view that the round change certificate moves us to?

asaj · 2019-09-06T01:57:05Z

consensus/istanbul/core/roundchange.go

+			if err := c.verifyPreparedCertificate(roundChange.PreparedCertificate); err != nil {
+				return err
+			}
+			// The prepared certificate with the highest round number carries the proposal we must use


Can you elaborate in the comment why the prepared certificate may have a previous view?

asaj · 2019-09-06T01:57:24Z

consensus/istanbul/core/roundchange.go

+			}
+			// The prepared certificate with the highest round number carries the proposal we must use
+			preparedView := roundChange.PreparedCertificate.View()
+			if preparedView != nil && preparedView.Round.Cmp(maxRound) > 0 {


What happens if the prepared certificate has a later view than the round change messages?

Good catch. My feeling is that we should reject the round change certificate for having an invalid round change message. I'd probably also need to change handleRoundChange to verify the prepared certificate view against the round change message view.

consensus/istanbul/core/roundchange.go

Longer name, but more accurate.

This builds a logger with the current sequence, round, and state.

Explains why we need to use the proposal from the prepared certificate with the largest round number.

Reject RC messages where the prepared view is greater than the proposal view.

consensus/istanbul/backend/backend.go

Reading through CalcProposer and LastProposal, the last proposer is the proposer of the last block, and calc proposer does not rely on the current proposer, thus I think it's fine if there is a large jump in round number in this function.

asaj

Wow, this is a big one, nice work 👍

Please wait until @timmoreton or @kevjue finish reviewing before merging

asaj · 2019-09-06T22:57:40Z

consensus/istanbul/core/core.go

@@ -96,6 +95,21 @@ type core struct {
 	consensusTimer metrics.Timer
 }

+// Appends the current view and state to the given context.
+func (c *core) NewLogger(ctx ...interface{}) log.Logger {


asaj · 2019-09-06T23:00:34Z

consensus/istanbul/core/preprepare.go

 				c.sendCommitForOldBlock(preprepare.View, preprepare.Proposal.Hash())
 				return nil
 			}
 		}
+		// Already in the next round b/c handleRoundChangeCertificate moves us to the next round.


Hmm okay I think I was just confused by the comment, consider making it more verbose/explicit

trianglesphere · 2019-09-16T17:57:50Z

@timmoreton @asaj Are you ready yet?

* Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2

* Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2

* contract_comm/currency/currency.go * fixed the txn price-sorted min-heap * merge master (#490) * Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2

* added new option --use-in-memory-discovery-table * merge master (#489) * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2

* Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * tx price heap fix (#471) * contract_comm/currency/currency.go * fixed the txn price-sorted min-heap * merge master (#490) * Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * added new option --use-in-memory-discovery-table (#479) * added new option --use-in-memory-discovery-table * merge master (#489) * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * Allow v4/v5 on a bootnode simultaneously, allow mobile to use discv5 (#454) * changes for isolating celo networks * changes to get unit tests working * changes to add salt in the discovery packets * removed checking for the ip address when handling a reply * added ping-ip-from-packet option to bootnode * for handling expected replies, don't filter on expected sender ip address if --pingIPFromPacket is used * Add v4 flag * Add unhandled and quicken docker builds * Add salt & logs * Add v4 flag * Add PeerDiscovery to mobile node config * Remove logs * Remove hardcoded bootnodes * Add salt & turn on discv5 * Delete hardcoded eth bootnodes * Make Discoveryv5 configurable * Lint * Add comment to bootnode v4/v5 handling * Change PeerDiscovery -> NoDiscovery * Remove mobile geth no discovery * Reduce istanbul default timeout, cap exp backoff (#475) * Reduce istanbul default timeout, cap exp backoff * Ensure round 0 timeout factors in block period * Sanitize logs (#495) * Change registry lookup and infrastructure lookup error logs to debug level * Sanitize logs regarding registry deployment * Change empty abi logging and comment * Lower log level from error to warning for potentially outdated istanbul messages * Change back to an error message * Add input length checks for precompiled contracts (#476) * add input length checks * check exact input length. add a new error for input length. check input in a few more places * add tests that verify the input-length checks for contracts that don't require an evm instance * fix formatting * add comments to explain input length checks * run the formatter * e2e transfer test was failing because it passes in a transaction options object, making the input larger than 96 bytes * e2e tests have revealed that our precompiled contracts need to be tolerant of inputs that are longer than the bytes that are actually read

* Log on ValidatorElections * merge master (#496) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * tx price heap fix (#471) * contract_comm/currency/currency.go * fixed the txn price-sorted min-heap * merge master (#490) * Add precompiles to access validator set (#441) * set max gas to double of the charged gas for the 'intrinsic' smart contract calls (#472) * set max gas to double of the charged gas for the 'intrinsic' evm operations * addressed PR comments * addressed pr comment * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * added new option --use-in-memory-discovery-table (#479) * added new option --use-in-memory-discovery-table * merge master (#489) * Adds Prepared Certificates to ensure Istanbul liveness (#366) * Check message address against signature (#477) * Check signing validator's address matches msg address * Add comments about use of sig data in tests * Try fix Circle build test failures * Try fix Circle build test failures, take 2 * Allow v4/v5 on a bootnode simultaneously, allow mobile to use discv5 (#454) * changes for isolating celo networks * changes to get unit tests working * changes to add salt in the discovery packets * removed checking for the ip address when handling a reply * added ping-ip-from-packet option to bootnode * for handling expected replies, don't filter on expected sender ip address if --pingIPFromPacket is used * Add v4 flag * Add unhandled and quicken docker builds * Add salt & logs * Add v4 flag * Add PeerDiscovery to mobile node config * Remove logs * Remove hardcoded bootnodes * Add salt & turn on discv5 * Delete hardcoded eth bootnodes * Make Discoveryv5 configurable * Lint * Add comment to bootnode v4/v5 handling * Change PeerDiscovery -> NoDiscovery * Remove mobile geth no discovery * Reduce istanbul default timeout, cap exp backoff (#475) * Reduce istanbul default timeout, cap exp backoff * Ensure round 0 timeout factors in block period * Sanitize logs (#495) * Change registry lookup and infrastructure lookup error logs to debug level * Sanitize logs regarding registry deployment * Change empty abi logging and comment * Lower log level from error to warning for potentially outdated istanbul messages * Change back to an error message * Add input length checks for precompiled contracts (#476) * add input length checks * check exact input length. add a new error for input length. check input in a few more places * add tests that verify the input-length checks for contracts that don't require an evm instance * fix formatting * add comments to explain input length checks * run the formatter * e2e transfer test was failing because it passes in a transaction options object, making the input larger than 96 bytes * e2e tests have revealed that our precompiled contracts need to be tolerant of inputs that are longer than the bytes that are actually read

Asa Oines added 30 commits May 13, 2019 13:36

WIP

90a2628

WIP

577124e

Some cleanup

5a4de8a

Wip

29ec4e7

Merge branch 'master' into asaj/liveness

80971aa

Compiling

9e73e89

Core tests compiling

bd9c6e0

Tests passing, woohoo

e3fb5ea

Test passing

2e4e758

Tests passing

97e1863

More testing

0b7d372

More testing

6fa3ef0

Share code

c31c7b7

More testing

a9f76ee

Test for handleRoundChange

961f9b7

More tests

21a79f2

Still working

3c87376

Liveness test passing

f46c2d5

CLeanup

5878c32

Check prepared certificatew message views

f029329

Cleanup

9887f48

More testing

d16ee8c

Handle round change messages in certificate

1e435dc

More testing

68f4061

Tests pass

3d47ba3

Cleanup

f40b739

Fix typo

4c9e93a

Address comments

e4a66d5

Create prepared certificate in handleCommit

5cec289

Compile

4dd80f3

Joshua Gutow added 3 commits September 5, 2019 15:48

Verify prepared certificate before use

a6550a6

Gofmt

0ffdb64

Fixes round change cert verification

c61d667

Edge case where prepared view could be round 0, but the prepared certificate would never be recorded as the largest prepared certificate seen in the round change certificate.

asaj reviewed Sep 6, 2019

View reviewed changes

Joshua Gutow added 12 commits September 6, 2019 09:57

Update circle ci config

5dcb4af

Renames nextPreprepare to be more clear

f8890f0

Longer name, but more accurate.

Adds logger builder function

11bb7eb

This builds a logger with the current sequence, round, and state.

Cleans up logging in handleTimeout

8e96b94

Cleans up comments

9b0b18e

Updates comment

38710de

Adds some logging

932e980

Removes comment

903897f

Clarifies comment about round change messages

0775b83

Adds a giant comment round change cert proposal selection

daf7297

Explains why we need to use the proposal from the prepared certificate with the largest round number.

Verifies prepared certificate view against round change view

41c4c8c

Rejects inconcistent RC messages in handleRoundChangeCert

55346ee

Reject RC messages where the prepared view is greater than the proposal view.

trianglesphere commented Sep 6, 2019

View reviewed changes

consensus/istanbul/backend/backend.go Outdated Show resolved Hide resolved

Joshua Gutow added 2 commits September 6, 2019 11:25

Removes comment

5468705

Reading through CalcProposer and LastProposal, the last proposer is the proposer of the last block, and calc proposer does not rely on the current proposer, thus I think it's fine if there is a large jump in round number in this function.

Removes IsKnownMessage

fbae278

asaj approved these changes Sep 6, 2019

View reviewed changes

Clarifies comment

c7c535a

asaj merged commit 0344742 into master Sep 24, 2019

trianglesphere deleted the trianglesphere/istanbul-liveness branch October 8, 2019 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds Prepared Certificates to ensure Istanbul liveness #366

Adds Prepared Certificates to ensure Istanbul liveness #366

trianglesphere commented Jul 30, 2019 •

edited

Loading

asaj Sep 6, 2019

asaj Sep 6, 2019

asaj Sep 6, 2019

asaj Sep 6, 2019

trianglesphere Sep 6, 2019

asaj left a comment

asaj Sep 6, 2019

asaj Sep 6, 2019

trianglesphere commented Sep 16, 2019

Adds Prepared Certificates to ensure Istanbul liveness #366

Adds Prepared Certificates to ensure Istanbul liveness #366

Conversation

trianglesphere commented Jul 30, 2019 • edited Loading

Description

Tested

Other changes

Related issues

Backwards compatibility

asaj Sep 6, 2019

Choose a reason for hiding this comment

asaj Sep 6, 2019

Choose a reason for hiding this comment

asaj Sep 6, 2019

Choose a reason for hiding this comment

asaj Sep 6, 2019

Choose a reason for hiding this comment

trianglesphere Sep 6, 2019

Choose a reason for hiding this comment

asaj left a comment

Choose a reason for hiding this comment

asaj Sep 6, 2019

Choose a reason for hiding this comment

asaj Sep 6, 2019

Choose a reason for hiding this comment

trianglesphere commented Sep 16, 2019

trianglesphere commented Jul 30, 2019 •

edited

Loading