Repository navigation
fix: use saved fees after gateway errors - #1701
Conversation
 [High impact] The PR appears safe to merge; no actionable issues were found. SummaryThis PR uses saved Bitcoin fees after two recognized Blocktank info-502 responses. It also keeps delayed payment preparation tied to the original wallet and blocks preparation while required recovery remains pending.
Diagram%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Payment link or scan] --> B{Original wallet current and recovery complete?}
B -->|No| C[Stop preparation]
B -->|Yes| D[Prepare payment]
D --> E[Fetch fresh fee schedule]
E -->|Success| F[Use fresh schedule]
E -->|Recognized info 502| G[Read saved schedule]
E -->|Other error| H[Fail]
G -->|Missing or unreadable| H
G -->|Available| I[Keep selected speed]
F --> J{Original wallet still current?}
I --> J
J -->|Yes| K[Present payment]
J -->|No| C
Reviews (1) · Last reviewed commit: "merge: adopt validated foreground recove..." · Reviewed by Greptile |
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
talosmachina
left a comment
talosmachina
left a comment
There was a problem hiding this comment.
1 finding (1 P2). Falls back to the saved Blocktank fee schedule when the info call fails with one of two recognised 502 Bad Gateway forms, and binds payment links, scans and Paykit request preparation to the wallet generation that started them, so they are dropped when required WALLET/USDT recovery is pending or the wallet was replaced. Reviewed 258f8a9, full tier, static only: per this repo's policy I do not build Android PR branches here, so nothing was compiled or run on my side.
What I checked, and 5 candidates I ruled out
Read in full (diff): CoreService.kt (BlocktankService.getFees, isInfoBadGateway), WalletConnectionRecovery.kt, Env.kt, BlocktankTestEndpoint.kt, SendSheet.kt, AppViewModel.kt (all 399 added lines), BlocktankFeeFallbackTest.kt, test names in AppViewModelSendFlowTest.kt
Call sites traced: getFees (LightningRepo.getFeeRates, getFeeRateForSpeed, used by transfer, boost, wallet and send flows), resetSendState (its only caller outside the view model is SendSheet), Keychain.hasPendingWalletRestore -> Keychain.exists, WalletConnectionRecovery.beginWipe (the only place the generation moves)
CI: build, lint and build-local still pending at review time; Greptile passed
Ruled out
- Fallback widens error handling: any non-502 failure is rethrown exactly as before (the old code let the
fees(refresh = true)exception propagate too), and a null fresh result still reads the cache as before. - 502 match misses the wrapped error:
ServiceQueue.backgroundwraps inAppError(it), andisInfoBadGatewaywalksAppError.causeup to 8 levels, which the HTML and JSON 502 tests exercise. - Stale
isLoadingafter an early return inrefreshOnchainSendIfNeeded: the early return only fires whensendStateGenerationmoved, and every place that moves it either rebuildsSendUiState(freshOnchainFeeUi) or leaves on-chain. - Debug Blocktank endpoint reaching release:
E2E_BLOCKTANK_API_URLis empty for non-debug variants inbuild.gradle.kts, andEnvalso requiresisDebug && isE2eTest; the URL must belocalhostwith an explicit port. - Deferred scan replayed for a replaced wallet:
DeferredScanandScheduledScannow carrywalletGeneration, duplicate merging compares it, andlaunchScanre-checks it before and inside the scan mutex.
Merge confidence: 4/5, no blocking findings but the hosted build and lint had not finished and I reviewed without building.
![ovi-reviewer[bot]](https://avatars.githubusercontent.com/in/4862686?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: ⛔️ Request changes — one reproduced send-flow regression.
Findings:
- MEDIUM — Recovery rejection permanently stops send-event handling (
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:6962).requireCurrentPaymentPreparationthrowsCancellationExceptiondirectly inside the soleobserveSendEventscollector. This check also runs for ordinary on-chain AmountContinue, before the Lightning-only early return. If required recovery becomes pending or unreadable, rejecting that one action ends the collector. Clearing recovery and reopening send do not restart it: later amount/address events and Continue are ignored until the activity's view model is recreated. The post-fee check has the same effect when the wallet changes during Lightning fee estimation. Keep the recovery gate, but stop only the individual stale action.
The separate blocking keychain-read finding is already reported and is not duplicated here.
QA for 258f8a9:
Built from 258f8a9. Android 16 / API 36, disposable regtest wallet with real test funds. The device APK adds a debug-only hook to set/clear the actual required-recovery marker and read send state; the production send handler is unchanged. No payment was broadcast.
Test 4: send after a recovery rejection ❌ Continue remains broken after recovery clears and the send sheet reopens.
I entered 1,000 sats, set the required WALLET marker through the debug hook, and tapped Continue. After clearing the marker, Continue still did nothing. Closing and reopening the same payment reset the amount, but entering a new amount no longer updated the view model. The hook confirmed its amount remained zero; the number field could still change locally.
test4-final-2x.2x.mp4
Recovery rejection, cleared marker, and a fresh send that cannot continue; recording at 2×.
Test 5: fresh-process control ✅ Restarting the same app and wallet restores payment preparation.
With the marker clear, I restarted the app, reopened the same owned recipient, entered 1,000 sats, and reached Confirm. Restart was checked by force-stopping and relaunching the app; the recording shows the resulting confirmation. This isolates the failure to the existing view model's event collector. I cancelled before payment.
test5-control-2x.2x.mp4
The same wallet reaches 1,000-sat payment review after restart; recording at 2×.
Check 1: head build and existing affected unit suites ✅ The unmodified head builds and its existing targeted suites pass.
Ran assembleDevDebug and targeted testDevDebugUnitTest with these suites:
BlocktankFeeFallbackTestAppViewModelSendFlowTestWalletConnectionRecoveryTestLightningRepoTestBlocktankTestEndpointTest
Check 2: collector-lifetime regressions ❌ Both added regression checks reproduce ignored later events.
One check replaces the wallet generation while Lightning fee estimation is suspended; the other makes the required-marker read temporarily fail during on-chain AmountContinue. After restoring valid state and calling resetSendState, each emits AddressReset. Both fail because addressInput stays unchanged: the collector has ended. These are temporary review checks, separate from the committed passing suites.
Manual 1–3 were not repeated: the author's acceptance report records the six cached-fee transfers, error controls, and authenticated required-restoration case. Those are author-reported results, not device passes from this review.
Hosted E2E is also red: onboarding and on-chain fail while waiting for “Yes, Delete”. I have not attributed those failures to this change.
Twin: no matching fee-fallback PR is linked; iOS already retries the saved schedule after fresh lookup fails. The fallback behavior matches; this review does not certify iOS recovery parity.
Coverage: full f9ff9d2..258f8a9, all 13 changed files and affected callers. Device work covers the send-event recovery regression and its control; fee-fallback broadcasts and complete authenticated restoration rely on the author's report above.
Reviewer model: gpt-6.1-sol
Reasoning effort: high
Reviewed by gpt-6.1-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full review of the PR diff against merge base f9ff9d27, at 258f8a9. No earlier assessment baseline was available.
2 actionable findings — resolve or provide an evidence-backed rebuttal.
Continuing an open send after required recovery becomes pending, or after the wallet generation changes, cancels the activity-scoped send-event collector. Amount Continue hits that guard before the Lightning-only return, and clearing recovery does not start the collector again.
BlocktankService.getFees uses the saved Fast, Normal, and Slow schedule for the two recognized info 502 responses and still fails other fresh-lookup errors. Payment-link, scan, and Paykit preparation keep the generation captured when they start and drop the presentation when that generation is no longer current. Generation still advances only in beginWipe on the parent at f9ff9d27 (#1685); that parent was comparison context, not a separate review.
iOS FeeEstimatesManager.getEstimates at 11c27421 returns nil on any fee fetch error and leaves the last published estimate unchanged. It does not special-case these two 502 strings, which is the narrower contract this PR states.
The new fee-fallback and send-preparation unit tests were read and not executed here. GitHub reported success for CI build 40520195678 and lint 40519956652 on this head. E2E 40520196618 failed; those logs were not re-diagnosed here.
Device testing: not performed in this review.
Suggested additional test cases
- Android. Open an ordinary on-chain send on the amount screen. Set a required WALLET or USDT restore marker, tap Continue, then clear the marker. Tap Continue again and swipe to pay. The first Continue stays off confirmation. After the marker is cleared, Continue opens confirmation and swipe still starts the payment without force-stopping the app.
- Android. Reach coin selection, set the marker, tap Continue, then clear the marker and continue that send. Coin selection and the later confirm swipe still work.
- Android. Start a Lightning amount Continue so fee estimation is in progress, and replace the wallet before it finishes. Open a new send on the replacement wallet and tap Continue. That Continue still opens confirmation.
Findings
- [HIGH] Recovery rejection stops all later send actions — inline at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:6962. - [LOW] Send entry reads restore markers on the main thread — inline at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7103.
Reviewer model: grok-4.7
Reasoning effort: high
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Code scan at 258f8a9: no findings.
Checked and clean:
- The fallback doesn't widen beyond the two recognised 502 responses.
- Cancellation doesn't leak.
- The fee path is keyed by network and wallet.
- The test endpoint can't reach a release build: it needs the debug build type,
isDebug && isE2eTest, and a localhost URL. - Under a 502, Confirm and the broadcast both resolve to the saved schedule.
The saved schedule has no age limit. That isn't a finding here, because master already falls back to it with no age limit when the fresh read is null, and iOS does the same on any error.
Out of scope, both pre-existing:
AppViewModel.sendOnchainnever passesfeeRates, so the broadcast fetches rates again and can pay a different fee from the one Confirm showed if rates move in between.- The fallback covers 502 only, so DNS, connect, timeout, 503 and 504 errors during the Blocktank wind-down still fail an ordinary send even when a saved schedule exists.
The PR body has no #### Journeys section. The 502 cases need the local fixture, so list them under Manual Tests.
Device gate (partial): 258f8a9, not run yet. Pending: an on-chain send on a regtest build pointed at the 502 test endpoint (Confirm shows the saved fee, and the broadcast uses it), plus a send with the endpoint healthy.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Full reassessment of the entire PR diff against merge base 825bb3a9, at 6bc0821e. The merge base changed and no coverage baseline was supplied, so inherited coverage is empty.
No new actionable code findings.
An on-chain send reuses the saved Blocktank schedule only when fresh info fails with one of the two native HTTP 502 texts from bitkit-core 0.8.0-rc11 and rust-blocktank-client 0.0.16. The selected speed is taken from that schedule at send time. A missing cache, cancellation, an unauthorized or malformed response, and any other HTTP error still fail before the cache is read. Payment links, scans, and request preparation keep the wallet generation captured at entry, including across an unauthenticated deferral, and stop before the payment-specific fee lookup when a PAYKIT or USDT restore marker is set or unreadable.
iOS FeeEstimatesManager at 3d16b620 keeps its last in-memory schedule after any fetch error. This PR limits Android to the two 502 responses above. bitkit-android#1685 at 825bb3a9 was checked only for the wipe generation this PR observes.
Validation: Hosted CI, including testDevDebugUnitTest, and Lint succeeded on this revision. Detekt succeeded. E2E failed on this revision: the send shard passed; @backup_1 and @widgets_1 through @widgets_3 did not find "Yes, Delete". Those logs were inspected and not re-run. The new unit tests were read and not executed locally.
Device testing: not performed in this review.
Suggested additional test cases
- Android dev/regtest. Prime a real fee cache, force Blocktank info to return HTTP 500, and repeat with HTTP 401. Open a 10,000 sat on-chain send and confirm. Info refresh can still show the saved fee, because it keeps the last cache for every error. Confirm must not broadcast and must not spend at that cached rate.
Earlier findings reconciled at this revision: 2 still present.
Findings
- [HIGH] Recovery rejection stops all later send actions — now at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:6962. Still present from the earlier review. - [LOW] Send entry reads restore markers on the main thread — now at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7103. Still present from the earlier review.
Reviewer model: grok-4.7
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. The blocking marker reads remain deferred to synonymdev/bitkit-android#1719.
QA for bfa61d3:
Android 16, disposable regtest wallets and owned recipient. The test build adds debug controls for the real recovery marker; the wallet-replacement case also holds the Lightning fee result in a local test hook. The send-event handler is unchanged. A local endpoint returned the selected HTTP faults after a successful upstream fee refresh.
Test 1: cached fees after gateway errors ✅ Six 10,000-sat transfers reached the recipient under HTML and JSON 502 responses at all three selected speeds.
For each response form, Fast paid 422 sats and Normal and Slow paid 141 sats. The saved schedule was 3, 1 and 1 sat/vB, respectively. Confirm showed those fees; the regtest mempool entries confirmed them and the recipient wallet confirmed each amount. Recordings are 2×.
HTML: Fast, Normal, Slow:
test1a-redo-2x.2x.mp4
test1b-2x.2x.mp4
test1c-final-2x.2x.mp4
JSON: Fast, Normal, Slow:
test1e-redo-2x.2x.mp4
test1d-2x.2x.mp4
test1f-2x.2x.mp4
Test 2: amount Continue after recovery refusal ✅ Clearing the required marker allowed Continue and a successful payment without restarting.
The debug control set and cleared the actual WALLET marker while the amount screen was open. The first Continue stayed on that screen; the fresh Continue reached Confirm and swipe sent 1,000 sats.
test4-pay-2x.2x.mp4
Test 3: coin selection after recovery refusal ✅ Clearing the required marker allowed coin selection to continue and the payment to be sent.
The debug control set and cleared the actual WALLET marker while coin selection was open. The first Continue stayed on selection; the fresh Continue reached Confirm and swipe sent 1,000 sats.
test6-pay-2x.2x.mp4
Test 4: replacement during Lightning fee estimation ✅ The stale result did not open Confirm, and a fresh send on the replacement wallet reached Confirm in the same process.
A local hook held the Lightning fee result after Continue. The funded wallet was wiped and a new wallet created before the result was released. The process ID and changed wallet address were checked through the debug control; the old amount screen did not advance. The new wallet was funded and a fresh 1,000-sat on-chain send reached Confirm; it was cancelled without broadcast. The hook supplies the held fee result, so this checks wallet ownership and collector behavior rather than Lightning fee calculation.
test7-2x.2x.mp4
test7-fresh-2x.2x.mp4
Test 5: non-502 errors with a saved schedule ✅ HTTP 500 and 401 each stopped the send before broadcast.
After a successful fee refresh, each 10,000-sat send reached Confirm and was swiped. The app's fee-lookup logs showed the respective error; the regtest transaction pool remained empty after each attempt.
test8-500-2x.2x.mp4
test8-401-2x.2x.mp4
Check 1: build and send-event tests ✅ The unmodified head built and the affected send-flow suite passed.
The suite covers unreadable and pending WALLET/USDT markers, wallet generation changes during held fee estimation, and cancellation of the owning scope.
The author's final-head report covers the authenticated restore and payment-link case. The earlier fresh-cache and malformed-response controls in that report exercise the unchanged fee-fallback path. The final-head E2E run passed its send shard. The earlier review documented the collector failure reproduced before this fix.
Reviewed the delta from 258f8a9 to bfa61d3 and the earlier open findings. Parent-only changes in the delta were read as integration context; the device cases above focus on fee fallback and send-event recovery.
No matching iOS twin PR was identified. The iOS fee manager retains its published estimate after a fetch error, while Android reuses saved fees only for the two recognized 502 forms.
Reviewer model: gpt-6.1-terra
Reasoning effort: high
Reviewed by gpt-6.1-terra-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Re-reviewed to bfa61d3. No new findings. The per-event catch (CancellationException) { ensureActive() } keeps the send-event collector alive while still rethrowing a real scope cancellation, and onAmountContinue / onConfirmPay still reset loading in finally.
Device gate (partial): bfa61d3, not run yet. Pending: an on-chain send against the 502 test endpoint (Confirm and the broadcast both use the saved schedule), and a send with the endpoint healthy.
jvsena42
left a comment
jvsena42
left a comment
There was a problem hiding this comment.
Approving. The code scan had no findings, and the device gate passed.
Device gate: bfa61d3, Gate_Throwaway emulator. I made an e2e debug build of this head pointed at a local stub on E2E_BLOCKTANK_API_URL that proxies to staging. In proxyfee mode the stub serves 7/5/2 sat/vB, so the saved schedule can be told apart from staging's live 3/1/1. Wallet: fresh, funded with 200 000 sat on regtest.
- HTML 502 on
/info: Confirm showedNormal (₿ 703), which is the saved 5 sat/vB, and the broadcast used it.07:57:21.896 WARN [CoreService.kt:2182] Failed to fetch fresh fee rate, using cached rate. 07:57:23.045 DEBUG [LightningRepo.kt:1558] sendOnChain: sats=10000, isTransfer=false, isMaxAmount=false, satsPerVByte=5 07:57:23.431 INFO [AppViewModel.kt:6021] Accepted on-chain send '1e89f519…7875' - JSON 502: Confirm used the saved schedule and logged the warning. I cancelled before broadcast.
- Healthy stub: Confirm showed
Normal (₿ 141)at the live 1 sat/vB, with no saved-rate warning, and the send went out (e75c894b…ccb3).
Perf: not relevant to this PR's own change.
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full pull request at bfa61d31 after no verified baseline coverage was supplied. The comparison base and merge base are both 2986fde9.
No new actionable code findings.
Info HTTP 502 from Blocktank get_info falls back to the saved fee schedule for the HTML and JSON body shapes produced by bitkit-core 0.8.0-rc11 and rust-blocktank-client 0.0.16. Cancellation, unauthorized responses, malformed data, unrelated HTTP failures, and a missing cache still fail. The broadcast uses that cached schedule for the selected speed through getFeeRateForSpeed. Payment links, scans, and request preparation keep the original wallet generation from bitkit-android#1685 and drop late presentation when that generation or a required WALLET/USDT restore marker no longer matches. A refused send event leaves the collector active; cancelling the owning scope still stops it. The debug end-to-end Blocktank URL accepts only an explicit localhost port and path, and non-debug builds leave it empty.
The new generation readers match the parent wipe and admission rules at 2986fde9. That check covers this contract only, not the rest of #1685. Restore-marker reads still run on the send collector's caller thread; that follow-up remains #1719, and this pass did not measure a stall.
Validation: source inspection of the pinned diff, callers, and the core/client error text. This review did not run the unit suite or a device. Hosted checks on this commit include a successful build, lint, detekt, and end-to-end. Migration jobs in 40562290865 were skipped. Those hosted results were not re-executed here, and the end-to-end run does not exercise the gateway-502 fee path.
The current iOS fee-fallback source was not compared in this pass.
Device testing: not performed in this review.
Ready for device testing.
Earlier findings reconciled at this revision: 1 still present, 1 verified fixed.
Findings
- [LOW] Send entry reads restore markers on the main thread — now at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7111. Still present from the earlier review.
Reviewer model: grok-4.7
Reasoning effort: high
piotr-iohk
left a comment
piotr-iohk
left a comment
There was a problem hiding this comment.
QA review
Scope: Re-reviewed the full PR at 0b963ef9 against its merge base 5b4674c3, because the comparison base moved from 2986fde9 to the current #1685 head and that change touches files this PR also edits. This is a full reassessment and no earlier coverage was carried over.
No new actionable code findings.
- Fee fallback:
BlocktankService.getFees()uses the saved schedule only when the error chain is aBlocktankException.DataExceptionwith one of the two recognized info-502 messages, and only when a saved schedule exists. Cancellation, other typed errors, unrelated text containing "502" and a missing cache still fail as before. The send flow passes its storedfeeRatesthrough, so the speed the user selected is kept. Other callers that look up fees without stored rates (for example boost viagetFeeRateForSpeed) now get the same fallback. - Wallet-generation gating:
WalletConnectionRecovery.generationchanges only when the wallet is wiped. Everyday reconnects therefore cannot invalidate scans, links or payment requests that are being prepared. The latest commit passes the captured generation through payment-request finalization, so a default argument no longer picks up the replacement wallet's generation. When preparation is refused,observeSendEventsswallows only the refusal'sCancellationExceptionand stops if its own scope is cancelled. The payment-request preparation sheet is still cleared by the existingfinallyblock. - iOS parity: at bitkit-ios 25f8225b (source only), iOS falls back to cached estimates after any fresh-fetch failure. Android is deliberately narrower and only handles the two 502 forms, as the PR describes.
Validation: I did not run tests locally because no Android SDK was available. Hosted CI 40721227903 (unit tests), Lint 40721227979 and E2E 40721228502 all passed at this head. I read the new unit tests; I did not run them.
Limitations: I could not check the two matched error prefixes against the source of the Blocktank client (version 0.0.16, used by bitkit-core v0.8.0-rc11). The crates.io download was refused, and the public client repo only has an older version (0.0.12) with a different message format. The match is supported only by the author's device report and the unit tests. Blocking keychain reads in the send-entry restore check are out of scope as stated and tracked in #1719.
Device testing: not performed in this review.
Suggested additional test cases
- Android, regtest: build a saved fee schedule with a successful Blocktank info fetch, then make Blocktank info return the HTML 502. Open Boost on an unconfirmed outgoing on-chain transaction. Expected: the boost fee uses the saved Fast rate and the sheet does not show a fee-unavailable error.
- Android, same setup: make Blocktank info return 503 or 504 and attempt a normal on-chain send. Expected: fee preparation fails with no broadcast. Only the two recognized 502 forms may use the saved schedule.
Earlier findings reconciled at this revision: 1 still present.
Findings
- [LOW] Send entry reads restore markers on the main thread — now at
app/src/main/java/to/bitkit/viewmodels/AppViewModel.kt:7099. Still present from the earlier review.
Reviewer model: claude-opus-5-5
Reasoning effort: high
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. Blocking restore-marker reads remain deferred to synonymdev/bitkit-android#1719.
QA for 0b963ef:
Android 16, disposable regtest wallets and an owned recipient. I built the head and matched the installed APK to the build. The device build adds a debug-only control for capturing wallet ownership and invoking the payment-scan handoff; the production send and admission code is unchanged.
Test 1: cached fees after gateway errors ✅ Six 10,000-sat transfers confirmed under HTML and JSON info-502 responses at all three speeds.
I primed the native cache with 7/5/2 sat/vB, then verified actual 502 traffic. Confirm showed Fast/Normal/Slow fees of 984/703/281 sats for both response forms. Node logs and the six confirmed transaction records independently matched those fees and each 10,000-sat recipient output. The integrated sendOnchain path passes the rates shown on Confirm into the broadcast.
HTML 502, recordings at 2×:
| Fast | Normal | Slow |
|---|---|---|
test1a-final-2x.2x.mp4 |
test1b-2x.2x.mp4 |
test1c-2x.2x.mp4 |
JSON 502, recordings at 2×:
| Fast | Normal | Slow |
|---|---|---|
test1d-2x.2x.mp4 |
test1e-2x.2x.mp4 |
test1f-2x.2x.mp4 |
Test 9: original owner at the scan handoff ✅ The old wallet owner was refused after replacement, while a fresh owner opened Bitcoin Amount in the same process.
The debug control captured the real wallet generation before an actual wipe and wallet creation. Calling openContactPayment with that captured owner left Home visible; calling it with the replacement owner opened the amount screen. The control receipts verified the changed wallet address, generation and unchanged process. No payment was submitted in this case.
This drives the public handoff's owner admission. The proof-sync and finalization races themselves are covered by the committed unit regressions below; this is not an authenticated Paykit-resolution journey.
Recordings at 2×:
| Wallet replacement | Old and fresh handoffs |
|---|---|
test9a-final-2x.2x.mp4 |
test9b-final-2x.2x.mp4 |
Test 10: Boost after gateway errors ✅ Boost used the saved Fast rate after both HTML and JSON 502 responses.
I created a real unconfirmed 10,000-sat outgoing transaction at 2 sat/vB, then opened Boost under each response form. It offered 7 sat/vB from the saved schedule. Native helper logs confirmed cache fallback on each lookup. I cancelled both boosts without submitting a replacement.
Recordings at 2×; the HTML recording includes creation of the unconfirmed transaction:
| HTML 502 | JSON 502 |
|---|---|
test10-html-2x.2x.mp4 |
test10-json-2x.2x.mp4 |
Test 11: HTTP 503 and 504 controls ✅ Fresh Boost lookups rejected both errors; ordinary sends kept the rates shown on Confirm.
Under each response, Boost dismissed with “Boost Failed” and native fee-lookup errors. An ordinary 10,000-sat send then showed Normal/703 sats and broadcast at exactly that fee; transaction records verified each recipient output and fee.
The ordinary-send expectation in the latest review is superseded by the inherited change:
- bitkit-android#1724 fix: send at the fee rates confirm showed
The broadcast receives the rates already shown on Confirm. These ordinary sends therefore do not make a fresh fee lookup at broadcast; the Boost controls verify that the helper still rejects 503/504.
Recordings at 2×, each showing rejected Boost preparation followed by the ordinary send:
| HTTP 503 | HTTP 504 |
|---|---|
test11-503-final-2x.2x.mp4 |
test11-504-final-2x.2x.mp4 |
Check 1: head build and affected unit suites ✅ The unmodified head built and all five affected suites passed.
Ran assembleDevDebug and targeted testDevDebugUnitTest for AppViewModelSendFlowTest, BlocktankFeeFallbackTest, WalletConnectionRecoveryTest, BlocktankTestEndpointTest and LightningRepoTest. Coverage includes replacement during proof synchronization and request finalization, required/unreadable restore markers, collector survival and genuine scope cancellation.
Check 2: earlier acceptance ✅ Passing reports cover the unchanged recovery controls.
I relied on the earlier device review at bfa61d3 for Amount and Coin Selection recovery after a refused action, and replacement during Lightning fee estimation. The author's QA notes retain passing missing-cache, malformed-response and authenticated required-restoration controls at their recorded revisions. Those cases were not re-driven here.
The earlier HTTP 500/401 swipe results are not reused: the broadcast now receives the reviewed fee rates. The narrow fee-lookup error controls were rechecked in the unit suites.
Reviewed the delta from bfa61d3 to 0b963ef and earlier open findings. The ownership fix carries the original wallet owner to scan admission through request finalization and both initial-subscription handoffs. The incoming parent changes were checked as integration context; subscription acceptance/retry has no new device result in this pass.
Twin: no matching fee-fallback PR is linked. The iOS fee manager retains its last published schedule after a fetch error; Android's fee helper recognizes the two specific 502 forms.
Reviewed by gpt-6.1-sol-high
The base branch was changed.
|
Retargeted this PR to master before merging its base, #1685, so deleting that branch would not close it. If the base change dismissed the approval, it needs a re-approval. |
There was a problem hiding this comment.
Verdict: ✅ Approve
Findings: No new findings. Blocking restore-marker reads remain deferred to bitkit-android#1719 Move send-entry restore-marker reads off the main thread.
QA for cce1c8d:
Test 1: Bitcoin confirmation ✅ Under HTML info-502, Confirm showed 10,000 sats and the saved Normal fee of 703 sats; Back returned Home with the balance unchanged.
Android 16, disposable regtest wallet. I built the clean head with the committed debug endpoint override and matched the installed APK to the build. The native cache was primed with 7/5/2 sat/vB. Endpoint logs verified HTTP 502 responses, and native logs confirmed saved-fee fallback. The recording shows confirmation details and cancellation back to the unchanged 200,000-sat balance. No payment was submitted.
confirm-html-2x.2x.mp4
Confirmation and cancellation, recording at 2×.
Check 1: build and affected regressions ✅ The unmodified head built and all five affected unit suites passed.
Ran assembleDevDebug and targeted testDevDebugUnitTest for AppViewModelSendFlowTest, BlocktankFeeFallbackTest, WalletConnectionRecoveryTest, BlocktankTestEndpointTest and LightningRepoTest. This includes the fixed-USDT and subscription confirmation checks: replacement during the sheet transition cannot present the old payment. Proof synchronization, request finalization, required/unreadable recovery markers, collector survival and genuine scope cancellation also passed.
Check 2: prior acceptance ✅ Earlier passing reports cover the unchanged fee and recovery paths.
I checked implementation and dependency identity before reusing:
- The previous review at 0b963ef: six HTML/JSON 502 transfers at Fast/Normal/Slow, original scan-owner admission, Boost fallback and HTTP 503/504 controls.
- The device review at bfa61d3: Amount and Coin Selection after recovery refusal, plus wallet replacement during Lightning fee estimation.
- The author's revision-scoped QA notes: missing-cache, malformed/unauthorized responses and authenticated required-restoration controls, including both-pending and WALLET-only admission.
Those cases were not re-driven. The changed ordinary confirmation UI was checked above. The earlier HTTP 500/401 ordinary-send rejection expectation remains superseded by the confirmed-rate send implementation; the narrow fee-lookup error controls passed in the unit suites.
Reviewed the contiguous 0b963ef..cce1c8d delta and earlier open findings. The master merge preserves fixed Bitcoin/USDT and subscription confirmation routing while carrying the original wallet generation through both confirmation branches and delayed presentation. Other incoming master changes were checked as integration context; authenticated Paykit request/subscription and complete VSS restoration journeys have no new device result in this pass.
Twin: no matching fee-fallback PR is linked. The iOS fee manager at 85e9d4c retains its published schedule after a refresh error; Android deliberately recognizes only the two typed info-502 forms.
Reviewed by gpt-6.1-sol-high
|
@ben-kaufman Merging this PR on the approvals of head cce1c8d, with ovi-reviewer's device run on that head, checks green and no open threads. #1725 is retargeted to master first. |
|
@ben-kaufman Merged, as it was approved and green. |
Parent #1685 merged; this PR now targets master.
Description
Ordinary Bitcoin sending could fail when Blocktank info returned HTTP 502 even though a fee schedule was already saved. Use that existing schedule for the two recognized native HTML/JSON info-502 responses, preserving the selected speed. Missing cache, cancellation, unauthorized responses, malformed data and unrelated errors still fail.
Payment links and scan/request preparation retain their original wallet generation across suspension and UI dispatch. Required or unreadable WALLET/USDT recovery blocks preparation before its payment-specific fee lookup. Late work cannot present a payment, report stale success or continue under a replacement wallet. A refused or superseded send event now leaves the collector active for a later fresh action; cancellation of the owning scope still stops it.
No default fee, cache-lifetime change, general HTTP retry, payment replay, backup-format change or contact CAS is introduced. iOS already attempts its existing saved schedule after fresh lookup failure.
Out of Scope
QA Notes
Final head:
cce1c8d3d9148de9703ca3c708b104279de67456, on livemaster2622c040cc80b234443e668435991267947aaf10. Parent #1685 merged externally; the signed additive master merge resolves the actual request-confirmation conflict while retaining the original wallet generation and upstream fixed-amount routing. Combined tree:d5b6f5d3e8af8e0e400e074eca95ae9e623a968a. All ten current-base commits verify locally and on GitHub (verified=true,reason=valid); no history rewrite.Final combined validation — October 28
Required
just format,just compile,just testandjust lintpassed: 4,663 tests / 256 suites, zero failures/errors/skips, zero format/Detekt/Android Lint findings. Complete logs, XML/SARIF/HTML and embedded Gradle diagnostics were inspected once. All 1,757 frozen entries match. The 562 send-flow cases include a new subscription confirmation case proving a replacement wallet cannot receive a delayed old confirmation. Independent affected source review is clean; unchanged ownership/fee coverage is reused by verified identity.The final arm64 APK
63b3a093545914358d93be12284136f7403cc3dcaefe8e47ba51dd2808497c76was installed over the owned disposable regtest wallet without clearing data, and its installed bytes match. The changed ordinary confirmation UI was exercised: a 10,000-sat payment reached Normal / 562 sats under actual HTML502, then Back cancelled to Home with the unchanged 936,628-sat balance and zero observed broadcasts. An independent evidence inspection verified the pictures, native public log evidence, all 2,357 APK member CRCs/hashes and the exact fee-control events. The new lane was returned healthy/accept and its emulator shut down with data retained.Initial setup failures are retained: the restarted emulator lacked its prior Electrum port mapping and refused node startup until that exact mapping was restored; Argent's isolated-device tap service required the documented adb fallback; a fresh XML dump exited 137, so no XML is credited; a control-state read used the wrong header once and was corrected read-only. These are not product failures or hidden passes.
Manual 3's negative restore phases remain at prior combined APK
379014fb…/ treefbf7c62c…: real native authentication and owned VSS/PostgreSQL rejected the exact USDT listing, retaining both required markers and blocking a Bitcoin link. Retry plus a held WALLET read exposed genuine WALLET-only pending after the empty-USDT listing was acknowledged; a second link stayed blocked. Releasing reads acknowledged WALLET and reached Home without payment replay. The guard, restore, scan and fee paths relevant to these phases are byte-identical; only the shared ordinary confirmation positive control was repeated on the new binary. The earlier audit inspected all 801 VSS / 113 fee events, retained ten intentional rejections, six held reads and two post-upstream-200 BrokenPipe delivery errors. No faulted mutation was replayed.This is an affirmative empty-USDT-listing restore, not a nonempty import or complete-WALLET redesign test. Manual 1's six settled sends and Manual 2's cold-cache/error controls remain scoped to their recorded binaries and unchanged fee/native implementation; no new six-send campaign or signing/broadcast claim is made. Three JVM and four actual-base-identical provider cursor notices remain documented.
Ready status remains verified (draft=false). Current-head CI 40755558405 passed 4,663 tests / 256 suites, zero failures/errors/skips; lint 40755558440 has zero findings. E2E 40755559443 completed with 39 eventual passes and one explicit settings skip. Two initial failures remain recorded: backup_1 could not find PriceWidget before the backup/restore portion, and multi_address_3 could not find Sent after SendSuccess/Close but before transaction details/RBF. Both passed the workflow's automatic second attempt; eight already-passed cases were marker-skipped. No workflow rerun was requested and no cause or first-attempt-clean claim is made. All complete logs, attempts, annotations and digest/CRC-verified artifacts were inspected. Synthetic merge
c3d93e3bhas the exact final head tree against master2622c040. Failure-only videos/app logs were not retained after eventual success. Migration remained skipped. Priorde28c103CI 40745891791 passed 4,661 tests, lint 40745891804 had zero findings, and E2E 40745892403 passed 39 first-attempt cases with one disabled settings skip, no failed executions. All completed logs, annotations and verified artifacts were inspected. That executed merge was based on old parentc747cc0b; it is not current-master validation. Older failed runs below remain preserved.The separate #1719 issue remains deferred. External review 5686754597 reports additional old-head cases; its raw media/transaction artifacts were not independently inspected here and add no author acceptance credit.
Setup
The dev/regtest app uses Core 0.8.0-rc11, Paykit 0.1.1-rc2 and VSS 0.5.26-rc1. Acceptance used disposable owned wallets and synthetic Bitcoin regtest funds, a real native node, owned recipient, authenticated native VSS/RS256/PostgreSQL and narrowly selected HTTP faults. The committed debug-only Blocktank endpoint seam permits the explicit loopback fixture; no TLS verification was bypassed and these cases do not claim TLS coverage. Existing wallets and signed operations were preserved.
Earlier completed acceptance
Manual 1/2 used APK
d201a18b…; Manual 3 and its positive control used then-current APK SHA-25640b5484fffc7a4de41da9d46dae95884648c45d12c83a2527b6fd89b74272cc0, independently matched to installed bytes. The fee service, native Core and send implementation supporting reuse of the earlier six transfers are unchanged. That APK also exercised the exact parent foreground repair: two native startup failures retained ordinary wallet/menu/Settings; restoring the endpoint started the node. A visual toast count was not captured.The ordinary source genuinely had no USDT wallet or Pubky identity. Its successful authenticated zero-record USDT listing is an affirmative native result, not a fallback for failed reads. No nonempty USDT import or full-app USDT-only intermediate phase is claimed. Generic background fee-refresh traffic was distinguished from the blocked payment-specific path.
Prior validated head
258f8a9Required compile, full applicable tests, formatting, Detekt and Android Lint passed: 4,753 tests / 259 suites, zero failures/errors/skips; zero lint/format findings. All 1,777 frozen inputs match the final tree and actual APK build. Complete logs, JUnit, SARIF/XML and Gradle diagnostics were inspected. Existing JVM/Gradle and provider cursor notices remain documented against actual-base evidence.
Two consecutive independent final source reviews (9/10) are clean. A separate independent acceptance review verified the raw transaction/acknowledgement records, restoration phases, installed bytes and reuse boundaries. Every commit in the complete four-commit live-base PR range verifies locally and on GitHub (
verified=true,reason=valid).Evidence retained in
work/validation/android1701-app-acceptance/,android1701-restore-final-h/,android1701-payment-entry-final-h/,android1701-ready-publication/andwork/android1701-readiness-evidence-review-20261027.md. Controlled earlier failures, intermediate fixture/compiler/formatter failures and the parent's historical nine 502 E2E failures remain recorded. This repair does not prove all historical failures had cached rates.Hosted CI
Ready-for-review is verified (
draft=false). Prior-head258f8a9CI 40520195678 passed 4,753 tests / 259 classes with zero failures/skips; lint 40519956652 reports zero findings. The synthetic merge tree exactly matches the validated head.E2E 40520196618 failed: five shards passed and two exhausted three automatic in-job attempts. Four distinct widget-deletion setup cases account for all twelve failed executions (
Yes, Deletedid not appear); 35 executions passed and 25 skip records include 24 retry skips of already-passed cases plus settings_08. Logs and videos show the Suggestions delete target followed by Profile navigation. An owned-device control reproduced the Suggestions target under the fixed Profile header, then showed the actual delete dialog after scrolling; it was cancelled without deletion. The shared harness repair belongs to the separately authorized E2E #287 writer. Its publication and new hosted evidence remain outstanding; this failure is not waived. No workflow rerun was requested. All fourteen completed job logs, six checksum/CRC-verified artifacts and 23 annotations were inspected. Earlier draft skips remain skips.Prior combined-tree validation — head
6bc0821eAdditive merge
6bc0821eincorporates its then-current #1685 parent and its live-master composition. All 13 feature files remain byte-identical to the previously validated feature patch; incoming parent paths match the parent. Full required compile/tests/format/Detekt/Android Lint passed: 4,760 tests / 259 suites, zero failures/errors/skips, zero lint/format findings, all 1,781 frozen inputs unchanged. Complete logs, JUnit, SARIF/XML and Gradle diagnostics were inspected; the three JVM notices, four provider cursor stacks and Gradle deprecation retain their actual-parent comparisons. Two consecutive independent final composition reviews are clean. All five live-base commits are cryptographically valid locally and on GitHub.That combined APK
e0d436a0c317973c7051ded4ce56c1764cafe6432ef7b5d1e8df1eea581ce462was installed over the owned disposable restore target without clearing data and matched installed bytes. Under actual HTML502, a new 10,000-sat link reached the correct recipient and Normal/562-sat review; cancelling returned Home with the same 936,628-sat balance and six total broadcasts. The controller was restored healthy. This is an additional final-composition check, not a claim that all six earlier sends were repeated. Initial post-install package lookup and launcher-tool failures were retained; resolving the installed MainActivity launched successfully.Ready status remains verified (
draft=false). Hosted CI for that revision was newly triggered and pending complete inspection; earlier hosted results above remain revision-scoped. The shared widget-harness failure must still be closed with the separately owned repair and appropriate new CI evidence.Prior collector and parent integration validation — head
bfa61d3Full required
just format,just compile,just testandjust lintpassed on the final integrated source: 4,780 tests / 259 suites, zero failures/errors/skips; zero format, Detekt or Android Lint findings. All 1,781 frozen inputs match the final source and APK build. Complete logs, JUnit, SARIF/XML and Gradle diagnostics were inspected. Existing JVM/provider cursor notices and the Detekt Gradle deprecation were checked against retained baseline/live-parent evidence.Twelve new tests exercise the public send events on one ViewModel: required and unreadable WALLET/USDT recovery for Amount Continue and Coin Selection, fresh continuation after clearing recovery without replay, wallet replacement while fees suspend, and actual owning-scope cancellation. Before the fix, ten regression cases failed while the two scope-cancellation controls passed; all twelve and the focused 546-test set pass afterward. Two consecutive independent final reviews are clean. The five incoming parent source/test files match the parent exactly. All seven commits in the complete live-base PR range verify cryptographically locally and on GitHub (
verified=true,reason=valid).The final APK SHA-256
f5254a044d874152c97c68b50c74bd4ca303df1014810e8be50972bac2f3f337matched installed bytes on a newly created disposable restore target. Manual 3 was repeated on this final binary: real authenticated backup listing rejection retained both required markers; after retry, USDT acknowledgement and a held metadata read exposed WALLET-only pending. Payment links stayed out of payment review in both phases. Releasing reads produced both marker-deletion acknowledgements and full restore success. Clearing recovery alone did not replay a payment. A fresh 10,000-sat link and Continue then reached the correct recipient and Normal/562-sat review under HTML502, in the same PID and Activity throughout; the review was cancelled with no seventh broadcast. The restored balance remained 936,628 sats, and both fixture controls were restored healthy/accept. No existing wallet was reset, no marker was manufactured and no app hook was added.An initial fresh-install unresponsive-app dialog recovered after Wait before phrase entry; the same PID subsequently completed the full check. Its logs and initial tool/input failures are retained without a root-cause claim. The earlier final-build warm/cold HTTP500 attempts on the already-used wallet reached review and are inconclusive negative controls, not substitutes for Manual 2. Earlier six-send acceptance remains scoped to its recorded binaries; it was not repeated.
Evidence is retained in the worker's
work/child-final-a/,work/child-review/,work/restore-final/,work/device/andwork/publication/. New-head hosted runs are CI 40562290268, Lint 40562290599 and E2E 40562290547, have now completed successfully and been fully inspected. CI passed 4,780 tests, zero failures/skips; lint reports contain zero findings. E2E executed harness4c0cbef0and passed 39 cases on their first attempt, zero failed executions or retries, with one explicitsettings_08skip. All 11 E2E logs, annotations, exact app/harness revisions and published artifacts were inspected; unit-report counters and every artifact CRC/digest were verified. The current synthetic merge has the exact final head tree. Migration 40562290865 remains skipped, not passed. The parent #1685's separate WidgetSave failures remain documented and are not diagnosed or waived by this run. Ready status remains verifieddraft=false.Design
N/A — no UI changes.
Models used
gpt-6-astra(reasoning:xhigh).gpt-6-astra(reasoning:xhigh).gpt-6-astra(reasoning:xhigh),gpt-6.1-sol(reasoning:high).Earlier parent integration and request ownership — head
0b963ef9At
0b963ef9, finalization and both internal subscription handoffs carry their originally captured wallet generation into scan admission. Previously, replacement during finalization could cause the helper's default argument to capture the replacement generation and admit old request details. The normal-source before-fix test reproduced the unwanted address decode with the existing post-proof guard still present. After the fix, both isolated proof-sync/finalization regressions and all 551 send-flow tests pass. A separate earlier reusable-mock failure did not isolate this defect and is retained as fixture evidence, not product causation.Required final
just format,just compile,just testandjust lintpass: 4,625 cases / 255 suites, zero failures/errors/skips/retries; zero format, Detekt or Android Lint findings. All 1,751 frozen inputs match. The complete 9,514-line test log, 513 XML snapshots, five SARIF reports and 31 embedded Gradle reports were inspected; duplicated report snapshots are not extra test executions. Compile legitimately reused Gradle UP-TO-DATE outputs. Three JVM notices, the pinned Detekt plugin deprecation and four cursor notices from actual-base-identical test/provider code remain disclosed; no warning-free runtime claim.The combined-source review plus its affected recheck and a second independent ownership review are clean. All eight commits in the complete live-parent range verify locally and on GitHub. Ready status remains
draft=false. Current-head CI 40721227903 passed all 4,625 tests with zero failures/skips, and Lint 40721227979 reports zero findings. E2E 40721228502 passed 39 cases on the first attempt, with one explicitsettings_08skip and no failed test attempts. All complete logs, reports, annotations and digest/CRC-verified artifacts were inspected. The executed synthetic merge238cef14, against parent5b4674c3, has the exact head tree. Seven shards used harness4c0cbef0; recovered toast/permission-helper behavior and infrastructure/npm notices remain disclosed. The E2E APK is distinct from the ordinary CI APK and was verified separately. These results do not waive older failures or prove skipped cases.A newly verified #1685 recovery-presentation finding is being fixed in the parent: required download could lose the restricted screen if a brief node-initialization state was not observed. Its payment-admission guards remained active. That repair must be incorporated and validated here before requesting the next affected human review; the results above cover only
0b963ef9. Parent integration retains the typed fee fallback and required restore gates; it removes the retired createCjit path without adding a new send or fee mechanism. Earlier full-app Manual 1–3 results retain their recorded binary/revision limits; the new owner regressions use controlled unit resources, not a new authenticated app journey. No existing failed CI attempt is waived by these local results.