From 732bb0bc7a5dd2fcb8f81376ee269940d05dee38 Mon Sep 17 00:00:00 2001 From: Antoine Eddi <5222525+aeddi@users.noreply.github.com> Date: Wed, 20 Nov 2024 03:36:43 +0100 Subject: [PATCH] refactor: remove useless code and fix a test (#3159) - Commit https://github.com/gnolang/gno/pull/3159/commits/4b6219c5b89aa21a9ae46ee7fede63779194f9f1 move `ValidateBlock` method from `blockExec` to `state` since `blockExec.db` was an unused parameter. (simplify + remove misleading comment) - Commit https://github.com/gnolang/gno/pull/3159/commits/4f02bcd90f4ba9625818c43d7d806a43f9dffd62 just removes useless `Sprintf` found in this package - Commit https://github.com/gnolang/gno/pull/3159/commits/c34bfd57466d0bf992ac3402865387c89b59393e improves `ValidateBasic` method (adding one more validation test that was marked with a `TODO` comment) - Commit https://github.com/gnolang/gno/pull/3159/commits/df75b32f06d922e85ef493284aa24be0ac4d3524 removes useless case testing the size of a fixed-sized array, see: https://github.com/gnolang/gno/blob/b3800b7dfb864396ac74dc20390e728bc0b2d88e/tm2/pkg/crypto/crypto.go#L24-L30
Contributors' checklist... - [x] Added new tests, or not needed, or not feasible - [x] Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory - [x] Updated the official documentation or not needed - [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message was included in the description - [x] Added references to related issues and PRs - [ ] Provided any useful hints for running manual tests
--- tm2/pkg/bft/consensus/replay.go | 2 +- tm2/pkg/bft/consensus/state.go | 14 +++++++------- tm2/pkg/bft/node/node_test.go | 2 +- tm2/pkg/bft/state/execution.go | 9 +-------- tm2/pkg/bft/state/helpers_test.go | 2 +- tm2/pkg/bft/state/validation.go | 9 +++------ tm2/pkg/bft/state/validation_test.go | 6 +++--- tm2/pkg/bft/store/store.go | 2 +- tm2/pkg/bft/types/block.go | 16 +++++----------- tm2/pkg/bft/types/validator_set_test.go | 4 ++-- tm2/pkg/bft/types/vote.go | 6 ------ 11 files changed, 25 insertions(+), 47 deletions(-) diff --git a/tm2/pkg/bft/consensus/replay.go b/tm2/pkg/bft/consensus/replay.go index 02e6dade72c..2ba297a3d1e 100644 --- a/tm2/pkg/bft/consensus/replay.go +++ b/tm2/pkg/bft/consensus/replay.go @@ -237,7 +237,7 @@ func (h *Handshaker) Handshake(proxyApp appconn.AppConns) error { // Handshake is done via ABCI Info on the query conn. res, err := proxyApp.Query().InfoSync(abci.RequestInfo{}) if err != nil { - return fmt.Errorf("Error calling Info: %w", err) + return fmt.Errorf("error calling Info: %w", err) } blockHeight := res.LastBlockHeight diff --git a/tm2/pkg/bft/consensus/state.go b/tm2/pkg/bft/consensus/state.go index 6faa40be20b..8b2653813e3 100644 --- a/tm2/pkg/bft/consensus/state.go +++ b/tm2/pkg/bft/consensus/state.go @@ -203,7 +203,7 @@ func (cs *ConsensusState) SetEventSwitch(evsw events.EventSwitch) { // String returns a string. func (cs *ConsensusState) String() string { // better not to access shared variables - return fmt.Sprintf("ConsensusState") // (H:%v R:%v S:%v", cs.Height, cs.Round, cs.Step) + return "ConsensusState" // (H:%v R:%v S:%v", cs.Height, cs.Round, cs.Step) } // GetConfig returns a copy of the chain state. @@ -1051,7 +1051,7 @@ func (cs *ConsensusState) defaultDoPrevote(height int64, round int) { } // Validate proposal block - err := cs.blockExec.ValidateBlock(cs.state, cs.ProposalBlock) + err := cs.state.ValidateBlock(cs.ProposalBlock) if err != nil { // ProposalBlock is invalid, prevote nil. logger.Error("enterPrevote: ProposalBlock is invalid", "err", err) @@ -1164,7 +1164,7 @@ func (cs *ConsensusState) enterPrecommit(height int64, round int) { if cs.ProposalBlock.HashesTo(blockID.Hash) { logger.Info("enterPrecommit: +2/3 prevoted proposal block. Locking", "hash", blockID.Hash) // Validate the block. - if err := cs.blockExec.ValidateBlock(cs.state, cs.ProposalBlock); err != nil { + if err := cs.state.ValidateBlock(cs.ProposalBlock); err != nil { panic(fmt.Sprintf("enterPrecommit: +2/3 prevoted for an invalid block: %v", err)) } cs.LockedRound = round @@ -1305,15 +1305,15 @@ func (cs *ConsensusState) finalizeCommit(height int64) { block, blockParts := cs.ProposalBlock, cs.ProposalBlockParts if !ok { - panic(fmt.Sprintf("Cannot finalizeCommit, commit does not have two thirds majority")) + panic("Cannot finalizeCommit, commit does not have two thirds majority") } if !blockParts.HasHeader(blockID.PartsHeader) { - panic(fmt.Sprintf("Expected ProposalBlockParts header to be commit header")) + panic("Expected ProposalBlockParts header to be commit header") } if !block.HashesTo(blockID.Hash) { - panic(fmt.Sprintf("Cannot finalizeCommit, ProposalBlock does not hash to commit hash")) + panic("Cannot finalizeCommit, ProposalBlock does not hash to commit hash") } - if err := cs.blockExec.ValidateBlock(cs.state, block); err != nil { + if err := cs.state.ValidateBlock(block); err != nil { panic(fmt.Sprintf("+2/3 committed an invalid block: %v", err)) } diff --git a/tm2/pkg/bft/node/node_test.go b/tm2/pkg/bft/node/node_test.go index e28464ff711..6e86a0bcc6f 100644 --- a/tm2/pkg/bft/node/node_test.go +++ b/tm2/pkg/bft/node/node_test.go @@ -304,7 +304,7 @@ func TestCreateProposalBlock(t *testing.T) { proposerAddr, ) - err = blockExec.ValidateBlock(state, block) + err = state.ValidateBlock(block) assert.NoError(t, err) } diff --git a/tm2/pkg/bft/state/execution.go b/tm2/pkg/bft/state/execution.go index 15a0f466341..a58a50c1877 100644 --- a/tm2/pkg/bft/state/execution.go +++ b/tm2/pkg/bft/state/execution.go @@ -82,20 +82,13 @@ func (blockExec *BlockExecutor) CreateProposalBlock( return state.MakeBlock(height, txs, commit, proposerAddr) } -// ValidateBlock validates the given block against the given state. -// If the block is invalid, it returns an error. -// Validation does not mutate state, but does require historical information from the stateDB -func (blockExec *BlockExecutor) ValidateBlock(state State, block *types.Block) error { - return validateBlock(blockExec.db, state, block) -} - // ApplyBlock validates the block against the state, executes it against the app, // fires the relevant events, commits the app, and saves the new state and responses. // It's the only function that needs to be called // from outside this package to process and commit an entire block. // It takes a blockID to avoid recomputing the parts hash. func (blockExec *BlockExecutor) ApplyBlock(state State, blockID types.BlockID, block *types.Block) (State, error) { - if err := blockExec.ValidateBlock(state, block); err != nil { + if err := state.ValidateBlock(block); err != nil { return state, InvalidBlockError(err) } diff --git a/tm2/pkg/bft/state/helpers_test.go b/tm2/pkg/bft/state/helpers_test.go index 0b8dba98221..948c1debe6d 100644 --- a/tm2/pkg/bft/state/helpers_test.go +++ b/tm2/pkg/bft/state/helpers_test.go @@ -52,7 +52,7 @@ func makeAndApplyGoodBlock(state sm.State, height int64, lastCommit *types.Commi blockExec *sm.BlockExecutor, ) (sm.State, types.BlockID, error) { block, _ := state.MakeBlock(height, makeTxs(height), lastCommit, proposerAddr) - if err := blockExec.ValidateBlock(state, block); err != nil { + if err := state.ValidateBlock(block); err != nil { return state, types.BlockID{}, err } blockID := types.BlockID{Hash: block.Hash(), PartsHeader: types.PartSetHeader{}} diff --git a/tm2/pkg/bft/state/validation.go b/tm2/pkg/bft/state/validation.go index 13274b6a38c..14191bea0d9 100644 --- a/tm2/pkg/bft/state/validation.go +++ b/tm2/pkg/bft/state/validation.go @@ -6,14 +6,12 @@ import ( "fmt" "github.com/gnolang/gno/tm2/pkg/bft/types" - "github.com/gnolang/gno/tm2/pkg/crypto" - dbm "github.com/gnolang/gno/tm2/pkg/db" ) // ----------------------------------------------------- // Validate block -func validateBlock(stateDB dbm.DB, state State, block *types.Block) error { +func (state State) ValidateBlock(block *types.Block) error { // Validate internal consistency. if err := block.ValidateBasic(); err != nil { return err @@ -137,9 +135,8 @@ func validateBlock(stateDB dbm.DB, state State, block *types.Block) error { // NOTE: We can't actually verify it's the right proposer because we dont // know what round the block was first proposed. So just check that it's - // a legit address and a known validator. - if len(block.ProposerAddress) != crypto.AddressSize || - !state.Validators.HasAddress(block.ProposerAddress) { + // a legit address from a known validator. + if !state.Validators.HasAddress(block.ProposerAddress) { return fmt.Errorf("Block.Header.ProposerAddress, %X, is not a validator", block.ProposerAddress, ) diff --git a/tm2/pkg/bft/state/validation_test.go b/tm2/pkg/bft/state/validation_test.go index 0eadd076be9..1830bb3f6da 100644 --- a/tm2/pkg/bft/state/validation_test.go +++ b/tm2/pkg/bft/state/validation_test.go @@ -142,7 +142,7 @@ func TestValidateBlockHeader(t *testing.T) { for _, tc := range testCases { block, _ := state.MakeBlock(height, makeTxs(height), lastCommit, proposerAddr) tc.malleateBlock(block) - err := blockExec.ValidateBlock(state, block) + err := state.ValidateBlock(block) assert.ErrorContains(t, err, tc.expectedError, tc.name) } @@ -179,7 +179,7 @@ func TestValidateBlockCommit(t *testing.T) { require.NoError(t, err, "height %d", height) wrongHeightCommit := types.NewCommit(state.LastBlockID, []*types.CommitSig{wrongHeightVote.CommitSig()}) block, _ := state.MakeBlock(height, makeTxs(height), wrongHeightCommit, proposerAddr) - err = blockExec.ValidateBlock(state, block) + err = state.ValidateBlock(block) _, isErrInvalidCommitHeight := err.(types.InvalidCommitHeightError) require.True(t, isErrInvalidCommitHeight, "expected InvalidCommitHeightError at height %d but got: %v", height, err) @@ -187,7 +187,7 @@ func TestValidateBlockCommit(t *testing.T) { #2589: test len(block.LastCommit.Precommits) == state.LastValidators.Size() */ block, _ = state.MakeBlock(height, makeTxs(height), wrongPrecommitsCommit, proposerAddr) - err = blockExec.ValidateBlock(state, block) + err = state.ValidateBlock(block) _, isErrInvalidCommitPrecommits := err.(types.InvalidCommitPrecommitsError) require.True(t, isErrInvalidCommitPrecommits, "expected InvalidCommitPrecommitsError at height %d but got: %v", height, err) } diff --git a/tm2/pkg/bft/store/store.go b/tm2/pkg/bft/store/store.go index e7671d8033e..e5b9f57a1af 100644 --- a/tm2/pkg/bft/store/store.go +++ b/tm2/pkg/bft/store/store.go @@ -152,7 +152,7 @@ func (bs *BlockStore) SaveBlock(block *types.Block, blockParts *types.PartSet, s panic(fmt.Sprintf("BlockStore can only save contiguous blocks. Wanted %v, got %v", w, g)) } if !blockParts.IsComplete() { - panic(fmt.Sprintf("BlockStore can only save complete block part sets")) + panic("BlockStore can only save complete block part sets") } // Save block meta diff --git a/tm2/pkg/bft/types/block.go b/tm2/pkg/bft/types/block.go index a8501775c4b..445f169f1d1 100644 --- a/tm2/pkg/bft/types/block.go +++ b/tm2/pkg/bft/types/block.go @@ -10,7 +10,6 @@ import ( "github.com/gnolang/gno/tm2/pkg/amino" typesver "github.com/gnolang/gno/tm2/pkg/bft/types/version" "github.com/gnolang/gno/tm2/pkg/bitarray" - "github.com/gnolang/gno/tm2/pkg/crypto" "github.com/gnolang/gno/tm2/pkg/crypto/merkle" "github.com/gnolang/gno/tm2/pkg/crypto/tmhash" "github.com/gnolang/gno/tm2/pkg/errors" @@ -54,10 +53,9 @@ func (b *Block) ValidateBasic() error { ) } - // TODO: fix tests so we can do this - /*if b.TotalTxs < b.NumTxs { + if b.TotalTxs < b.NumTxs { return fmt.Errorf("Header.TotalTxs (%d) is less than Header.NumTxs (%d)", b.TotalTxs, b.NumTxs) - }*/ + } if b.TotalTxs < 0 { return errors.New("Negative Header.TotalTxs") } @@ -115,11 +113,6 @@ func (b *Block) ValidateBasic() error { return fmt.Errorf("wrong Header.LastResultsHash: %w", err) } - if len(b.ProposerAddress) != crypto.AddressSize { - return fmt.Errorf("expected len(Header.ProposerAddress) to be %d, got %d", - crypto.AddressSize, len(b.ProposerAddress)) - } - return nil } @@ -265,8 +258,9 @@ func (h *Header) GetTime() time.Time { return h.Time } func MakeBlock(height int64, txs []Tx, lastCommit *Commit) *Block { block := &Block{ Header: Header{ - Height: height, - NumTxs: int64(len(txs)), + Height: height, + NumTxs: int64(len(txs)), + TotalTxs: int64(len(txs)), }, Data: Data{ Txs: txs, diff --git a/tm2/pkg/bft/types/validator_set_test.go b/tm2/pkg/bft/types/validator_set_test.go index e543104b15d..bc02ae754c5 100644 --- a/tm2/pkg/bft/types/validator_set_test.go +++ b/tm2/pkg/bft/types/validator_set_test.go @@ -249,7 +249,7 @@ func TestProposerSelection3(t *testing.T) { got := vset.GetProposer().Address expected := proposerOrder[j%4].Address if got != expected { - t.Fatalf(fmt.Sprintf("vset.Proposer (%X) does not match expected proposer (%X) for (%d, %d)", got, expected, i, j)) + t.Fatalf("vset.Proposer (%X) does not match expected proposer (%X) for (%d, %d)", got, expected, i, j) } // serialize, deserialize, check proposer @@ -263,7 +263,7 @@ func TestProposerSelection3(t *testing.T) { computed := vset.GetProposer() // findGetProposer() if i != 0 { if got != computed.Address { - t.Fatalf(fmt.Sprintf("vset.Proposer (%X) does not match computed proposer (%X) for (%d, %d)", got, computed.Address, i, j)) + t.Fatalf("vset.Proposer (%X) does not match computed proposer (%X) for (%d, %d)", got, computed.Address, i, j) } } diff --git a/tm2/pkg/bft/types/vote.go b/tm2/pkg/bft/types/vote.go index caaaa3f8c34..66fc40919f1 100644 --- a/tm2/pkg/bft/types/vote.go +++ b/tm2/pkg/bft/types/vote.go @@ -141,12 +141,6 @@ func (vote *Vote) ValidateBasic() error { if !vote.BlockID.IsZero() && !vote.BlockID.IsComplete() { return fmt.Errorf("BlockID must be either empty or complete, got: %v", vote.BlockID) } - if len(vote.ValidatorAddress) != crypto.AddressSize { - return fmt.Errorf("expected ValidatorAddress size to be %d bytes, got %d bytes", - crypto.AddressSize, - len(vote.ValidatorAddress), - ) - } if vote.ValidatorAddress.IsZero() { return fmt.Errorf("expected ValidatorAddress to be non-zero") }