Consensus upgrade for ExtraProgramPages fix (#2694)

Make sure there is no overflow on TotalExtraAppPages calculation + test

config/consensus.go

@@ -971,9 +971,21 @@ func initConsensusProtocols() {
 	// v27 can be upgraded to v28, with an update delay of 7 days ( see calculation above )
 	v27.ApprovedUpgrades[protocol.ConsensusV28] = 140000
 
+	// v29 fixes application update by using ExtraProgramPages in size calculations
+	v29 := v28
+	v29.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{}
+
+	// Enable ExtraProgramPages for application update
+	v29.EnableExtraPagesOnAppUpdate = true
+
+	Consensus[protocol.ConsensusV29] = v29
+
+	// v28 can be upgraded to v29, with an update delay of 3 days ( see calculation above )
+	v28.ApprovedUpgrades[protocol.ConsensusV29] = 60000
+
 	// ConsensusFuture is used to test features that are implemented
 	// but not yet released in a production protocol version.
-	vFuture := v28
+	vFuture := v29
 	vFuture.ApprovedUpgrades = map[protocol.ConsensusVersion]uint64{}
 
 	// FilterTimeout for period 0 should take a new optimized, configured value, need to revisit this later
@@ -989,9 +1001,6 @@ func initConsensusProtocols() {
 	// Enable TEAL 5 / AVM 1.0
 	vFuture.LogicSigVersion = 5
 
-	// Enable ExtraProgramPages for application update
-	vFuture.EnableExtraPagesOnAppUpdate = true
-
 	Consensus[protocol.ConsensusFuture] = vFuture
 }
 

daemon/algod/api/client/restClient.go

@@ -434,6 +434,14 @@ func (client RestClient) PendingTransactionInformation(transactionID string) (re
 	return
 }
 
+// PendingTransactionInformationV2 gets information about a recently issued
+// transaction. See PendingTransactionInformation for more details.
+func (client RestClient) PendingTransactionInformationV2(transactionID string) (response generatedV2.PendingTransactionResponse, err error) {
+	transactionID = stripTransaction(transactionID)
+	err = client.get(&response, fmt.Sprintf("/v2/transactions/pending/%s", transactionID), nil)
+	return
+}
+
 // SuggestedFee gets the recommended transaction fee from the node
 func (client RestClient) SuggestedFee() (response v1.TransactionFee, err error) {
 	err = client.get(&response, "/v1/transactions/fee", nil)

data/basics/overflow.go

@@ -33,6 +33,13 @@ func OAdd16(a uint16, b uint16) (res uint16, overflowed bool) {
 	return
 }
 
+// OAdd32 adds 2 uint32 values with overflow detection
+func OAdd32(a uint32, b uint32) (res uint32, overflowed bool) {
+	res = a + b
+	overflowed = res < a
+	return
+}
+
 // OAdd adds 2 values with overflow detection
 func OAdd(a uint64, b uint64) (res uint64, overflowed bool) {
 	res = a + b
@@ -47,6 +54,13 @@ func OSub(a uint64, b uint64) (res uint64, overflowed bool) {
 	return
 }
 
+// OSub32 subtracts b from a with overflow detection
+func OSub32(a uint32, b uint32) (res uint32, overflowed bool) {
+	res = a - b
+	overflowed = res > a
+	return
+}
+
 // OMul multiplies 2 values with overflow detection
 func OMul(a uint64, b uint64) (res uint64, overflowed bool) {
 	if b == 0 {
@@ -78,6 +92,15 @@ func AddSaturate(a uint64, b uint64) uint64 {
 	return res
 }
 
+// AddSaturate32 adds 2 uint32 values with saturation on overflow
+func AddSaturate32(a uint32, b uint32) uint32 {
+	res, overflowed := OAdd32(a, b)
+	if overflowed {
+		return math.MaxUint32
+	}
+	return res
+}
+
 // SubSaturate subtracts 2 values with saturation on underflow
 func SubSaturate(a uint64, b uint64) uint64 {
 	res, overflowed := OSub(a, b)
@@ -87,6 +110,15 @@ func SubSaturate(a uint64, b uint64) uint64 {
 	return res
 }
 
+// SubSaturate32 subtracts 2 uint32 values with saturation on underflow
+func SubSaturate32(a uint32, b uint32) uint32 {
+	res, overflowed := OSub32(a, b)
+	if overflowed {
+		return 0
+	}
+	return res
+}
+
 // Add16 adds 2 uint16 values with overflow detection
 func (t *OverflowTracker) Add16(a uint16, b uint16) uint16 {
 	res, overflowed := OAdd16(a, b)

data/basics/units_test.go

@@ -17,6 +17,7 @@
 package basics
 
 import (
+	"math"
 	"testing"
 
 	"github.com/algorand/go-algorand/test/partitiontest"
@@ -33,6 +34,27 @@ func TestSubSaturate(t *testing.T) {
 	require.Equal(t, b.SubSaturate(a), Round(1))
 }
 
+func TestSubSaturate32(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	require.Equal(t, uint32(0), SubSaturate32(0, 1))
+	require.Equal(t, uint32(0), SubSaturate32(1, 2))
+	require.Equal(t, uint32(0), SubSaturate32(1, 1))
+	require.Equal(t, uint32(0), SubSaturate32(1, math.MaxUint32))
+	require.Equal(t, uint32(1), SubSaturate32(2, 1))
+	require.Equal(t, uint32(math.MaxUint32-1), SubSaturate32(math.MaxUint32, 1))
+}
+
+func TestAddSaturate32(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	require.Equal(t, uint32(1), AddSaturate32(0, 1))
+	require.Equal(t, uint32(math.MaxUint32-1), AddSaturate32(math.MaxUint32-2, 1))
+	require.Equal(t, uint32(math.MaxUint32), AddSaturate32(math.MaxUint32, 0))
+	require.Equal(t, uint32(math.MaxUint32), AddSaturate32(math.MaxUint32-1, 1))
+	require.Equal(t, uint32(math.MaxUint32), AddSaturate32(math.MaxUint32, 2))
+}
+
 func TestRoundUpToMultipleOf(t *testing.T) {
 	partitiontest.PartitionTest(t)
 

ledger/apply/application.go

@@ -118,7 +118,7 @@ func createApplication(ac *transactions.ApplicationCallTxnFields, balances Balan
 	// Update the cached TotalExtraAppPages for this account, used
 	// when computing MinBalance
 	totalExtraPages := record.TotalExtraAppPages
-	totalExtraPages += ac.ExtraProgramPages
+	totalExtraPages = basics.AddSaturate32(totalExtraPages, ac.ExtraProgramPages)
 	record.TotalExtraAppPages = totalExtraPages
 
 	// Write back to the creator's balance record
@@ -143,25 +143,29 @@ func deleteApplication(balances Balances, creator basics.Address, appIdx basics.
 		return err
 	}
 
+	record.AppParams = cloneAppParams(record.AppParams)
+
 	// Update the TotalAppSchema used for MinBalance calculation,
 	// since the creator no longer has to store the GlobalState
 	totalSchema := record.TotalAppSchema
 	globalSchema := record.AppParams[appIdx].GlobalStateSchema
 	totalSchema = totalSchema.SubSchema(globalSchema)
 	record.TotalAppSchema = totalSchema
 
-	// Delete the AppParams
-	record.AppParams = cloneAppParams(record.AppParams)
-	delete(record.AppParams, appIdx)
-
 	// Delete app's extra program pages
 	totalExtraPages := record.TotalExtraAppPages
 	if totalExtraPages > 0 {
-		extraPages := record.AppParams[appIdx].ExtraProgramPages
-		totalExtraPages -= extraPages
+		proto := balances.ConsensusParams()
+		if proto.EnableExtraPagesOnAppUpdate {
+			extraPages := record.AppParams[appIdx].ExtraProgramPages
+			totalExtraPages = basics.SubSaturate32(totalExtraPages, extraPages)
+		}
 		record.TotalExtraAppPages = totalExtraPages
 	}
 
+	// Delete the AppParams
+	delete(record.AppParams, appIdx)
+
 	err = balances.Put(creator, record)
 	if err != nil {
 		return err
@@ -189,7 +193,7 @@ func updateApplication(ac *transactions.ApplicationCallTxnFields, balances Balan
 	proto := balances.ConsensusParams()
 	// when proto.EnableExtraPageOnAppUpdate is false, WellFormed rejects all updates with a multiple-page program
 	if proto.EnableExtraPagesOnAppUpdate {
-		allowed := int(1+params.ExtraProgramPages) * proto.MaxAppProgramLen
+		allowed := int(1+params.ExtraProgramPages) * proto.MaxAppTotalProgramLen
 		actual := len(ac.ApprovalProgram) + len(ac.ClearStateProgram)
 		if actual > allowed {
 			return fmt.Errorf("updateApplication app programs too long, %d. max total len %d bytes", actual, allowed)

ledger/apply/application_test.go

@@ -1025,6 +1025,7 @@ func TestAppCallApplyUpdate(t *testing.T) {
 
 	b.pass = true
 	err = ApplicationCall(ac, h, &b, ad, &ep, txnCounter)
+	a.Error(err)
 	a.Contains(err.Error(), "updateApplication app programs too long")
 
 	// check extraProgramPages is used
@@ -1065,6 +1066,7 @@ func TestAppCallApplyDelete(t *testing.T) {
 		StateSchemas: basics.StateSchemas{
 			GlobalStateSchema: basics.StateSchema{NumUint: 1},
 		},
+		ExtraProgramPages: 1,
 	}
 	h := transactions.Header{
 		Sender: sender,
@@ -1074,15 +1076,19 @@ func TestAppCallApplyDelete(t *testing.T) {
 	var b testBalances
 
 	b.balances = make(map[basics.Address]basics.AccountData)
+	// cbr is to ensure the original balance record is not modified but copied when updated in apply
 	cbr := basics.AccountData{
-		AppParams: map[basics.AppIndex]basics.AppParams{appIdx: params},
+		AppParams:          map[basics.AppIndex]basics.AppParams{appIdx: params},
+		TotalExtraAppPages: 1,
 	}
 	cp := basics.AccountData{
-		AppParams: map[basics.AppIndex]basics.AppParams{appIdx: params},
+		AppParams:          map[basics.AppIndex]basics.AppParams{appIdx: params},
+		TotalExtraAppPages: 1,
 	}
 	b.balances[creator] = cp
 	b.appCreators = map[basics.AppIndex]basics.Address{appIdx: creator}
 
+	// check if it fails nothing changes
 	b.SetProto(protocol.ConsensusFuture)
 	proto := b.ConsensusParams()
 	ep.Proto = &proto
@@ -1096,7 +1102,11 @@ func TestAppCallApplyDelete(t *testing.T) {
 	a.Equal(cbr, br)
 	a.Equal(basics.EvalDelta{}, ad.EvalDelta)
 
-	// check deletion on empty balance record - happy case
+	// check calculation on ConsensusV28. TotalExtraAppPages does not change
+	b.SetProto(protocol.ConsensusV28)
+	proto = b.ConsensusParams()
+	ep.Proto = &proto
+
 	b.pass = true
 	b.balances[sender] = basics.AccountData{}
 	err = ApplicationCall(ac, h, &b, ad, &ep, txnCounter)
@@ -1109,7 +1119,43 @@ func TestAppCallApplyDelete(t *testing.T) {
 	a.Equal(basics.AppParams{}, br.AppParams[appIdx])
 	a.Equal(basics.StateSchema{}, br.TotalAppSchema)
 	a.Equal(basics.EvalDelta{}, ad.EvalDelta)
-	a.Equal(uint32(0), br.TotalExtraAppPages)
+	a.Equal(uint32(1), br.TotalExtraAppPages)
+	b.ResetWrites()
+
+	b.SetProto(protocol.ConsensusFuture)
+	proto = b.ConsensusParams()
+	ep.Proto = &proto
+
+	// check deletion
+	for initTotalExtraPages := uint32(0); initTotalExtraPages < 3; initTotalExtraPages++ {
+		cbr = basics.AccountData{
+			AppParams:          map[basics.AppIndex]basics.AppParams{appIdx: params},
+			TotalExtraAppPages: initTotalExtraPages,
+		}
+		cp := basics.AccountData{
+			AppParams:          map[basics.AppIndex]basics.AppParams{appIdx: params},
+			TotalExtraAppPages: initTotalExtraPages,
+		}
+		b.balances[creator] = cp
+		b.pass = true
+		b.balances[sender] = basics.AccountData{}
+		err = ApplicationCall(ac, h, &b, ad, &ep, txnCounter)
+		a.NoError(err)
+		a.Equal(appIdx, b.deAllocatedAppIdx)
+		a.Equal(1, b.put)
+		br = b.balances[creator]
+		a.Equal(cbr, br)
+		br = b.putBalances[creator]
+		a.Equal(basics.AppParams{}, br.AppParams[appIdx])
+		a.Equal(basics.StateSchema{}, br.TotalAppSchema)
+		a.Equal(basics.EvalDelta{}, ad.EvalDelta)
+		if initTotalExtraPages <= params.ExtraProgramPages {
+			a.Equal(uint32(0), br.TotalExtraAppPages)
+		} else {
+			a.Equal(initTotalExtraPages-1, br.TotalExtraAppPages)
+		}
+		b.ResetWrites()
+	}
 }
 
 func TestAppCallApplyCreateClearState(t *testing.T) {

libgoal/libgoal.go

@@ -746,6 +746,16 @@ func (c *Client) PendingTransactionInformation(txid string) (resp v1.Transaction
 	return
 }
 
+// PendingTransactionInformationV2 returns information about a recently issued
+// transaction based on its txid.
+func (c *Client) PendingTransactionInformationV2(txid string) (resp generatedV2.PendingTransactionResponse, err error) {
+	algod, err := c.ensureAlgodClient()
+	if err == nil {
+		resp, err = algod.PendingTransactionInformationV2(txid)
+	}
+	return
+}
+
 // Block takes a round and returns its block
 func (c *Client) Block(round uint64) (resp v1.Block, err error) {
 	algod, err := c.ensureAlgodClient()

protocol/consensus.go

@@ -153,6 +153,11 @@ const ConsensusV28 = ConsensusVersion(
 	"https://github.com/algorandfoundation/specs/tree/65b4ab3266c52c56a0fa7d591754887d68faad0a",
 )
 
+// ConsensusV29 fixes application update by using ExtraProgramPages in size calculations
+const ConsensusV29 = ConsensusVersion(
+	"https://github.com/algorandfoundation/specs/tree/abc54f79f9ad679d2d22f0fb9909fb005c16f8a1",
+)
+
 // ConsensusFuture is a protocol that should not appear in any production
 // network, but is used to test features before they are released.
 const ConsensusFuture = ConsensusVersion(
@@ -165,7 +170,7 @@ const ConsensusFuture = ConsensusVersion(
 
 // ConsensusCurrentVersion is the latest version and should be used
 // when a specific version is not provided.
-const ConsensusCurrentVersion = ConsensusV28
+const ConsensusCurrentVersion = ConsensusV29
 
 // Error is used to indicate that an unsupported protocol has been detected.
 type Error ConsensusVersion