Skip to content

Rebuild the transaction list during a rescan, and stop mislabelling it expired - #73

Open
peachbits wants to merge 2 commits into
masterfrom
matthew/fix/rescan-expired-mislabel
Open

Rebuild the transaction list during a rescan, and stop mislabelling it expired#73
peachbits wants to merge 2 commits into
masterfrom
matthew/fix/rescan-expired-mislabel

Conversation

@peachbits

@peachbits peachbits commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

CHANGELOG

Does this branch warrant an entry to the CHANGELOG?

  • Yes
  • No

Dependencies

none

Description

Fixes half of ZEC - resync shows failed transactions and incorrect sync status — the half where a resync flashes every transaction in the wallet as Failed until the rescan finishes — and then makes a resync actually rebuild the transaction list, on both platforms.

Two commits:

  1. isExpired from the wallet's scan floor, not the network tip (Android) — the reported "Failed" bug.
  2. Report transactions as the rescan finds them, not all at once (both platforms) — a resync now empties the list and rebuilds it one transaction at a time, instead of the whole set reappearing immediately.

Root cause. parseTx reported isExpired from TransactionState.Expired. That state compares an unmined transaction's expiry height against the live network tip:

minedHeight == null && expiryHeight != 0 && expiryHeight <= networkTip  ->  Expired

A resync calls rewindToNearestHeight(birthday), which un-mines every stored transaction until the scan re-reaches its block. During that window the whole history is unmined with expiry heights far below the tip, so every transaction is reported expired. Downstream that becomes confirmations: 'failed' in edge-currency-accountbased and a red "Failed" row in the GUI, which clears itself as the rescan re-mines each transaction — matching QA's note that it resolves once the wallet finishes syncing.

iOS never had this: it reports the wallet database's expired_unmined column (isExpiredUmined) rather than recomputing against the tip. The database's rule is wallet-relative:

mined_height IS NULL AND expiry_height BETWEEN 1 AND blocks_max_height.max_height

Fix. Reach that same verdict from public API. isTxExpired() compares expiry against processor.fullyScannedHeight — the wallet's own contiguous scan floor — so a transaction counts as expired only once this wallet has scanned past its expiry window without finding it mined. The floor trails the database's MAX(blocks.height) while ranges scan out of order, so this is equal-or-more conservative than the DB flag and converges with it (and with iOS) once synced.

The emitted-transaction tracking now also records the computed verdict. It can flip without any tracked SDK field changing — a transaction stuck unmined keeps its minedHeight and its (tip-derived) transactionState while the scan floor crosses its expiry window — so without the extra trigger a genuine expiry would only surface on the next subscribe.

No JS or iOS changes: the event shape (isExpired: boolean) is unchanged and iOS is already correct.

Verification.

  • npm run fix-kotlin (ktlint) clean.
  • Compiles against the pinned SDK: :react-native-zcash:compileDebugKotlin via edge-react-gui's gradle with this file in node_modules — build successful, no warnings from the new code. This also confirms processor.fullyScannedHeight is public at the pinned 2.7.0-rc.4 (it is not lifted onto the Synchronizer interface until a later upstream release).
  • verify-repo.sh passed.

Device-verified on a Pixel 10 Pro emulator against a real funded wallet (6 transactions), by syncing to 100% and then resyncing. The rewind reported:

Rewinding to requested height: 3364881 with last local block: 3439915
Rewound to BlockHeight(value=3364881) successfully
→ chainTipHeight = 3,439,915   fullyScannedHeight = 3,364,891

All six transactions have expiry heights between 3,365,360 and 3,432,555 — every one of them inside that 75,024-block gap, i.e. above the scan floor but below the network tip. That is exactly the range where the two implementations disagree, so the bug condition was fully exercised rather than merely absent:

old code (vs tip) DB expired_unmined (what iOS reports) this branch (vs scan floor)
transactions marked expired 6 of 6 2 0

During the rescan every row — sent and received alike — rendered "Pending" rather than "Failed", and each returned to confirmed as the scan re-reached its block.

The DB flag reading 2 while this branch reports 0 is the conservatism described above, not a discrepancy: fullyScannedHeight trails MAX(blocks.height) while ranges scan out of order. Both transactions had been mined before the rewind and were re-mined by the end of the rescan, so not calling them expired was the correct answer.


Second commit: rebuilding the list during a rescan

The app empties its own transaction list for a resync and rebuilds it from what we report. Both platforms sent the whole set straight back, so the list refilled before the rescan had scanned anything.

Instrumenting the Android module showed it happening twice:

14:32:42.601  rescan() called
14:32:43.123  EMITTING 6 tx: mined=3432515, 3432300, 3431970, 3430879 …   ← pre-rewind heights
14:32:44.938  EMITTING 6 tx: mined=null ×6                                 ← rewind landed

