Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head a008aae12. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — Certificate failures on a correct TLS port get the "check TCP or TLS" message
Checked: Traced the error types from ElectrumProbeService.probe through LightningRepo.validateElectrumServer and restartWithElectrumServer into the view model: NetworkMismatch and ProtocolMismatch are thrown unwrapped from inside runSuspendCatching (the inner runCatching at ElectrumProbeService.kt:91-93 re-throws existing ElectrumProbeErrors as-is), and restartWithElectrumServer returns the probe failure without wrapping, so the when in toServerErrorDescriptionRes sees the concrete subclass. restartWithElectrumServer has a single caller (ElectrumConfigViewModel.connectToServer), and on master that caller's runCatching already turned a thrown settings write into an error toast, so the new Result.failure return keeps the user-facing outcome identical while fixing the stuck-loading path greptile flagged; runSuspendCatching still propagates cancellation. ElectrumServer.fromUserInput cannot throw, so dropping the view model's runCatching leaves no unguarded throw before the repository call. Toasts go through ToastEventBus, collected by the activity-scoped AppViewModel, and the success path's settings write re-seeds host/port through observeState as before.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no HIGH/MEDIUM findings at head 57b4e9415. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up on 57b4e94: the certificate case is now classified separately. One pre-existing LOW (the probe skips hostname verification) is posted as a reply on the earlier thread. Re-reviewed at 57b4e94. toTlsProbeError walks the handshake failure's cause chain (bounded at 8, cycle-safe) and maps CertificateException, CertPathValidatorException and SSLPeerUnverifiedException to the new UntrustedCertificate. On Conscrypt an untrusted or self-signed chain arrives as SSLHandshakeException -> CertificateException -> CertPathValidatorException, and an expired cert adds CertificateExpiredException (a CertificateException subclass), so both now get the certificate toast. The genuine mismatch cases keep ProtocolMismatch: a plaintext server answering the ClientHello fails with an SSLException/SSLProtocolException that has no certificate cause, and a handshake timeout is a SocketTimeoutException. TLS against a TCP port never reaches the handshake (TCP returns the plain socket at ElectrumProbeService.kt:111), so it still ends as NotElectrum as before. settings__es__server_error_certificate sits alphabetically before _description in strings.xml and is resolved through toServerErrorDescriptionRes, not hardcoded. The new probe tests exercise the new classifier and the view-model test fails on the old when (it would fall to the generic string). The rest of the PR (repository runSuspendCatching around the settings write, toast mapping for network/protocol) is unchanged by this commit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
✅ Self contained change
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The CI failure of Cause. Robolectric installs Conscrypt as the JVM's first security provider and never removes it, and unit tests share one JVM, so every test class that runs after a Robolectric one resolves It passes locally because Conscrypt ships no darwin-aarch64 native in Reproduced by running the compiled test classes on the Gradle test classpath under Linux x86_64: Fix. The TLS tests now build one The production change is unchanged and still correct: hostname verification behaved identically under SunJSSE and Conscrypt, on JDK 17 and 21, on macOS and Linux. Negative control rerun: with the Local: |
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rror-toasts # Conflicts: # journeys/README.md
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 2e36ed64b. (Posted as a comment because GitHub does not allow approving your own PR.)
Inline: LOW — Selecting TLS after typing port 50002 silently rewrites the port to 60002, so the journey's stated state does not hold
Checked: Follow-up on 5291fe2 (hostname verification), f9ce270 (tests) and 3c03cfc (journey): the earlier hostname LOW is fixed. Re-reviewed the three own commits since 57b4e94. Hostname fix (5291fe2): endpointIdentificationAlgorithm = "HTTPS" is applied through the get/set SSLParameters round trip at ElectrumProbeService.kt:125, before startHandshake() at :126, on the socket built with createSocket(plain, server.host, port, true), so the name checked is the user-typed host. On Android this reaches Conscrypt's TrustManagerImpl.checkTrusted(chain, authType, session, parameters) via RootTrustManager -> NetworkSecurityTrustManager -> getTrustedChainForServer(certs, authType, socket) (the app has no network security config), which throws CertificateException on a name mismatch; that is wrapped as SSLHandshakeException and isCertificateFailure() (:242-253) maps it to UntrustedCertificate -> settings__es__server_error_certificate, so a hostname mismatch shows the certificate toast, as the PR body states. Self-signed certs are unchanged: chain validation still fails with a CertPathValidatorException/CertificateException cause, same class as before this commit; hostname verification never turns a previously-rejected chain into an accepted one. IP hosts: the node's client is ldk-node 0.7.0-rc.66 -> electrum-client 0.24 with use-rustls-ring and ConfigBuilder::new() defaults (validate_domain = true, ldk-node/src/chain/electrum.rs:565-568), which passes the URL's host to ServerName::try_from (IP literal -> IpAddress, requires an iPAddress SAN; DNS name -> dNSName SAN, no CN fallback). Conscrypt's OkHostnameVerifier has the same IP-SAN/DNS-SAN split and no CN fallback since API 28 (minSdk), so a server the probe now rejects is one the node would reject after the restart; the paired positive test (IP SAN 127.0.0.1) shows a correctly named certificate still passes. .onion hosts over TLS never reach the TLS branch: plain.connect fails DNS resolution and reports Unreachable (:109-113), unchanged. Provider decoupling (f9ce270): HttpModule.provideSslSocketFactory returns SSLSocketFactory.getDefault(), the same factory the probe called inline before, and ElectrumProbeService is the only consumer of the binding. Tests: tlsContext pins SunJSSE for the context, key managers and trust managers and hands its socket factory to the probe, so no JVM-wide SSL state is read or written; the negative test discriminates because the only difference from the passing positive test is the SAN, so it can only fail through name checking. CI testDevDebugUnitTest passed at head 2e36ed6. Journey: every testTag it names exists at head (HeaderMenu, DrawerSettings, Tab-advanced via Tab-${tab.name.lowercase()}, ElectrumConfig, Connected, HostInput, PortInput, ElectrumProtocol on the TLS row, ConnectToHost, ResetToDefault, NavigationBack), the Auto label logic and the disabled-Connect assertion match AdvancedSettingsViewModel.electrumHost and enabled = !isLoading && hasEdited || !isConnected, the suites and cross-platform tables are updated, and no bitkit-ios twin exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jvsena42
left a comment
There was a problem hiding this comment.
Approved — no blocking HIGH/MEDIUM findings at head 8f00d51b2. (Posted as a comment because GitHub does not allow approving your own PR.)
Checked: Follow-up: the earlier journey finding is fixed. Follow-up on 8f00d51: the earlier port-rewrite LOW is fixed. ElectrumConfigViewModel.setProtocol (ElectrumConfigViewModel.kt:115-130) replaces the port with protocol.getDefaultPort() whenever it is empty or in defaultElectrumPorts (51002/50002/51001/50001), and getDefaultPort() returns 60002 for TLS on REGTEST (ElectrumServer.kt:110), so the old order rewrote 50002. The journey now selects TLS before typing host and port, verifies the field still reads 50002, and changes the port to 50001 without re-tapping a protocol row; setPort only trims, so both typed values survive. A failed connect writes no settings, so observeState does not re-seed the fields between steps. The description records the rewrite rule accurately, including the network-dependent default. No later step taps a protocol row.
|
@jvsena42 conflicts |
|
Both points are covered centrally in #1275 (comment): the conflict is the shared I will confirm the result on this PR once the merge lands and CI re-runs on the new head. |
…rror-toasts # Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 5 files.
Findings:
N/A
Audit:
Audited - no findings.
QA: emu-1 redroid container on Android 15, dev build 2.4.1 (188)
-
✅ passed: Network mismatch warning appeared; the connected server stayed unchanged.
-
✅ passed: TLS on port
50001showed the protocol warning; the connected server stayed unchanged. -
✅ passed: The unreachable endpoint showed the generic connection warning; the connected server stayed unchanged.
-
✅ passed: Every failure preserved the connected server and the Advanced row.
-
✅ passed: The valid regtest server showed the success toast and updated the connected server.
-
✅ passed: The self-signed TLS endpoint showed the certificate warning; the connected server stayed unchanged.
-
✅ passed: The CA-valid certificate reached by IP showed the certificate warning without a node restart.
Tip
Test 1 worth a journey:
- Open the menu and go to Settings
- Open Advanced and then Electrum Server
- Select TLS
- Enter electrum.blockstream.info and port 50002
- Tap Connect To Host
- Verify the network warning and unchanged connected server
Test 2 worth a journey:
- Keep TLS selected
- Change the port to 50001
- Tap Connect To Host
- Verify the protocol warning and unchanged connected server
Test 3 worth a journey:
- Enter 10.255.255.1 and port 9999
- Tap Connect To Host
- Wait for the connection timeout
- Verify the generic warning and unchanged connected server
Test 4 worth a journey:
- Verify the connected server after each failed connection
- Tap Reset To Default
- Verify the default fields and disabled Connect button
- Go back
- Verify the Electrum Server row still shows Auto
Test 5 worth a journey:
- Open Advanced and then Electrum Server
- Enter a valid regtest Electrum host and port
- Tap Connect To Host
- Verify the success toast and updated connected server
Coverage:
Unit tests: 90% - ElectrumProbeServiceTest.kt drives both TLS cases through the injected factory, so the certificate tests fail if the probe stops using it; the Hilt provider has no test of its own.
QA: 7 of 7 manual tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)
# Conflicts: # journeys/README.md
There was a problem hiding this comment.
Verdict: ✅ Approve
Reaudit: diff 1 file.
Findings:
N/A
Audit:
Awaits approval.
QA: carried
- ⏭️ skipped: tested on dc458c7, recording
- ⏭️ skipped: tested on dc458c7, recording
- ⏭️ skipped: tested on dc458c7, recording
- ⏭️ skipped: tested on dc458c7, recording
- ⏭️ skipped: tested on dc458c7, recording
- ⏭️ skipped: tested on dc458c7, recording
- ⏭️ skipped: tested on dc458c7, recording
Coverage:
QA: 7 of 7 manual tests passed
Reviewed by gpt-5.6-sol-high via gh-pr-review-loop skill
Commands: @ovi-reviewer review · test · retest · audit (author or owner)





















