From 96dcaa1e247385b69cef5ca26483f6f707dfd98d Mon Sep 17 00:00:00 2001 From: jiahuihu Date: Tue, 18 Aug 2026 12:14:53 -0400 Subject: [PATCH 1/2] fix(indexer): keep same-ledger REMOVE so deletes are never lost --- internal/indexer/indexer.go | 8 +- internal/indexer/indexer_buffer.go | 143 +++++++----------------- internal/indexer/indexer_buffer_test.go | 105 +++++++++++------ internal/indexer/indexer_test.go | 21 ++-- 4 files changed, 131 insertions(+), 146 deletions(-) diff --git a/internal/indexer/indexer.go b/internal/indexer/indexer.go index c6e382bc1..e4d8d3db3 100644 --- a/internal/indexer/indexer.go +++ b/internal/indexer/indexer.go @@ -280,10 +280,10 @@ func (i *Indexer) processTransaction(ctx context.Context, tx ingest.LedgerTransa } // Process trustline, account, SAC balance, and liquidity-pool changes from ledger changes, - // walking operations in ascending opID (chronological) order. pushWithTombstone's - // create+remove netting at the fold requires each change family to arrive in ascending - // order value per key — CREATE before REMOVE — and ranging over the opsParticipants map - // would emit them in random order (#653). + // walking operations in ascending opID (chronological) order so each change family is emitted + // deterministically in chronological order. The fold (pushHighestOrder) keeps the highest-order + // change per key and no longer depends on arrival order for correctness, but ranging over the + // opsParticipants map directly would still produce nondeterministic slice ordering (#653). sortedOpIDs := make([]int64, 0, len(opsParticipants)) for opID := range opsParticipants { sortedOpIDs = append(sortedOpIDs, opID) diff --git a/internal/indexer/indexer_buffer.go b/internal/indexer/indexer_buffer.go index 760b24f3f..2ed091aec 100644 --- a/internal/indexer/indexer_buffer.go +++ b/internal/indexer/indexer_buffer.go @@ -77,15 +77,7 @@ type IndexerBuffer struct { sacBalanceChangesByKey map[SACBalanceChangeKey]types.SACBalanceChange lpShareChangesByKey map[LiquidityPoolShareChangeKey]types.LiquidityPoolShareChange lpChangesByPoolID map[string]types.LiquidityPoolChange - // Tombstones record the order value at which a create/add was cancelled by a same-ledger - // remove. They keep the highest-order-wins invariant intact across the delete, so a later - // lower-order change cannot resurrect a removed key. See pushWithTombstone. - accountTombstones map[string]int64 - trustlineTombstones map[TrustlineChangeKey]int64 - sacTombstones map[SACBalanceChangeKey]int64 - lpShareTombstones map[LiquidityPoolShareChangeKey]int64 - lpTombstones map[string]int64 - uniqueTrustlineAssets map[uuid.UUID]data.TrustlineAsset + uniqueTrustlineAssets map[uuid.UUID]data.TrustlineAsset // parsedAssetsByString memoizes the parse + deterministic-ID derivation per unique asset // string (nil value = string is known-invalid). It is content-derived — the same string always // yields the same result — so it is never cleared in Clear(). Both ingestion paths reuse one @@ -113,11 +105,6 @@ func NewIndexerBuffer() *IndexerBuffer { sacBalanceChangesByKey: make(map[SACBalanceChangeKey]types.SACBalanceChange), lpShareChangesByKey: make(map[LiquidityPoolShareChangeKey]types.LiquidityPoolShareChange), lpChangesByPoolID: make(map[string]types.LiquidityPoolChange), - accountTombstones: make(map[string]int64), - trustlineTombstones: make(map[TrustlineChangeKey]int64), - sacTombstones: make(map[SACBalanceChangeKey]int64), - lpShareTombstones: make(map[LiquidityPoolShareChangeKey]int64), - lpTombstones: make(map[string]int64), uniqueTrustlineAssets: make(map[uuid.UUID]data.TrustlineAsset), parsedAssetsByString: make(map[string]*data.TrustlineAsset), sacContractsByID: make(map[string]*data.Contract), @@ -185,74 +172,39 @@ func (b *IndexerBuffer) GetTransactionsParticipants() map[int64]map[string]struc return b.participantsByToID } -// pushWithTombstone deduplicates change into m, keeping the highest-ordered change per key. +// pushHighestOrder stores change into m under key, keeping whichever change for that key has the +// highest order value (operation ID, or SortKey for accounts). The highest-order change is the key's +// final state within the ledger, so it is exactly what must be persisted: // -// A create/add that is later removed within the same ledger nets to nothing: the key is deleted -// and a tombstone is recorded at the remove's order value. The tombstone drops any subsequent -// change whose order is <= it (a chronologically-earlier change can no longer resurrect the key), -// while a strictly-higher order — a genuine later re-create/re-add of the same key — lifts the -// tombstone and wins. This keeps the highest-order-wins invariant intact across the delete; a bare -// delete would break it, since the key would look absent and a lower-order change would re-insert a -// stale phantom. -func pushWithTombstone[K comparable, V any]( - m map[K]V, - tombstones map[K]int64, - key K, - change V, - order func(V) int64, - isNoopRemove func(existing, incoming V) bool, -) { - if tomb, ok := tombstones[key]; ok { - if order(change) <= tomb { - return - } - delete(tombstones, key) - } - - existing, exists := m[key] - if exists && order(existing) > order(change) { - return - } - - if exists && isNoopRemove(existing, change) { - delete(m, key) - tombstones[key] = order(change) +// - ends on an add/update -> upsert the entry +// - ends on a remove -> delete the entry +// +// A trailing remove therefore survives as a delete instead of being netted away against an earlier +// same-ledger add. That netting was unsafe: the buffer cannot see whether a row for this key was +// written by an earlier ledger, so cancelling an add+remove to "no write" strands any such row +// (see the create-then-remove-then-recreate case). Persisting the delete is always safe — it is a +// harmless no-op when no row exists and the correct cleanup when one does. +// +// Because the highest order always wins, a lower-order change can never displace or resurrect a +// higher-order one regardless of the order in which changes are pushed, so no tombstone bookkeeping +// is needed to guard against out-of-order or replayed changes. +func pushHighestOrder[K comparable, V any](m map[K]V, key K, change V, order func(V) int64) { + if existing, exists := m[key]; exists && order(existing) > order(change) { return } - m[key] = change } func accountOrder(c types.AccountChange) int64 { return c.SortKey } -func accountIsNoopRemove(existing, incoming types.AccountChange) bool { - return existing.Operation == types.AccountOpCreate && incoming.Operation == types.AccountOpRemove -} - func trustlineOrder(c types.TrustlineChange) int64 { return c.OperationID } -func trustlineIsNoopRemove(existing, incoming types.TrustlineChange) bool { - return existing.Operation == types.TrustlineOpAdd && incoming.Operation == types.TrustlineOpRemove -} - func sacBalanceOrder(c types.SACBalanceChange) int64 { return c.OperationID } -func sacBalanceIsNoopRemove(existing, incoming types.SACBalanceChange) bool { - return existing.Operation == types.SACBalanceOpAdd && incoming.Operation == types.SACBalanceOpRemove -} - func lpShareOrder(c types.LiquidityPoolShareChange) int64 { return c.OperationID } -func lpShareIsNoopRemove(existing, incoming types.LiquidityPoolShareChange) bool { - return existing.Operation == types.LiquidityPoolShareOpAdd && incoming.Operation == types.LiquidityPoolShareOpRemove -} - func lpOrder(c types.LiquidityPoolChange) int64 { return c.OperationID } -func lpIsNoopRemove(existing, incoming types.LiquidityPoolChange) bool { - return existing.Operation == types.LiquidityPoolOpAdd && incoming.Operation == types.LiquidityPoolOpRemove -} - // PushTrustlineChange adds a trustline change to the buffer and tracks unique assets. // The parse + deterministic-ID derivation is memoized per asset string (see parsedAssetsByString), // so a repeated asset — valid or invalid — skips re-parsing and re-validation. @@ -284,7 +236,7 @@ func (b *IndexerBuffer) PushTrustlineChange(trustlineChange types.TrustlineChang AccountID: trustlineChange.AccountID, TrustlineID: asset.ID, } - pushWithTombstone(b.trustlineChangesByTrustlineKey, b.trustlineTombstones, changeKey, trustlineChange, trustlineOrder, trustlineIsNoopRemove) + pushHighestOrder(b.trustlineChangesByTrustlineKey, changeKey, trustlineChange, trustlineOrder) } // GetTrustlineChanges returns the buffer's internal map of trustline changes; @@ -294,11 +246,10 @@ func (b *IndexerBuffer) GetTrustlineChanges() map[TrustlineChangeKey]types.Trust } // PushAccountChange adds an account change to the buffer with deduplication. -// Keeps the change with highest SortKey per account. A CREATE→REMOVE within the same ledger nets -// to nothing and is tombstoned so a later lower-key change cannot resurrect it (see -// pushWithTombstone). +// Keeps the change with the highest SortKey per account: a trailing REMOVE persists as a delete +// rather than being netted against an earlier same-ledger CREATE (see pushHighestOrder). func (b *IndexerBuffer) PushAccountChange(accountChange types.AccountChange) { - pushWithTombstone(b.accountChangesByAccountID, b.accountTombstones, accountChange.AccountID, accountChange, accountOrder, accountIsNoopRemove) + pushHighestOrder(b.accountChangesByAccountID, accountChange.AccountID, accountChange, accountOrder) } // GetAccountChanges returns the buffer's internal map of account changes; @@ -308,15 +259,15 @@ func (b *IndexerBuffer) GetAccountChanges() map[string]types.AccountChange { } // PushSACBalanceChange adds a SAC balance change to the buffer with deduplication. -// Keeps the change with highest OperationID per (AccountID, ContractID). An ADD→REMOVE within the -// same ledger nets to nothing and is tombstoned so a later lower-key change cannot resurrect it -// (see pushWithTombstone). +// Keeps the change with the highest OperationID per (AccountID, ContractID): a trailing REMOVE +// persists as a delete rather than being netted against an earlier same-ledger ADD (see +// pushHighestOrder). func (b *IndexerBuffer) PushSACBalanceChange(sacBalanceChange types.SACBalanceChange) { key := SACBalanceChangeKey{ AccountID: sacBalanceChange.AccountID, ContractID: sacBalanceChange.ContractID, } - pushWithTombstone(b.sacBalanceChangesByKey, b.sacTombstones, key, sacBalanceChange, sacBalanceOrder, sacBalanceIsNoopRemove) + pushHighestOrder(b.sacBalanceChangesByKey, key, sacBalanceChange, sacBalanceOrder) } // GetSACBalanceChanges returns the buffer's internal map of SAC balance @@ -326,15 +277,14 @@ func (b *IndexerBuffer) GetSACBalanceChanges() map[SACBalanceChangeKey]types.SAC } // PushLiquidityPoolShareChange adds a pool-share balance change to the buffer with deduplication. -// Keeps the change with highest OperationID per (AccountID, PoolID). An ADD→REMOVE within the same -// ledger nets to nothing and is tombstoned so a later lower-key change cannot resurrect it (see -// pushWithTombstone). +// Keeps the change with the highest OperationID per (AccountID, PoolID): a trailing REMOVE persists +// as a delete rather than being netted against an earlier same-ledger ADD (see pushHighestOrder). func (b *IndexerBuffer) PushLiquidityPoolShareChange(change types.LiquidityPoolShareChange) { key := LiquidityPoolShareChangeKey{ AccountID: change.AccountID, PoolID: change.PoolID, } - pushWithTombstone(b.lpShareChangesByKey, b.lpShareTombstones, key, change, lpShareOrder, lpShareIsNoopRemove) + pushHighestOrder(b.lpShareChangesByKey, key, change, lpShareOrder) } // GetLiquidityPoolShareChanges returns the buffer's internal map of @@ -344,11 +294,10 @@ func (b *IndexerBuffer) GetLiquidityPoolShareChanges() map[LiquidityPoolShareCha } // PushLiquidityPoolChange adds a pool reserve change to the buffer with deduplication. -// Keeps the change with highest OperationID per PoolID. An ADD→REMOVE within the same ledger nets -// to nothing and is tombstoned so a later lower-key change cannot resurrect it (see -// pushWithTombstone). +// Keeps the change with the highest OperationID per PoolID: a trailing REMOVE persists as a delete +// rather than being netted against an earlier same-ledger ADD (see pushHighestOrder). func (b *IndexerBuffer) PushLiquidityPoolChange(change types.LiquidityPoolChange) { - pushWithTombstone(b.lpChangesByPoolID, b.lpTombstones, change.PoolID, change, lpOrder, lpIsNoopRemove) + pushHighestOrder(b.lpChangesByPoolID, change.PoolID, change, lpOrder) } // GetLiquidityPoolChanges returns the buffer's internal map of pool reserve @@ -423,15 +372,12 @@ func (b *IndexerBuffer) GetStateChanges() []types.StateChange { // StateChanges (state-change → operation association). StateChanges is already filtered by the // worker: entries with an empty AccountID or an OperationID with no matching operation are dropped. // -// Netting at the fold (pushWithTombstone) requires only that a key's create/add precedes the remove -// that cancels it — not that a change-family slice is globally sorted by order value. -// processTransaction walks operations in ascending opID order, which gives that for every family -// (TrustlineChanges, AccountChanges, SACBalanceChanges, LPShareChanges, LPChanges). AccountChanges is -// the one slice that is not globally ascending: the fee-phase changes are appended after the operation -// walk even though phaseFee sorts below every operation (see processors.accountSortKey). That is -// harmless because a fee debit or Soroban refund always updates an account entry that already exists -// — it never creates or removes one — so those changes pair with nothing to net, and the -// highest-order-wins guard discards them whenever an operation already wrote a higher key. +// The fold (pushHighestOrder) keeps the highest-order change per key, so it is independent of the +// order in which changes reach it: a change-family slice need not be globally sorted by order value. +// AccountChanges in particular is not globally ascending — the fee-phase changes are appended after +// the operation walk even though phaseFee sorts below every operation (see processors.accountSortKey) +// — and that is harmless, because the highest-order-wins guard discards a fee/refund update whenever +// an operation already wrote a higher key for the same account. type TransactionResult struct { Transaction *types.Transaction TxParticipants []string @@ -516,9 +462,9 @@ func (b *IndexerBuffer) IngestTransactionResult(r *TransactionResult) { // ingestion paths reuse a single buffer and clear it around each unit of work: live before every // ledger, backfill after every flushed batch. // -// Clearing the balance-change maps and their tombstones is load-bearing for the live path, the only -// one that persists native balances: processors.accountSortKey deliberately omits the ledger from its -// key, so changes from two different ledgers must never coexist in those maps. +// Clearing the balance-change maps is load-bearing for the live path, the only one that persists +// native balances: processors.accountSortKey deliberately omits the ledger from its key, so changes +// from two different ledgers must never coexist in those maps. func (b *IndexerBuffer) Clear() { // Clear maps (keep allocated backing arrays) clear(b.txByHash) @@ -543,13 +489,6 @@ func (b *IndexerBuffer) Clear() { clear(b.sacBalanceChangesByKey) clear(b.lpShareChangesByKey) clear(b.lpChangesByPoolID) - - // Clear tombstones - clear(b.accountTombstones) - clear(b.trustlineTombstones) - clear(b.sacTombstones) - clear(b.lpShareTombstones) - clear(b.lpTombstones) } // GetUniqueTrustlineAssets returns all unique trustline assets with pre-computed IDs. diff --git a/internal/indexer/indexer_buffer_test.go b/internal/indexer/indexer_buffer_test.go index e63e9f7bc..5e14b054d 100644 --- a/internal/indexer/indexer_buffer_test.go +++ b/internal/indexer/indexer_buffer_test.go @@ -281,7 +281,7 @@ func TestIndexerBuffer_IngestTransactionResult(t *testing.T) { assert.Len(t, buffer.GetLiquidityPoolChanges(), 1) }) - t.Run("🟢 LP ADD→REMOVE across folded results nets to nothing (tombstone)", func(t *testing.T) { + t.Run("🟢 LP ADD→REMOVE across folded results keeps the REMOVE", func(t *testing.T) { buffer := NewIndexerBuffer() tx := types.Transaction{Hash: "e76b7b0133690fbfb2de8fa9ca2273cb4f2e29447e0cf0e14a5f82d0daa48760", ToID: 1} @@ -299,16 +299,23 @@ func TestIndexerBuffer_IngestTransactionResult(t *testing.T) { buffer.IngestTransactionResult(add) buffer.IngestTransactionResult(remove) - assert.Len(t, buffer.GetLiquidityPoolShareChanges(), 0) - assert.Len(t, buffer.GetLiquidityPoolChanges(), 0) + // The trailing REMOVE (highest OperationID) survives as a delete rather than netting to + // nothing, so a pool-share/pool row persisted by an earlier ledger is cleaned up. + shareChanges := buffer.GetLiquidityPoolShareChanges() + require.Len(t, shareChanges, 1) + assert.Equal(t, types.LiquidityPoolShareOpRemove, shareChanges[LiquidityPoolShareChangeKey{AccountID: "alice", PoolID: "pool1"}].Operation) + poolChanges := buffer.GetLiquidityPoolChanges() + require.Len(t, poolChanges, 1) + assert.Equal(t, types.LiquidityPoolOpRemove, poolChanges["pool1"].Operation) }) - t.Run("🟢 dedups across multiple folded results (CREATE→REMOVE tombstone)", func(t *testing.T) { + t.Run("🟢 dedups across multiple folded results (CREATE→REMOVE keeps the REMOVE)", func(t *testing.T) { buffer := NewIndexerBuffer() tx := types.Transaction{Hash: "e76b7b0133690fbfb2de8fa9ca2273cb4f2e29447e0cf0e14a5f82d0daa48760", ToID: 1} // One result creates the account, a later result removes it within the same ledger. Folded - // through the same buffer, the CREATE→REMOVE nets to nothing (see pushWithTombstone). + // through the same buffer, the highest-order change (the REMOVE) wins and persists as a + // delete (see pushHighestOrder). create := &TransactionResult{ Transaction: &tx, AccountChanges: []types.AccountChange{{AccountID: "GACCT", SortKey: 1, Operation: types.AccountOpCreate, Balance: 100}}, @@ -321,7 +328,9 @@ func TestIndexerBuffer_IngestTransactionResult(t *testing.T) { buffer.IngestTransactionResult(create) buffer.IngestTransactionResult(remove) - assert.Len(t, buffer.GetAccountChanges(), 0) + changes := buffer.GetAccountChanges() + require.Len(t, changes, 1) + assert.Equal(t, types.AccountOpRemove, changes["GACCT"].Operation) }) } @@ -386,7 +395,7 @@ func TestIndexerBuffer_PushSACBalanceChange(t *testing.T) { assert.Equal(t, int64(200), changes[key].OperationID) }) - t.Run("🟢 handles ADD→REMOVE no-op case", func(t *testing.T) { + t.Run("🟢 ADD→REMOVE keeps the REMOVE", func(t *testing.T) { buffer := NewIndexerBuffer() accountID := "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC" @@ -410,9 +419,11 @@ func TestIndexerBuffer_PushSACBalanceChange(t *testing.T) { buffer.PushSACBalanceChange(addChange) buffer.PushSACBalanceChange(removeChange) - // ADD→REMOVE in same batch is a no-op - entry should be removed + // ADD→REMOVE in the same batch keeps the trailing REMOVE as a delete, so a row persisted by + // an earlier ledger is cleaned up rather than left stale. changes := buffer.GetSACBalanceChanges() - assert.Len(t, changes, 0) + require.Len(t, changes, 1) + assert.Equal(t, types.SACBalanceOpRemove, changes[SACBalanceChangeKey{AccountID: accountID, ContractID: contractID}].Operation) }) t.Run("🟢 UPDATE→REMOVE is NOT a no-op", func(t *testing.T) { @@ -478,25 +489,27 @@ func TestIndexerBuffer_PushSACBalanceChange(t *testing.T) { assert.Equal(t, "300", result[key3].Balance) }) - t.Run("🟢 tombstone blocks lower-key resurrection after ADD→REMOVE", func(t *testing.T) { + t.Run("🟢 ADD→REMOVE keeps the REMOVE and a lower-key change cannot resurrect it", func(t *testing.T) { buffer := NewIndexerBuffer() accountID := "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC" contractID := "CCWAMYJME4H5CKG7OLXGC2T4M6FL52XCZ3OQOAV6LL3GLA4RO4WH3ASP" buffer.PushSACBalanceChange(types.SACBalanceChange{AccountID: accountID, ContractID: contractID, Balance: "100", Operation: types.SACBalanceOpAdd, OperationID: 100}) buffer.PushSACBalanceChange(types.SACBalanceChange{AccountID: accountID, ContractID: contractID, Balance: "0", Operation: types.SACBalanceOpRemove, OperationID: 200}) - // A lower-OperationID change afterward must NOT re-insert the removed balance. + // A lower-OperationID change afterward must NOT displace the REMOVE. buffer.PushSACBalanceChange(types.SACBalanceChange{AccountID: accountID, ContractID: contractID, Balance: "50", Operation: types.SACBalanceOpUpdate, OperationID: 50}) - assert.Len(t, buffer.GetSACBalanceChanges(), 0) + changes := buffer.GetSACBalanceChanges() + require.Len(t, changes, 1) + assert.Equal(t, types.SACBalanceOpRemove, changes[SACBalanceChangeKey{AccountID: accountID, ContractID: contractID}].Operation) }) - t.Run("🟢 higher-key change re-adds a tombstoned SAC balance", func(t *testing.T) { + t.Run("🟢 higher-key change re-adds a removed SAC balance", func(t *testing.T) { buffer := NewIndexerBuffer() accountID := "CDLZFC3SYJYDZT7K67VZ75HPJVIEUVNIXF47ZG2FB2RMQQVU2HHGCYSC" contractID := "CCWAMYJME4H5CKG7OLXGC2T4M6FL52XCZ3OQOAV6LL3GLA4RO4WH3ASP" buffer.PushSACBalanceChange(types.SACBalanceChange{AccountID: accountID, ContractID: contractID, Balance: "100", Operation: types.SACBalanceOpAdd, OperationID: 100}) buffer.PushSACBalanceChange(types.SACBalanceChange{AccountID: accountID, ContractID: contractID, Balance: "0", Operation: types.SACBalanceOpRemove, OperationID: 200}) - // A genuine later re-add (higher OperationID) lifts the tombstone and wins. + // A genuine later re-add (higher OperationID) wins over the REMOVE. buffer.PushSACBalanceChange(types.SACBalanceChange{AccountID: accountID, ContractID: contractID, Balance: "700", Operation: types.SACBalanceOpAdd, OperationID: 300}) changes := buffer.GetSACBalanceChanges() @@ -619,12 +632,15 @@ func TestIndexerBuffer_PushAccountChange(t *testing.T) { assert.Equal(t, int64(200), changes[accountChangeAddr].Balance) }) - t.Run("🟢 handles CREATE→REMOVE no-op case", func(t *testing.T) { + t.Run("🟢 CREATE→REMOVE keeps the REMOVE", func(t *testing.T) { buffer := NewIndexerBuffer() buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 1), 100, types.AccountOpCreate)) buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 2), 0, types.AccountOpRemove)) - assert.Len(t, buffer.GetAccountChanges(), 0) + // The trailing REMOVE (highest key) survives as a delete rather than netting to nothing. + changes := buffer.GetAccountChanges() + require.Len(t, changes, 1) + assert.Equal(t, types.AccountOpRemove, changes[accountChangeAddr].Operation) }) t.Run("🟢 UPDATE→REMOVE is NOT a no-op", func(t *testing.T) { @@ -723,22 +739,24 @@ func TestIndexerBuffer_PushAccountChange(t *testing.T) { assert.Equal(t, int64(300), changes[accountChangeAddr].Balance) }) - t.Run("🟢 tombstone blocks lower-key resurrection after CREATE→REMOVE", func(t *testing.T) { + t.Run("🟢 CREATE→REMOVE keeps the REMOVE and a lower-key change cannot resurrect it", func(t *testing.T) { buffer := NewIndexerBuffer() - // Account created then merged within the ledger's operations → nets to nothing. + // Account created then merged within the ledger's operations → the REMOVE (highest key) wins. buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 1), 100, types.AccountOpCreate)) buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 2), 0, types.AccountOpRemove)) - // A lower-key change arriving afterward must NOT re-insert the removed account. + // A lower-key change arriving afterward must NOT displace the REMOVE. buffer.PushAccountChange(accountChange(accountRank(rankFee, 1, 0), 999, types.AccountOpUpdate)) - assert.Len(t, buffer.GetAccountChanges(), 0) + changes := buffer.GetAccountChanges() + require.Len(t, changes, 1) + assert.Equal(t, types.AccountOpRemove, changes[accountChangeAddr].Operation) }) - t.Run("🟢 higher-key change re-creates a tombstoned account", func(t *testing.T) { + t.Run("🟢 higher-key change re-creates a removed account", func(t *testing.T) { buffer := NewIndexerBuffer() buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 1), 100, types.AccountOpCreate)) buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 2), 0, types.AccountOpRemove)) - // A genuine later re-creation (higher key) lifts the tombstone and wins. + // A genuine later re-creation (higher key) wins over the REMOVE. buffer.PushAccountChange(accountChange(accountRank(rankOp, 5, 1), 700, types.AccountOpCreate)) changes := buffer.GetAccountChanges() @@ -747,19 +765,19 @@ func TestIndexerBuffer_PushAccountChange(t *testing.T) { assert.Equal(t, types.AccountOpCreate, changes[accountChangeAddr].Operation) }) - t.Run("🟢 Clear drops tombstones so the next ledger's lower-key change is not suppressed", func(t *testing.T) { + t.Run("🟢 Clear resets change maps so the next ledger's lower-key change is not suppressed", func(t *testing.T) { buffer := NewIndexerBuffer() - // Ledger N: created then merged within the ledger's operations → nets to nothing, leaving a - // tombstone at the remove's key. + // Ledger N: created then merged within the ledger's operations → the REMOVE (highest key) wins + // and is retained as a delete. buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 1), 100, types.AccountOpCreate)) buffer.PushAccountChange(accountChange(accountRank(rankOp, 1, 2), 0, types.AccountOpRemove)) - require.Empty(t, buffer.GetAccountChanges()) + require.Len(t, buffer.GetAccountChanges(), 1) buffer.Clear() // Ledger N+1 through the same reused buffer (both ingestion paths do this — see Clear). Sort // keys carry no ledger term, so this fee change ranks BELOW the previous ledger's remove; it - // must still land. A tombstone surviving Clear would silently drop a real balance. + // must still land. A change-map entry surviving Clear would silently drop a real balance. buffer.PushAccountChange(accountChange(accountRank(rankFee, 1, 0), 999, types.AccountOpUpdate)) changes := buffer.GetAccountChanges() @@ -768,7 +786,7 @@ func TestIndexerBuffer_PushAccountChange(t *testing.T) { }) } -func TestIndexerBuffer_TrustlineTombstone(t *testing.T) { +func TestIndexerBuffer_TrustlineHighestOrderWins(t *testing.T) { const asset = "USDC:GA5ZSEJYB37JRC5AVCIA5MOP4RHTM335X2KGX3IHOJAPP5RE34K4KZVN" trustline := func(opID, balance int64, op types.TrustlineOpType) types.TrustlineChange { return types.TrustlineChange{ @@ -780,21 +798,42 @@ func TestIndexerBuffer_TrustlineTombstone(t *testing.T) { } } - t.Run("🟢 tombstone blocks lower-key resurrection after ADD→REMOVE", func(t *testing.T) { + t.Run("🟢 ADD→REMOVE keeps the REMOVE and a lower-key change cannot resurrect it", func(t *testing.T) { buffer := NewIndexerBuffer() buffer.PushTrustlineChange(trustline(100, 1000, types.TrustlineOpAdd)) buffer.PushTrustlineChange(trustline(200, 0, types.TrustlineOpRemove)) - // A lower-OperationID change afterward must NOT re-insert the removed trustline. + // A lower-OperationID change afterward must NOT displace the REMOVE. buffer.PushTrustlineChange(trustline(50, 500, types.TrustlineOpUpdate)) - assert.Len(t, buffer.GetTrustlineChanges(), 0) + changes := buffer.GetTrustlineChanges() + require.Len(t, changes, 1) + for _, c := range changes { + assert.Equal(t, types.TrustlineOpRemove, c.Operation) + } + }) + + // Regression for the same-ledger REMOVE→ADD→REMOVE lost-delete bug: a trustline that existed + // before this ledger is removed, re-created, and removed again. The final REMOVE (highest + // OperationID) must survive so the pre-existing row is deleted rather than left stale. + t.Run("🟢 REMOVE→ADD→REMOVE keeps the final REMOVE", func(t *testing.T) { + buffer := NewIndexerBuffer() + buffer.PushTrustlineChange(trustline(100, 0, types.TrustlineOpRemove)) + buffer.PushTrustlineChange(trustline(200, 0, types.TrustlineOpAdd)) + buffer.PushTrustlineChange(trustline(300, 0, types.TrustlineOpRemove)) + + changes := buffer.GetTrustlineChanges() + require.Len(t, changes, 1) + for _, c := range changes { + assert.Equal(t, types.TrustlineOpRemove, c.Operation) + assert.Equal(t, int64(300), c.OperationID) + } }) - t.Run("🟢 higher-key change re-adds a tombstoned trustline", func(t *testing.T) { + t.Run("🟢 higher-key change re-adds a removed trustline", func(t *testing.T) { buffer := NewIndexerBuffer() buffer.PushTrustlineChange(trustline(100, 1000, types.TrustlineOpAdd)) buffer.PushTrustlineChange(trustline(200, 0, types.TrustlineOpRemove)) - // A genuine later re-add (higher OperationID) lifts the tombstone and wins. + // A genuine later re-add (higher OperationID) wins over the REMOVE. buffer.PushTrustlineChange(trustline(300, 700, types.TrustlineOpAdd)) changes := buffer.GetTrustlineChanges() diff --git a/internal/indexer/indexer_test.go b/internal/indexer/indexer_test.go index 8708db907..9b22d3a12 100644 --- a/internal/indexer/indexer_test.go +++ b/internal/indexer/indexer_test.go @@ -1512,10 +1512,11 @@ func TestIndexer_ProcessLedgerTransactions_RealLedgerParallel(t *testing.T) { } // TestIndexer_ProcessTransaction_EmitsChangesInOpOrder is the regression test for issue #653: -// pushWithTombstone's create+remove netting requires each change family to reach the fold in -// ascending order-value per key (CREATE before REMOVE), so processTransaction must walk -// operations in ascending opID order. Under map-order iteration this test fails with -// overwhelming probability (12 ops → 1/12! chance of accidentally sorted output). +// processTransaction must walk operations in ascending opID order so each change family reaches the +// fold in deterministic chronological order. Under map-order iteration this test fails with +// overwhelming probability (12 ops → 1/12! chance of accidentally sorted output). The fold itself +// (pushHighestOrder) no longer depends on arrival order for correctness — it keeps the highest-order +// change per key regardless — but the emitted slice ordering is still asserted here. func TestIndexer_ProcessTransaction_EmitsChangesInOpOrder(t *testing.T) { const numOps = 12 const asset = "USDC:GBWAH7AOBZYAYLT76Z7MQDDRRJCCERRVRSCJ4GAEGV2S5W474ZLEOH4U" @@ -1605,12 +1606,18 @@ func TestIndexer_ProcessTransaction_EmitsChangesInOpOrder(t *testing.T) { "trustline changes must be emitted in ascending opID order") } - // ...so the fold nets the same-tx ADD→REMOVE to nothing instead of keeping a spurious REMOVE. + // ...so the fold keeps the highest-order change per key: the shared-key ADD→REMOVE resolves to + // the trailing REMOVE (a delete), while the other keys keep their single UPDATE. buffer := NewIndexerBuffer() buffer.IngestTransactionResult(result) trustlines := buffer.GetTrustlineChanges() - assert.Len(t, trustlines, numOps-2, "shared-key ADD→REMOVE should net to nothing") + assert.Len(t, trustlines, numOps-1, "shared-key ADD→REMOVE should resolve to a single REMOVE") + var sharedRemoveSeen bool for _, change := range trustlines { - assert.NotEqual(t, "shared", change.AccountID, "spurious REMOVE for the netted key must not persist") + if change.AccountID == "shared" { + sharedRemoveSeen = true + assert.Equal(t, types.TrustlineOpRemove, change.Operation, "shared-key change must persist as a REMOVE") + } } + assert.True(t, sharedRemoveSeen, "shared-key REMOVE must persist as a delete") } From 60d16e97d1ac7b3cd039b79ef9f591ce2fc79ff4 Mon Sep 17 00:00:00 2001 From: Jiahui Hu Date: Tue, 18 Aug 2026 12:34:48 -0400 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- internal/indexer/indexer_buffer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/indexer/indexer_buffer.go b/internal/indexer/indexer_buffer.go index 2ed091aec..5652432bd 100644 --- a/internal/indexer/indexer_buffer.go +++ b/internal/indexer/indexer_buffer.go @@ -182,7 +182,7 @@ func (b *IndexerBuffer) GetTransactionsParticipants() map[int64]map[string]struc // A trailing remove therefore survives as a delete instead of being netted away against an earlier // same-ledger add. That netting was unsafe: the buffer cannot see whether a row for this key was // written by an earlier ledger, so cancelling an add+remove to "no write" strands any such row -// (see the create-then-remove-then-recreate case). Persisting the delete is always safe — it is a +// (see the remove-then-recreate-then-remove case). Persisting the delete is always safe — it is a // harmless no-op when no row exists and the correct cleanup when one does. // // Because the highest order always wins, a lower-order change can never displace or resurrect a