Skip to content

Fix pbts tests#7413

Merged
ancazamfir merged 6 commits intowb/proposer-based-timestampsfrom
anca/fix_pbts_tests
Dec 21, 2021
Merged

Fix pbts tests#7413
ancazamfir merged 6 commits intowb/proposer-based-timestampsfrom
anca/fix_pbts_tests

Conversation

@ancazamfir
Copy link
Contributor

wb/proposer-based-timestamps pbts tests are broken.

  • fix compilation
  • fix the tests

@williambanfield
Copy link
Contributor

williambanfield commented Dec 9, 2021

Thanks for putting this up!

I see the issue that is causing the tests that you have marked t.Skip to fail. The log line for the failure is this one:

pbts_test.go:154: Proposed block does not match expected block (3014B6B9C2CDC40F88CE4C04EEA0BA377C1EB1B4F69993D92ADBC6EFB0CCF493:1:650370E0444C != FD815DF96B5355BFDB228A2DF0E8E18906903658CE8717E80094834A9F25D660:1:780296F1521A)

The corresponding line in the test harness is this one:

ensureProposalWithTimeout(p.t, p.ensureProposalCh, p.currentHeight, p.currentRound, bid, timeout)

What is happening here is that we are now creating a different proposal in the test function than what the state machine is creating. The test is creating the block proposal here:

bid := types.BlockID{Hash: propBlock.Hash(), PartSetHeader: partSet.Header()}
, which now calls time.Now. This used to just construct the time using the data from the previous precommits, i.e. it was consistent across calls within the same height. This method is no longer consistent because a second call will produce a block with a different timestamp. One solution would be to add a tmtime.Source onto the package consensus's State struct so that we can control the time from within a test. This would mean adding a field to the struct and then initializing it with the DefaultSource in the normal case and with our test clock in tests.
The other option is to not check the block ID from within the test since it's not entirely relevant to the test. I don't have a strong preference, but option 2 is likely simpler and adds less complexity.

@ancazamfir
Copy link
Contributor Author

ancazamfir commented Dec 15, 2021

This method is no longer consistent because a second call will produce a block with a different timestamp. One solution would be to add a tmtime.Source onto the package consensus's State struct so that we can control the time from within a test. This would mean adding a field to the struct and then initializing it with the DefaultSource in the normal case and with our test clock in tests.
The other option is to not check the block ID from within the test since it's not entirely relevant to the test. I don't have a strong preference, but option 2 is likely simpler and adds less complexity.

I just tried with option 2. The two pbts tests that are left are both failing at height: 5 where 3 out of 4 validators prevote for block_id that is different than the proposer's.

@ancazamfir ancazamfir marked this pull request as ready for review December 21, 2021 19:37
@ancazamfir ancazamfir merged commit e6d8b7c into wb/proposer-based-timestamps Dec 21, 2021
@ancazamfir ancazamfir deleted the anca/fix_pbts_tests branch December 21, 2021 19:39
williambanfield pushed a commit that referenced this pull request Jan 15, 2022
* Allow nil block ID check in ensureProposalWithTimout

* William's suggestion to get the proposal from the proposer instead of
generating it.

* Remove error check on service stop
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants