8000 test(consensus): PBTS should be enabled by default in test units by cason · Pull Request #2329 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

test(consensus): PBTS should be enabled by default in test units #2329

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.

Sign up for GitHub

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

Merged
merged 14 commits into from
Feb 26, 2024
Merged

test(consensus): PBTS should be enabled by default in test units #2329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
11c09f7
hack: consensus panics if PBTS is disabled
cason Feb 20, 2024
ba1e2ca
consensus: tests have PBTS enabled by default
cason Feb 20, 2024
9537c7a
consensus: genesis file for tests have PBTS enabled
cason Feb 20, 2024
2424a9c
state: test consensus params with PBTS enabled
cason Feb 20, 2024
c2fc2ec
consensus: removed featureParams from PBTS tests
cason Feb 20, 2024
5c72f06
consensus: fix byzantine tests with default params
cason Feb 20, 2024
ee9aeae
consensus: random genesis doc with PBTS enabled
cason Feb 20, 2024
b36a348
hack: temporarily disabling test with BFT Time
cason Feb 20, 2024
40189f3
Revert "hack: consensus panics if PBTS is disabled"
cason Feb 20, 2024
723ca99
Revert "hack: temporarily disabling test with BFT Time"
cason Feb 20, 2024
938c728
Merge branch 'feature/pbts' into cason/2324-pbts-ok-tests
cason Feb 22, 2024
f9bdaa2
tests: reverting unnecessary changes, by reviewers
cason Feb 23, 2024
2e61683
tests: reverting unnecessary change, by @sergio-mena
cason Feb 23, 2024
de246aa
Merge branch 'feature/pbts' into cason/2324-pbts-ok-tests
cason Feb 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions internal/consensus/byzantine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,8 +190,8 @@ func TestByzantinePrevoteEquivocation(t *testing.T) {
extCommit = &types.ExtendedCommit{}
case lazyProposer.LastCommit.HasTwoThirdsMajority():
// Make the commit from LastCommit
veHeightParam := types.DefaultFeatureParams()
veHeightParam.VoteExtensionsEnableHeight = height
// Vote extensions are enabled by default for test units
veHeightParam := lazyProposer.state.ConsensusParams.Feature
extCommit = lazyProposer.LastCommit.MakeExtendedCommit(veHeightParam)
Copy link
Contributor

Choose a reason for hiding this comment

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

Very minor (I know this is not directly related to this PR).
Why are we passing the whole Feature struct to MakeExtendedCommit? Can't we just past the VE enable height?
If you agree but doing this takes much effort, just ignore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The method uses the helper: fp.VoteExtensionsEnabled(ec.Height). We could use the same logic used there here, but it does not look great.

Copy link
Contributor

Choose a reason for hiding this comment

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

Up to you

default: // This shouldn't happen.
lazyProposer.Logger.Error("enterPropose: Cannot propose anything: No commit for the previous block")
Expand Down Expand Up @@ -499,9 +499,9 @@ func byzantineDecideProposalFunc(ctx context.Context, t *testing.T, height int64
t.Logf("Byzantine: broadcasting conflicting proposals to %d peers", len(peers))
for i, peer := range peers {
if i < len(peers)/2 {
go sendProposalAndParts(height, round, cs, peer, proposal1, block1Hash, blockParts1)
go sendProposalAndParts(height, round, cs, peer, proposal1, block1, block1Hash, blockParts1)
} else {
go sendProposalAndParts(height, round, cs, peer, proposal2, block2Hash, blockParts2)
go sendProposalAndParts(height, round, cs, peer, proposal2, block2, block2Hash, blockParts2)
}
}
}
Expand All @@ -512,6 +512,7 @@ func sendProposalAndParts(
cs *State,
peer p2p.Peer,
proposal *types.Proposal,
block *types.Block,
blockHash []byte,
parts *types.PartSet,
) {
Expand Down Expand Up @@ -541,7 +542,7 @@ func sendProposalAndParts(
// votes
cs.mtx.Lock()
prevote, _ := cs.signVote(types.PrevoteType, blockHash, parts.Header(), nil)
precommit, _ := cs.signVote(types.PrecommitType, blockHash, parts.Header(), nil)
precommit, _ := cs.signVote(types.PrecommitType, blockHash, parts.Header(), block)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise, vote extensions don't work as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a different PR?

Copy link
Contributor
@sergio-mena sergio-mena Feb 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think you can leave the prevote one (line 544) to nil and it'll still work.
As for the precommit... oh my! I looked into it and I think you're right, there's no other way, as the production code is calling ExtendVote with the block (and checking that the block we give makes sense).

@lasarojc I think it has to be this PR, otherwise the tests won't pass. @cason unless you decide to run just this test case with vote extensions disabled (but I'm also OK with the current solution)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you can leave the prevote one (line 544) to nil and it'll still work.

Indeed, reverting.

cs.mtx.Unlock()
peer.Send(p2p.Envelope{
ChannelID: VoteChannel,
Expand Down
6 changes: 5 additions & 1 deletion internal/consensus/common_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,12 +493,12 @@ func randStateWithAppWithHeight(

func randStateWithAppWithBFTTime(nValidators int) (*State, []*validatorStub) {
c := test.ConsensusParams()
c.Feature.PbtsEnableHeight = 0 // Disable PBTS
return randStateWithAppImpl(nValidators, kvstore.NewInMemoryApplication(), c)
}

func randStateWithApp(nValidators int, app abci.Application) (*State, []*validatorStub) {
c := test.ConsensusParams()
c.Feature.PbtsEnableHeight = 1
return randStateWithAppImpl(nValidators, app, c)
}

Expand Down Expand Up @@ -920,6 +920,10 @@ func randGenesisDoc(numValidators int,
}
sort.Sort(types.PrivValidatorsByAddress(privValidators))

if consensusParams == nil {
consensusParams = test.ConsensusParams()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enables both vote extensions and PBTS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not force the callers to pass a non-nil value. They could (should?) run with different configurations.

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked around and my understanding is that test cases that don't care, just provide nil as ConsensusParams. And those that do care, they provide their ConsensusParams (and we don't touch them here). This approach is fine IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are only two calls to randGenesisDoc method. But it is also invoked by other methods, which together have 20+ calls. I think it is good enough if nil means "please use the defaults".

}

return &types.GenesisDoc{
GenesisTime: genesisTime,
InitialHeight: 1,
Expand Down
9 changes: 1 addition & 8 deletions internal/consensus/pbts_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,8 +67,6 @@ type pbtsTestConfiguration struct {
// The timestamp consensus parameters to be used by the state machine under test.
synchronyParams types.SynchronyParams

featureParams types.FeatureParams

// The setting to use for the TimeoutPropose configuration parameter.
timeoutPropose time.Duration

Expand Down Expand Up @@ -108,7 +106,7 @@ func newPBTSTestHarness(ctx context.Context, t *testing.T, tc pbtsTestConfigurat
cfg.Consensus.TimeoutPropose = tc.timeoutPropose
consensusParams := types.DefaultConsensusParams()
consensusParams.Synchrony = tc.synchronyParams
consensusParams.Feature = tc.featureParams
consensusParams.Feature.PbtsEnableHeight = 1

state, privVals := randGenesisStateWithTime(validators, consensusParams, tc.genesisTime)
cs := newStateWithConfig(cfg, state, privVals[0], kvstore.NewInMemoryApplication())
Expand Down Expand Up @@ -381,7 +379,6 @@ func TestPBTSProposerWaitsForGenesisTime(t *testing.T) {
height2ProposalTimeDeliveryOffset: 10 * time.Millisecond,
height2ProposedBlockOffset: 10 * time.Millisecond,
height4ProposedBlockOffset: 30 * time.Millisecond,
featureParams: pbtsFromHeightParams(1),
}

pbtsTest := newPBTSTestHarness(ctx, t, cfg)
Expand All @@ -406,7 +403,6 @@ func TestPBTSProposerWaitsForPreviousBlock(t *testing.T) {
Precision: 100 * time.Millisecond,
MessageDelay: 500 * time.Millisecond,
},
featureParams: pbtsFromHeightParams(1),
timeoutPropose: 50 * time.Millisecond,
genesisTime: initialTime,
height2ProposalTimeDeliveryOffset: 150 * time.Millisecond,
Expand Down Expand Up @@ -476,7 +472,6 @@ func TestPBTSTimelyProposal(t *testing.T) {
Precision: 10 * time.Millisecond,
MessageDelay: 140 * time.Millisecond,
},
featureParams: pbtsFromHeightParams(1),
timeoutPropose: 40 * time.Millisecond,
genesisTime: initialTime,
height2ProposedBlockOffset: 15 * time.Millisecond,
Expand All @@ -498,7 +493,6 @@ func TestPBTSTooFarInThePastProposal(t *testing.T) {
Precision: 1 * time.Millisecond,
MessageDelay: 10 * time.Millisecond,
},
featureParams: pbtsFromHeightParams(1),
timeoutPropose: 50 * time.Millisecond,
height2ProposedBlockOffset: 15 * time.Millisecond,
height2ProposalTimeDeliveryOffset: 27 * time.Millisecond,
Expand All @@ -520,7 +514,6 @@ func TestPBTSTooFarInTheFutureProposal(t *testing.T) {
Precision: 1 * time.Millisecond,
MessageDelay: 10 * time.Millisecond,
},
featureParams: pbtsFromHeightParams(1),
timeoutPropose: 50 * time.Millisecond,
height2ProposedBlockOffset: 100 * time.Millisecond,
height2ProposalTimeDeliveryOffset: 10 * time.Millisecond,
Expand Down
2 changes: 2 additions & 0 deletions internal/state/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1025,6 +1025,8 @@ func TestConsensusParamsChangesSaveLoad(t *testing.T) {
params[0] = state.ConsensusParams
for i := 1; i < N+1; i++ {
params[i] = *types.DefaultConsensusParams()
// FIXME: shouldn't PBTS be enabled by default?
params[i].Feature.PbtsEnableHeight = 1
params[i].Block.MaxBytes += int64(i)
}

Expand Down
2 changes: 1 addition & 1 deletion internal/test/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ var testGenesisFmt = `{
"version": {},
"feature": {
"vote_extensions_enable_height": "0",
"pbts_enable_height": "0"
"pbts_enable_height": "1"
}
},
"validators": [
Expand Down
2 changes: 2 additions & 0 deletions internal/test/params.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,7 @@ func ConsensusParams() *types.ConsensusParams {
c := types.DefaultConsensusParams()
// enable vote extensions
c.Feature.VoteExtensionsEnableHeight = 1
// enabled PBTS
c.Feature.PbtsEnableHeight = 1
return c
}
0