refactor: remove useless code and fix a test (#3159)

- Commit
4b6219c
move `ValidateBlock` method from `blockExec` to `state` since
`blockExec.db` was an unused parameter. (simplify + remove misleading
comment)
- Commit
4f02bcd
just removes useless `Sprintf` found in this package
- Commit
c34bfd5
improves `ValidateBasic` method (adding one more validation test that
was marked with a `TODO` comment)
- Commit
df75b32
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

<details><summary>Contributors' checklist...</summary>

- [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
</details>

Author: aeddi

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

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))
 	}
 

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)
 }
 

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)
 	}
 

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{}}

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,
 		)

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,15 +179,15 @@ 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)
 
 			/*
 				#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)
 		}

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

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,

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)
 			}
 		}