internal/consensus: prevote nil if proposal timestamp does not match #7391
Conversation
williambanfield
requested review from
cmwaters,
creachadair,
ebuchman and
tychoish
as code owners
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
This change introduces the logic to have the proposer wait until the previous block time has passed before attempting to propose the next block. The change achieves this by by adding a new clause into the enterPropose state machine method. The method now checks if the validator is the proposer and if the validator's clock is behind the previous block's time. If the validator's clock is behind the previous block time, it schedules a timeout to re-enter the enter propose method after enough time has passed.
* Remove MedianTime, set block time to Now() * Fix goimports * Fix import ordering
| @@ -1314,6 +1314,12 @@ func (cs *State) defaultDoPrevote(height int64, round int32) { | |||
| return | |||
| } | |||
|
|
|||
| if !cs.Proposal.Timestamp.Equal(cs.ProposalBlock.Header.Time) { | |||
| logger.Debug("proposal timestamp not equal, prevoting nil") | |||
There was a problem hiding this comment.
I think that this means that the proposed block is not valid, should we not include this checking in the ValidateBlock logic? I don't know how this method is now, with the removal of bfftime checks.
There was a problem hiding this comment.
In any case, a Error log message could be produced.
There was a problem hiding this comment.
should we not include this checking in the ValidateBlock logic?
This is a good question. I've been going back and forth on this trying to understand what the right way to shim this new information into that method would be. It means that one of three things would have to happen:
state.Statewould need to have the proposal as a field. State is currently composed of all fields that can be determined from the previous block itself.ValidateBlockwould need to take a proposal as a parameter.BlockExecutorwould need a reference to the proposal.
I'm not convinced these are great ideas. For 1., the State is currently made up of only things that can be determined from previous blocks, so we'd be changing the purpose of that type.
For 2., we use ValidateBlock when replaying old blocks. This would mean keeping old proposals as part of block verification for old blocks. I'm not sure this is a meaningful thing to do, but maybe I don't see how it is yet.
Finally, 3. Is a similar issue to 2, with the added difficulty that block executor would need some concurrency safe way to access the current round's proposal.
I think overall that 1. would make the most sense, but I'm not certain it's necessary. Let me know if you disagree or if I've missed something.
| // TestStateTimestamp_ProposalMatch tests that a validator prevotes a | ||
| // proposed block if the timestamp in the block matches the timestamp in the | ||
| // corresponding proposal message. | ||
| func TestStateTimestamp_ProposalMatch(t *testing.T) { |
There was a problem hiding this comment.
Would it be a regular check to be added in the tests of this round step? In other words, a matching proposal time and block time is the default, while the test above ensures that when it is not the case, the proposal is rejected.
There was a problem hiding this comment.
I think you're asking if we can just remove this test since it's effectively the default behavior anyway. I think that having an additional test is useful for documentation purposes. It makes it expressly clear that these two things should always match. If this relationship changes, many tests will break but this test being broken will help make it clear what the issue is for the engineer trying to understand how they broke something.
types/proposal_test.go
Outdated
| @@ -154,7 +155,7 @@ func TestProposalValidateBasic(t *testing.T) { | |||
| t.Run(tc.testName, func(t *testing.T) { | |||
| prop := NewProposal( | |||
| 4, 2, 2, | |||
| blockID) | |||
| blockID, time.Now()) | |||
There was a problem hiding this comment.
Are you using time and tmtime interchangeably?
There was a problem hiding this comment.
Ah, yeah, we canonicalize it anyway in the proposal method, but I'll change it to be consistent.
…7391) This change updates the proposal logic to use the block's timestamp in the proposal message. It adds an additional piece of validation logic to the prevote step to check that the block's timestamp matches the proposal message's timestamp.
…2221) Addresses #2119. ADR 112, describing the PBTS implementation, [states](https://github.com/cometbft/cometbft/pull/2223/files#diff-81d55f6e52eabd6cce264a76fba015de46092b9ccda241de10cd265890ab8ad9R302): The precommit step will not require much modification. Its proposal validation rules will change in the same ways that validation will change in the prevote step with the exception of the `timely` check: precommit validation will never check that the timestamp is `timely`. The changes proposed here are in line with this description. Moreover, this PR in fact revert the changes to the precommit step (`enterPrecommit` method) added by tendermint/tendermint#7480 and tendermint/tendermint#7391. --- #### PR checklist - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments - [x] Title follows the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
This change updates the proposal logic to use the block's timestamp in the proposal message. It adds an additional piece of validation logic to the prevote step to check that the block's timestamp matches the proposal message's timestamp.
This change also adds two tests that ensure that 1. if the proposal message timestamp does match, we prevote the proposal, 2. if the proposal message does not match we prevote nil.
related to: #6942