The first is pure noise — it fired before the rewind landed, describing nothing that had changed, purely because rescan() cleared the emitted-transaction tracking and every row then looked new. The second is what left settled history reading as pending. iOS did the equivalent once, explicitly re-sending allTransactions in rescan's completion handler.

Now: keep the tracking across a rescan, and treat a transaction losing its mined height as the rewind undoing our own scan rather than news about the transaction. Tracking still follows it to the unmined state, so re-mining reads as a change and reports normally — which is how the list rebuilds one transaction at a time. On iOS the post-rewind emission is simply gone; its event stream already reports each transaction as the scan finds it.

Nothing is carried across a resync — not even a send still waiting to be mined. Preserving one was the original design here, and it was wrong: scanning only ever rediscovers transactions in mined blocks, so a preserved unmined send is a row nothing can ever clear. A send that never confirms would survive every resync and stay in the list forever. Letting the synchronizer be the only thing that reintroduces a transaction keeps the list honest about what the wallet can actually see; the send reappears when it is mined.

That also removes the discriminator entirely, which is what three review findings on this PR were about — there is no longer an expiry test to break, no chain tip to read, and no still-mineable set to build.

One related suppression falls out of it. A rewind drops the scan floor back below an expiry window, so a transaction already called expired stops looking expired. That reads as a change worth reporting and would put a failed send back in the list as pending. It is suppressed too, and reported again once the scan floor climbs past its expiry — as a genuine expiry.

Emissions are triggered only by what the wallet itself established: a transaction being new to us, gaining or losing a mined height, or our own scan-floor expiry verdict changing. Notably not by the SDK's transactionState, which is derived from the live network tip — the same value the first commit exists to stop trusting, and which turns Expired mid-rescan for transactions the scan has not reached yet. It never reached JavaScript, so it is gone from the tracking entirely.

A chain reorg unmines a transaction the same way a rewind does and is suppressed the same way. That is a deliberate trade: separating the two needs a flag scoped to our own rewind whose clear condition has no clean answer, and the cost of not separating them is bounded — a reorged transaction reads as confirmed until it is mined again, and one that never returns is reported as expired once the scan floor passes its expiry window.

Verified on device — Pixel 10 Pro emulator, real funded wallet, seven transactions. Two rewinds were exercised, including a second resync fired while the first rescan was still running (the repeat-resync case raised in review):

12:01:12  rescan() CALLED — tracking left intact
12:01:21  total=7 unmined=7 floor=3364891  → emitting NOTHING     ← rewind #1
12:01:25  total=7 unmined=5 floor=3365891  → EMITTING 2 tx: mined=3365334, mined=3365320

12:02:00  rescan() CALLED — tracking left intact                   ← resync during rescan
12:05:04  total=7 unmined=7 floor=3364891  → (no emission)         ← rewind #2
12:05:07  → EMITTING 2 tx: mined=3365334, mined=3365320

Across both rewinds and nine emissions, zero carried mined=null: every emission was a transaction gaining a real mined height. The app showed Loading Transactions… X% Complete in place of the list and rebuilt it as the scan progressed, with transactions returning confirmed — no "Pending" or "Failed" in between. A transaction that arrived genuinely new during the session was reported normally.

The pending-send path was confirmed separately by hand: a send made before a resync disappears from the list and returns once it is mined.

iOS was verified on the iPhone 17 simulator against the same wallet before this simplification, showing the same empty-then-rebuild behavior. The iOS change has since become strictly smaller — the post-rewind emission is gone entirely rather than filtered — so it has not been re-run in its current form.


The other half of the task (sync status reading 100% during the rescan) is a separate bug in edge-currency-accountbased, fixed in companion PR EdgeApp/edge-currency-accountbased#1083; the two are independent and can land in either order.


Note

Medium Risk
Touches native transaction event semantics during rescan and expiry on Android, which directly drives UI confirmation state; behavior is intentional but subtle (rewind vs reorg suppression).

Overview
Fixes resync behavior on Android and iOS so the app’s emptied transaction list rebuilds from the scan instead of refilling immediately, and stops Android from marking the whole history failed while a rescan is in progress.

On Android, isExpired no longer follows TransactionState.Expired (network tip). It uses fullyScannedHeight, matching iOS / the wallet DB: expired only after the wallet’s contiguous scan passes the expiry window without mining the tx. Emission tracking now keys off isExpired instead of transactionState, and suppresses spurious updates when a rewind drops mined height or clears an expired verdict until the scan catches up again.

On both platforms, rescan no longer dumps the full transaction set (Android stopped clearing emitted-transaction tracking; iOS removed post-rewind allTransactions). Transactions are reported again only as the scan rediscovers them; nothing is preserved across resync, including pending sends.

