Repository navigation
chore: send regtest auth from env - #1284
Conversation
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
[Low impact] The PR appears safe to merge; no actionable issues were found. SummaryAdds environment-supplied credentials to Blocktank regtest calls.
No actionable issues were found. Review was based on source inspection; no build or runtime checks were run. Reviews (1) · Last reviewed commit: "chore: bump bitkit-core to 0.8.0-rc14" · Reviewed by Greptile |
talosmachina
left a comment
There was a problem hiding this comment.
No findings. Bumps bitkit-core to 0.8.0-rc14 and passes Env.blocktankRegtestAuth (Debug and E2E only) as basic auth to the four regtest calls, failing fast with a clear message on a 401. Reviewed a3a35ae, full tier. Static review: the app was not built for this review, so it is reasoned from the code, the rc14 bindings and CI.
What I checked, and 4 candidates I ruled out
Read in full: Bitkit/Constants/Env.swift (configValue, isDebug, isE2E), BlocktankService.executeWithRetry and the four regtest wrappers in CoreService.swift, ServiceQueue.background / queueError, AppError.init(error:).
Dependency: v0.8.0-rc14 resolves to 8fbf1d7, matching Package.resolved. In bindings/ios/bitkitcore.swift the only removed public signatures between rc11 and rc14 are the five regtest functions, each replaced by one with a trailing auth: String? = nil; no public enum case or init was removed, so existing call sites still compile. regtestGetPayment is not called anywhere in the app.
Ruled out
- 401 string match never fires after
ServiceQueuewraps the error: the wrap keeps the core error'slocalizedDescriptionindebugMessage, andBlocktankError.errorDescriptionisString(reflecting: self), which carrieserror_details. rc14'sregtest_requestreturnsERR_REGTEST_UNAUTHORIZED(contains401 Unauthorized) on a 401, so"\(error)"matches. The author's without-auth screenshot shows the message. - Credential reaching release builds:
blocktankRegtestAuthreturnsnilunlessisDebug || isE2E, andisE2Eisfalsein a non-DEBUG, non-E2E build. The value comes only from the environment, launch arguments or Info.plist, none of which the diff commits. - Malformed value without a colon: rc14 splits on the first
:and sends an empty password; Blocktank answers 401 and the new message is shown. No crash path. - Short-circuiting the retry loop on 401 hides transient failures: a 401 is not transient, and every other error still goes through the six retries.
CI: SwiftFormat, validate and Greptile green; Run Tests, Run Integration Tests and build-local were still pending at review time.
Merge confidence: 4/5, no test exercises the auth path and the build and unit jobs had not finished when this was read.
pwltr
left a comment
There was a problem hiding this comment.
Reviewed a3a35ae.
The regtest auth change is sound: release builds never read BLOCKTANK_REGTEST_AUTH, nothing is committed, and a 401 fails at once with a clear message.
Medium candidate — The bitkit-core bump brings untested behaviour changes the description calls additive. Bitkit.xcodeproj/project.pbxproj:1244 moves bitkit-core from 0.8.0-rc11 to 0.8.0-rc14. Besides the additive modules, that range changes paths the app already calls:
- duplicate BIP21 parameter keys are now rejected by the scanner (bitkit-core #151), which is what #909 is meant to adopt;
onchain_compose_transaction, used by hardware-wallet sends, now reuses a synced wallet and times out connections holding the wallet lock;- trezor-connect-rs is upgraded to 10.0.1;
- RBF replacement fee rates are preserved;
- unknown stored fee rates are filled on sync.
The QA notes cover only the regtest screens. Running the hardware-wallet send journey, a boost and the BIP21 scan cases on rc14, or splitting the bump from the regtest change, would settle it.
Review details
General review
Model: Claude Opus 5.5
Reasoning effort: high
|
@pwltr You're right, that range isn't additive. I corrected the description to name the five changed paths: synonymdev/bitkit-core#151, synonymdev/bitkit-core#166, synonymdev/bitkit-core#188, synonymdev/bitkit-core#192 and synonymdev/bitkit-core#187. Splitting the bump wouldn't help, because the regtest auth needs rc14. So I'll run the hardware-wallet send, a boost and the BIP21 duplicate-key scan cases on rc14 and add the results to QA Notes here. |
There was a problem hiding this comment.
Code scan at a3a35aee: no findings.
- Debug and E2E only:
Env.swift:240-243returns nil unlessisDebug || isE2E, and the environment and launch-argument lookup inconfigValueis itself inside#if DEBUG || E2E_BUILD. No Info.plist or xcconfig carries the key. - Regtest only: all four callers in
CoreService.swift(:2307-2365) guardEnv.network == .regtestbefore passing it. - Not committed, not logged:
git grepfinds only the variable name and the workflow secret reference. The new 401 branch throws the core error text, which in rc14 is a fixed constant with the URL redacted (bitkit-coreblocktank/api.rs).
The bitkit-core bump from rc11 to rc14 also reaches release paths this PR's QA doesn't cover: duplicate BIP21 keys refused (core#151), the hardware onchain_compose_transaction cache (core#188), the trezor-connect-rs 10.0.1 THP check (core#192), and RBF fee rates (core#166). Those came in with rc12/rc13, which #909 adopts; I gated that today.
rc13 → rc14 adds three things:
- the regtest
auth:parameter, and regtest requests sent from core (core#201, #203); - the Pubky Swaps module (core#182);
- two new
PaykitRouteRequestfields (providers,fee_rate_sat_vb) and aPaykitRoute.Swapcase.
Nothing on iOS master calls the route API, which is why this builds. #1191 does call it, so it will need the new fields and the .swap case once it is on rc14.
The PR body's Twin: synonymdev/bitkit-android#TWIN is a placeholder.
Device gate (partial): a3a35aee — not run yet; a regtest deposit and mine against staging, with and without BLOCKTANK_REGTEST_AUTH, is next.
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
|
Rebased onto master after #909 merged (force push to |
a3a35ae to
4b872da
Compare
|
@pwltr #909 has since moved master to |
jvsena42
left a comment
There was a problem hiding this comment.
Device gate at 4b872daa6: all three cases pass. The rebase onto master (#909) is clean: git range-diff shows 42842f89a and 0def4fea0 unchanged, and 4b872daa6 only moves the base of the core pin from rc11 to rc13, with rc14 at 8fbf1d75 as before. Approving.
| Case | Result |
|---|---|
| No variable | Deposit and Mine fail at once with no retries: "Regtest action failed: Blocktank regtest auth is missing or was rejected. Launch the app with BLOCKTANK_REGTEST_AUTH=user:password." (core: 401 Unauthorized). |
SIMCTL_CHILD_BLOCKTANK_REGTEST_AUTH set |
Deposit of 100,000 sats (txid 931471e3…7a83) and 3 blocks mined, and Savings went 200,000 → 300,000. |
| Credential in logs | 0 hits for the value, "Basic" or "authorization" in both app logs. The built Info.plist doesn't contain it. |
Device gate: 4b872daa6 — 3 cases passed (iPhone 17 Pro, staging regtest); perf: not relevant; Android: synonymdev/bitkit-android#1867 gate running.
Reviewer model: Claude Opus 5.5 (claude-opus-5-5)
Reasoning effort: not exposed
|
@pwltr Here's the follow-up on the extra rc14 runs I said I'd post. They're incomplete: the test Mac was overloaded, and the simulator UI automation kept timing out. Only Trezor emulator pairing over Bridge ran, and it passed on rc14 (synonymdev/bitkit-core#192). The hardware-wallet send, the boost and the BIP21 duplicate-key scan didn't run. Since #909 those changes come from master's rc13, not from this PR's rc13 to rc14 bump, so I'm not rerunning them here. |
Closes #1283
Twin: synonymdev/bitkit-android#1867
Dependency: synonymdev/bitkit-core#202 (released as https://github.com/synonymdev/bitkit-core/releases/tag/v0.8.0-rc14)
Staging Blocktank protects its
/regtest/*routes with HTTP basic auth, so the Settings > Blocktank Regtest actions failed with 401. This PR sends the credentials from a local variable in debug builds.Description
0.8.0-rc13to0.8.0-rc14, whose regtest calls take an optionalauth(user:password). The only other change since rc13 is new API the app does not call yet (pubky swaps as a Paykit route, feat: pubky swaps as paykit route bitkit-core#182).Env.blocktankRegtestAuthreadsBLOCKTANK_REGTEST_AUTHfrom the launch environment or config, only in Debug and E2E builds; release builds getnil.CoreServicepasses it asauthtoregtest_mine,regtest_deposit,regtest_payandregtest_close_channel. SetBLOCKTANK_REGTEST_AUTH=user:passwordin the scheme's environment, or launch withSIMCTL_CHILD_BLOCKTANK_REGTEST_AUTH. The value is never committed.journeys/README.mddocuments the variable.Design
N/A — no UI changes.
Preview
Settings > Blocktank Regtest > Deposit against staging, without and with
BLOCKTANK_REGTEST_AUTH:QA Notes
Manual Tests
BLOCKTANK_REGTEST_AUTH, Settings > Blocktank Regtest > Deposit shows the 401 auth error.Automated Checks
xcodebuild buildfor the simulator: passed with rc14 resolved from the release.swiftformat --linton the changed files: passed.Evidence
Cases 1 and 2 passed on
a3a35ae(this branch before the rebase onto master, with the same rc14 and app changes), against staging regtest on an iPhone 17 Pro simulator (iOS 27). Without the variable, Deposit failed at once with the auth message; with it, Deposit succeeded (txid2b7f4926…).