From 4f02bcd90f4ba9625818c43d7d806a43f9dffd62 Mon Sep 17 00:00:00 2001 From: aeddi Date: Mon, 18 Nov 2024 18:32:12 +0900 Subject: [PATCH 1/4] refacto: remove useless Sprintf --- tm2/pkg/bft/consensus/replay.go | 2 +- tm2/pkg/bft/consensus/state.go | 8 ++++---- tm2/pkg/bft/store/store.go | 2 +- tm2/pkg/bft/types/validator_set_test.go | 4 ++-- 4 files changed, 8 insertions(+), 8 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..627b0bcfb08 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. @@ -1305,13 +1305,13 @@ 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 { panic(fmt.Sprintf("+2/3 committed an invalid block: %v", 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/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) } } From 4b6219c5b89aa21a9ae46ee7fede63779194f9f1 Mon Sep 17 00:00:00 2001 From: aeddi Date: Mon, 18 Nov 2024 19:22:08 +0900 Subject: [PATCH 2/4] refacto: move ValidateBlock from blockExec to state --- tm2/pkg/bft/consensus/state.go | 6 +++--- 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 | 3 +-- tm2/pkg/bft/state/validation_test.go | 6 +++--- 6 files changed, 10 insertions(+), 18 deletions(-) diff --git a/tm2/pkg/bft/consensus/state.go b/tm2/pkg/bft/consensus/state.go index 627b0bcfb08..8b2653813e3 100644 --- a/tm2/pkg/bft/consensus/state.go +++ b/tm2/pkg/bft/consensus/state.go @@ -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 @@ -1313,7 +1313,7 @@ func (cs *ConsensusState) finalizeCommit(height int64) { if !block.HashesTo(blockID.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..c9b584f92f1 100644 --- a/tm2/pkg/bft/state/validation.go +++ b/tm2/pkg/bft/state/validation.go @@ -7,13 +7,12 @@ import ( "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 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) } From df75b32f06d922e85ef493284aa24be0ac4d3524 Mon Sep 17 00:00:00 2001 From: aeddi Date: Tue, 19 Nov 2024 09:39:56 +0900 Subject: [PATCH 3/4] chore: remove ineffective test --- tm2/pkg/bft/state/validation.go | 6 ++---- tm2/pkg/bft/types/block.go | 6 ------ tm2/pkg/bft/types/vote.go | 6 ------ 3 files changed, 2 insertions(+), 16 deletions(-) diff --git a/tm2/pkg/bft/state/validation.go b/tm2/pkg/bft/state/validation.go index c9b584f92f1..14191bea0d9 100644 --- a/tm2/pkg/bft/state/validation.go +++ b/tm2/pkg/bft/state/validation.go @@ -6,7 +6,6 @@ import ( "fmt" "github.com/gnolang/gno/tm2/pkg/bft/types" - "github.com/gnolang/gno/tm2/pkg/crypto" ) // ----------------------------------------------------- @@ -136,9 +135,8 @@ func (state State) ValidateBlock(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/types/block.go b/tm2/pkg/bft/types/block.go index a8501775c4b..9672b609569 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" @@ -115,11 +114,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 } 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") } From c34bfd57466d0bf992ac3402865387c89b59393e Mon Sep 17 00:00:00 2001 From: aeddi Date: Tue, 19 Nov 2024 09:40:37 +0900 Subject: [PATCH 4/4] test: fix block test --- tm2/pkg/bft/types/block.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tm2/pkg/bft/types/block.go b/tm2/pkg/bft/types/block.go index 9672b609569..445f169f1d1 100644 --- a/tm2/pkg/bft/types/block.go +++ b/tm2/pkg/bft/types/block.go @@ -53,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") } @@ -259,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,