Reviewed by Cursor Bugbot for commit ae5aaa2. Bugbot is set up for automated code reviews on this repo. Configure here.


A resync rewinds the wallet to its birthday, which un-mines every stored
transaction until the scan re-reaches its block. TransactionState.Expired
compares an unmined transaction's expiry height against the live network
tip, so during that window the entire history counts as expired and the
app flashes every transaction as failed - Android only, since iOS reports
the database's expired_unmined column instead of recomputing against the
tip.

Reach the database's own verdict from public API: unmined, expiry enabled,
and the fully-scanned floor past the expiry window. The floor trails
MAX(blocks.height) while ranges scan out of order, so this is equal-or-
more conservative than the DB flag and converges with it (and with iOS)
once the wallet is synced.

The emitted-transaction tracking also records the computed verdict, since
it can flip without any tracked SDK field changing: a transaction stuck
unmined keeps its minedHeight and (tip-expired) transactionState while the
scan floor crosses its expiry window. Without the extra trigger, a genuine
expiry would only surface on the next subscribe.
@peachbits peachbits changed the title Report isExpired from the wallet's scan floor, not the network tip Rebuild the transaction list during a rescan, and stop mislabelling it expired Aug 7, 2026
@peachbits
peachbits marked this pull request as ready for review August 7, 2026 23:17
Comment thread android/src/main/java/app/edge/rnzcash/RNZcashModule.kt Outdated
@peachbits
peachbits force-pushed the matthew/fix/rescan-expired-mislabel branch from 773f1ce to cea092c Compare August 10, 2026 18:01
Comment thread ios/RNZcash.swift Outdated
Comment thread ios/RNZcash.swift Outdated
previousState?.minedHeight != null && tx.minedHeight == null

if (isNew || minedHeightChanged || stateChanged || expiredChanged) {
if (!unminedByRewind) transactionsToEmit.add(tx)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewind suppress hides real reorgs

Medium Severity

unminedByRewind drops any emission where a tracked transaction loses its mined height, not only during a user rescan. A chain-error rewind takes the same path, so the app can keep showing a transaction as confirmed after the SDK has unmined it, until a later remine or expiry update happens to land.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit cea092c. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, and accepted as a deliberate trade rather than fixed — now documented at the suppression in b2cf053 so it reads as a decision instead of an oversight.

Telling a reorg apart from our own rewind needs a flag scoped to the rescan, and the clear condition for that flag — "the scan has caught up again" — has no clean answer. Three earlier attempts on this PR to carry exactly that kind of inferred state are what produced the other findings here, so adding more of it to cover a rare case is a poor trade.

The cost is bounded in both directions. A reorged transaction keeps reading as confirmed until it is mined again, which on this chain is normally the next few blocks and is reported as soon as it happens. One that never comes back is reported as expired once the scan floor passes its expiry window, so it does not linger indefinitely. The alternative — reporting it unmined immediately — is the pending flicker this PR exists to remove, and it would fire on every resync rather than only on a reorg.

@peachbits
peachbits force-pushed the matthew/fix/rescan-expired-mislabel branch from cea092c to 67c4611 Compare August 10, 2026 18:53
Comment thread android/src/main/java/app/edge/rnzcash/RNZcashModule.kt
@peachbits
peachbits force-pushed the matthew/fix/rescan-expired-mislabel branch from 67c4611 to b2cf053 Compare August 10, 2026 21:23

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b2cf053. Configure here.

Comment thread android/src/main/java/app/edge/rnzcash/RNZcashModule.kt
The app empties its transaction list for a resync and rebuilds it from
what we report, but both platforms sent the whole set straight back, so
the list refilled before the rescan had scanned anything.

Android did it twice. rescan() cleared the emitted-transaction tracking,
so the next collector pass saw every transaction as new and reported the
lot at their pre-rewind heights - 19 seconds before the rewind had even
landed, describing nothing that had changed. The rewind then unmined
every row, which read as a change and sent the same set again, this time
as unmined, which is what left settled history looking pending.

iOS did it once, explicitly: rescan re-sent allTransactions as soon as
the rewind finished.

Keep the tracking across a rescan instead of clearing it, and treat a
transaction losing its mined height as the rewind undoing our own scan
rather than news about the transaction. Tracking still follows it to the
unmined state, so re-mining reads as a change and reports normally, which
is how the list rebuilds one transaction at a time.

Unmined transactions are the exception on both platforms. Scanning only
discovers transactions in mined blocks, so nothing would bring back a
send still waiting to be mined; those are re-reported so they survive the
resync. iOS has to collect them before the rewind, since afterwards every
transaction looks unmined.
@peachbits
peachbits force-pushed the matthew/fix/rescan-expired-mislabel branch from b2cf053 to ae5aaa2 Compare August 10, 2026 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants