update comments for CR

algorand · gmalouf · Jan 8, 2025 · Jan 3, 2025 · Jan 3, 2025 · Jan 3, 2025
commit 4fdd46a6da4d109865215f68c856ef242f15a5e4
diff --git a/ledger/catchpointtracker.go b/ledger/catchpointtracker.go
@@ -238,9 +238,9 @@
 		onlineExcludeBefore = (dbRound + 1).SubSaturate(basics.Round(params.MaxBalLookback))
 	} else {
 		// The previous flush left less online-related rows than we want in the DB. This should not happen; return error
 		ct.log.Errorf("catchpointTracker.finishFirstStage: dbRound %d and onlineAccountsForgetBefore %d has less history than MaxBalLookback %d",
 			dbRound, onlineAccountsForgetBefore, params.MaxBalLookback)
 		return errors.New("catchpointTracker.finishFirstStage: onlineAccountsForgetBefore doesn't provide enough history")
 	}

 	if params.EnableCatchpointsWithSPContexts {
@@ -326,11 +326,11 @@
 		return err
 	}
 
-	// pass dbRound+1-maxBalLookback as the onlineExcludeBefore parameter: since we can't be sure whether
+	// pass dbRound+1-maxBalLookback as the onlineAccountsForgetBefore parameter: since we can't be sure whether
 	// there are more than 320 rounds of history in the online accounts tables, this ensures the catchpoint
 	// will only contain the most recent 320 rounds.
-	onlineExcludeBefore := (dbRound + 1).SubSaturate(basics.Round(config.Consensus[blockProto].MaxBalLookback))
-	return ct.finishFirstStage(context.Background(), dbRound, onlineExcludeBefore, blockProto, 0)
+	onlineAccountsForgetBefore := (dbRound + 1).SubSaturate(basics.Round(config.Consensus[blockProto].MaxBalLookback))
+	return ct.finishFirstStage(context.Background(), dbRound, onlineAccountsForgetBefore, blockProto, 0)
 }
 
 func (ct *catchpointTracker) finishCatchpointsAfterCrash(blockProto protocol.ConsensusVersion, catchpointLookback uint64) error {

diff --git a/ledger/store/trackerdb/sqlitedriver/kvsIter.go b/ledger/store/trackerdb/sqlitedriver/kvsIter.go
@@ -69,55 +69,69 @@
 }

 func (iter *tableIterator[T]) Next() bool { return iter.rows.Next() }
 func (iter *tableIterator[T]) Close() {
 	iter.rows.Close()
 	if iter.onClose != nil {
 		iter.onClose()
 	}
 }
 func (iter *tableIterator[T]) GetItem() (T, error) {
 	return iter.scan(iter.rows)
 }
 
-// MakeOnlineAccountsIter creates an onlineAccounts iterator.
+// MakeOnlineAccountsIter creates an onlineAccounts iterator, used by the catchpoint system to dump the
+// onlineaccounts table to a catchpoint snapshot file.
+//
+// If excludeBefore is non-zero, the iterator will exclude all data that would have been deleted if
+// OnlineAccountsDelete(excludeBefore) were called on this DB before calling MakeOnlineAccountsIter.
 func MakeOnlineAccountsIter(ctx context.Context, q db.Queryable, useStaging bool, excludeBefore basics.Round) (trackerdb.TableIterator[*encoded.OnlineAccountRecordV6], error) {
 	table := "onlineaccounts"
 	if useStaging {
 		table = "catchpointonlineaccounts"
 	}
 
 	var onClose func()
 	if excludeBefore != 0 {
-		// cheat: use Rdb to make a temporary table that we will delete later
+		// This is a special case to resolve the issue found in #6214. When the state proof votersTracker has not
+		// yet validated the recent state proof, the onlineaccounts table will hold more than 320 rows,
+		// to support state proof recovery (votersTracker.lowestRound() sets deferredCommitRange.lowestRound).
+		//
+		// While rare, this may happen e.g. during catchup, where blocks may be flying by so quickly that the
+		// catchpoint snapshot is started before the latest state proof was validated. In this case, excludeBefore
+		// will be set to R-320 (MaxBalLookback) where R is the DB snapshot round (specified by CatchpointLookback).
+		//
+		// Unfortunately catchpoint snapshots occur within a SnapshotScope, and so a db.Queryable cannot
+		// execute DDL statements. To work around this, we create a temporary table that we will delete
+		// when the iterator is closed.
 		e, ok := q.(*sql.Tx)
 		if !ok {
 			return nil, fmt.Errorf("MakeOnlineAccountsIter: cannot convert Queryable to sql.Tx, q is %T", q)
 		}
 		// create a new table by selecting from the original table
 		destTable := table + "_iterator"
 		_, err := e.ExecContext(ctx, fmt.Sprintf("DROP TABLE IF EXISTS %s", destTable))
 		if err != nil {
 			return nil, err
 		}
 		_, err = e.ExecContext(ctx, fmt.Sprintf("CREATE TABLE %s AS SELECT * FROM %s", destTable, table))
 		if err != nil {
 			return nil, err
 		}
 		// call prune on the new copied table, using the same logic as OnlineAccountsDelete
 		aw := accountsV2Writer{e: e}
 		err = aw.onlineAccountsDelete(excludeBefore, destTable)
 		if err != nil {
 			return nil, err
 		}
 		// remember to drop the table when the iterator is closed
 		onClose = func() {
 			_, err = e.ExecContext(ctx, fmt.Sprintf("DROP TABLE %s", destTable))
 			if err != nil {
 				logging.Base().Errorf("Failed to drop table %s: %v", destTable, err)
 			}
 		}
 		// use the new table to create the iterator
 		table = destTable
 	}

 	rows, err := q.QueryContext(ctx, fmt.Sprintf("SELECT address, updround, normalizedonlinebalance, votelastvalid, data FROM %s ORDER BY address, updround", table))
@@ -125,11 +139,11 @@
 		return nil, err
 	}

 	return &tableIterator[*encoded.OnlineAccountRecordV6]{
 		rows:    rows,
 		scan:    scanOnlineAccount,
 		onClose: onClose,
 	}, nil
 }

 func scanOnlineAccount(rows *sql.Rows) (*encoded.OnlineAccountRecordV6, error) {
@@ -181,18 +195,18 @@
 }

 // MakeOnlineRoundParamsIter creates an onlineRoundParams iterator.
 func MakeOnlineRoundParamsIter(ctx context.Context, q db.Queryable, useStaging bool, excludeBefore basics.Round) (trackerdb.TableIterator[*encoded.OnlineRoundParamsRecordV6], error) {
 	table := "onlineroundparamstail"
 	if useStaging {
 		table = "catchpointonlineroundparamstail"
 	}

 	where := ""
 	if excludeBefore != 0 {
 		where = fmt.Sprintf("WHERE rnd >= %d", excludeBefore)
 	}

 	rows, err := q.QueryContext(ctx, fmt.Sprintf("SELECT rnd, data FROM %s %s ORDER BY rnd", table, where))
 	if err != nil {
 		return nil, err
 	}