Summary
The crash-safe idempotency store added in #1096 has its reset tied to --rescan. Those two things are needed for unrelated reasons and at very different frequencies, so any deployment that keeps --rescan on for its Stellar-side purpose silently loses the crash-safety the store exists to provide.
Suggesting we decouple them.
What --rescan does today
pkg/bridge/bridge.go does three things behind the one flag:
blockPersistency.SaveStellarCursor("0") // re-read Stellar history from the start
blockPersistency.SaveHeight(0) // re-read TFChain history from the start
...
if cfg.RescanBridgeAccount {
idempotency.Reset() // wipe withdraw/refund processing state
}
Its documented purpose is the first of those — the help text is "if true is provided, we rescan the bridge stellar account and mint all transactions again".
Why the coupling is wrong
The two key spaces in the store have different provenance:
|
key |
source |
can it collide after a reset? |
| withdraw |
withdrawReady.ID (uint64) |
TFChain burn tx id — see subClient.GetBurnTransaction(types.U64(txID)) |
Yes, but only if TFChain is reset — ids restart from low numbers |
| refund |
RefundTransactionHash |
Stellar deposit tx hash — matched via FindRefundByReturnHashInPage on a MemoReturn |
No — content-derived, never repeats |
So the store only ever needs clearing when TFChain is reset, which is the reason given in the code comment. Nothing about a Stellar reset can cause a key collision: refund keys are hashes, and withdraw keys aren't Stellar-derived at all.
But --rescan is also the only tool for the Stellar-side need — recovering a stale Stellar cursor and re-reading deposits — and that need arises far more often than a TFChain reset. Which means an operator who sets --rescan for a Stellar reason unavoidably discards TFChain-scoped state they had no reason to touch.
This is already happening
The devnet bridges run rescan: true (chart value). The idempotency store is therefore wiped on every start, so:
The flag is presumably on for cursor-recovery reasons rather than because anyone wants the store cleared. Turning it off would fix the store but changes deposit-scanning behaviour — which is the coupling this issue is about, not a real choice between the two.
Suggested improvement
Preferred: scope withdraw keys by TFChain genesis hash. Key withdraws as (genesis_hash, txID) instead of txID. A chain reset changes the genesis, old keys become unreachable, and collisions are impossible by construction. No operator action, no flag to remember, and the store can then safely persist across every --rescan.
Refund keys need no change — Stellar tx hashes are already collision-free.
The reset call can then be dropped from the --rescan path entirely, leaving that flag meaning only what its help text says.
Alternative if genesis scoping is more than we want: split the behaviour out into its own flag, e.g. --reset-idempotency, used only when TFChain is actually reset. Less good, because it keeps a footgun that has to be documented and remembered, but it decouples the two.
Note on growth
Keys are deliberately never pruned — the comment reasons ~50 bytes per entry, so ~18 MB/year at 1000 tx/day, omitted "for simplicity and auditability". Genesis scoping would leave entries from a superseded chain as dead weight, which at those volumes is immaterial; worth mentioning only so it's a conscious choice rather than a surprise.
Not urgent
Nothing is broken in production by this. It is an improvement to make the guarantee hold regardless of how --rescan is used, and to stop devnet from silently running without the protection it is supposed to be validating.
Summary
The crash-safe idempotency store added in #1096 has its reset tied to
--rescan. Those two things are needed for unrelated reasons and at very different frequencies, so any deployment that keeps--rescanon for its Stellar-side purpose silently loses the crash-safety the store exists to provide.Suggesting we decouple them.
What
--rescandoes todaypkg/bridge/bridge.godoes three things behind the one flag:Its documented purpose is the first of those — the help text is "if true is provided, we rescan the bridge stellar account and mint all transactions again".
Why the coupling is wrong
The two key spaces in the store have different provenance:
withdrawReady.ID(uint64)subClient.GetBurnTransaction(types.U64(txID))RefundTransactionHashFindRefundByReturnHashInPageon a MemoReturnSo the store only ever needs clearing when TFChain is reset, which is the reason given in the code comment. Nothing about a Stellar reset can cause a key collision: refund keys are hashes, and withdraw keys aren't Stellar-derived at all.
But
--rescanis also the only tool for the Stellar-side need — recovering a stale Stellar cursor and re-reading deposits — and that need arises far more often than a TFChain reset. Which means an operator who sets--rescanfor a Stellar reason unavoidably discards TFChain-scoped state they had no reason to touch.This is already happening
The devnet bridges run
rescan: true(chart value). The idempotency store is therefore wiped on every start, so:The flag is presumably on for cursor-recovery reasons rather than because anyone wants the store cleared. Turning it off would fix the store but changes deposit-scanning behaviour — which is the coupling this issue is about, not a real choice between the two.
Suggested improvement
Preferred: scope withdraw keys by TFChain genesis hash. Key withdraws as
(genesis_hash, txID)instead oftxID. A chain reset changes the genesis, old keys become unreachable, and collisions are impossible by construction. No operator action, no flag to remember, and the store can then safely persist across every--rescan.Refund keys need no change — Stellar tx hashes are already collision-free.
The reset call can then be dropped from the
--rescanpath entirely, leaving that flag meaning only what its help text says.Alternative if genesis scoping is more than we want: split the behaviour out into its own flag, e.g.
--reset-idempotency, used only when TFChain is actually reset. Less good, because it keeps a footgun that has to be documented and remembered, but it decouples the two.Note on growth
Keys are deliberately never pruned — the comment reasons ~50 bytes per entry, so ~18 MB/year at 1000 tx/day, omitted "for simplicity and auditability". Genesis scoping would leave entries from a superseded chain as dead weight, which at those volumes is immaterial; worth mentioning only so it's a conscious choice rather than a surprise.
Not urgent
Nothing is broken in production by this. It is an improvement to make the guarantee hold regardless of how
--rescanis used, and to stop devnet from silently running without the protection it is supposed to be validating.