ledger: fix error shadowing in onlineAccountsNewRoundImpl (#5188)

data/transactions/logic/eval.go

@@ -709,7 +709,7 @@ func EvalContract(program []byte, gi int, aid basics.AppIndex, params *EvalParam
 		return false, nil, errors.New("no ledger in contract eval")
 	}
 	if params.SigLedger == nil {
-		params.SigLedger = params.Ledger
+		return false, nil, errors.New("no sig ledger in contract eval")
 	}
 	if aid == 0 {
 		return false, nil, errors.New("0 appId in contract eval")

data/transactions/logic/evalAppTxn_test.go

@@ -2634,6 +2634,7 @@ itxn_submit
 	txg := []transactions.SignedTxnWithAD{tx}
 	ep := NewEvalParams(txg, MakeTestProto(), &transactions.SpecialAddresses{})
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	TestApp(t, callpay3+"int 1", ep, "insufficient balance") // inner contract needs money
 
 	ledger.NewAccount(appAddr(222), 1_000_000)

data/transactions/logic/evalStateful_test.go

@@ -362,6 +362,7 @@ func testApps(t *testing.T, programs []string, txgroup []transactions.SignedTxn,
 	}
 	ledger.Reset()
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	testAppsBytes(t, codes, ep, expected...)
 }
 
@@ -1301,6 +1302,7 @@ intc_1
 				},
 			)
 			ep.Ledger = ledger
+			ep.SigLedger = ledger
 
 			saved := ops.Program[firstCmdOffset]
 			require.Equal(t, OpsByName[0]["intc_0"].Opcode, saved)
@@ -1353,6 +1355,7 @@ func TestAppLocalStateReadWrite(t *testing.T) {
 		},
 	)
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	ledger.NewApp(txn.Txn.Sender, 100, basics.AppParams{})
 	ledger.NewLocals(txn.Txn.Sender, 100)
 
@@ -1733,6 +1736,7 @@ int 0x77
 		},
 	)
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	ledger.NewApp(txn.Txn.Sender, 100, basics.AppParams{})
 
 	delta := testApp(t, source, ep)
@@ -1908,6 +1912,7 @@ int 7
 	ledger := NewLedger(nil)
 	ledger.NewAccount(txn.Txn.Sender, 1)
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	ledger.NewApp(txn.Txn.Sender, 100, basics.AppParams{})
 
 	delta := testApp(t, source, ep)
@@ -2111,6 +2116,7 @@ int 1
 		},
 	)
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	ledger.NewApp(txn.Txn.Sender, 100, basics.AppParams{})
 	ledger.NewLocals(txn.Txn.Sender, 100)
 	ledger.NewAccount(txn.Txn.Receiver, 1)

data/transactions/logic/eval_test.go

