Repository navigation
fix: publish rbf replacements before success - #1813
Conversation
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
[Medium impact] The latest wording change appears safe to merge. SummaryThe PR publishes replacement transaction details before showing Boost success. The latest change updates pending feedback in all 16 locales without changing Kotlin behavior.
Reviews (3) · Last reviewed commit: "fix: avoid claiming pending boosts were ..." · Reviewed by Greptile |
|
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 |
talosmachina
left a comment
There was a problem hiding this comment.
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 insideServiceQueue.CORE.background, but its own work iswithContexton the LDK dispatcher, so the CORE coroutine suspends rather than blocking the single CORE thread.- Event and publication both rewrite the replacement:
updateReplacementActivityreturns early when the original txid is already inboostTxIds, andmarkReplacedActivityis idempotent, so either order produces one link. - Generic replaced toast for a local boost:
isLocalRbfBoostinnotifyTransactionReplacedsuppresses it once publication has recorded the replacement. An event handled before that can still toast, butmasterhad the same window (it flaggedisBoostedonly after the broadcast returned), so it is not introduced here. - Stale success after a re-setup:
setupActivitynow wraps the activity in a fresh instance, so theactivity === originchecks 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 sameDeferredand 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
left a comment
There was a problem hiding this comment.
Code scan at d28d15c54: three LOW, inline. The approval waits for the device gate.
Checked and clean:
- Double broadcast:
onConfirmBoostreturns early while boosting, and the per-txidDeferredcache allows onebumpFeeByRbfper view model, including after the sheet reopens. - Double counting:
markReplacedActivitysetsdoesExist = false, and the original joins the replacement'sboostTxIds, sofilterOutReplacedSentTransactionshides it once published. A partial failure insidepublishRbfBoostconverges 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
feePaidMsatrather than the UI estimate. - Wallet reset: the generation guard in
ActivityRepoholds, andisLocalRbfBoostuses the sameWalletScope.defaultkey. - An app kill between broadcast and publication heals through
OnchainTransactionReplacedon 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.
|
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
left a comment
There was a problem hiding this comment.
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
RbfBroadcastAttemptis removed only on a broadcast failure or cancellation (removeRbfBroadcast).setupActivityfinds it on reopen and callsrestoreBroadcastFee, so the swipe awaits the same completedDeferredand onlypublishRbfBoostruns again. - Fee edits slip past the lock:
onClickEdit,onClickUseSuggestedFeeandonChangeAmountall return onfeeLocked. A latecalculateTotalFeeresult is dropped by theactivity !== origin || feeLockedcheck, andupdateUiStateWithFeeDatarestores the frozen quote while a broadcast is held. - A failed conflict aborts the rest: each conflict now has its own
runSuspendCatching, andmarkReplacedActivitystill 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
left a comment
There was a problem hiding this comment.
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 electrumright 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 newbumpFeeByRbfcall, 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)
talosmachina
left a comment
There was a problem hiding this comment.
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
jvsena42
left a comment
There was a problem hiding this comment.
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
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
Out of Scope
rbf-own-send.xmlassertion/order parity: tracked in Port immediate RBF journey assertions to iOS #1816 for 3.2.0 (review thread); this LOW documentation follow-up requires coordinated iOS journey work.Design
N/A — no design available.
Preview
N/A — no recording of the new pending-publication feedback is available.
QA Notes
Setup
E2E=false E2E_BACKEND=network just runand fund Savings with at least 100,000 confirmed staging-regtest sats.rbf-own-send.xmluses the checkout's staging./lspcommands.E2E=true, the standard isolated local Bitcoin/Electrum backend, Pixel 6/API33 x86_64, Node 22 and 30s UI waits. E2E source is pinned to46fc6ae2ee5310d63d21bf84e5add87d7f40cdd6; the temporary CI branch pins the app checkout too. Local-backend deposits/mining come from that E2E fixture rather than staging./lsp.Journeys
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
Automated Checks
ActivityServiceRbfTest.kt— delayed publication, fresh metadata, per-conflict tag-read/write and discovery failures, retry repair, duplicate events, contact detachment, retirement and cancellation.ActivityRepoTest.kt— stale-row successor routing, successive boosts, confirmed-original routing, wallet scoping/reset and publication failure.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.AppViewModelSendFlowTest.kt— late local replacement feedback preserves Boost success while external incoming/outgoing replacement notifications still appear.Evidence
Wording-only follow-up
2e78e2568replaces 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) andjust lintpass 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 E2E265a050057c6cc9cd03f965ba81fb1700f7361ee. The app was built from merge4cb40072d593bf3168a69d01e9e41bc2dd8a8eaa(parents master108d4fd12bf043a92f957b1833288767c134c8bdand this head), native tree40ab8ba58c113d63b8c22a902c3df45844cdcbdf. 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
d368822records 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 wasE2E=true E2E_BACKEND=localon 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 master74c8b2cd(3 samples each); swipe→toast was 5.90 s versus 1.84 s. These are reviewer's device results atd368822, 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 E2Ef3fd590e8e77491a340c06512df5f5e0ab2e30a0(30 tests); the onchain/boost/receive/widgets shard passed its first inner test attempt in workflow attempt 3 on E2E68817889cba3ca0148725cce30424771ae8b50cb(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 wase7faee816d244cc7c9c70aa61b225490bc791855via merge0cf5e0cf288f6cbed066885c39a4ada9d8de52ae. This is the configured PR gate across actual attempts and harness pins, not a reliability cohort or the entire Android E2E suite.Follow-ups
6fab411andd368822address 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 compileand the fulljust testpassed 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, andjust test file ActivityRepoTestplusjust lintpassed 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 coverd368822; build/Compose/unit CI and lint passed there.Historical evidence below remains pinned to its actual revisions.
Native
just compile,just testandjust lintall pass atd28d15c54200679d2abd3c38a8a8d04068a14d77: 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/ E2E46fc6ae, 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 atd28d15cawaits 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 E2E46fc6ae2ee5310d63d21bf84e5add87d7f40cdd6, temporary workflowcd57c5fb4e15260909274f0ef1fb55b1a22d32fa. 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 at2e78e2568has 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 atd28d15cwas cancelled after six shards failed; its seven archives and actual prior outcomes remain separate.Models used
gpt-6.1-sol(reasoning:high)gpt-6.1-sol(reasoning:high)gpt-6.1-sol(reasoning:high)