Repository navigation
fix: use saved fees after gateway errors - #1701
ben-kaufman wants to merge 4 commits into
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.
| private fun canPrepareSend(walletGeneration: Long?): Boolean { | ||
| if (!connectionRecovery.isCurrentGeneration(walletGeneration)) return false | ||
| return try { | ||
| !keychain.hasPendingWalletRestore() |
There was a problem hiding this comment.
P2: blocking keychain reads on the main thread at send entry. canPrepareSend calls keychain.hasPendingWalletRestore(), which is two Keychain.exists calls; each goes through snapshot (runBlocking { keychain.data.first() }) and String.indexed (runBlocking { db.configDao().getAll().first() }). It now runs synchronously from main-dispatcher paths: SendSheet's LaunchedEffect -> resetSendState checks it four times before the first suspension point does any work, and every showSheet/setSendEffect/mainScreenEffect with a canPresent lambda evaluates it again (up to three times per showSheet). Before this PR resetSendState only suspended on settingsStore. Each read is usually short once DataStore is warm, but it is a Room round trip under runBlocking on the UI thread, repeated per check. Reading the markers once per check via backupRepo.hasPendingWalletRestore() (already withContext(ioDispatcher)) in the suspend paths, or exposing them as a StateFlow the lambdas can read without I/O, would keep the gate and drop the blocking calls.
![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
| private suspend fun estimateLightningRoutingFeesIfNeeded( | ||
| walletGeneration: Long? = connectionRecovery.currentGeneration(), | ||
| ) { | ||
| requireCurrentPaymentPreparation(walletGeneration) |
There was a problem hiding this comment.
[MEDIUM] Keep recovery rejection local to one send action. This new check throws CancellationException directly out of the sole observeSendEvents collector, including for ordinary on-chain AmountContinue before the Lightning-only return. When a required-marker check rejects one action, the collector ends permanently. Clearing the marker and reopening/resetting send do not restart it, so later amount/address events and Continue are ignored until the activity's view model is recreated. I reproduced this on API 36 using the real marker; restarting the same wallet restores Confirm. Added regression checks also reproduce this after wallet replacement during Lightning fee estimation and after a transient unreadable marker. Preserve the gates while isolating cancellation to the individual action rather than the long-lived collector.
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
| private suspend fun estimateLightningRoutingFeesIfNeeded( | ||
| walletGeneration: Long? = connectionRecovery.currentGeneration(), | ||
| ) { | ||
| requireCurrentPaymentPreparation(walletGeneration) |
There was a problem hiding this comment.
[HIGH] Recovery rejection stops all later send actions
Trigger: an on-chain or Lightning send is already on the amount screen, a required WALLET or USDT restore marker becomes set or the wallet generation changes, and the user taps Continue. The same gate is reached from coin-selection Continue.
Mechanism: observeSendEvents collects sendEvents on one viewModelScope job started from AppViewModel init. SendEvent.AmountContinue calls onAmountContinue inside that collector. onAmountContinue then calls estimateLightningRoutingFeesIfNeeded, whose first action is requireCurrentPaymentPreparation. That throws CancellationException when canPrepareSend is false, before the Lightning pay-method return, so an on-chain continue takes this path. SendEvent.CoinSelectionContinue calls refreshOnchainFeeUi directly in the same collector, and that function now throws the same exception. The try/finally around AmountContinue clears the loading flag and does not catch the exception, so the collector is cancelled. viewModelScope does not start it again.
Consequence: later amount, address, coin-selection, and swipe-to-pay events are never handled. The next Continue sets amountContinuePending and then sendEvents.emit suspends with no collector, so that flag stays set. Clearing the restore marker or finishing wallet replacement does not restore send controls. This AppViewModel lives with MainActivity, so the send sheet stays inert until the activity is recreated.
Expected: that Continue should stay off confirmation, and later send actions should work again once the original wallet generation is current and required recovery is readable and clear.
Evidence basis: source analysis at 258f8a9. This path was not executed in this review.
| private fun canPrepareSend(walletGeneration: Long?): Boolean { | ||
| if (!connectionRecovery.isCurrentGeneration(walletGeneration)) return false | ||
| return try { | ||
| !keychain.hasPendingWalletRestore() |
There was a problem hiding this comment.
[LOW] Send entry reads restore markers on the main thread
Trigger: the user opens the send sheet, or a send route is presented, so resetSendState or a canPresent check runs.
Mechanism: canPrepareSend calls keychain.hasPendingWalletRestore(), which calls exists twice. Each exists reads snapshot with runBlocking on the keychain IO scope for DataStore, and String.indexed does another runBlocking Room read. resetSendState invokes this before its first suspend point. SendSheet calls resetSendState from LaunchedEffect. showSheet and setSendEffect evaluate canPresent on viewModelScope, which uses the main dispatcher.
Consequence: each gate check blocks the UI thread on DataStore and Room. A warm read is short. A check while restore is using the IO dispatcher stalls the frame until those reads finish. The keychain scope is the IO dispatcher, so this blocks the caller rather than deadlocking it.
Expected: the recovery gate should read the markers without runBlocking on the main thread.
Evidence basis: source analysis at 258f8a9. This path was not executed in this review.
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.
Dependency: #1685
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.
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.
QA Notes
Final head:
258f8a9cf4cbef5584f70e7460949defe9ff7535. Live parent:f9ff9d2716fa25c6d71be98582369ce16791d2e8. Both parent adoptions are additive; no history rewrite. The final parent merge preserves the exact validated tree5b8afb2441381346a9b0a7b96248154609889d49.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.
Completed acceptance
Manual 1/2 used APK
d201a18b…; Manual 3 and its positive control used final 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. The final 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.
Final validation
Required 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). Final-head CI 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. Header interception is under investigation, not a proven cause or waived failure. 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.Models used
gpt-6-astra, reasoningxhigh.