Merge pull request #4735 from almog-t/merge-recoverability-master

Merge recoverability master

.golangci-warnings.yml

@@ -8,8 +8,8 @@ linters:
     - deadcode
     - partitiontest
     - structcheck
-    - typecheck
     - varcheck
+    - unconvert
     - unused
 
 
@@ -52,24 +52,19 @@ issues:
     # be more lenient with test code
     - path: _test\.go
       linters:
-        - staticcheck
+        - deadcode
         - structcheck
-        - typecheck
         - varcheck
-        - deadcode
-        - gosimple
+        - unconvert
         - unused
     # Add all linters here -- Comment this block out for testing linters
     - path: test/linttest/lintissues\.go
       linters:
-        - staticcheck
+        - deadcode
         - structcheck
-        - typecheck
         - varcheck
-        - deadcode
-        - gosimple
+        - unconvert
         - unused
-        - partitiontest
     - path: crypto/secp256k1/secp256_test\.go
       linters:
         - partitiontest

.golangci.yml

@@ -33,6 +33,43 @@ linters-settings:
     - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).error
     - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).warnf
     - (*github.com/algorand/go-algorand/data/transactions/logic.OpStream).warn
+    # We do this 121 times and never check the error.
+    - (*github.com/spf13/cobra.Command).MarkFlagRequired
+  govet:
+    settings:
+      printf:
+        # Comma-separated list of print function names to check (in addition to default, see `go tool vet help printf`).
+        # Default: []
+        funcs:
+          - (github.com/algorand/go-algorand/logging.Logger).Debugf
+          - (github.com/algorand/go-algorand/logging.Logger).Infof
+          - (github.com/algorand/go-algorand/logging.Logger).Warnf
+          - (github.com/algorand/go-algorand/logging.Logger).Errorf
+          - (github.com/algorand/go-algorand/logging.Logger).Fatalf
+          - (github.com/algorand/go-algorand/logging.Logger).Panicf
+          - (github.com/algorand/go-algorand/logging.Logger).Debugln
+          - (github.com/algorand/go-algorand/logging.Logger).Infoln
+          - (github.com/algorand/go-algorand/logging.Logger).Warnln
+          - (github.com/algorand/go-algorand/logging.Logger).Errorln
+          - (github.com/algorand/go-algorand/logging.Logger).Fatalln
+          - (github.com/algorand/go-algorand/logging.Logger).Panicln
+          - (github.com/algorand/go-algorand/logging.Logger).Debug
+          - (github.com/algorand/go-algorand/logging.Logger).Info
+          - (github.com/algorand/go-algorand/logging.Logger).Warn
+          - (github.com/algorand/go-algorand/logging.Logger).Error
+          - (github.com/algorand/go-algorand/logging.Logger).Fatal
+          - (github.com/algorand/go-algorand/logging.Logger).Panic
+          - (github.com/algorand/go-algorand/data/transactions/logic.OpStream).warnf
+          - (github.com/algorand/go-algorand/data/transactions/logic.OpStream).errorf
+          - (github.com/algorand/go-algorand/data/transactions/logic.OpStream).lineErrorf
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportInfof
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportInfoln
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportWarnf
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportWarnln
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportWarnRawf
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportWarnRawln
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportErrorf
+          - (github.com/algorand/go-algorand/cmd/goal/main).reportErrorln
 
 issues:
   # Work our way back over time to be clean against all these
@@ -69,9 +106,9 @@ issues:
     - path: _test\.go
       linters:
         - errcheck
-        - gofmt
+        # - gofmt
         - gosimple
-        - govet
+        # - govet
         - ineffassign
         - misspell
         - nolintlint

agreement/actions.go

@@ -26,8 +26,7 @@ import (
 )
 
 //go:generate stringer -type=actionType
