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.

Sign up for GitHub

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

Jump to bottom

make ineffassign linter pass #3386

Merged
merged 9 commits into from
Mar 8, 2019
Merged

make ineffassign linter pass #3386

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9257f7f
make ineffassign linter pass
melekes Mar 6, 2019
ecd5fea
fix more ineffassign errors
melekes Mar 6, 2019
f2626ad
Fix remaining ineffassign lints: assigned but not checked errors in t…
liamsi Mar 6, 2019
9d9d222
Merge branch 'develop' into anton/ineffassign
melekes Mar 7, 2019
77379e0
lite/dbprovider: return ok instead of true
melekes Mar 7, 2019
1409800
comment out error statement
melekes Mar 7, 2019
ff2b8e0
remove reflect from the error statement
melekes Mar 7, 2019
9ac7197
bring back reflect
melekes Mar 7, 2019
90f9b77
Revert "bring back reflect"
melekes Mar 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 0 additions & 1 deletion .golangci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ linters:
- maligned
- errcheck
- staticcheck
- ineffassign
- interfacer
- unconvert
- goconst
Expand Down
20 changes: 12 additions & 8 deletions consensus/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -670,28 +670,31 @@ func (cs *ConsensusState) handleMsg(mi msgInfo) {
cs.mtx.Lock()
defer cs.mtx.Unlock()

var err error
var (
err error
added bool
)
msg, peerID := mi.Msg, mi.PeerID
switch msg := msg.(type) {
case *ProposalMessage:
// will not cause transition.
// once proposal is set, we can receive block parts
err = cs.setProposal(msg.Proposal)
_ = cs.setProposal(msg.Proposal)
case *BlockPartMessage:
// if the proposal is complete, we'll enterPrevote or tryFinalizeCommit
added, err := cs.addProposalBlockPart(msg, peerID)
added, err = cs.addProposalBlockPart(msg, peerID)
if added {
cs.statsMsgQueue <- mi
}

if err != nil && msg.Round != cs.Round {
cs.Logger.Debug("Received block part from wrong round", "height", cs.Height, "csRound", cs.Round, "blockRound", msg.Round)
err = nil
// err = nil
}
case *VoteMessage:
// attempt to add the vote and dupeout the validator if its a duplicate signature
// if the vote gives us a 2/3-any or 2/3-one, we transition
added, err := cs.tryAddVote(msg.Vote, peerID)
added, err = cs.tryAddVote(msg.Vote, peerID)
if added {
cs.statsMsgQueue <- mi
}
Expand All @@ -713,9 +716,10 @@ func (cs *ConsensusState) handleMsg(mi msgInfo) {
default:
cs.Logger.Error("Unknown msg type", reflect.TypeOf(msg))
}
if err != nil {
cs.Logger.Error("Error with msg", "height", cs.Height, "round", cs.Round, "type", reflect.TypeOf(msg), "peer", peerID, "err", err, "msg", msg)
}
// if err != nil {
// cs.Logger.Error("Error with msg", "height", cs.Height, "round", cs.Round,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uncommentting it will lead to TestReactorValidatorSetChanges failing with 5m0s timeout

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ebuchman any ideas why this may be the case?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TestReactorValidatorSetChanges uses a TestingLoggerWithOutput which inits a NewSyncWriter. Could it be the case that some other go-routine is writing to stdout such that this blocks and never becomes available?

I could not reproduce this locally so not sure what is going on.

Copy link
Contributor

@liamsi liamsi Mar 7, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the -v flag from go test. Then tests run with a NewNopLogger instead. Interestingly, the test passes (only tried once though): https://github.com/tendermint/tendermint/compare/develop...non-verbose/anton/ineffassign?expand=1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it has something to do with reflect.TypeOf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't replicate it anymore. Super weird

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reflect.TypeOf doesn't block on anything. The logger potentially does as there is a lock involved (I have to admit I do not understand how and why but it seems more plausible than reflect).

// "type", reflect.TypeOf(msg), "peer", peerID, "err", err, "msg", msg)
// }
}

func (cs *ConsensusState) handleTimeout(ti timeoutInfo, rs cstypes.RoundState) {
Expand Down
7 changes: 4 additions & 3 deletions lite/dbprovider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,14 +258,15 @@ func parseKey(key []byte) (chainID string, height int64, part string, ok bool) {
}

func parseSignedHeaderKey(key []byte) (chainID string, height int64, ok bool) {
chainID, height, part, ok := parseKey(key)
var part string
chainID, height, part, ok = parseKey(key)
if part != "sh" {
return "", 0, false
}
return chainID, height, true
return
}

func parseChainKeyPrefix(key []byte) (chainID string, height int64, ok bool) {
chainID, height, _, ok = parseKey(key)
return chainID, height, true
return
}
1 change: 1 addition & 0 deletions lite/dynamic_verifier_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ func TestConcurrencyInquirerVerify(t *testing.T) {
cert.SetLogger(log.TestingLogger())

err = source.SaveFullCommit(fcz[7])
require.Nil(err, "%+v", err)
err = source.SaveFullCommit(fcz[8])
require.Nil(err, "%+v", err)
sh := fcz[8].SignedHeader
Expand Down
2 changes: 2 additions & 0 deletions lite/proxy/query_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ func _TestAppProofs(t *testing.T) {
// verify a query before the tx block has no data (and valid non-exist proof)
bs, height, proof, err := GetWithProof(prt, k, brh-1, cl, cert)
require.NoError(err, "%#v", err)
require.NotNil(proof)
require.Equal(height, brh-1)
// require.NotNil(proof)
// TODO: Ensure that *some* keys will be there, ensuring that proof is nil,
// (currently there's a race condition)
Expand Down
8 changes: 5 additions & 3 deletions p2p/conn/connection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,10 @@ func TestMConnectionMultiplePongsInTheBeginning(t *testing.T) {
serverGotPing := make(chan struct{})
go func() {
// read ping (one byte)
var packet, err = Packet(nil), error(nil)
var (
packet Packet
err error
)
_, err = cdc.UnmarshalBinaryLengthPrefixedReader(server, &packet, maxPingPongPacketSize)
require.Nil(t, err)
serverGotPing <- struct{}{}
Expand Down Expand Up @@ -492,8 +495,7 @@ func TestMConnectionReadErrorUnknownMsgType(t *testing.T) {
defer mconnServer.Stop()

// send msg with unknown msg type
err := error(nil)
err = amino.EncodeUvarint(mconnClient.conn, 4)
err := amino.EncodeUvarint(mconnClient.conn, 4)
assert.Nil(t, err)
_, err = mconnClient.conn.Write([]byte{0xFF, 0xFF, 0xFF, 0xFF})
assert.Nil(t, err)
Expand Down
2 changes: 1 addition & 1 deletion rpc/core/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ import (
// }
// ```
func Status() (*ctypes.ResultStatus, error) {
var latestHeight int64 = -1
var latestHeight int64
if consensusReactor.FastSync() {
latestHeight = blockStore.Height()
} else {
Expand Down
1 change: 1 addition & 0 deletions state/execution_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ func TestApplyBlock(t *testing.T) {
block := makeBlock(state, 1)
blockID := types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}

//nolint:ineffassign
state, err = blockExec.ApplyBlock(state, blockID, block)
require.Nil(t, err)

Expand Down
6 changes: 6 additions & 0 deletions state/state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,6 +668,7 @@ func TestLargeGenesisValidator(t *testing.T) {
blockID := types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}

updatedState, err := updateState(oldState, blockID, &block.Header, abciResponses, validatorUpdates)
require.NoError(t, err)
// no changes in voting power (ProposerPrio += VotingPower == Voting in 1st round; than shiftByAvg == 0,
// than -Total == -Voting)
// -> no change in ProposerPrio (stays zero):
Expand All @@ -692,6 +693,7 @@ func TestLargeGenesisValidator(t *testing.T) {
block := makeBlock(oldState, oldState.LastBlockHeight+1)
blockID := types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}
updatedState, err := updateState(oldState, blockID, &block.Header, abciResponses, validatorUpdates)
require.NoError(t, err)

lastState := updatedState
for i := 0; i < 200; i++ {
Expand All @@ -706,6 +708,7 @@ func TestLargeGenesisValidator(t *testing.T) {
blockID := types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}

updatedStateInner, err := updateState(lastState, blockID, &block.Header, abciResponses, validatorUpdates)
require.NoError(t, err)
lastState = updatedStateInner
}
// set state to last state of above iteration
Expand Down Expand Up @@ -735,6 +738,7 @@ func TestLargeGenesisValidator(t *testing.T) {
block := makeBlock(oldState, oldState.LastBlockHeight+1)
blockID := types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}
state, err = updateState(state, blockID, &block.Header, abciResponses, validatorUpdates)
require.NoError(t, err)
}
require.Equal(t, 10+2, len(state.NextValidators.Validators))

Expand Down Expand Up @@ -766,6 +770,7 @@ func TestLargeGenesisValidator(t *testing.T) {
block = makeBlock(curState, curState.LastBlockHeight+1)
blockID = types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}
curState, err = updateState(curState, blockID, &block.Header, abciResponses, validatorUpdates)
require.NoError(t, err)
if !bytes.Equal(curState.Validators.Proposer.Address, curState.NextValidators.Proposer.Address) {
isProposerUnchanged = false
}
Expand All @@ -790,6 +795,7 @@ func TestLargeGenesisValidator(t *testing.T) {
blockID := types.BlockID{block.Hash(), block.MakePartSet(testPartSize).Header()}

updatedState, err = updateState(updatedState, blockID, &block.Header, abciResponses, validatorUpdates)
require.NoError(t, err)
if i > numVals { // expect proposers to cycle through after the first iteration (of numVals blocks):
if proposers[i%numVals] == nil {
proposers[i%numVals] = updatedState.NextValidators.Proposer
Expand Down