Add unit tests for `zksync_consensus_bft` (BFT-360) #39

moshababo · 2023-11-20T01:24:50Z

Adding new unit tests coverage for zksync_consensus_bft crate.

Notes

Tests are located at the leader/replica mods level.
testonly::ut_harness::UTHarness struct was added to encapsulate all testing operations, and moreover to minimize redundancy of test code.

Coverage

replica prepare
- replica_prepare_sanity
- replica_prepare_sanity_yield_leader_prepare
- replica_prepare_sanity_yield_leader_prepare_reproposal
- replica_prepare_old_view
- replica_prepare_during_commit
- replica_prepare_not_leader_in_view
- replica_prepare_already_exists
- replica_prepare_num_received_below_threshold
- replica_prepare_invalid_sig
- replica_prepare_invalid_commit_qc
- replica_prepare_high_qc_of_current_view
- replica_prepare_high_qc_of_future_view
- replica_prepare_non_validator_signer // FAILS
leader prepare
- leader_prepare_sanity
- leader_prepare_reproposal_sanity
- leader_prepare_sanity_yield_replica_commit
- leader_prepare_invalid_leader
- leader_prepare_old_view
- leader_prepare_invalid_sig
- leader_prepare_invalid_prepare_qc
- leader_prepare_invalid_high_qc
- leader_prepare_proposal_oversized_payload
- leader_prepare_proposal_mismatched_payload
- leader_prepare_proposal_when_previous_not_finalized
- leader_prepare_proposal_invalid_parent_hash
- leader_prepare_proposal_non_sequential_number
- leader_prepare_reproposal_without_quorum
- leader_prepare_reproposal_when_finalized
- leader_prepare_reproposal_invalid_block
replica commit
- replica_commit_sanity
- replica_commit_sanity_yield_leader_commit
- replica_commit_old
- replica_commit_not_leader_in_view
- replica_commit_already_exists
- replica_commit_num_received_below_threshold
- replica_commit_invalid_sig
- replica_commit_unexpected_proposal
- replica_commit_protocol_version_mismatch // FAILS
leader commit
- leader_commit_sanity
- leader_commit_sanity_yield_replica_prepare
- leader_commit_invalid_leader
- leader_commit_old
- leader_commit_invalid_sig
- leader_commit_invalid_commit_qc

brunoffranca

The direction looks good. I would just ask you to eventually move the utils to the testonly folder.

node/libs/crypto/src/bn254/error.rs

node/actors/bft/src/tests/unit_tests.rs

node/actors/bft/src/tests/mod.rs

node/actors/bft/src/replica/leader_prepare.rs

node/actors/bft/src/tests/unit_tests.rs

moshababo · 2023-11-23T21:56:24Z

The replica_prepare and replica_commit message processing triggers a panic if the creation of the respective prepare/commit quorum certificate fails. This occurs after completing all checks and updating the local state. While it shouldn't be reproducible, I added two tests that demonstrate it:

replica_commit_protocol_version_mismatch
replica_prepare_non_validator_signer

The first is protected against during top-level processing, but I couldn't find safeguards for the latter.

brunoffranca · 2023-11-24T14:57:18Z

Regarding the first test, we indeed do check the protocol version earlier. But its placement is not ideal, it's separate from all the other checks. And it only produces a tracing warning, it never produces an error that we can actually test. So I would move that check to later in the flow, so that we do all the checks for the message in the same place.
The second test shows an actual bug, we never check that the replica that is sending us a message belongs to the current validator set, we need to check it. Add that extra check for both the prepare and commit phases and we're good.

brunoffranca · 2023-11-24T14:58:21Z

Another point is if failing to create a QC should panic. Technically we could have that method just return an error, but I'm not sure how we could recover from it. By that time, the only thing we could do is discard all the replica prepare messages we got and let our turn timeout. Which I guess is better than panicking but it would still let an attacker stop the chain anyway.

moshababo · 2023-11-27T01:21:22Z

Another point is if failing to create a QC should panic. Technically we could have that method just return an error, but I'm not sure how we could recover from it. By that time, the only thing we could do is discard all the replica prepare messages we got and let our turn timeout. Which I guess is better than panicking but it would still let an attacker stop the chain anyway.

Does the current implementation support liveness in the event of a leader's timeout?

moshababo · 2023-11-27T01:33:34Z

Since all shared util code is now located in testonly mod, the tests can be moved to replica/leader mods, closer to the tested code. Any opinions?

pompon0 · 2023-11-27T08:08:13Z

Does the current implementation support liveness in the event of a leader's timeout?

Yes, and we should write tests for that.

Since all shared util code is now located in testonly mod, the tests can be moved to replica/leader mods, closer to the tested code. Any opinions?

+1, tests should be as close to the code under test as possible.

node/actors/bft/src/leader/tests.rs

node/actors/bft/src/leader/mod.rs

node/actors/bft/src/replica/tests.rs

node/actors/bft/src/testonly/ut_harness.rs

slowli · 2023-11-28T12:41:41Z

node/actors/bft/src/testonly/ut_harness.rs

