Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: remove useless code and fix a test #3159

Merged
merged 5 commits into from
Nov 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tm2/pkg/bft/consensus/replay.go
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@
// 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)

Check warning on line 240 in tm2/pkg/bft/consensus/replay.go

View check run for this annotation

Codecov / codecov/patch

tm2/pkg/bft/consensus/replay.go#L240

Added line #L240 was not covered by tests
}

blockHeight := res.LastBlockHeight
Expand Down
14 changes: 7 additions & 7 deletions tm2/pkg/bft/consensus/state.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@
// 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)

Check warning on line 206 in tm2/pkg/bft/consensus/state.go

View check run for this annotation

Codecov / codecov/patch

tm2/pkg/bft/consensus/state.go#L206

Added line #L206 was not covered by tests
}

// GetConfig returns a copy of the chain state.
Expand Down Expand Up @@ -1051,7 +1051,7 @@
}

// 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)
Expand Down Expand Up @@ -1164,7 +1164,7 @@
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
Expand Down Expand Up @@ -1305,15 +1305,15 @@
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")

Check warning on line 1308 in tm2/pkg/bft/consensus/state.go

View check run for this annotation

Codecov / codecov/patch

tm2/pkg/bft/consensus/state.go#L1308

Added line #L1308 was not covered by tests
}
if !blockParts.HasHeader(blockID.PartsHeader) {
panic(fmt.Sprintf("Expected ProposalBlockParts header to be commit header"))
panic("Expected ProposalBlockParts header to be commit header")

Check warning on line 1311 in tm2/pkg/bft/consensus/state.go

View check run for this annotation

Codecov / codecov/patch

tm2/pkg/bft/consensus/state.go#L1311

Added line #L1311 was not covered by tests
}
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")

Check warning on line 1314 in tm2/pkg/bft/consensus/state.go

View check run for this annotation

Codecov / codecov/patch

tm2/pkg/bft/consensus/state.go#L1314

Added line #L1314 was not covered by tests
}
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))
}

Expand Down
2 changes: 1 addition & 1 deletion tm2/pkg/bft/node/node_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ func TestCreateProposalBlock(t *testing.T) {
proposerAddr,
)

err = blockExec.ValidateBlock(state, block)
err = state.ValidateBlock(block)
assert.NoError(t, err)
}

Expand Down
9 changes: 1 addition & 8 deletions tm2/pkg/bft/state/execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion tm2/pkg/bft/state/helpers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}}
Expand Down
9 changes: 3 additions & 6 deletions tm2/pkg/bft/state/validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
)
Expand Down
6 changes: 3 additions & 3 deletions tm2/pkg/bft/state/validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion tm2/pkg/bft/store/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 5 additions & 11 deletions tm2/pkg/bft/types/block.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
"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"
Expand Down Expand Up @@ -54,10 +53,9 @@
)
}

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

Check warning on line 58 in tm2/pkg/bft/types/block.go

View check run for this annotation

Codecov / codecov/patch

tm2/pkg/bft/types/block.go#L58

Added line #L58 was not covered by tests
if b.TotalTxs < 0 {
return errors.New("Negative Header.TotalTxs")
}
Expand Down Expand Up @@ -115,11 +113,6 @@
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
}

Expand Down Expand Up @@ -265,8 +258,9 @@
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)),
aeddi marked this conversation as resolved.
Show resolved Hide resolved
},
Data: Data{
Txs: txs,
Expand Down
4 changes: 2 additions & 2 deletions tm2/pkg/bft/types/validator_set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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)
}
}

Expand Down
6 changes: 0 additions & 6 deletions tm2/pkg/bft/types/vote.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
Expand Down
Loading