Skip to content

fix: publish rbf replacements before success - #1813

Merged
piotr-iohk merged 8 commits into
masterfrom
codex/fix-android-boost-publication
Nov 9, 2026
Merged

piotr-iohk merged 8 commits into
masterfrom
codex/fix-android-boost-publication

Conversation

@piotr-iohk

@piotr-iohk piotr-iohk commented Nov 9, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #1815

Companion: synonymdev/bitkit-e2e-tests#298

This PR publishes a boosted send's replacement activity before reporting success, so opening the send shows the new TXID and actual fee.

Description

  • Creates and links the actual RBF replacement before returning home, preserving the original transaction's fee and copying its contact and tags. If the returned TXID is not yet available, awaits one wallet sync and resolves that exact replacement before publishing success.
  • Resolves a stale activity-list row to the published replacement, including successive boosts, and keeps a late replacement notification from interrupting the Boost success toast.
  • Retains the native RBF attempt and approved quote while its Activity Detail session remains alive, so reopening the sheet retries publication without another native bump; cancellation and wallet reset preserve the originating operation's ownership.
  • Keeps external replacement retirement independent of metadata-copy failures, isolates failures per conflict, and commits tags before ancestry so later events can repair incomplete copies without overwriting subsequent edits.
  • Shows Boost Pending if the replacement cannot be published after native RBF returns a TXID, using wording that makes no network-acceptance claim; Retry within the same Activity Detail session retains the approved quote and republishes the same replacement.
  • Opens a confirmed original directly if it confirms instead of the cached replacement.

Out of Scope

Design

N/A — no design available.

Preview

N/A — no recording of the new pending-publication feedback is available.

QA Notes

Setup

  • Use this PR's dev/regtest build. For the manual journey, build with E2E=false E2E_BACKEND=network just run and fund Savings with at least 100,000 confirmed staging-regtest sats. rbf-own-send.xml uses the checkout's staging ./lsp commands.
  • Automated Boost2 uses E2E=true, the standard isolated local Bitcoin/Electrum backend, Pixel 6/API33 x86_64, Node 22 and 30s UI waits. E2E source is pinned to 46fc6ae2ee5310d63d21bf84e5add87d7f40cdd6; the temporary CI branch pins the app checkout too. Local-backend deposits/mining come from that E2E fixture rather than staging ./lsp.

Journeys

  • J1: updated rbf-own-send.xml — immediately opening the outgoing row after Boost success reaches the replacement with the original payment amount, higher fee, changed TXID, replacement ancestry and final confirmation.

Manual Tests

  • 1. After native RBF returns a replacement TXID, interrupt publication sync, then restore connectivity → Boost Pending; reopen the sheet within the same Activity Detail session → Retry retains the approved quote, fee edits are disabled, and publication succeeds without another native bump — controlled post-broadcast sync failure not in Capabilities.
  • 2. After an RBF broadcast, confirm the original instead of the replacement and evict the replacement → opening the confirmed original shows its own confirmed TXID — selecting which conflicting transaction a miner includes not in Capabilities.

Automated Checks

  • added ActivityServiceRbfTest.kt — delayed publication, fresh metadata, per-conflict tag-read/write and discovery failures, retry repair, duplicate events, contact detachment, retirement and cancellation.
  • updated ActivityRepoTest.kt — stale-row successor routing, successive boosts, confirmed-original routing, wallet scoping/reset and publication failure.
  • updated BoostTransactionViewModelTest.kt — success ordering, pending-publication feedback, frozen retry quotes, late estimates, reentry without another swipe, cancellation recovery and original transaction ownership; existing CPFP checks remain.
  • updated AppViewModelSendFlowTest.kt — late local replacement feedback preserves Boost success while external incoming/outgoing replacement notifications still appear.

Evidence