@@ -5404,6 +5404,7 @@ func TestOpJSONRef(t *testing.T) {
 	ledger.NewApp(txn.Txn.Receiver, 0, basics.AppParams{})
 	ep := defaultEvalParams(txn)
 	ep.Ledger = ledger
+	ep.SigLedger = ledger
 	testCases := []struct {
 		source             string
 		previousVersErrors []Expect

ledger/acctdeltas.go

@@ -1000,8 +1000,9 @@ func onlineAccountsNewRoundImpl(
 							err = fmt.Errorf("empty voting data for online account %s: %v", data.address.String(), newAcct)
 						} else {
 							// create a new entry.
+							var ref trackerdb.OnlineAccountRef
 							normBalance := newAcct.NormalizedOnlineBalance(proto)
-							ref, err := writer.InsertOnlineAccount(data.address, normBalance, newAcct, updRound, uint64(newAcct.VoteLastValid))
+							ref, err = writer.InsertOnlineAccount(data.address, normBalance, newAcct, updRound, uint64(newAcct.VoteLastValid))
 							if err == nil {
 								updated := trackerdb.PersistedOnlineAccountData{
 									Addr:        data.address,
@@ -1025,7 +1026,8 @@ func onlineAccountsNewRoundImpl(
 					if newStatus == basics.Online {
 						err = fmt.Errorf("empty voting data but online account %s: %v", data.address.String(), newAcct)
 					} else {
-						ref, err := writer.InsertOnlineAccount(data.address, 0, trackerdb.BaseOnlineAccountData{}, updRound, 0)
+						var ref trackerdb.OnlineAccountRef
+						ref, err = writer.InsertOnlineAccount(data.address, 0, trackerdb.BaseOnlineAccountData{}, updRound, 0)
 						if err == nil {
 							updated := trackerdb.PersistedOnlineAccountData{
 								Addr:        data.address,
@@ -1041,8 +1043,9 @@ func onlineAccountsNewRoundImpl(
 					}
 				} else {
 					if prevAcct.AccountData != newAcct {
+						var ref trackerdb.OnlineAccountRef
 						normBalance := newAcct.NormalizedOnlineBalance(proto)
-						ref, err := writer.InsertOnlineAccount(data.address, normBalance, newAcct, updRound, uint64(newAcct.VoteLastValid))
+						ref, err = writer.InsertOnlineAccount(data.address, normBalance, newAcct, updRound, uint64(newAcct.VoteLastValid))
 						if err == nil {
 							updated := trackerdb.PersistedOnlineAccountData{
 								Addr:        data.address,

ledger/acctdeltas_test.go

@@ -2797,6 +2797,107 @@ func TestOnlineAccountsDeletion(t *testing.T) {
 	}
 }
 
+type mockOnlineAccountsErrorWriter struct {
+}
+
+var errMockOnlineAccountsErrorWriter = errors.New("synthetic err")
+
+func (w *mockOnlineAccountsErrorWriter) InsertOnlineAccount(addr basics.Address, normBalance uint64, data trackerdb.BaseOnlineAccountData, updRound uint64, voteLastValid uint64) (ref trackerdb.OnlineAccountRef, err error) {
+	return nil, errMockOnlineAccountsErrorWriter
+}
+
+func (w *mockOnlineAccountsErrorWriter) Close() {}
+
+// TestOnlineAccountsNewRoundError checks onlineAccountsNewRoundImpl propagates errors to the caller
+func TestOnlineAccountsNewRoundError(t *testing.T) {
+	partitiontest.PartitionTest(t)
+	t.Parallel()
+
+	writer := &mockOnlineAccountsErrorWriter{}
+	proto := config.Consensus[protocol.ConsensusCurrentVersion]
+
+	addrA := ledgertesting.RandomAddress()
+
+	// acct A is new, offline and then online => exercise new entry for account
+	deltaA := onlineAccountDelta{
+		address: addrA,
+		newAcct: []trackerdb.BaseOnlineAccountData{
+			{
+				MicroAlgos: basics.MicroAlgos{Raw: 100_000_000},
+			},
+			{
+				MicroAlgos:     basics.MicroAlgos{Raw: 100_000_000},
+				BaseVotingData: trackerdb.BaseVotingData{VoteFirstValid: 100},
+			},
+		},
+		updRound:  []uint64{1, 2},
+		newStatus: []basics.Status{basics.Offline, basics.Online},
+	}
+	updates := compactOnlineAccountDeltas{}
+	updates.deltas = append(updates.deltas, deltaA)
+	lastUpdateRound := basics.Round(1)
+	updated, err := onlineAccountsNewRoundImpl(writer, updates, proto, lastUpdateRound)
+	require.Error(t, err)
+	require.Equal(t, errMockOnlineAccountsErrorWriter, err)
+	require.Empty(t, updated)
+
+	// update acct A => exercise "update"
+	deltaA2 := onlineAccountDelta{
+		address: addrA,
+		newAcct: []trackerdb.BaseOnlineAccountData{
+			{
+				MicroAlgos:     basics.MicroAlgos{Raw: 100_000_000},
+				BaseVotingData: trackerdb.BaseVotingData{VoteFirstValid: 200},
+			},
+		},
+		updRound:  []uint64{3},
+		newStatus: []basics.Status{basics.Online},
+		oldAcct: trackerdb.PersistedOnlineAccountData{
+			Addr: addrA,
+			Ref:  &mockEntryRef{},
+			AccountData: trackerdb.BaseOnlineAccountData{
+				MicroAlgos:     basics.MicroAlgos{Raw: 100_000_000},
+				BaseVotingData: trackerdb.BaseVotingData{VoteFirstValid: 100},
+			},
+		},
+	}
+	updates = compactOnlineAccountDeltas{}
+	updates.deltas = append(updates.deltas, deltaA2)
+	lastUpdateRound = basics.Round(3)
+	updated, err = onlineAccountsNewRoundImpl(writer, updates, proto, lastUpdateRound)
+	require.Error(t, err)
+	require.Equal(t, errMockOnlineAccountsErrorWriter, err)
+	require.Empty(t, updated)
+
+	// make acct A offline => exercise "deletion"
+	deltaA3 := onlineAccountDelta{
+		address: addrA,
+		newAcct: []trackerdb.BaseOnlineAccountData{
+			{
+				MicroAlgos:     basics.MicroAlgos{Raw: 100_000_000},
+				BaseVotingData: trackerdb.BaseVotingData{}, // empty
+			},
+		},
+		updRound:  []uint64{4},
+		newStatus: []basics.Status{basics.Offline},
+		oldAcct: trackerdb.PersistedOnlineAccountData{
+			Addr: addrA,
+			Ref:  &mockEntryRef{},
+			AccountData: trackerdb.BaseOnlineAccountData{
+				MicroAlgos:     basics.MicroAlgos{Raw: 100_000_000},
+				BaseVotingData: trackerdb.BaseVotingData{VoteFirstValid: 200},
+			},
+		},
+	}
+	updates = compactOnlineAccountDeltas{}
+	updates.deltas = append(updates.deltas, deltaA3)
+	lastUpdateRound = basics.Round(4)
+	updated, err = onlineAccountsNewRoundImpl(writer, updates, proto, lastUpdateRound)
+	require.Error(t, err)
+	require.Equal(t, errMockOnlineAccountsErrorWriter, err)
+	require.Empty(t, updated)
+}
+
 func randomBaseAccountData() trackerdb.BaseAccountData {
 	vd := trackerdb.BaseVotingData{
 		VoteFirstValid:  basics.Round(crypto.RandUint64()),

ledger/acctonline.go

@@ -478,6 +478,7 @@ func (ao *onlineAccounts) postCommit(ctx context.Context, dcc *deferredCommitCon
 
 	for _, persistedAcct := range dcc.updatedPersistedOnlineAccounts {
 		ao.baseOnlineAccounts.write(persistedAcct)
+		// add account into onlineAccountsCache only if prior history exists
 		ao.onlineAccountsCache.writeFrontIfExist(
 			persistedAcct.Addr,
 			cachedOnlineAccount{

ledger/internal/appcow.go

@@ -456,9 +456,15 @@ func MakeDebugBalances(l LedgerForCowBase, round basics.Round, proto protocol.Co
 func (cb *roundCowState) StatefulEval(gi int, params *logic.EvalParams, aidx basics.AppIndex, program []byte) (pass bool, evalDelta transactions.EvalDelta, err error) {
 	// Make a child cow to eval our program in
 	calf := cb.child(1)
-	defer calf.recycle()
+	defer func() {
+		// get rid of references to the object that is about to be recycled
+		params.Ledger = nil 
+		params.SigLedger = nil
+		calf.recycle()
+	}()
 
 	params.Ledger = calf
+	params.SigLedger = calf
 
 	// Eval the program
 	pass, cx, err := logic.EvalContract(program, gi, aidx, params)