-//msgp:ignore actionType
-type actionType int
+type actionType uint8
 
 const (
 	noop actionType = iota
@@ -103,7 +102,7 @@ type networkAction struct {
 
 	UnauthenticatedVotes []unauthenticatedVote
 
-	Err serializableError
+	Err *serializableError
 }
 
 func (a networkAction) t() actionType {
@@ -181,7 +180,7 @@ type cryptoAction struct {
 	Period    period
 	Step      step
 	Pinned    bool
-	TaskIndex int
+	TaskIndex uint64
 }
 
 func (a cryptoAction) t() actionType {
@@ -388,7 +387,7 @@ func (a pseudonodeAction) do(ctx context.Context, s *Service) {
 		case nil:
 			// no error.
 			persistCompleteEvents := s.persistState(persistStateDone)
-			// we want to place there two one after the other. That way, the second would not get executed up until the first one is complete.
+			// we want to place these two one after the other. That way, the second would not get executed up until the first one is complete.
 			s.demux.prioritize(persistCompleteEvents)
 			s.demux.prioritize(voteEvents)
 		default:
@@ -403,12 +402,12 @@ func (a pseudonodeAction) do(ctx context.Context, s *Service) {
 	}
 }
 
-func ignoreAction(e messageEvent, err serializableError) action {
-	return networkAction{T: ignore, Err: err, h: e.Input.MessageHandle}
+func ignoreAction(e messageEvent, err *serializableError) action {
+	return networkAction{T: ignore, Err: err, h: e.Input.messageHandle}
 }
 
-func disconnectAction(e messageEvent, err serializableError) action {
-	return networkAction{T: disconnect, Err: err, h: e.Input.MessageHandle}
+func disconnectAction(e messageEvent, err *serializableError) action {
+	return networkAction{T: disconnect, Err: err, h: e.Input.messageHandle}
 }
 
 func broadcastAction(tag protocol.Tag, o interface{}) action {
@@ -427,7 +426,7 @@ func broadcastAction(tag protocol.Tag, o interface{}) action {
 }
 
 func relayAction(e messageEvent, tag protocol.Tag, o interface{}) action {
-	a := networkAction{T: relay, h: e.Input.MessageHandle, Tag: tag}
+	a := networkAction{T: relay, h: e.Input.messageHandle, Tag: tag}
 	// TODO would be good to have compiler check this (and related) type switch
 	// by specializing one method per type
 	switch tag {
@@ -441,7 +440,7 @@ func relayAction(e messageEvent, tag protocol.Tag, o interface{}) action {
 	return a
 }
 
-func verifyVoteAction(e messageEvent, r round, p period, taskIndex int) action {
+func verifyVoteAction(e messageEvent, r round, p period, taskIndex uint64) action {
 	return cryptoAction{T: verifyVote, M: e.Input, Round: r, Period: p, TaskIndex: taskIndex}
 }
 
@@ -479,7 +478,7 @@ type checkpointAction struct {
 	Round  round
 	Period period
 	Step   step
-	Err    serializableError
+	Err    *serializableError
 	done   chan error // an output channel to let the pseudonode that we're done processing. We don't want to serialize that, since it's not needed in recovery/autopsy
 }
 

agreement/asyncVoteVerifier.go

@@ -29,7 +29,7 @@ type asyncVerifyVoteRequest struct {
 	l       LedgerReader
 	uv      *unauthenticatedVote
 	uev     *unauthenticatedEquivocationVote
-	index   int
+	index   uint64
 	message message
 
 	// a channel that holds the response
@@ -39,7 +39,7 @@ type asyncVerifyVoteRequest struct {
 type asyncVerifyVoteResponse struct {
 	v         vote
 	ev        equivocationVote
-	index     int
+	index     uint64
 	message   message
 	err       error
 	cancelled bool
@@ -131,7 +131,7 @@ func (avv *AsyncVoteVerifier) executeEqVoteVerification(task interface{}) interf
 	}
 }
 
-func (avv *AsyncVoteVerifier) verifyVote(verctx context.Context, l LedgerReader, uv unauthenticatedVote, index int, message message, out chan<- asyncVerifyVoteResponse) error {
+func (avv *AsyncVoteVerifier) verifyVote(verctx context.Context, l LedgerReader, uv unauthenticatedVote, index uint64, message message, out chan<- asyncVerifyVoteResponse) error {
 	select {
 	case <-avv.ctx.Done(): // if we're quitting, don't enqueue the request
 	// case <-verctx.Done(): DO NOT DO THIS! otherwise we will lose the vote (and forget to clean up)!
@@ -151,7 +151,7 @@ func (avv *AsyncVoteVerifier) verifyVote(verctx context.Context, l LedgerReader,
 	return nil
 }
 
-func (avv *AsyncVoteVerifier) verifyEqVote(verctx context.Context, l LedgerReader, uev unauthenticatedEquivocationVote, index int, message message, out chan<- asyncVerifyVoteResponse) error {
+func (avv *AsyncVoteVerifier) verifyEqVote(verctx context.Context, l LedgerReader, uev unauthenticatedEquivocationVote, index uint64, message message, out chan<- asyncVerifyVoteResponse) error {
 	select {
 	case <-avv.ctx.Done(): // if we're quitting, don't enqueue the request
 	// case <-verctx.Done(): DO NOT DO THIS! otherwise we will lose the vote (and forget to clean up)!

agreement/bundle.go

@@ -202,7 +202,8 @@ func (b unauthenticatedBundle) verifyAsync(ctx context.Context, l LedgerReader,
 
 		rv := rawVote{Sender: auth.Sender, Round: b.Round, Period: b.Period, Step: b.Step, Proposal: b.Proposal}
 		uv := unauthenticatedVote{R: rv, Cred: auth.Cred, Sig: auth.Sig}
-		avv.verifyVote(ctx, l, uv, i, message{}, results)
+
+		avv.verifyVote(ctx, l, uv, uint64(i), message{}, results) //nolint:errcheck // verifyVote will call EnqueueBacklog, which blocks until the verify task is queued, or returns an error when ctx.Done(), which we are already checking
 	}
 
 	// create verification requests for equivocation votes
@@ -222,7 +223,8 @@ func (b unauthenticatedBundle) verifyAsync(ctx context.Context, l LedgerReader,
 			Proposals: auth.Proposals,
 			Sigs:      auth.Sigs,
 		}
-		avv.verifyEqVote(ctx, l, uev, i, message{}, results)
+		avv.verifyEqVote(ctx, l, uev, uint64(i), message{}, results) //nolint:errcheck // verifyVote will call EnqueueBacklog, which blocks until the verify task is queued, or returns an error when ctx.Done(), which we are already checking
+
 	}
 
 	return func() (bundle, error) {

agreement/cadaver.go

@@ -123,7 +123,7 @@ func (c *cadaver) trySetup() bool {
 	if c.out.bytesWritten >= c.fileSizeTarget {
 		err := c.out.Close()
 		if err != nil {
-			logging.Base().Warn("unable to close cadaver file : %v", err)
+			logging.Base().Warnf("unable to close cadaver file : %v", err)
 		}
 		err = os.Rename(c.filename(), c.filename()+".archive")
 		if err != nil {

agreement/cryptoVerifier.go

@@ -82,37 +82,41 @@ type (
 		Quit()
 	}
 
+	//msgp:ignore cryptoVoteRequest
 	cryptoVoteRequest struct {
 		message                   // the message we would like to verify.
-		TaskIndex int             // Caller specific number that would be passed back in the asyncVerifyVoteResponse.TaskIndex field
+		TaskIndex uint64          // Caller specific number that would be passed back in the asyncVerifyVoteResponse.TaskIndex field
 		Round     round           // The round that we're going to test against.
 		Period    period          // The period associated with the message we're going to test.
 		ctx       context.Context // A context for this request, if the context is cancelled then the request is stale.
 	}
 
+	//msgp:ignore cryptoProposalRequest
 	cryptoProposalRequest struct {
 		message                   // the message we would like to verify.
-		TaskIndex int             // Caller specific number that would be passed back in the cryptoResult.TaskIndex field
+		TaskIndex uint64          // Caller specific number that would be passed back in the cryptoResult.TaskIndex field
 		Round     round           // The round that we're going to test against.
 		Period    period          // The period associated with the message we're going to test.
 		Pinned    bool            // A flag that is set if this is a pinned value for the given round.
 		ctx       context.Context // A context for this request, if the context is cancelled then the request is stale.
 	}
 
+	//msgp:ignore cryptoBundleRequest
 	cryptoBundleRequest struct {
 		message                   // the message we would like to verify.
-		TaskIndex int             // Caller specific number that would be passed back in the asyncVerifyVoteResponse.TaskIndex field
+		TaskIndex uint64          // Caller specific number that would be passed back in the asyncVerifyVoteResponse.TaskIndex field
 		Round     round           // The round that we're going to test against.
 		Period    period          // The period associated with the message we're going to test.
 		Certify   bool            // A flag that set if this is a cert bundle.
 		ctx       context.Context // A context for this request, if the context is cancelled then the request is stale.
 	}
 
+	//msgp:ignore cryptoResult
 	cryptoResult struct {
 		message
-		Err       serializableError
-		TaskIndex int  // the TaskIndex that was passed to the cryptoVerifier during the Verify call on the cryptoRequest.TaskIndex
-		Cancelled bool // whether the corresponding request was cancelled before verification completed
+		Err       *serializableError
+		TaskIndex uint64 // the TaskIndex that was passed to the cryptoVerifier during the Verify call on the cryptoRequest.TaskIndex
+		Cancelled bool   // whether the corresponding request was cancelled before verification completed
 	}
 
 	// A poolCryptoVerifier uses asynchronous goroutines to implement cryptoVerifier.
@@ -146,9 +150,10 @@ type (
 		out chan cryptoResult
 	}
 
+	//msgp:ignore bundleFuture
 	bundleFuture struct {
 		message
-		index int
+		index uint64
 		wait  func() (bundle, error)
 		ctx   context.Context
 	}

agreement/cryptoVerifier_test.go

@@ -93,7 +93,7 @@ func makeMessage(msgHandle int, tag protocol.Tag, sender basics.Address, l Ledge
 		}
 
 		return message{
-			MessageHandle:       MessageHandle(msgHandle),
+			messageHandle:       MessageHandle(msgHandle),
 			Tag:                 tag,
 			UnauthenticatedVote: makeUnauthenticatedVote(l, sender, selection, voting, Round, Period, Step, proposal),
 		}
@@ -103,13 +103,13 @@ func makeMessage(msgHandle int, tag protocol.Tag, sender basics.Address, l Ledge
 			Block: e,
 		}
 		return message{
-			MessageHandle:           MessageHandle(msgHandle),
+			messageHandle:           MessageHandle(msgHandle),
 			Tag:                     tag,
 			UnauthenticatedProposal: payload,
 		}
 	default: // protocol.VoteBundleTag
 		return message{
-			MessageHandle: MessageHandle(msgHandle),
+			messageHandle: MessageHandle(msgHandle),
 			Tag:           tag,
 			UnauthenticatedBundle: unauthenticatedBundle{
 				Round:    Round,
@@ -180,9 +180,9 @@ func TestCryptoVerifierBuffers(t *testing.T) {
 	for _, msgType := range msgTypes {
 		for i := getSelectorCapacity(msgType) * 5; i > 0; i-- {
 			msg := <-verifier.Verified(msgType)
-			_, has := usedMsgIDs[msg.MessageHandle]
+			_, has := usedMsgIDs[msg.messageHandle]
 			assert.True(t, has)
-			delete(usedMsgIDs, msg.MessageHandle)
+			delete(usedMsgIDs, msg.messageHandle)
 		}
 		assert.False(t, verifier.ChannelFull(msgType))
 		assert.Zero(t, len(verifier.Verified(msgType)))
@@ -230,8 +230,8 @@ func TestCryptoVerifierBuffers(t *testing.T) {
 		}
 		msgIDMutex.Lock()
 		defer msgIDMutex.Unlock()
-		_, has := usedMsgIDs[msg.MessageHandle]
-		delete(usedMsgIDs, msg.MessageHandle)
+		_, has := usedMsgIDs[msg.messageHandle]
+		delete(usedMsgIDs, msg.messageHandle)
 		return assert.True(t, has)
 	}
 
@@ -333,7 +333,7 @@ func BenchmarkCryptoVerifierProposalVertification(b *testing.B) {
 	c := verifier.Verified(protocol.ProposalPayloadTag)
 	request := cryptoProposalRequest{
 		message: message{
-			MessageHandle:           MessageHandle(0),
+			messageHandle:           MessageHandle(0),
 			Tag:                     protocol.ProposalPayloadTag,
 			UnauthenticatedProposal: proposals[0].unauthenticatedProposal,
 		},
@@ -402,11 +402,11 @@ func TestCryptoVerifierVerificationFailures(t *testing.T) {
 	cryptoVerifier := makeCryptoVerifier(nil, nil, voteVerifier, logging.TestingLog(t))
 	defer cryptoVerifier.Quit()
 
-	cryptoVerifier.VerifyVote(context.Background(), cryptoVoteRequest{message: message{Tag: protocol.AgreementVoteTag}, Round: basics.Round(8), TaskIndex: 14})
+	cryptoVerifier.VerifyVote(context.Background(), cryptoVoteRequest{message: message{Tag: protocol.AgreementVoteTag}, Round: basics.Round(8), TaskIndex: uint64(14)})
 	// read the failed response from VerifiedVotes:
 	votesout := cryptoVerifier.VerifiedVotes()
 	voteResponse := <-votesout
 	require.Equal(t, context.Canceled, voteResponse.err)
 	require.True(t, voteResponse.cancelled)
-	require.Equal(t, 14, voteResponse.index)
+	require.Equal(t, uint64(14), voteResponse.index)
 }