AVM: Allow 0 as app in LocalRef, specifying current app (#6483)

Author: jannotti

config/consensus.go

@@ -539,11 +539,6 @@ type ConsensusParams struct {
 	// EnableBoxRefNameError specifies that box ref names should be validated early
 	EnableBoxRefNameError bool
 
-	// EnableUnnamedBoxAccessInNewApps allows newly created (in this group) apps to
-	// create boxes that were not named in a box ref. Each empty box ref in the
-	// group allows one such creation.
-	EnableUnnamedBoxAccessInNewApps bool
-
 	// ExcludeExpiredCirculation excludes expired stake from the total online stake
 	// used by agreement for Circulation, and updates the calculation of StateProofOnlineTotalWeight used
 	// by state proofs to use the same method (rather than excluding stake from the top N stakeholders as before).
@@ -572,17 +567,16 @@ type ConsensusParams struct {
 	// EnableSha512BlockHash adds an additional SHA-512 hash to the block header.
 	EnableSha512BlockHash bool
 
-	// EnableInnerClawbackWithoutSenderHolding allows an inner clawback (axfer
-	// w/ AssetSender) even if the Sender holding of the asset is not
-	// available. This parameters can be removed and assumed true after the
-	// first consensus release in which it is set true.
-	EnableInnerClawbackWithoutSenderHolding bool
-
 	// AppSizeUpdates allows application update transactions to change
 	// the extra-program-pages and global schema sizes. Since it enables newly
 	// legal transactions, this parameter can be removed and assumed true after
 	// the first consensus release in which it is set true.
 	AppSizeUpdates bool
+
+	// AllowZeroLocalAppRef allows for a 0 in a LocalRef of the access list to
+	// specify the current app. This parameter can be removed and assumed true
+	// after the first consensus release in which it is set true.
+	AllowZeroLocalAppRef bool
 }
 
 // ProposerPayoutRules puts several related consensus parameters in one place. The same
@@ -1444,13 +1438,10 @@ func initConsensusProtocols() {
 	v41.EnableAppVersioning = true
 	v41.EnableSha512BlockHash = true
 
-	v41.EnableUnnamedBoxAccessInNewApps = true
-
 	// txn.Access work
 	v41.MaxAppTxnAccounts = 8       // Accounts are no worse than others, they should be the same
 	v41.MaxAppAccess = 16           // Twice as many, though cross products are explicit
 	v41.BytesPerBoxReference = 2048 // Count is more important that bytes, loosen up
-	v41.EnableInnerClawbackWithoutSenderHolding = true
 	v41.LogicSigMsig = false
 	v41.LogicSigLMsig = true
 
@@ -1469,6 +1460,7 @@ func initConsensusProtocols() {
 	vFuture.LogicSigVersion = 13 // When moving this to a release, put a new higher LogicSigVersion here
 
 	vFuture.AppSizeUpdates = true
+	vFuture.AllowZeroLocalAppRef = true
 
 	Consensus[protocol.ConsensusFuture] = vFuture
 

daemon/algod/api/server/v2/handlers.go

@@ -1318,7 +1318,7 @@ func (v2 *Handlers) SimulateTransaction(ctx echo.Context, params model.SimulateT
 		}
 	}
 
-	response := convertSimulationResult(simulationResult, proto.EnableUnnamedBoxAccessInNewApps)
+	response := convertSimulationResult(simulationResult)
 
 	handle, contentType, err := getCodecHandle((*string)(params.Format))
 	if err != nil {

daemon/algod/api/server/v2/utils.go

@@ -473,13 +473,13 @@ func convertTxnTrace(txnTrace *simulation.TransactionTrace) *model.SimulationTra
 	}
 }
 
-func convertTxnResult(txnResult simulation.TxnResult, simplify bool) PreEncodedSimulateTxnResult {
+func convertTxnResult(txnResult simulation.TxnResult) PreEncodedSimulateTxnResult {
 	result := PreEncodedSimulateTxnResult{
 		Txn:                      ConvertInnerTxn(&txnResult.Txn),
 		AppBudgetConsumed:        omitEmpty(txnResult.AppBudgetConsumed),
 		LogicSigBudgetConsumed:   omitEmpty(txnResult.LogicSigBudgetConsumed),
 		TransactionTrace:         convertTxnTrace(txnResult.Trace),
-		UnnamedResourcesAccessed: convertUnnamedResourcesAccessed(txnResult.UnnamedResourcesAccessed, simplify),
+		UnnamedResourcesAccessed: convertUnnamedResourcesAccessed(txnResult.UnnamedResourcesAccessed),
 	}
 
 	if !txnResult.FixedSigner.IsZero() {
@@ -490,13 +490,11 @@ func convertTxnResult(txnResult simulation.TxnResult, simplify bool) PreEncodedS
 	return result
 }
 
-func convertUnnamedResourcesAccessed(resources *simulation.ResourceTracker, simplify bool) *model.SimulateUnnamedResourcesAccessed {
+func convertUnnamedResourcesAccessed(resources *simulation.ResourceTracker) *model.SimulateUnnamedResourcesAccessed {
 	if resources == nil {
 		return nil
 	}
-	if simplify {
-		resources.Simplify()
-	}
+	resources.Simplify()
 	return &model.SimulateUnnamedResourcesAccessed{
 		Accounts: sliceOrNil(stringSlice(slices.Collect(maps.Keys(resources.Accounts)))),
 		Assets:   sliceOrNil(slices.Collect(maps.Keys(resources.Assets))),
@@ -568,18 +566,15 @@ func convertSimulateInitialStates(initialStates *simulation.ResourcesInitialStat
 	}
 }
 
-func convertTxnGroupResult(txnGroupResult simulation.TxnGroupResult, simplify bool) PreEncodedSimulateTxnGroupResult {
-	txnResults := make([]PreEncodedSimulateTxnResult, len(txnGroupResult.Txns))
-	for i, txnResult := range txnGroupResult.Txns {
-		txnResults[i] = convertTxnResult(txnResult, simplify)
-	}
+func convertTxnGroupResult(txnGroupResult simulation.TxnGroupResult) PreEncodedSimulateTxnGroupResult {
+	txnResults := util.Map(txnGroupResult.Txns, convertTxnResult)
 
 	encoded := PreEncodedSimulateTxnGroupResult{
 		Txns:                     txnResults,
 		FailureMessage:           omitEmpty(txnGroupResult.FailureMessage),
 		AppBudgetAdded:           omitEmpty(txnGroupResult.AppBudgetAdded),
 		AppBudgetConsumed:        omitEmpty(txnGroupResult.AppBudgetConsumed),
-		UnnamedResourcesAccessed: convertUnnamedResourcesAccessed(txnGroupResult.UnnamedResourcesAccessed, simplify),
+		UnnamedResourcesAccessed: convertUnnamedResourcesAccessed(txnGroupResult.UnnamedResourcesAccessed),
 	}
 
 	if len(txnGroupResult.FailedAt) > 0 {
@@ -590,7 +585,7 @@ func convertTxnGroupResult(txnGroupResult simulation.TxnGroupResult, simplify bo
 	return encoded
 }
 
-func convertSimulationResult(result simulation.Result, simplify bool) PreEncodedSimulateResponse {
+func convertSimulationResult(result simulation.Result) PreEncodedSimulateResponse {
 	var evalOverrides *model.SimulationEvalOverrides
 	if result.EvalOverrides != (simulation.ResultEvalOverrides{}) {
 		evalOverrides = &model.SimulationEvalOverrides{
@@ -604,11 +599,9 @@ func convertSimulationResult(result simulation.Result, simplify bool) PreEncoded
 	}
 
 	return PreEncodedSimulateResponse{
-		Version:   result.Version,
-		LastRound: result.LastRound,
-		TxnGroups: util.Map(result.TxnGroups, func(tg simulation.TxnGroupResult) PreEncodedSimulateTxnGroupResult {
-			return convertTxnGroupResult(tg, simplify)
-		}),
+		Version:         result.Version,
+		LastRound:       result.LastRound,
+		TxnGroups:       util.Map(result.TxnGroups, convertTxnGroupResult),
 		EvalOverrides:   evalOverrides,
 		ExecTraceConfig: result.TraceConfig,
 		InitialStates:   convertSimulateInitialStates(result.InitialStates),

data/transactions/application.go

@@ -210,7 +210,7 @@ func (rr ResourceRef) Empty() bool {
 // wellFormed checks that a ResourceRef is a proper member of `access. `rr` is
 // either empty a single kind of resource. Any internal indices point to proper
 // locations inside `access`.
-func (rr ResourceRef) wellFormed(access []ResourceRef, proto config.ConsensusParams) error {
+func (rr ResourceRef) wellFormed(access []ResourceRef, inCreate bool, proto config.ConsensusParams) error {
 	// Count the number of non-empty fields
 	count := 0
 	// The "basic" resources are inherently wellFormed
@@ -231,7 +231,13 @@ func (rr ResourceRef) wellFormed(access []ResourceRef, proto config.ConsensusPar
 		count++
 	}
 	if !rr.Locals.Empty() {
-		if _, _, err := rr.Locals.Resolve(access, basics.Address{}); err != nil {
+		if !proto.AllowZeroLocalAppRef && rr.Locals.App == 0 {
+			return errors.New("0 App in LocalsRef is not supported")
+		}
+		if inCreate && rr.Locals.App == 0 {
+			return errors.New("0 App in LocalsRef during app create is not allowed or necessary")
+		}
+		if _, _, err := rr.Locals.Resolve(access, basics.Address{}, 0); err != nil {
 			return err
 		}
 		count++
@@ -309,9 +315,9 @@ func (lr LocalsRef) Empty() bool {
 	return lr == LocalsRef{}
 }
 
-// Resolve looks up the referenced address and app in the access list. 0 is
-// returned if the App index is 0, meaning "current app".
-func (lr LocalsRef) Resolve(access []ResourceRef, sender basics.Address) (basics.Address, basics.AppIndex, error) {
+// Resolve looks up the referenced address and app in the access list. Zero
+// values are translated to the supplied sender or current app.
+func (lr LocalsRef) Resolve(access []ResourceRef, sender basics.Address, current basics.AppIndex) (basics.Address, basics.AppIndex, error) {
 	address := sender // Returned when lr.Address == 0
 	if lr.Address != 0 {
 		if lr.Address > uint64(len(access)) { // recall that Access is 1-based
@@ -322,12 +328,15 @@ func (lr LocalsRef) Resolve(access []ResourceRef, sender basics.Address) (basics
 			return basics.Address{}, 0, fmt.Errorf("locals Address reference %d is not an Address", lr.Address)
 		}
 	}
-	if lr.App == 0 || lr.App > uint64(len(access)) { // 1-based
-		return basics.Address{}, 0, fmt.Errorf("locals App reference %d outside tx.Access", lr.App)
-	}
-	app := access[lr.App-1].App
-	if app == 0 {
-		return basics.Address{}, 0, fmt.Errorf("locals App reference %d is not an App", lr.App)
+	app := current // Returned when lr.App == 0
+	if lr.App != 0 {
+		if lr.App > uint64(len(access)) { // 1-based
+			return basics.Address{}, 0, fmt.Errorf("locals App reference %d outside tx.Access", lr.App)
+		}
+		app = access[lr.App-1].App
+		if app == 0 {
+			return basics.Address{}, 0, fmt.Errorf("locals App reference %d is not an App", lr.App)
+		}
 	}
 	return address, app, nil
 }
@@ -512,7 +521,7 @@ func (ac ApplicationCallTxnFields) wellFormed(proto config.ConsensusParams) erro
 		}
 
 		for _, rr := range ac.Access {
-			if err := rr.wellFormed(ac.Access, proto); err != nil {
+			if err := rr.wellFormed(ac.Access, ac.ApplicationID == 0, proto); err != nil {
 				return err
 			}
 		}

data/transactions/application_test.go

@@ -203,14 +203,37 @@ func TestAppCallAccessWellFormed(t *testing.T) {
 			},
 		},
 		{
-			expectedError: "locals App reference 0 outside tx.Access",
+			// eliminate this test after AllowZeroAppInLocalsRef is removed
+			expectedError: "0 App in LocalsRef is not supported",
 			ac: ApplicationCallTxnFields{
 				ApplicationID: 1,
 				Access: []ResourceRef{
 					{Address: basics.Address{0xaa}},
 					{Locals: LocalsRef{Address: 1}},
 				},
 			},
+			cv: protocol.ConsensusV41,
+		},
+		{
+			ac: ApplicationCallTxnFields{
+				ApplicationID: 1,
+				Access: []ResourceRef{
+					{Address: basics.Address{0xaa}},
+					{Locals: LocalsRef{Address: 1}},
+				},
+			},
+		},
+		{
+			expectedError: "0 App in LocalsRef during app create is not allowed or necessary",
+			ac: ApplicationCallTxnFields{
+				ApplicationID:     0,
+				ApprovalProgram:   []byte{0x05},
+				ClearStateProgram: []byte{0x05},
+				Access: []ResourceRef{
+					{Address: basics.Address{0xaa}},
+					{Locals: LocalsRef{Address: 1}},
+				},
+			},
 		},
 		{
 			ac: ApplicationCallTxnFields{

data/transactions/logic/box.go

@@ -52,7 +52,7 @@ func (cx *EvalContext) availableBox(name string, operation BoxOperation, createS
 	// we don't have to go to the disk. but we only allow one such access for
 	// each spare (empty) box ref. that way, we can't end up needing to write
 	// many separate newly created boxes.
-	if !ok && cx.Proto.EnableUnnamedBoxAccessInNewApps {
+	if !ok {
 		if _, newAppAccess = cx.available.createdApps[cx.appID]; newAppAccess {
 			if cx.available.unnamedAccess > 0 {
 				ok = true                    // allow it

data/transactions/logic/resources.go

@@ -252,11 +252,9 @@ func (cx *EvalContext) requireLocals(acct basics.Address, id basics.AppIndex) er
 }
 
 func (cx *EvalContext) allowsAssetTransfer(hdr *transactions.Header, tx *transactions.AssetTransferTxnFields) error {
-	// After EnableInnerClawbackWithoutSenderHolding appears in a consensus
-	// update, we should remove it from consensus params and assume it's true in
-	// the next release. It only needs to be in there so that it gates the
-	// behavior change in the release it first appears.
-	if !cx.Proto.EnableInnerClawbackWithoutSenderHolding || tx.AssetSender.IsZero() {
+	// When AssetSender is set (we're doing a clawback) we don't need the
+	// Sender's holding. The Sender/ClawbackAddress may not even have the asset.
+	if tx.AssetSender.IsZero() {
 		err := cx.requireHolding(hdr.Sender, tx.XferAsset)
 		if err != nil {
 			return fmt.Errorf("axfer Sender: %w", err)
@@ -323,17 +321,14 @@ func (r *resources) fillApplicationCallAccess(ep *EvalParams, hdr *transactions.
 			r.shareHolding(address, asset)
 		case !rr.Locals.Empty():
 			// ApplicationCallTxnFields.wellFormed ensures no error here.
-			address, app, _ := rr.Locals.Resolve(tx.Access, hdr.Sender)
+			address, app, _ := rr.Locals.Resolve(tx.Access, hdr.Sender, tx.ApplicationID)
 			r.shareLocal(address, app)
 		case !rr.Box.Empty():
 			// ApplicationCallTxnFields.wellFormed ensures no error here.
 			app, name, _ := rr.Box.Resolve(tx.Access)
 			r.shareBox(basics.BoxRef{App: app, Name: name}, tx.ApplicationID)
 		default:
-			// all empty equals an "empty boxref" which allows one unnamed access
-			if ep.Proto.EnableUnnamedBoxAccessInNewApps {
-				r.unnamedAccess++
-			}
+			r.unnamedAccess++
 		}
 	}
 }
@@ -374,7 +369,7 @@ func (r *resources) fillApplicationCallForeign(ep *EvalParams, hdr *transactions
 	}
 
 	for _, br := range tx.Boxes {
-		if ep.Proto.EnableUnnamedBoxAccessInNewApps && br.Empty() {
+		if br.Empty() {
 			r.unnamedAccess++
 		}
 		var app basics.AppIndex