Skip to content

Commit

Permalink
fix: ValidateUpdate: allow no-change updates regardless of current …
Browse files Browse the repository at this point in the history
…height (#2112)

Closes #2109

In `v0.38.2`, when the App was setting `VoteExtensionsEnableHeight` to a
value that was the same as the current one, even if is useless as a
ConsensusParams update, we were accepting it because it doesn't do any
harm.

When we released `v0.38.3` we refactored function `ValidateUpdate` and
we started rejecting those (useless but harmless) cases.

Since `v0.38.3` is a point release, we shouldn't add restrictions on
inputs to a function without a good reason for it. This PR is adding a
rule to `ValidateUpdate` so re-allow all cases where the app is not
really updating the value of `VoteExtensionsEnableHeight`, regardless of
which height we are currently in.

### Testing done

Apart form CI tasks, we carried out the following extra (manual)
activities:
* We reproduced the error reported (#2109) using our e2e test infra, by
configuring the manifest to set a vote extension to a height in the
past, which is the same height that was previously passed via the
genesis file
* [THE FIX] In the production code, we introduced an early rule in
function `ValidateUpdate` to return no error if the value to update is
equal to the current value
* We also updated two unit tests that should not be returning an error
now (and that were returning an error in `v0.38.3`)
* We re-ran the e2e tests as explained above with the new fix, and the
problem reported in #2109 no longer shows up
* Then, to make sure no other extra restrictions were introduced in
`v0.38.3`, we did the following:
* We temporarily replaced the implementation of `ValidateUpdate` with
the one existing before `v0.38.3` (these temporary changes are not
included in the PR, it was just done for testing pruposes)
* We ran the new exhaustive unit tests introduced in `v0.38.3` and
modified in this PR against the replaced (`v0.38.3`) implementation
* We went through all the UTs that are now failing in
`TestConsensusParamsUpdate_VoteExtensionsEnableHeight` (getting an
unexpected error, or not getting an expected error), to make sure they
don't represent a set of inputs to `ValidateUpdate` that were accepted
in v0.38.2 but no longer accepted with the current version (the one in
this PR)
  * We found no problematic test case

---

#### PR checklist

- [x] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [x] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Greg Szabo <16846635+greg-szabo@users.noreply.github.com>
Co-authored-by: Greg Szabo <greg@philosobear.com>
  • Loading branch information
3 people authored Jan 24, 2024
1 parent 7a2d3b4 commit 1d99e05
Show file tree
Hide file tree
Showing 4 changed files with 82 additions and 46 deletions.
10 changes: 10 additions & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -134,3 +134,13 @@ require (
)

replace github.com/cometbft/cometbft/api => ./api

retract (
// a regression was introduced
v0.38.4
// a breaking change was introduced
v0.38.3
// superseeded by v0.38.3 because of ASA-2024-001
[v0.38.0, v0.38.2]
)

77 changes: 49 additions & 28 deletions spec/abci/abci++_app_requirements.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,32 +5,53 @@ title: Requirements for the Application

# Requirements for the Application

- [Formal Requirements](#formal-requirements)
- [Consensus Connection Requirements](#consensus-connection-requirements)
- [Mempool Connection Requirements](#mempool-connection-requirements)
- [Managing the Application state and related topics](#managing-the-application-state-and-related-topics)
- [Connection State](#connection-state)
- [Concurrency](#concurrency)
- [Finalize Block](#finalizeblock)
- [Commit](#commit)
- [Candidate States](#candidate-states)
- [States and ABCI++ Connections](#states-and-abci%2B%2B-connections)
- [Consensus Connection](#consensus-connection)
- [Mempool Connection](#mempool-connection)
- [Info/Query Connection](#infoquery-connection)
- [Snapshot Connection](#snapshot-connection)
- [Transaction Results](#transaction-results)
- [Updating the Validator Set](#updating-the-validator-set)
- [Consensus Parameters](#consensus-parameters)
- [List of Parameters](#list-of-parameters)
- [Updating Consensus Parameters](#updating-consensus-parameters)
- [Query](#query)
- [Query Proofs](#query-proofs)
- [Peer Filtering](#peer-filtering)
- [Paths](#paths)
- [Crash Recovery](#crash-recovery)
- [State Sync](#state-sync)
- [Application configuration required to switch to ABCI2.0](#application-configuration-required-to-switch-to-abci-20)
- [Requirements for the Application](#requirements-for-the-application)
- [Formal Requirements](#formal-requirements)
- [Consensus Connection Requirements](#consensus-connection-requirements)
- [Mempool Connection Requirements](#mempool-connection-requirements)
- [Managing the Application state and related topics](#managing-the-application-state-and-related-topics)
- [Connection State](#connection-state)
- [Concurrency](#concurrency)
- [FinalizeBlock](#finalizeblock)
- [Commit](#commit)
- [Candidate States](#candidate-states)
- [States and ABCI++ Connections](#states-and-abci-connections)
- [Consensus Connection](#consensus-connection)
- [Mempool Connection](#mempool-connection)
- [Replay Protection](#replay-protection)
- [Info/Query Connection](#infoquery-connection)
- [Snapshot Connection](#snapshot-connection)
- [Transaction Results](#transaction-results)
- [Gas](#gas)
- [Specifics of `CheckTxResponse`](#specifics-of-checktxresponse)
- [Specifics of `ExecTxResult`](#specifics-of-exectxresult)
- [Updating the Validator Set](#updating-the-validator-set)
- [Consensus Parameters](#consensus-parameters)
- [List of Parameters](#list-of-parameters)
- [BlockParams.MaxBytes](#blockparamsmaxbytes)
- [BlockParams.MaxGas](#blockparamsmaxgas)
- [EvidenceParams.MaxAgeDuration](#evidenceparamsmaxageduration)
- [EvidenceParams.MaxAgeNumBlocks](#evidenceparamsmaxagenumblocks)
- [EvidenceParams.MaxBytes](#evidenceparamsmaxbytes)
- [ValidatorParams.PubKeyTypes](#validatorparamspubkeytypes)
- [VersionParams.App](#versionparamsapp)
- [ABCIParams.VoteExtensionsEnableHeight](#abciparamsvoteextensionsenableheight)
- [Updating Consensus Parameters](#updating-consensus-parameters)
- [`InitChain`](#initchain)
- [`FinalizeBlock`, `PrepareProposal`/`ProcessProposal`](#finalizeblock-prepareproposalprocessproposal)
- [`Query`](#query)
- [Query Proofs](#query-proofs)
- [Peer Filtering](#peer-filtering)
- [Paths](#paths)
- [Crash Recovery](#crash-recovery)
- [State Sync](#state-sync)
- [Taking Snapshots](#taking-snapshots)
- [Bootstrapping a Node](#bootstrapping-a-node)
- [Snapshot Discovery](#snapshot-discovery)
- [Snapshot Restoration](#snapshot-restoration)
- [Snapshot Verification](#snapshot-verification)
- [Transition to Consensus](#transition-to-consensus)
- [Application configuration required to switch to ABCI 2.0](#application-configuration-required-to-switch-to-abci-20)


## Formal Requirements
Expand Down Expand Up @@ -773,8 +794,8 @@ include the vote extensions from height `H`. For all heights after `H`
attached. Nevertheless, the application MAY provide 0-length
extensions.

Must always be set to a future height. Once set to a value different from
0, its value must not be changed.
Must always be set to a future height, 0, or the same height that was previously set.
Once the chain's height reaches the value set, it cannot be changed to a different value.

#### Updating Consensus Parameters

Expand Down
37 changes: 21 additions & 16 deletions types/params.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,16 @@ func (params ConsensusParams) ValidateBasic() error {

// ValidateUpdate validates the updated VoteExtensionsEnableHeight.
// | r | params...EnableHeight | updated...EnableHeight | result (nil == pass)
// | 1 | * | (nil) | nil
// | 2 | * | < 0 | VoteExtensionsEnableHeight must be positive
// | 3 | <=0 | 0 | nil
// | 4 | > 0; <=height | 0 | vote extensions cannot be disabled once enabled
// | 5 | > 0; > height | 0 | nil (disable a previous proposal)
// | 6 | * | <=height | vote extensions cannot be updated to a past height
// | 7 | <=0 | > height (*) | nil
// | 8 | (> 0) <=height | > height (*) | vote extensions cannot be modified once enabled
// | 9 | (> 0) > height | > height (*) | nil
// | 1 | * | (nil) | nil
// | 2 | * | < 0 | VoteExtensionsEnableHeight must be positive
// | 3 | <=0 | 0 | nil
// | 4 | X | X (>=0) | nil
// | 5 | > 0; <=height | 0 | vote extensions cannot be disabled once enabled
// | 6 | > 0; > height | 0 | nil (disable a previous proposal)
// | 7 | * | <=height | vote extensions cannot be updated to a past height
// | 8 | <=0 | > height (*) | nil
// | 9 | (> 0) <=height | > height (*) | vote extensions cannot be modified once enabled
// | 10 | (> 0) > height | > height (*) | nil
// The table above reflects all cases covered.
func (params ConsensusParams) ValidateUpdate(updated *cmtproto.ConsensusParams, h int64) error {
// 1
Expand All @@ -230,34 +231,38 @@ func (params ConsensusParams) ValidateUpdate(updated *cmtproto.ConsensusParams,
if params.ABCI.VoteExtensionsEnableHeight <= 0 && updated.Abci.VoteExtensionsEnableHeight == 0 {
return nil
}
// 4 & 5
// 4 (implicit: updated.Abci.VoteExtensionsEnableHeight >= 0)
if params.ABCI.VoteExtensionsEnableHeight == updated.Abci.VoteExtensionsEnableHeight {
return nil
}
// 5 & 6
if params.ABCI.VoteExtensionsEnableHeight > 0 && updated.Abci.VoteExtensionsEnableHeight == 0 {
// 4
// 5
if params.ABCI.VoteExtensionsEnableHeight <= h {
return fmt.Errorf("vote extensions cannot be disabled once enabled"+
"old enable height: %d, current height %d",
params.ABCI.VoteExtensionsEnableHeight, h)
}
// 5
// 6
return nil
}
// 6 (implicit: updated.Abci.VoteExtensionsEnableHeight > 0)
// 7 (implicit: updated.Abci.VoteExtensionsEnableHeight > 0)
if updated.Abci.VoteExtensionsEnableHeight <= h {
return fmt.Errorf("vote extensions cannot be updated to a past or current height, "+
"enable height: %d, current height %d",
updated.Abci.VoteExtensionsEnableHeight, h)
}
// 7 (implicit: updated.Abci.VoteExtensionsEnableHeight > h)
// 8 (implicit: updated.Abci.VoteExtensionsEnableHeight > h)
if params.ABCI.VoteExtensionsEnableHeight <= 0 {
return nil
}
// 8 (implicit: params.ABCI.VoteExtensionsEnableHeight > 0 && updated.Abci.VoteExtensionsEnableHeight > h)
// 9 (implicit: params.ABCI.VoteExtensionsEnableHeight > 0 && updated.Abci.VoteExtensionsEnableHeight > h)
if params.ABCI.VoteExtensionsEnableHeight <= h {
return fmt.Errorf("vote extensions cannot be modified once enabled"+
"enable height: %d, current height %d",
params.ABCI.VoteExtensionsEnableHeight, h)
}
// 9 (implicit: params.ABCI.VoteExtensionsEnableHeight > h && updated.Abci.VoteExtensionsEnableHeight > h)
// 10 (implicit: params.ABCI.VoteExtensionsEnableHeight > h && updated.Abci.VoteExtensionsEnableHeight > h)
return nil
}

Expand Down
4 changes: 2 additions & 2 deletions types/params_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,8 +165,8 @@ func TestConsensusParamsUpdate_VoteExtensionsEnableHeight(t *testing.T) {
// no change
{"current: 3, 0 -> 0", 3, 0, 0, false},
{"current: 3, 100 -> 100, ", 3, 100, 100, false},
{"current: 100, 100 -> 100, ", 100, 100, 100, true},
{"current: 300, 100 -> 100, ", 300, 100, 100, true},
{"current: 100, 100 -> 100, ", 100, 100, 100, false},
{"current: 300, 100 -> 100, ", 300, 100, 100, false},
// set for the first time
{"current: 3, 0 -> 5, ", 3, 0, 5, false},
{"current: 4, 0 -> 5, ", 4, 0, 5, false},
Expand Down

0 comments on commit 1d99e05

Please sign in to comment.