Skip to content

Commit

Permalink
Fast reject invalid consensus params (#331)
Browse files Browse the repository at this point in the history
## Describe your changes and provide context
Previously, a proposal to change Tendermint consensus params to an
invalid value could be made, but could potentially halt the chain b/c at
the block in which the proposal passes, Tendermint will verify that the
params are valid and panic if the change is not valid (see
https://github.com/sei-protocol/sei-tendermint/blob/d426f1fe475eb0c406296770ff5e9f8869b3887e/internal/state/execution.go#L320),
thus halting the change. This PR early rejects the proposal so that the
chain will no longer halt.
## Testing performed to validate your change
- Added unit tests
- Verified on a internal chain:
```
root@ip-172-31-34-209:/home/ubuntu/sei-chain# seid tx gov submit-proposal param-change prop.json --from admin -b block --fees 20000usei
Enter keyring passphrase:
Error: block.MaxGas must be greater or equal to -1. Got -2
...
root@ip-172-31-34-209:/home/ubuntu/sei-chain# seid tx gov submit-proposal param-change prop.json --from admin -b block --fees 2000usei
Enter keyring passphrase:
Error: block.MaxBytes is too big. 2202009600000 > 104857600
```
  • Loading branch information
philipsu522 authored Oct 16, 2023
1 parent bf1648f commit e6817bd
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 0 deletions.
32 changes: 32 additions & 0 deletions x/params/types/proposal/proposal.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package proposal

import (
"encoding/json"
"fmt"
"github.com/tendermint/tendermint/types"
"strings"

yaml "gopkg.in/yaml.v2"
Expand Down Expand Up @@ -96,7 +98,37 @@ func ValidateChanges(changes []ParamChange) error {
if len(pc.Value) == 0 {
return ErrEmptyValue
}
// We need to verify ConsensusParams since they are only validated once the proposal passes.
// If any of them are invalid at time of passing, this will cause a chain halt since validation is done during
// ApplyBlock: https://github.com/sei-protocol/sei-tendermint/blob/d426f1fe475eb0c406296770ff5e9f8869b3887e/internal/state/execution.go#L320
// Therefore, we validate when we get a param-change msg for ConsensusParams
if pc.Subspace == "baseapp" {
if err := verifyConsensusParamsUsingDefault(changes); err != nil {
return err
}
}
}

return nil
}

func verifyConsensusParamsUsingDefault(changes []ParamChange) error {
// Start with a default (valid) set of parameters, and update based on proposal then check
defaultCP := types.DefaultConsensusParams()
for _, change := range changes {
// Note: BlockParams seems to be the only support ConsensusParams available for modifying with proposal
switch change.Key {
case "BlockParams":
blockParams := types.DefaultBlockParams()
err := json.Unmarshal([]byte(change.Value), &blockParams)
if err != nil {
return err
}
defaultCP.Block = blockParams
}
}
if err := defaultCP.ValidateConsensusParams(); err != nil {
return err
}
return nil
}
13 changes: 13 additions & 0 deletions x/params/types/proposal/proposal_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package proposal

import (
"fmt"
"github.com/tendermint/tendermint/types"
"testing"

"github.com/stretchr/testify/require"
Expand All @@ -25,3 +27,14 @@ func TestParameterChangeProposal(t *testing.T) {
pcp = NewParameterChangeProposal("test title", "test description", []ParamChange{pc4}, true)
require.Error(t, pcp.ValidateBasic())
}

func TestConsensusParameterChangeProposal(t *testing.T) {
// Valid block max_bytes (
pc1 := NewParamChange("baseapp", "BlockParams", fmt.Sprintf("{\"max_bytes\":\"%d\"}", types.MaxBlockSizeBytes))
pcp := NewParameterChangeProposal("test title", "test description", []ParamChange{pc1}, true)
require.Nil(t, pcp.ValidateBasic())

pc1 = NewParamChange("baseapp", "BlockParams", fmt.Sprintf("{\"max_bytes\":\"%d\"}", types.MaxBlockSizeBytes+1))
pcp = NewParameterChangeProposal("test title", "test description", []ParamChange{pc1}, true)
require.Error(t, pcp.ValidateBasic())
}

0 comments on commit e6817bd

Please sign in to comment.