Wording-only follow-up 2e78e2568 replaces the broadcast claim with pending/updated-details copy in all 16 locales. No Kotlin behavior changed. The commit is signed and GitHub Verified; independent review, just compile, all 4,968 unit tests (zero failures/errors/skips) and just lint pass on its exact source. Greptile's 5/5 result and Talos's no-findings re-review cover this head. Build/unit, Compose UI and lint CI passed. Configured PR E2E run 42657204273 passed in workflow attempt 1: all seven shards executed 39 tests, each shard's first inner attempt succeeded and attempts 2/3 were skipped. All shards used E2E 265a050057c6cc9cd03f965ba81fb1700f7361ee. The app was built from merge 4cb40072d593bf3168a69d01e9e41bc2dd8a8eaa (parents master 108d4fd12bf043a92f957b1833288767c134c8bd and this head), native tree 40ab8ba58c113d63b8c22a902c3df45844cdcbdf. This is the configured PR gate, with its actual source pins, and does not establish a reliability cohort or entire Android-suite result. jvsena42 approved this head with no new findings. All review threads are resolved; GitHub reports APPROVED and CLEAN.

The device approval at d368822 records J1's 13 steps passing: immediate row open retained 10,000 sats, fee 141→422, a changed TXID and RBF ancestry, then mined confirmation. Manual case 1 also passed using an Electrum pause after the swipe: Boost Pending, the locked original quote on sheet reopen, and a successful publication retry with no new native bump. Setup was E2E=true E2E_BACKEND=local on Gate_Throwaway with a fresh wallet and local bitcoind funding/mining. The same review measured native return→toast medians of 4.13 s on the PR versus 0.35 s on master 74c8b2cd (3 samples each); swipe→toast was 5.90 s versus 1.84 s. These are reviewer's device results at d368822, not a new execution at the copy-only head.

Configured PR E2E run 42608608578 eventually passed at d368822, workflow attempt 3. Six shards passed in workflow attempt 2 on E2E f3fd590e8e77491a340c06512df5f5e0ab2e30a0 (30 tests); the onchain/boost/receive/widgets shard passed its first inner test attempt in workflow attempt 3 on E2E 68817889cba3ca0148725cce30424771ae8b50cb (9 tests). Attempt 1's seven startup-failing shards are retained, and attempt 2's onchain job was cancelled after AVD snapshot/teardown stalled before APK download. All job logs and failed archives are preserved. The native build tree was e7faee816d244cc7c9c70aa61b225490bc791855 via merge 0cf5e0cf288f6cbed066885c39a4ada9d8de52ae. This is the configured PR gate across actual attempts and harness pins, not a reliability cohort or the entire Android E2E suite.

Follow-ups 6fab411 and d368822 address the per-conflict failure, publication-only retry and confirmed-original review findings. Both commits are signed and GitHub Verified. Independent correction reviews found no remaining actionable findings.

Native validation for d368822: just compile and the full just test passed on the same production source: 4,968 unit tests, zero failures/errors/skips. Detekt then required only argument wrapping in the new test; that formatting-only delta was independently reconciled, and just test file ActivityRepoTest plus just lint passed on the committed tree. Detecting negative controls reproduced skipped later conflicts, the changed retry quote, confirmed-original misrouting and the reopened-sheet lock before their fixes. Greptile's 5/5 result and Talos's no-findings re-review cover d368822; build/Compose/unit CI and lint passed there.

Historical evidence below remains pinned to its actual revisions.

Native just compile, just test and just lint all pass at d28d15c54200679d2abd3c38a8a8d04068a14d77: 4,960 unit tests, zero failures/errors/skips. Independent implementation review verified both external-event retirement and delayed native replacement readiness. All five app commits are signed and GitHub Verified.

The first targeted Android CI run 42591327878, app 18b041c / E2E 46fc6ae, failed all three attempts waiting for BoostSuccessToast. All three artifact sets were inspected: broadcast succeeded, but the one-shot lookup ran before native wallet sync discovered the replacement. The replacement appeared about 1.1 seconds later and ancestry was linked about 2.1 seconds after broadcast. The correction at d28d15c awaits one actual wallet sync on an exact-TXID lookup miss, then resolves that same replacement before publication; no sleep or broadcast retry.

Corrected targeted Android CI run 42595261569 passed on its first attempt; attempts 2/3 were skipped. The Boost2 job reports 16/16 action/outcome observations passed, with all preserved payment, fee/TXID, RBF metadata, restore and mined-confirmation assertions completed. The payment remains 10,000 sats, fee increases 141→422 sats, and the opened TXID matches the returned replacement. Native logcat shows broadcast→awaited LDK sync→replacement link before success observation. Suite time 3m 43.7s; worker time 4m 04s.

