Remove old buggy version of proposal LowestOutput (algorand#1094)

Now that the protocol upgrade fixing the bug has gone through, we no longer need to keep the buggy version around for compatibility.

agreement/player_test.go

@@ -24,20 +24,10 @@ import (
 
 	"github.com/algorand/go-algorand/config"
 	"github.com/algorand/go-algorand/crypto"
-	"github.com/algorand/go-algorand/data/committee" //TODO(upgrade) remove this line
 	"github.com/algorand/go-algorand/logging"
 	"github.com/algorand/go-algorand/protocol"
 )
 
-// TODO(upgrade) remove the entire lessMaybeBuggy function once the upgrade goes through
-func lessMaybeBuggy(cred, other committee.Credential) bool {
-	// this function calls either Less or LessBuggy depending on ConsensusCurrentVersion, which is what the agreement tests use
-	if config.Consensus[protocol.ConsensusCurrentVersion].UseBuggyProposalLowestOutput {
-		return cred.LessBuggy(other)
-	}
-	return cred.Less(other)
-}
-
 var playerTracer tracer
 
 func init() {
@@ -69,8 +59,7 @@ func generateProposalEvents(t *testing.T, player player, accs testAccountData, f
 	lowestCredential := votes[0].Cred
 	lowestProposal = votes[0].R.Proposal
 	for _, vote := range votes {
-		// if vote.Cred.Less(lowestCredential) { //TODO(upgrade) uncomment this line
-		if lessMaybeBuggy(vote.Cred, lowestCredential) { // TODO(upgrade) remove this line
+		if vote.Cred.Less(lowestCredential) {
 			lowestCredential = vote.Cred
 			lowestProposal = vote.R.Proposal
 		}

agreement/proposalTracker.go

@@ -19,7 +19,6 @@ package agreement
 import (
 	"fmt"
 
-	"github.com/algorand/go-algorand/config" // TODO(upgrade): Please remove this line after the upgrade goes through
 	"github.com/algorand/go-algorand/data/basics"
 	"github.com/algorand/go-algorand/logging"
 )
@@ -38,23 +37,14 @@ type proposalSeeker struct {
 
 // accept compares a given vote with the current lowest-credentialled vote and
 // sets it if freeze has not been called.
-// TODO(upgrade): Please remove the "useBuggyLowestOutput" argument as soon as the protocol upgrade goes through
-func (s proposalSeeker) accept(v vote, useBuggyLowestOutput bool) (proposalSeeker, error) {
+func (s proposalSeeker) accept(v vote) (proposalSeeker, error) {
 	if s.Frozen {
 		return s, errProposalSeekerFrozen{}
 	}
 
-	// TODO(upgrade): Please remove the lines below as soon as the upgrade goes through
-	if useBuggyLowestOutput {
-		if s.Filled && !v.Cred.LessBuggy(s.Lowest.Cred) {
-			return s, errProposalSeekerNotLess{NewSender: v.R.Sender, LowestSender: s.Lowest.R.Sender}
-		}
-	} else {
-		// TODO(upgrade): Please remove the lines above as soon as the upgrade goes through
-		if s.Filled && !v.Cred.Less(s.Lowest.Cred) {
-			return s, errProposalSeekerNotLess{NewSender: v.R.Sender, LowestSender: s.Lowest.R.Sender}
-		}
-	} // TODO(upgrade): Please remove this line when the upgrade goes through
+	if s.Filled && !v.Cred.Less(s.Lowest.Cred) {
+		return s, errProposalSeekerNotLess{NewSender: v.R.Sender, LowestSender: s.Lowest.R.Sender}
+	}
 
 	s.Lowest = v
 	s.Filled = true
@@ -156,7 +146,7 @@ func (t *proposalTracker) handle(r routerHandle, p player, e event) event {
 		}
 
 		var err error
-		t.Freezer, err = t.Freezer.accept(v, config.Consensus[e.Proto.Version].UseBuggyProposalLowestOutput) // TODO(upgrade): Please remove the second argument as soon as the upgrade goes through
+		t.Freezer, err = t.Freezer.accept(v)
 		if err != nil {
 			err := errProposalTrackerPS{Sub: err}
 			return filteredEvent{T: voteFiltered, Err: makeSerErr(err)}

agreement/proposalTracker_test.go

@@ -62,19 +62,19 @@ func TestProposalTrackerProposalSeeker(t *testing.T) {
 	assert.False(t, s.Filled)
 
 	// issue events in the following order: 2, 3, 1, (freeze), 0
-	s, err = s.accept(votes[2], false) //TODO(upgrade) delete the ", false"
+	s, err = s.accept(votes[2])
 	assert.NoError(t, err)
 	assert.False(t, s.Frozen)
 	assert.True(t, s.Filled)
 	assert.True(t, s.Lowest.equals(votes[2]))
 
-	s, err = s.accept(votes[3], false) //TODO(upgrade) delete the ", false"
+	s, err = s.accept(votes[3])
 	assert.Error(t, err)
 	assert.False(t, s.Frozen)
 	assert.True(t, s.Filled)
 	assert.True(t, s.Lowest.equals(votes[2]))
 
-	s, err = s.accept(votes[1], false) //TODO(upgrade) delete the ", false"
+	s, err = s.accept(votes[1])
 	assert.NoError(t, err)
 	assert.False(t, s.Frozen)
 	assert.True(t, s.Filled)
@@ -85,7 +85,7 @@ func TestProposalTrackerProposalSeeker(t *testing.T) {
 	assert.True(t, s.Filled)
 	assert.True(t, s.Lowest.equals(votes[1]))
 
-	s, err = s.accept(votes[0], false) //TODO(upgrade) delete the ", false"
+	s, err = s.accept(votes[0])
 	assert.Error(t, err)
 	assert.True(t, s.Frozen)
 	assert.True(t, s.Filled)

config/consensus.go

@@ -211,10 +211,6 @@ type ConsensusParams struct {
 	// max decimal precision for assets
 	MaxAssetDecimals uint32
 
-	// whether to use the old buggy Credential.lowestOutput function
-	// TODO(upgrade): Please remove as soon as the upgrade goes through
-	UseBuggyProposalLowestOutput bool
-
 	// SupportRekeying indicates support for account rekeying (the RekeyTo and AuthAddr fields)
 	SupportRekeying bool
 
@@ -501,7 +497,6 @@ func initConsensusProtocols() {
 		MaxBalLookback: 320,
 
 		MaxTxGroupSize:               1,
-		UseBuggyProposalLowestOutput: true, // TODO(upgrade): Please remove as soon as the upgrade goes through
 	}
 
 	v7.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{}
@@ -670,7 +665,6 @@ func initConsensusProtocols() {
 	// v21 fixes a bug in Credential.lowestOutput that would cause larger accounts to be selected to propose disproportionately more often than small accounts
 	v21 := v20
 	v21.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{}
-	v21.UseBuggyProposalLowestOutput = false // TODO(upgrade): Please remove this line as soon as the protocol upgrade goes through
 	Consensus[protocol.ConsensusV21] = v21
 	// v20 can be upgraded to v21.
 	v20.ApprovedUpgrades[protocol.ConsensusV21] = 0

data/committee/credential.go

@@ -206,45 +206,6 @@ func (cred Credential) lowestOutput() *big.Int {
 	return &lowest
 }
 
-// TODO(upgrade): Please remove the entire lowestOutputBuggy function as soon as the corresponding protocol upgrade goes through.
-func (cred Credential) lowestOutputBuggy() *big.Int {
-	var lowest big.Int
-
-	h1 := cred.VrfOut
-	for i := uint64(0); i < cred.Weight; i++ {
-		var h crypto.Digest
-		if cred.DomainSeparationEnabled {
-			cred.Hashable.Iter = i
-			h = crypto.HashObj(cred.Hashable)
-		} else {
-			var h2 crypto.Digest
-			binary.BigEndian.PutUint64(h2[:], i)
-			h = crypto.Hash(append(h1[:], h2[:]...))
-		}
-
-		if i == 0 {
-			lowest.SetBytes(h[:])
-		} else {
-			var temp big.Int
-			temp.SetBytes(h[:])
-			if temp.Cmp(&lowest) < 0 {
-				lowest.Set(&temp)
-			}
-		}
-	}
-
-	return &lowest
-}
-
-// LessBuggy is the buggy version of Less
-// TODO(upgrade): Please remove the entire LessBuggy function as soon as the corresponding protocol upgrade goes through
-func (cred Credential) LessBuggy(otherCred Credential) bool {
-	i1 := cred.lowestOutputBuggy()
-	i2 := otherCred.lowestOutputBuggy()
-
-	return i1.Cmp(i2) < 0
-}
-
 // LowestOutputDigest gives the lowestOutput as a crypto.Digest, which allows
 // pretty-printing a proposal's lowest output.
 // This function is only used for debugging.