Match old BuildEvalDelta behaviour - no delete missing keys (#3594)

3 files changed, 99 insertions, 6 deletions

diff --git a/data/transactions/logic/eval.go b/data/transactions/logic/eval.go
index d3aea8279..135e06d43 100644
--- a/data/transactions/logic/eval.go
+++ b/data/transactions/logic/eval.go
@@ -3705,15 +3705,27 @@ func opAppLocalDel(cx *EvalContext) {
 	}
 
 	addr, accountIdx, err := cx.mutableAccountReference(cx.stack[prev])
-	if err == nil {
+	if err != nil {
+		cx.err = err
+		return
+	}
+
+	// if deleting a non-existant value, do nothing, matching ledger behavior
+	// with previous BuildEvalDelta mechanism
+	if _, ok, err := cx.Ledger.GetLocal(addr, cx.appID, key, accountIdx); ok {
+		if err != nil {
+			cx.err = err
+			return
+		}
 		if _, ok := cx.Txn.EvalDelta.LocalDeltas[accountIdx]; !ok {
 			cx.Txn.EvalDelta.LocalDeltas[accountIdx] = basics.StateDelta{}
 		}
 		cx.Txn.EvalDelta.LocalDeltas[accountIdx][key] = basics.ValueDelta{
 			Action: basics.DeleteAction,
 		}
-		err = cx.Ledger.DelLocal(addr, cx.appID, key, accountIdx)
 	}
+
+	err = cx.Ledger.DelLocal(addr, cx.appID, key, accountIdx)
 	if err != nil {
 		cx.err = err
 		return
@@ -3732,9 +3744,18 @@ func opAppGlobalDel(cx *EvalContext) {
 		return
 	}
 
-	cx.Txn.EvalDelta.GlobalDelta[key] = basics.ValueDelta{
-		Action: basics.DeleteAction,
+	// if deleting a non-existant value, do nothing, matching ledger behavior
+	// with previous BuildEvalDelta mechanism
+	if _, ok, err := cx.Ledger.GetGlobal(cx.appID, key); ok {
+		if err != nil {
+			cx.err = err
+			return
+		}
+		cx.Txn.EvalDelta.GlobalDelta[key] = basics.ValueDelta{
+			Action: basics.DeleteAction,
+		}
 	}
+
 	err := cx.Ledger.DelGlobal(cx.appID, key)
 	if err != nil {
 		cx.err = err
diff --git a/data/transactions/logic/evalStateful_test.go b/data/transactions/logic/evalStateful_test.go
index 6d5af8de6..9c59fadc6 100644
--- a/data/transactions/logic/evalStateful_test.go
+++ b/data/transactions/logic/evalStateful_test.go
@@ -1133,9 +1133,43 @@ func TestAcctParams(t *testing.T) {
 	testApp(t, source, ep)
 }
 
-func TestAppLocalReadWriteDeleteErrors(t *testing.T) {
+func TestGlobalNonDelete(t *testing.T) {
 	partitiontest.PartitionTest(t)
+	t.Parallel()
+
+	ep, txn, ledger := makeSampleEnv()
+	source := `
+byte "none"
+app_global_del
+int 1
+`
+	ledger.NewApp(txn.Sender, 888, makeApp(0, 0, 1, 0))
+	delta := testApp(t, source, ep)
+	require.Empty(t, delta.GlobalDelta)
+	require.Empty(t, delta.LocalDeltas)
+}
 
+func TestLocalNonDelete(t *testing.T) {
+	partitiontest.PartitionTest(t)
+	t.Parallel()
+
+	ep, txn, ledger := makeSampleEnv()
+	source := `
+int 0
+byte "none"
+app_local_del
+int 1
+`
+	ledger.NewAccount(txn.Sender, 100000)
+	ledger.NewApp(txn.Sender, 888, makeApp(0, 0, 1, 0))
+	ledger.NewLocals(txn.Sender, 888)
+	delta := testApp(t, source, ep)
+	require.Empty(t, delta.GlobalDelta)
+	require.Empty(t, delta.LocalDeltas)
+}
+
+func TestAppLocalReadWriteDeleteErrors(t *testing.T) {
+	partitiontest.PartitionTest(t)
 	t.Parallel()
 
 	sourceRead := `intcblock 0 100 0x77 1
@@ -1294,7 +1328,7 @@ int 0x77
 &&
 `
 	delta := testApp(t, source, ep)
-	require.Empty(t, 0, delta.GlobalDelta)
+	require.Empty(t, delta.GlobalDelta)
 	require.Len(t, delta.LocalDeltas, 1)
 
 	require.Len(t, delta.LocalDeltas[0], 2)
diff --git a/ledger/internal/eval_blackbox_test.go b/ledger/internal/eval_blackbox_test.go
index d2a1e0d37..15a032a44 100644
--- a/ledger/internal/eval_blackbox_test.go
+++ b/ledger/internal/eval_blackbox_test.go
@@ -963,6 +963,44 @@ func TestModifiedAppLocalStates(t *testing.T) {
 	}
 }
 
+// TestDeleteNonExistantKeys checks if the EvalDeltas from deleting missing keys are correct
+func TestDeleteNonExistantKeys(t *testing.T) {
+	partitiontest.PartitionTest(t)
+
+	genesisInitState, addrs, _ := ledgertesting.Genesis(10)
+
+	l, err := ledger.OpenLedger(logging.TestingLog(t), "", true, genesisInitState, config.GetDefaultLocal())
+	require.NoError(t, err)
+	defer l.Close()
+
+	const appid basics.AppIndex = 1
+
+	createTxn := txntest.Txn{
+		Type:   "appl",
+		Sender: addrs[0],
+		ApprovalProgram: main(`
+byte "missing_global"
+app_global_del
+int 0
+byte "missing_local"
+app_local_del
+`),
+	}
+
+	optInTxn := txntest.Txn{
+		Type:          "appl",
+		Sender:        addrs[1],
+		ApplicationID: appid,
+		OnCompletion:  transactions.OptInOC,
+	}
+
+	eval := nextBlock(t, l, true, nil)
+	txns(t, l, eval, &createTxn, &optInTxn)
+	vb := endBlock(t, l, eval)
+	require.Len(t, vb.Block().Payset[1].EvalDelta.GlobalDelta, 0)
+	require.Len(t, vb.Block().Payset[1].EvalDelta.LocalDeltas, 0)
+}
+
 // TestAppInsMinBalance checks that accounts with MaxAppsOptedIn are accepted by block evaluator
 // and do not cause any MaximumMinimumBalance problems
 func TestAppInsMinBalance(t *testing.T) {