Fixes #1177
Related to #1275
This PR shows the specific cause when connecting to a custom Electrum server fails, instead of the same generic message for every error.
Description
SSLSocketvalidates the chain but not the name, so a server reached by IP or alias that presents a CA-valid certificate for a different name used to probe clean and was only rejected afterwards by the node's own Electrum client, which validates the domain — the user got the generic message after a full node restart. It now fails in the probe and shows the certificate message.ElectrumUpdatedToast,ElectrumErrorToast) are unchanged, so the e2e settings tests still match.Result.failurefromLightningRepo.restartWithElectrumServerwhen the settings write after a successful restart fails, so that failure reaches the toast instead of escaping the caller's coroutine.SSLSocketFactoryas a dependency instead of readingSSLSocketFactory.getDefault()inside it. Production still gets the platform default through Hilt; the change is so the TLS tests can hand the probe a factory of their own rather than swapping the JVM's default SSL configuration underneath it.Out of Scope
restartWithRgsServerstill writes its setting unguarded; not part of the issue.bitkit-ios, along with porting the new journey below.Design
Preview
Captured on a dev (regtest) build connected to the staging Electrum server. The wrong-network case uses
electrum.blockstream.info:50002over TLS, the protocol case useselectrum.blockstream.info:50001over TLS, and the unreachable case uses10.255.255.1:9999. TLS is selected before the port is typed, because selecting a protocol rewrites a port that is still a default one.QA Notes
Manual Tests
electrum.blockstream.info/50002(selecting a protocol rewrites a default port, so set it first) → Connect To Host: warning toast "This server is on a different Bitcoin network. Choose a server for the network Bitkit is using."electrum.blockstream.info/50001→ Connect To Host: warning toast "Secure connection failed. Check that the protocol (TCP or TLS) matches the server port."10.255.255.1/9999→ Connect To Host: after ~5s, warning toast "Bitkit could not establish a connection to Electrum."regression:After each failure above: "Currently connected to" still shows the previous server, and back → Electrum Server row is unchanged.regression:Electrum Server → enter a valid server for the current network → Connect To Host: success toast "Electrum Server Updated" and the connected server updates.The steps above are also written as a journey, committed in this PR as
journeys/settings/electrum-server-error-toasts.xml, that passes on this branch on a Pixel 9emulator. Its row is added to the suites table and the cross-platform table in
journeys/README.md.journeys/settings/electrum-server-error-toasts.xml
Automated Checks
ElectrumConfigViewModelTest.ktcovers the toast type, title, description, test tag and loading state for network mismatch, protocol mismatch, untrusted certificate, unreachable, not-Electrum, other failures and success.ElectrumProbeServiceTest.ktpins how a failed TLS handshake is classified: a certificate path failure or an unverified peer becomesUntrustedCertificate, while a handshake timeout or a non-TLS reply staysProtocolMismatch. It also probes a real TLS Electrum server whose certificate is trusted but issued for another name and assertsUntrustedCertificate, plus the paired case of a certificate that does name the host, which must still succeed. Reverting the hostname-verification line locally fails the first of those two and leaves the second passing. Both TLS tests build their ownSSLContextfrom the JDK's own JSSE provider and pass its socket factory to the probe, so they no longer read or write any JVM-wide SSL state: Robolectric installs Conscrypt as the JVM's first security provider and never removes it, which made the paired success case fail on Linux CI while passing on macOS.LightningRepoTest.ktpins that a failed settings write after a successful restart comes back asResult.failureinstead of throwing out of the returned result.just compile,just test,just lint.