App checkout d28d15c54200679d2abd3c38a8a8d04068a14d77, unchanged E2E 46fc6ae2ee5310d63d21bf84e5add87d7f40cdd6, temporary workflow cd57c5fb4e15260909274f0ef1fb55b1a22d32fa. Node 22.23.4, Pixel 6/API33 x86_64, local backend and 30s UI waits. This is one targeted case, not a full-suite or reliability comparison.

Gaps

J1 and manual case 1 have device evidence at d368822, linked above. The neutral pending-message text at 2e78e2568 has compile/XML/lint verification but has not been captured on device; the Kotlin behavior is unchanged. Manual case 2 remains unrun because selecting the miner's conflicting transaction is outside the standard journey environment; its resolver mechanism has a detecting unit regression. Retry reuse is limited to the same Activity Detail ViewModel, with cross-detail persistence tracked in #1821. The targeted Boost2 pass and configured PR E2E gate do not establish an entire Android-suite pass or a reliability cohort. The earlier ready-triggered run 42603535285 at d28d15c was cancelled after six shards failed; its seven archives and actual prior outcomes remain separate.

Models used

  • Planning/scoping: gpt-6.1-sol (reasoning: high)
  • Implementation: gpt-6.1-sol (reasoning: high)
  • Review: gpt-6.1-sol (reasoning: high)

@github-actions

github-actions Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 2e78e25 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@greptile-apps

greptile-apps Bot commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The latest wording change appears safe to merge.

Summary

The PR publishes replacement transaction details before showing Boost success. The latest change updates pending feedback in all 16 locales without changing Kotlin behavior.

  • Boost success waits until the replacement send is ready.
  • Tapping an old send opens its published replacement.
  • A pending boost can retry with the same fee quote.

Reviews (3) · Last reviewed commit: "fix: avoid claiming pending boosts were ..." · Reviewed by Greptile

@ovitrif

ovitrif commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

Triage: in 3.2.0, a non-blocking bug fixed after the 3.1 cut. This PR closed no issue, so I filed #1815 to carry the milestone and added Closes at the top of the body. The issue is on the project at 👀 Review. No iOS twin.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No findings. Publishes the RBF replacement activity (ancestry, original contact and tags, one awaited wallet sync on a lookup miss) before the Boost success effect, routes stale activity rows to the published replacement, and suppresses the generic replaced toast for a local boost. Reviewed d28d15c, full tier, static.

What I checked, and 5 candidates I ruled out

Read in full: the changed parts of CoreService.kt (publishRbfBoost, handleOnchainTransactionReplaced, markReplacedActivity, getOrCreateReplacementActivity, updateReplacementActivity), ActivityRepo.kt, BoostTransactionViewModel.kt, AppViewModel.kt (handleOnchainTransactionReplaced, notifyTransactionReplaced), the ContentView.kt and HomeScreen.kt call sites, LightningService.sync, LightningRepo.sync, ServiceQueue.
Build: not built here. On this track an auto review does not build the branch; CI is the gate. At review time build, detekt, lint passed and build-and-test and compose-ui / ui-tests were still running.

Ruled out

  • lightningService.sync() stalls the CORE queue: it runs inside ServiceQueue.CORE.background, but its own work is withContext on the LDK dispatcher, so the CORE coroutine suspends rather than blocking the single CORE thread.
  • Event and publication both rewrite the replacement: updateReplacementActivity returns early when the original txid is already in boostTxIds, and markReplacedActivity is idempotent, so either order produces one link.
  • Generic replaced toast for a local boost: isLocalRbfBoost in notifyTransactionReplaced suppresses it once publication has recorded the replacement. An event handled before that can still toast, but master had the same window (it flagged isBoosted only after the broadcast returned), so it is not introduced here.
  • Stale success after a re-setup: setupActivity now wraps the activity in a fresh instance, so the activity === origin checks drop effects from an earlier sheet instance.
  • Retry broadcasts twice: a successful broadcast stays in broadcastRbfReplacements, so a retry after a publication failure awaits the same Deferred and only republishes; a failed or cancelled broadcast is removed, so a real retry can rebroadcast.