+        .unwrap()
+    }
+
+    pub(crate) async fn recv_signed(&mut self) -> Option<Signed<ConsensusMsg>> {


This method seems to always return Some; could it be simplified to return Signed<_> instead of an Option?

Yes, originally it had None which turned out to be unnecessary, but the method signature remained.

I changed it now to return Result which will propagate the error type from self.pipe.recv instead of just unwrapping it: aa85e36

node/actors/bft/src/testonly/ut_harness.rs

node/actors/bft/src/leader/tests.rs

node/actors/bft/src/replica/tests.rs

# Conflicts: # node/actors/bft/src/leader/tests.rs # node/actors/bft/src/replica/tests.rs

brunoffranca

Looks good. There's only one change that needs to be committed. Approved!

node/actors/bft/src/testonly/ut_harness.rs

moshababo added 2 commits November 19, 2023 19:03

[WIP] Add unit tests for zksync_consensus_bft

5d7b2e0

Minimize defaults

8567ec8

brunoffranca reviewed Nov 20, 2023

View reviewed changes

slowli reviewed Nov 20, 2023

View reviewed changes

Use assert_matches!

ef8cc5f

moshababo force-pushed the bft_unit_tests branch from 2f2eff6 to 14be873 Compare November 21, 2023 19:39

mv make new

01b9c5d

moshababo force-pushed the bft_unit_tests branch from 14be873 to 01b9c5d Compare November 21, 2023 19:40

moshababo added 9 commits November 21, 2023 13:40

remove redundant mut

e4b260e

Remove mutate fn Option wrapper

eef02c6

Use rng from ctx

1069d7a

Move util to testonly

b489f0f

Remove redundant Ok(()) assertions

ebbcc39

Use validator::{self}

498ae64

Rename tests mods

9718ed1

Add more tests

04c9e12

Add more tests

a399c7f

Add more tests

5b98849

moshababo added 2 commits November 26, 2023 14:58

Merge branch 'main' into bft_unit_tests

3a5bf68

Post-merge fixes, code cleanup

188f3b7

Enable commented-out test

ecdea8b

moshababo force-pushed the bft_unit_tests branch from 3e47bff to ecdea8b Compare November 27, 2023 01:28

moshababo changed the title ~~[WIP] Add unit tests for zksync_consensus_bft (BFT-360)~~ Add unit tests for zksync_consensus_bft (BFT-360) Nov 27, 2023

moshababo marked this pull request as ready for review November 27, 2023 01:50

moshababo added 2 commits November 27, 2023 22:04

Fix coverage docs

1d6131c

Add docs

fab519d

moshababo requested a review from brunoffranca November 28, 2023 04:51

slowli reviewed Nov 28, 2023

View reviewed changes

moshababo added 6 commits November 28, 2023 11:00

#[cfg(test)] to tests re-exports

5488c1a

Fix indentation

bd5d831

Add explicit self:: prefix to re-exports

1aee2fe

Fix indentation

ec03c7a

Add semicolons for asserts

3600a13

Better code formatting

8a70f2c

brunoffranca reviewed Nov 28, 2023

View reviewed changes

moshababo added 6 commits November 28, 2023 15:01

Use into_iter() and zip()

8a577f9

Propagate pipe.recv error

aa85e36

num_validators as usize

7853c0b

Remove test coverage from docs

05436e3

Add fields assertions to yielded messages

1c06852

Change sanity tests to use minimally-significant validator set size

9cd6e59

moshababo force-pushed the bft_unit_tests branch from ca78013 to 9cd6e59 Compare November 29, 2023 05:15

moshababo added 5 commits November 28, 2023 23:38

Add _one suffix to relevant methods

9db19ca

Refactor leader_prepare_reproposal_without_quorum

502e791

Add reproposal sanity tests

d02ee90

Merge branch 'main' into bft_unit_tests

b03b621

# Conflicts: # node/actors/bft/src/leader/tests.rs # node/actors/bft/src/replica/tests.rs

Post-merge fixes

7690bad

moshababo requested a review from brunoffranca November 29, 2023 22:57

Small name change

6eba0bc

brunoffranca approved these changes Nov 30, 2023

View reviewed changes

node/actors/bft/src/testonly/ut_harness.rs Outdated Show resolved Hide resolved

brunoffranca added 2 commits November 30, 2023 16:45

Update node/actors/bft/src/testonly/ut_harness.rs

c33a3b6

Fix.

81bff50

brunoffranca merged commit 4acf065 into main Nov 30, 2023
4 checks passed

brunoffranca deleted the bft_unit_tests branch November 30, 2023 17:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit tests for `zksync_consensus_bft` (BFT-360) #39

Add unit tests for `zksync_consensus_bft` (BFT-360) #39

moshababo commented Nov 20, 2023 •

edited

Loading

brunoffranca left a comment

moshababo commented Nov 23, 2023

brunoffranca commented Nov 24, 2023 •

edited

Loading

brunoffranca commented Nov 24, 2023

moshababo commented Nov 27, 2023

moshababo commented Nov 27, 2023

pompon0 commented Nov 27, 2023

slowli Nov 28, 2023

moshababo Nov 28, 2023

brunoffranca left a comment

Add unit tests for zksync_consensus_bft (BFT-360) #39

Add unit tests for zksync_consensus_bft (BFT-360) #39

Conversation

moshababo commented Nov 20, 2023 • edited Loading

Notes

Coverage

brunoffranca left a comment

Choose a reason for hiding this comment

moshababo commented Nov 23, 2023

brunoffranca commented Nov 24, 2023 • edited Loading

brunoffranca commented Nov 24, 2023

moshababo commented Nov 27, 2023

moshababo commented Nov 27, 2023

pompon0 commented Nov 27, 2023

slowli Nov 28, 2023

Choose a reason for hiding this comment

moshababo Nov 28, 2023

Choose a reason for hiding this comment

brunoffranca left a comment

Choose a reason for hiding this comment

Add unit tests for `zksync_consensus_bft` (BFT-360) #39

Add unit tests for `zksync_consensus_bft` (BFT-360) #39

moshababo commented Nov 20, 2023 •

edited

Loading

brunoffranca commented Nov 24, 2023 •

edited

Loading