Merge confidence: 4/5, no findings, but not built here and the unit test job was still running at d28d15c.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code scan at d28d15c54: three LOW, inline. The approval waits for the device gate.

Checked and clean:

  • Double broadcast: onConfirmBoost returns early while boosting, and the per-txid Deferred cache allows one bumpFeeByRbf per view model, including after the sheet reopens.
  • Double counting: markReplacedActivity sets doesExist = false, and the original joins the replacement's boostTxIds, so filterOutReplacedSentTransactions hides it once published. A partial failure inside publishRbfBoost converges through the idempotent replaced-event handler.
  • Original kept on failure: sync and lookup failures throw before any write.
  • Fee shown on success: it now comes from LDK's feePaidMsat rather than the UI estimate.
  • Wallet reset: the generation guard in ActivityRepo holds, and isLocalRbfBoost uses the same WalletScope.default key.
  • An app kill between broadcast and publication heals through OnchainTransactionReplaced on restart.

Not raised: the replaced toast can still play before the success toast during the sync window, but ToastQueueManager queues them, so nothing is cut off.

perf: relevant. Every RBF boost now waits on a full node.syncWallets() (plus watch-only reconcile) before the success toast, then fires syncAsync() again. To measure on device: bumpFeeByRbf success → BoostSuccessToast, PR vs master.

Device gate (partial): d28d15c54 — not run yet; boost/rbf-own-send.xml and the boost-to-toast timing are pending. Device result follows in a second review.

Comment thread journeys/boost/rbf-own-send.xml Resolved

Copy link
Copy Markdown
Collaborator Author

Follow-up to the d28d15c review: the correctness findings are addressed at d368822 in their inline threads, and journey parity is tracked in #1816. J1 and the two new fault cases remain explicit device gaps. On the performance observation, the full native sync is conditional on an exact replacement lookup miss; the already-present-record unit case verifies it is skipped. The prior targeted Boost2 evidence remains pinned to d28d15c, and no PR-versus-master timing comparison has been run. Fresh bot/CI coverage is pending before the human re-request.

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review of d368822 after d28d15c. 0 earlier findings, so nothing to carry over. 0 new findings. The two follow-ups: each conflict's replacement handling now runs on its own, so one failed metadata copy no longer skips the conflicts after it. A broadcast that succeeded but failed to publish now shows Boost Pending, and its Retry keeps the approved quote and only republishes. A confirmed original now opens as itself. Full tier on the delta, static.

What I checked, and 4 candidates I ruled out

Read in full: BoostTransactionViewModel.kt at the new head, resolvePublishedRbfActivity and publishRbfBoost in ActivityRepo.kt, handleOnchainTransactionReplaced and getOrCreateReplacementActivity in CoreService.kt, the new onPending wiring in BoostTransactionSheet.kt and ActivityDetailScreen.kt, and every resolvePublishedRbfActivity call site (ContentView.kt, HomeScreen.kt).
Diff: d28d15c..d368822, linear, no merge from master.
Build: not built here. On this track an auto review does not build the branch. build, build-and-test, compose-ui / ui-tests, lint and detekt are green at d368822.
E2E: send, multi_address_local, settings and lightning_security fail in their before each/before all hooks with Wallet home or USDT introduction did not appear and electrs failed. That is wallet setup on the local backend, before any boost step. Run 42604080455 (feat/spark-emergency-exit) and run 42603023592 (feat/move-to-spark) fail their shards the same way today, while this diff touches nothing on the onboarding path. So I could not tie the red to this change, and I could not clear it either.

Ruled out

  • Retry rebroadcasts after Boost Pending: a successful RbfBroadcastAttempt is removed only on a broadcast failure or cancellation (removeRbfBroadcast). setupActivity finds it on reopen and calls restoreBroadcastFee, so the swipe awaits the same completed Deferred and only publishRbfBoost runs again.
  • Fee edits slip past the lock: onClickEdit, onClickUseSuggestedFee and onChangeAmount all return on feeLocked. A late calculateTotalFee result is dropped by the activity !== origin || feeLocked check, and updateUiStateWithFeeData restores the frozen quote while a broadcast is held.
  • A failed conflict aborts the rest: each conflict now has its own runSuspendCatching, and markReplacedActivity still runs once before the loop, so retiring the original does not depend on any metadata copy.
  • Confirmed check short-circuits a successive boost: the walk returns early only for a row whose snapshot is confirmed. A replaced transaction cannot confirm alongside its replacement, so a chain A→B→C still resolves to C unless A itself confirmed, which is the intended routing.

Merge confidence: 3/5, E2E shards red in wallet setup for a reason I could not tie to this change.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Device gate: d36882269. Journeys: 1 passed (J1 boost/rbf-own-send). perf: bumpFeeByRbf success → success toast, 4.13 s on the PR vs 0.35 s on master (medians of 3; swipe → toast 5.90 s vs 1.84 s).

  • J1: all 13 steps passed. Right after "Boosted!" the row showed the original 10 000 sats, the fee rose from 141 to 422, the TXID changed, and RBFBoosted listed the original. After 1 block it was Confirmed.
  • Boost Pending (manual case 1, run with docker pause electrum right after the swipe): the sheet closed with "Boost Pending". Reopening showed Retry with the quote unchanged and fee editing disabled. After unpause, Retry succeeded with no new bumpFeeByRbf call, and only that replacement was in the mempool.
  • The residual on my F1 thread reproduces on device; the details are there. It stays LOW.
  • No app crashes.

Setup: E2E=true E2E_BACKEND=local builds of this PR and of master 74c8b2cd, on Gate_Throwaway with a fresh wallet. Funded with 4 × 500 000 from the local bitcoind and mined with local bitcoin-cli instead of ./lsp. Evidence is in .ai/gate1813_1794229796/ on my machine.

Approving: nothing above LOW is left. The two new LOWs are inline.

Reviewer model: Claude Fable 5.1 (claude-fable-5-1), delta review; Claude Opus 5.5 (claude-opus-5-5), device gate and summary
Reasoning effort: not exposed (Fable); low (gate)

Comment thread app/src/main/res/values/strings.xml Resolved

@talosmachina talosmachina left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Re-review of 2e78e25 after d368822. 0 earlier findings, so nothing to carry over. 0 new findings. The one commit since rewords wallet__boost_pending_msg in all 16 locales so Boost Pending no longer claims the replacement was broadcast; no Kotlin changed. Light tier on the delta (16 changed lines, strings only).

What I checked

Delta: git diff d368822..2e78e25 touches only values*/strings.xml, the same key in each, so no locale is left with the old broadcast claim. All 16 files parse as XML.
Call site: the key's only reader is the BoostPendingToast in ActivityDetailScreen.kt (onPending), unchanged.
Assertions on the old text: none in app/src/test, journeys/ or bitkit-e2e-tests, so the reword cannot break a test.
Translations: each locale now says the boost is pending and the updated details are not ready; none claims network acceptance.
CI at 2e78e25: build-and-test, build-local, lint and detekt green; the E2E shards and compose-ui were still running when I read them. The configured E2E run 42608608578 that was red at my last review finished green at d368822 (attempt 3), and this delta does not touch anything those shards run.

Merge confidence: 5/5, copy-only delta with build, unit tests and lint green; the E2E shards at this head were still running.

Reviewer model: claude-opus-5-5
Reasoning effort: Not exposed

@piotr-iohk
piotr-iohk requested review from a team and jvsena42 and removed request for a team November 9, 2026 17:19

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Delta d36882269..2e78e2568: no findings. wallet__boost_pending_msg no longer says the boost was broadcast, and all 16 locales carry the neutral pending wording, so my strings LOW is fixed. The 4 s feedback LOW is tracked in #1822.

Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed

@piotr-iohk
piotr-iohk merged commit c619436 into master Nov 9, 2026
24 checks passed
@piotr-iohk
piotr-iohk deleted the codex/fix-android-boost-publication branch November 9, 2026 20:06
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.

bug: boosted send shows the old txid and fee

4 participants