feat: Add Electrum plugin — ElectrumX/Fulcrum backend#126
Closed
Kukks wants to merge 19 commits into
Closed
Conversation
Design for a BTCPay Server plugin that replaces NBXplorer with an ElectrumX/Fulcrum backend using the Shadow & Replace architecture.
Replace NBXplorer with an ElectrumX/Fulcrum server as the blockchain backend using the Shadow & Replace DI pattern. The plugin removes NBXplorer service registrations and re-registers Electrum-backed implementations. Key components: - ElectrumClient: Custom TCP/TLS JSON-RPC client for the Electrum protocol with auto-reconnect and subscription management - ElectrumWalletTracker: Wallet tracking engine with derivation, gap limit management, UTXO diffing, and Postgres persistence - ElectrumHttpHandler: Intercepts ExplorerClient HTTP calls and routes them to the Electrum engine, allowing BTCPayWallet to work unmodified - Shadow services replacing ExplorerClientProvider, NBXplorerListener, NBXplorerWaiters, FeeProviderFactory, SyncSummaryProvider, and NBXplorerConnectionFactory - Admin UI for server-level Electrum configuration - EF Core schema under 'electrum' Postgres schema
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Electrum plugin that replaces NBXplorer: a TCP/TLS Electrum JSON-RPC client with reconnect/subscription logic, EF Core/Postgres persistence (electrum schema), wallet/address/UTXO/tx tracking and sync, NBXplorer service replacements, hosted services, HTTP handler shim, admin UI, and migrations. Changes
Sequence Diagram(s)sequenceDiagram
participant ElectrumServer as Electrum Server
participant ElectrumClient as ElectrumClient
participant Tracker as ElectrumWalletTracker
participant DB as Postgres DB
participant Listener as ElectrumListener
participant InvoiceSvc as Invoice Service
ElectrumClient->>ElectrumServer: Connect (TCP/TLS)
ElectrumServer-->>ElectrumClient: Connection established
ElectrumClient->>ElectrumClient: Start read loop & keepalive
Tracker->>DB: Load tracked wallets/addresses
Tracker->>ElectrumClient: Subscribe scripthashes & headers
ElectrumServer->>ElectrumClient: scripthash notification
ElectrumClient->>Tracker: OnScripthashNotification(scripthash,status)
Tracker->>ElectrumClient: ScripthashGetHistory(scripthash)
ElectrumClient->>ElectrumServer: blockchain.scripthash.get_history
ElectrumServer-->>ElectrumClient: history
Tracker->>DB: Insert/update transactions & utxos
Tracker-->>Listener: NewTransactionInfo[]
loop for each matched output
Listener->>InvoiceSvc: Find invoice by address/amount
alt invoice found
Listener->>InvoiceSvc: AddPayment(...)
InvoiceSvc-->>Listener: Ack
end
end
ElectrumServer->>ElectrumClient: header notification
ElectrumClient->>Listener: OnNewBlock(height)
Listener->>Tracker: HandleNewBlockAsync(height)
Tracker->>DB: Update confirmations and utxos
sequenceDiagram
participant App as BTCPayServer App
participant Handler as ElectrumHttpHandler
participant Tracker as ElectrumWalletTracker
participant ElectrumClient as ElectrumClient
App->>Handler: GET /derivations/{strategy}/address?feature=...
Handler->>Tracker: GetNextUnusedAddressAsync(...)
Tracker->>ElectrumClient: (may query cache/Electrum)
Tracker-->>Handler: Address + KeyPath
Handler-->>App: 200 OK {address,keypath}
App->>Handler: GET /fees/{blockTarget}
Handler->>Tracker: GetFeeRateAsync(blockTarget)
Tracker->>ElectrumClient: EstimateFeeAsync(blockTarget)
ElectrumClient-->>Tracker: btc/kB
Tracker-->>Handler: FeeRate (sat/vB)
Handler-->>App: 200 OK {blockTarget,rate}
App->>Handler: POST /transactions (raw tx)
Handler->>Tracker: BroadcastAsync(rawTx)
Tracker->>ElectrumClient: TransactionBroadcastAsync(rawTx)
ElectrumClient-->>Tracker: txid
Handler-->>App: 200 OK {transactionId: txid}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (9)
Plugins/BTCPayServer.Plugins.Electrum/BTCPayServer.Plugins.Electrum.csproj (1)
4-5: Potential C# language version mismatch with target framework.The project targets
net10.0but specifiesLangVersion 12. .NET 10 ships with C# 14 as the default language version. WhileLangVersion 12will compile (it's a lower version), this prevents use of C# 13/14 features and may not be the intended configuration.If you want the latest language features for the target framework, consider removing the explicit
LangVersionor setting it to14(orlatest).Suggested fix
<PropertyGroup> <TargetFramework>net10.0</TargetFramework> - <LangVersion>12</LangVersion> + <LangVersion>latest</LangVersion> </PropertyGroup>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/BTCPayServer.Plugins.Electrum.csproj` around lines 4 - 5, The project sets TargetFramework (net10.0) but pins LangVersion to 12, preventing C# 13/14 features; update the project file to either remove the LangVersion element or set LangVersion to "14" or "latest" so the compiler can use the language features that match net10.0 (modify the <LangVersion> element in the BTCPayServer.Plugins.Electrum.csproj or remove it entirely and rely on the SDK default to ensure C# language level aligns with TargetFramework).Plugins/BTCPayServer.Plugins.Electrum/ElectrumSettings.cs (1)
3-9: Consider adding validation annotations or nullable reference type annotations.The
Serverproperty can be null or empty, which could cause issues downstream. Consider:
- Adding
requiredkeyword or[Required]attribute for properties that must have values- Using nullable reference type annotations to clarify intent
Example with validation attributes
+using System.ComponentModel.DataAnnotations; + namespace BTCPayServer.Plugins.Electrum; public class ElectrumSettings { - public string Server { get; set; } + [Required] + public string Server { get; set; } = string.Empty; public bool UseTls { get; set; } = true; + [Range(5, 100)] public int GapLimit { get; set; } = 20; public string CryptoCode { get; set; } = "BTC"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumSettings.cs` around lines 3 - 9, ElectrumSettings.Server is currently nullable which can cause downstream null/empty issues; modify the ElectrumSettings class to enforce non-null for required fields by either marking Server with the C# required keyword or annotating it with [Required], and enable nullable reference types (e.g., string? vs string) as appropriate so intent is explicit; also consider adding validation annotations (e.g., [Required], [MinLength], or custom validation) for Server and bounds validation for GapLimit, and update any consumers that construct ElectrumSettings (e.g., deserialization/code that reads Server) to satisfy the new non-null contract.Plugins/BTCPayServer.Plugins.Electrum/Views/_ViewImports.cshtml (1)
1-4: Redundant@usingfor TagHelpers namespace.Line 2 imports
Microsoft.AspNetCore.Mvc.TagHelpersvia@using, but this is typically unnecessary since Line 3 already registers tag helpers via@addTagHelper. The@usingdirective is only needed if you're referencing types from that namespace directly in code blocks, which is uncommon for tag helpers.Consider removing the redundant using
`@using` BTCPayServer.Plugins.Electrum -@using Microsoft.AspNetCore.Mvc.TagHelpers `@addTagHelper` *, Microsoft.AspNetCore.Mvc.TagHelpers `@addTagHelper` *, BTCPayServer.Abstractions🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Views/_ViewImports.cshtml` around lines 1 - 4, Remove the redundant using of the TagHelpers namespace: delete the line containing "@using Microsoft.AspNetCore.Mvc.TagHelpers" from _ViewImports.cshtml because tag helpers are already registered via the "@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers" directive; keep the existing "@addTagHelper *, BTCPayServer.Abstractions" and other using directives intact.Plugins/BTCPayServer.Plugins.Electrum/Views/Electrum/Settings.cshtml (2)
13-36: Consider adding a validation summary for server-side errors.The form handles
ViewBag.StatusMessagefor test results but doesn't include anasp-validation-summaryfor model validation errors. If server-side validation fails (e.g., invalid Server format), users won't see the specific validation messages.Add validation summary
<form method="post"> + <div asp-validation-summary="ModelOnly" class="text-danger"></div> <div class="form-group"> <label asp-for="Server" class="form-label">Electrum Server</label> <input asp-for="Server" class="form-control" placeholder="electrum.example.com:50002" /> + <span asp-validation-for="Server" class="text-danger"></span> <span class="text-muted">Host and port of the ElectrumX or Fulcrum server</span> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Views/Electrum/Settings.cshtml` around lines 13 - 36, Add a server-side validation summary to the Electrum settings form so model validation errors are visible to users: in Views/Electrum/Settings.cshtml insert an asp-validation-summary (e.g., a div with asp-validation-summary="ModelOnly" and a suitable CSS class like text-danger) near the top of the <form> (above the inputs and below any status message handling such as ViewBag.StatusMessage) and ensure the existing inputs that use asp-for (Server, UseTls, GapLimit) have corresponding asp-validation-for elements or rely on the summary to surface errors; keep the Save/Test buttons (SaveElectrumSettings id) unchanged.
38-41: Status message styling could indicate success vs. failure.The status message always uses
alert-inforegardless of whether the test succeeded or failed. Consider passing the alert type (success/danger/info) from the controller for clearer feedback.Example improvement
- `@if` (ViewBag.StatusMessage != null) + `@if` (ViewBag.StatusMessage != null) { - <div class="alert alert-info mt-3">@ViewBag.StatusMessage</div> + <div class="alert @(ViewBag.StatusClass ?? "alert-info") mt-3">@ViewBag.StatusMessage</div> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Views/Electrum/Settings.cshtml` around lines 38 - 41, The view currently always renders the status banner with class "alert-info" using ViewBag.StatusMessage; modify both the controller action that sets ViewBag.StatusMessage (e.g., the method that performs the connection/test) to also set a ViewBag.StatusType (values like "success", "danger", "info") based on test result, and update the Razor view (Settings.cshtml) to use that type when rendering the alert div (e.g., class="alert alert-@ViewBag.StatusType mt-3") so success vs failure displays with appropriate styling. Ensure ViewBag.StatusType has a safe default ("info") if not provided.docs/superpowers/plans/2026-04-06-electrum-plugin.md (1)
13-13: Heading levels should increment by one level at a time.The document jumps from
#(h1) directly to###(h3). Use##(h2) for task sections to maintain proper Markdown hierarchy.📝 Proposed fix
-### Task 1: Project Scaffolding +## Task 1: Project ScaffoldingApply the same change to all subsequent task headings (Tasks 2-8).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-06-electrum-plugin.md` at line 13, The document uses an h1 main title then jumps to h3 for task headings; update each task heading like "Task 1: Project Scaffolding" (and Tasks 2–8) from ### to ## so headings increment by one level at a time and maintain proper Markdown hierarchy throughout the file.Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumFeeProvider.cs (1)
24-24: Consider acceptingCancellationTokenparameter.The method signature doesn't include a
CancellationToken, butEstimateFeeAsyncis called withdefault. If the interface allows, propagate a token to support caller-initiated cancellation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumFeeProvider.cs` at line 24, GetFeeRateAsync lacks a CancellationToken parameter and calls EstimateFeeAsync with default; add a CancellationToken token = default parameter to the GetFeeRateAsync signature (and to the interface if applicable), pass that token into client.EstimateFeeAsync(...) instead of default, and update any call sites and interface definition (e.g., IElectrumFeeProvider.GetFeeRateAsync) so cancellation from callers is propagated to the ElectrumEstimate/EstimateFeeAsync call.Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.cs (1)
16-25: Unused_electrumClientfield.The injected
ElectrumClientis stored but never used. The test path creates its own temporary client. Either remove this dependency or use it for the test connection.♻️ Proposed fix - remove unused dependency
public class UIElectrumController : Controller { private readonly SettingsRepository _settingsRepository; - private readonly ElectrumClient _electrumClient; public UIElectrumController( - SettingsRepository settingsRepository, - ElectrumClient electrumClient) + SettingsRepository settingsRepository) { _settingsRepository = settingsRepository; - _electrumClient = electrumClient; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.cs` around lines 16 - 25, The injected ElectrumClient (_electrumClient) in UIElectrumController is stored but never used; either remove the unused field and constructor parameter or actually use it for the test connection path. To fix, update UIElectrumController by deleting the private readonly ElectrumClient _electrumClient field and removing ElectrumClient electrumClient from the constructor signature and assignment (or alternatively modify the test connection method, e.g., TestConnectionAsync or whatever method creates a temporary client, to use the injected _electrumClient instead of creating a new one), ensuring all references and unit tests are adjusted accordingly.Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs (1)
52-52: Consider using compiled staticRegexfor performance.Multiple
Regex.IsMatchandRegex.Matchcalls recreate the regex engine on every request. For a high-traffic handler, precompile these patterns asstatic readonly Regexfields withRegexOptions.Compiled.Also applies to: 63-63, 82-82, 94-94, 106-106, 120-120, 132-132, 145-145, 153-153
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs` at line 52, The handler repeatedly calls Regex.IsMatch/Regex.Match on request paths (e.g., the check if (method == HttpMethod.Post && Regex.IsMatch(path, @"/v1/cryptos/\w+/derivations/[^/]+$")) and other similar checks) which allocates and parses patterns per request; fix this by defining static readonly Regex fields (e.g., V1CryptoDerivationRegex, V1SomeOtherRegex, etc.) with RegexOptions.Compiled | RegexOptions.CultureInvariant at class scope in ElectrumHttpHandler and replace each Regex.IsMatch/Regex.Match call with the corresponding precompiledRegex.IsMatch(path) / precompiledRegex.Match(path) to improve performance and avoid per-request regex instantiation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/superpowers/specs/2026-04-06-electrum-plugin-design.md`:
- Around line 52-69: The fenced code block in the spec is missing a language tag
which triggers markdownlint; edit the fence to include a language (e.g., "text"
or "plaintext") right after the opening triple backticks so the block containing
ElectrumClient, ElectrumWalletTracker, and ElectrumDbContext is annotated and
the linter stops flagging it.
In `@Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.cs`:
- Around line 39-54: The ElectrumClient instance (testClient) can leak if
ServerVersionAsync throws because DisposeAsync is only called on the happy path;
change the creation/usage of testClient to use await using (or otherwise ensure
DisposeAsync runs in a finally) so ConnectAsync, ServerVersionAsync and
DisposeAsync are all covered — specifically update the block that constructs
ElectrumClient and calls ConnectAsync/ServerVersionAsync/DisposeAsync
(testClient, ConnectAsync, ServerVersionAsync, DisposeAsync) to use await using
or a try/finally to guarantee deterministic cleanup.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumClient.cs`:
- Around line 104-124: DisconnectAsync currently cancels the read loop but
ReadLoopAsync's finally always starts ReconnectLoopAsync, causing unwanted
reconnects during shutdown and racing ConnectAsync; fix by adding a rendezvous
flag (e.g. private volatile bool _allowReconnect or _isShuttingDown) that you
set to false in DisconnectAsync/DisposeAsync and check in ReadLoopAsync before
scheduling ReconnectLoopAsync (only call ReconnectLoopAsync when _allowReconnect
is true and _cts is not cancelled), and also ensure ReconnectLoopAsync is
re-entrant-safe (guard with a single _reconnectTask or skip if cancellation
requested) to prevent overlapping transports when ElectrumStatusMonitor calls
ConnectAsync.
- Around line 86-90: The TLS code in ElectrumClient.cs is bypassing certificate
validation by passing a custom callback to SslStream; remove the insecure
callback so SslStream uses the system/default certificate validation (i.e.,
construct SslStream(stream, false) and call AuthenticateAsClientAsync(host) on
it), and if you must support self-signed servers implement certificate pinning
or explicit validation logic instead of returning true; look for the
_settings.UseTls branch, the SslStream creation and
AuthenticateAsClientAsync(host) usage and update those lines accordingly.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`:
- Around line 870-883: The current block always uses matchedAddr for OutputInfo
even when the scriptHash came from _subscribedScripthashes, causing wrong
address/keypath attribution; update the branch in ElectrumWalletTracker.cs so
that if scriptHash is found in _subscribedScripthashes you fetch that tracked
entry (e.g., var tracked = _subscribedScripthashes[scriptHash]) and populate
OutputInfo.Address, KeyPath and compute KeyIndex from tracked.KeyPath (instead
of matchedAddr), otherwise keep using matchedAddr as before; ensure you
reference scriptHash, matchedAddr, _subscribedScripthashes and OutputInfo when
making the change so each output is populated from the actual tracked address.
- Around line 317-338: The allocation code in ElectrumWalletTracker (using
ctx.TrackedAddresses, TrackWalletAsync, IsUsed, KeyPath and the reserve flag) is
racy and orders KeyPath lexicographically; make selection-and-reserve atomic and
use numeric ordering: perform the find-and-update inside a DB transaction (or
use a single UPDATE ... WHERE WalletId==strategyStr AND IsChange==isChange AND
IsUsed==false ORDER BY parsed KeyPath LIMIT 1 RETURNING *) so the row is marked
IsUsed=true atomically (or use EF Core ExecuteSqlRaw with OUTPUT/RETURNING), and
if none found call TrackWalletAsync then retry; also replace OrderBy(a =>
a.KeyPath) with ordering by parsed numeric index (e.g., parse the key path
segment as int or use an indexed numeric column) and after reserving ensure you
extend the gap by invoking TrackWalletAsync for that wallet if reservation
consumed into the gap window.
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs`:
- Around line 56-67: The Configure<SettingsRepository> delegate is using an
async lambda which becomes fire-and-forget and allows ElectrumClient and
ElectrumWalletTracker (which consume IOptions<ElectrumSettings>) to be created
with default/incomplete values; replace the async Configure with a synchronous
initialization: call SettingsRepository.GetSettingAsync(...) and wait for it
synchronously (e.g. saved = repo.GetSettingAsync<>().GetAwaiter().GetResult())
before registering options/singletons, or alternatively resolve and load the
saved ElectrumSettings from the service provider during startup (or inside an
IHostedService) and then register a populated IOptions<ElectrumSettings> /
configure services.AddOptions<ElectrumSettings>() with the loaded values so
ElectrumSettings, ElectrumClient and ElectrumWalletTracker always receive
fully-initialized settings and no async void is used.
In
`@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.cs`:
- Around line 95-98: The NullHttpClientFactory.CreateClient implementation
currently constructs a new HttpClient per call causing potential socket
exhaustion; change NullHttpClientFactory to return a shared, long-lived
HttpClient instance instead (e.g., a static readonly HttpClient field) so
CreateClient(string name) reuses that single client; update the
NullHttpClientFactory class accordingly and ensure the shared HttpClient is
configured appropriately for reuse rather than creating a new HttpClient on
every CreateClient call.
- Around line 46-78: The ElectrumExplorerClientProvider class is hiding base
methods using the new modifier on GetExplorerClient(string),
GetExplorerClient(BTCPayNetworkBase), IsAvailable(string),
IsAvailable(BTCPayNetworkBase) and GetAll(), which is a code smell; either make
the corresponding methods in the base ExplorerClientProvider virtual and replace
new with override in ElectrumExplorerClientProvider, or stop inheriting
ExplorerClientProvider and instead implement IExplorerClientProvider directly
and remove the new methods; update method signatures in
ElectrumExplorerClientProvider (remove new, add override or implement interface
members) and adjust the DI registration in ElectrumPlugin if necessary so the
provider implementation matches the chosen approach.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumFeeProvider.cs`:
- Around line 54-60: The catch-all in the ElectrumFeeProvider.cs method is
swallowing OperationCanceledException; change the exception handling so
cancellation is respected by rethrowing OperationCanceledException (e.g.,
explicitly handle OperationCanceledException and throw) and only treat
non-cancellation errors by falling back to the
cache/_cache.TryGetValue(blockTarget, out var stale) and returning stale.Rate or
new FeeRate(1.0m); ensure references to _cache, blockTarget, stale and FeeRate
remain and that only non-cancellation exceptions are swallowed for the fallback
path.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs`:
- Around line 170-177: The catch block in ElectrumHttpHandler currently returns
ex.Message to the client which can leak sensitive info; keep the
_logger.LogError(ex, ...) as-is but change the HttpResponseMessage Content to a
generic message (e.g., "Internal server error" or "An error occurred") and
optionally set a safe content type, ensuring the response remains
HttpStatusCode.InternalServerError; locate the catch that wraps
_logger.LogError(ex, "Error handling {Method} {Path}", method, path) in
ElectrumHttpHandler (ElectrumHttpHandler.cs) and replace ex.Message with the
generic text.
- Around line 144-150: The POST handler in ElectrumHttpHandler uses
request.Content! which can throw a NullReferenceException if a POST arrives
without a body; update the block handling HttpMethod.Post and path regex to
defensively check request.Content for null before calling ReadAsStringAsync, and
if null return an appropriate error response (e.g., BadRequest indicating a
missing body) or supply an empty payload to _tracker.BroadcastAsync; reference
the request.Content usage, the _tracker.BroadcastAsync call, and the OkResponse
return so you modify the correct code path.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumListener.cs`:
- Around line 251-305: The loop is scanning all coins for the full derivation
strategy (wallet.GetUnspentCoins(strategy)) which lets the same UTXO be assigned
to every invoice that shares the xpub; limit the scan to the invoice's own
destination/keypath before creating payments. After you call promptDetails =
handler.ParsePaymentPromptDetails(prompt.Details) use the promptDetails (or
invoice/prompt) field that identifies the destination (address/script or
keypath/index) to filter the returned coins (or, if wallet.GetUnspentCoins
supports a more specific call, pass the specific keypath/index) so you only
process coins whose coin.KeyPath/coin.KeyIndex (or derived scriptPubKey/address)
matches the invoice destination, then proceed to build PaymentData and call
_paymentService.AddPayment as before.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumStatusMonitor.cs`:
- Around line 103-113: The code sets State = NBXplorerState.Ready after calling
_client.ServerFeaturesAsync(ct) but never validates the remote chain; update the
logic in ElectrumStatusMonitor to inspect the returned features (from
_client.ServerFeaturesAsync) and compare the remote chain/genesis identifier
against the configured BTCPayNetwork (e.g., BTCPayNetwork.Genesis or other
unique chain id exposed on the network object) and only set State =
NBXplorerState.Ready (and call BuildStatusResult/PublishDashboard) when they
match; if they don’t match, transition to a non-ready/error state and log the
mismatch for diagnostics.
In
`@Plugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/ElectrumSyncSummary.cshtml`:
- Line 18: The sync height display currently shows status.ChainHeight /
status.SyncHeight (target / current); update the view in
ElectrumSyncSummary.cshtml to reverse the order so it shows status.SyncHeight /
status.ChainHeight to present progress as "current / target" (swap the two
status properties in the <td> that renders the heights).
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-06-electrum-plugin.md`:
- Line 13: The document uses an h1 main title then jumps to h3 for task
headings; update each task heading like "Task 1: Project Scaffolding" (and Tasks
2–8) from ### to ## so headings increment by one level at a time and maintain
proper Markdown hierarchy throughout the file.
In `@Plugins/BTCPayServer.Plugins.Electrum/BTCPayServer.Plugins.Electrum.csproj`:
- Around line 4-5: The project sets TargetFramework (net10.0) but pins
LangVersion to 12, preventing C# 13/14 features; update the project file to
either remove the LangVersion element or set LangVersion to "14" or "latest" so
the compiler can use the language features that match net10.0 (modify the
<LangVersion> element in the BTCPayServer.Plugins.Electrum.csproj or remove it
entirely and rely on the SDK default to ensure C# language level aligns with
TargetFramework).
In `@Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.cs`:
- Around line 16-25: The injected ElectrumClient (_electrumClient) in
UIElectrumController is stored but never used; either remove the unused field
and constructor parameter or actually use it for the test connection path. To
fix, update UIElectrumController by deleting the private readonly ElectrumClient
_electrumClient field and removing ElectrumClient electrumClient from the
constructor signature and assignment (or alternatively modify the test
connection method, e.g., TestConnectionAsync or whatever method creates a
temporary client, to use the injected _electrumClient instead of creating a new
one), ensuring all references and unit tests are adjusted accordingly.
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumSettings.cs`:
- Around line 3-9: ElectrumSettings.Server is currently nullable which can cause
downstream null/empty issues; modify the ElectrumSettings class to enforce
non-null for required fields by either marking Server with the C# required
keyword or annotating it with [Required], and enable nullable reference types
(e.g., string? vs string) as appropriate so intent is explicit; also consider
adding validation annotations (e.g., [Required], [MinLength], or custom
validation) for Server and bounds validation for GapLimit, and update any
consumers that construct ElectrumSettings (e.g., deserialization/code that reads
Server) to satisfy the new non-null contract.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumFeeProvider.cs`:
- Line 24: GetFeeRateAsync lacks a CancellationToken parameter and calls
EstimateFeeAsync with default; add a CancellationToken token = default parameter
to the GetFeeRateAsync signature (and to the interface if applicable), pass that
token into client.EstimateFeeAsync(...) instead of default, and update any call
sites and interface definition (e.g., IElectrumFeeProvider.GetFeeRateAsync) so
cancellation from callers is propagated to the ElectrumEstimate/EstimateFeeAsync
call.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs`:
- Line 52: The handler repeatedly calls Regex.IsMatch/Regex.Match on request
paths (e.g., the check if (method == HttpMethod.Post && Regex.IsMatch(path,
@"/v1/cryptos/\w+/derivations/[^/]+$")) and other similar checks) which
allocates and parses patterns per request; fix this by defining static readonly
Regex fields (e.g., V1CryptoDerivationRegex, V1SomeOtherRegex, etc.) with
RegexOptions.Compiled | RegexOptions.CultureInvariant at class scope in
ElectrumHttpHandler and replace each Regex.IsMatch/Regex.Match call with the
corresponding precompiledRegex.IsMatch(path) / precompiledRegex.Match(path) to
improve performance and avoid per-request regex instantiation.
In `@Plugins/BTCPayServer.Plugins.Electrum/Views/_ViewImports.cshtml`:
- Around line 1-4: Remove the redundant using of the TagHelpers namespace:
delete the line containing "@using Microsoft.AspNetCore.Mvc.TagHelpers" from
_ViewImports.cshtml because tag helpers are already registered via the
"@addTagHelper *, Microsoft.AspNetCore.Mvc.TagHelpers" directive; keep the
existing "@addTagHelper *, BTCPayServer.Abstractions" and other using directives
intact.
In `@Plugins/BTCPayServer.Plugins.Electrum/Views/Electrum/Settings.cshtml`:
- Around line 13-36: Add a server-side validation summary to the Electrum
settings form so model validation errors are visible to users: in
Views/Electrum/Settings.cshtml insert an asp-validation-summary (e.g., a div
with asp-validation-summary="ModelOnly" and a suitable CSS class like
text-danger) near the top of the <form> (above the inputs and below any status
message handling such as ViewBag.StatusMessage) and ensure the existing inputs
that use asp-for (Server, UseTls, GapLimit) have corresponding
asp-validation-for elements or rely on the summary to surface errors; keep the
Save/Test buttons (SaveElectrumSettings id) unchanged.
- Around line 38-41: The view currently always renders the status banner with
class "alert-info" using ViewBag.StatusMessage; modify both the controller
action that sets ViewBag.StatusMessage (e.g., the method that performs the
connection/test) to also set a ViewBag.StatusType (values like "success",
"danger", "info") based on test result, and update the Razor view
(Settings.cshtml) to use that type when rendering the alert div (e.g.,
class="alert alert-@ViewBag.StatusType mt-3") so success vs failure displays
with appropriate styling. Ensure ViewBag.StatusType has a safe default ("info")
if not provided.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c4b68f15-775f-4637-a699-b2a198468622
📒 Files selected for processing (24)
Plugins/BTCPayServer.Plugins.Electrum/BTCPayServer.Plugins.Electrum.csprojPlugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.csPlugins/BTCPayServer.Plugins.Electrum/Data/ElectrumDbContext.csPlugins/BTCPayServer.Plugins.Electrum/Data/ElectrumDbContextFactory.csPlugins/BTCPayServer.Plugins.Electrum/Data/Models.csPlugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumClient.csPlugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.csPlugins/BTCPayServer.Plugins.Electrum/Electrum/ScriptHashUtility.csPlugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.csPlugins/BTCPayServer.Plugins.Electrum/ElectrumSettings.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumConnectionFactory.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumFeeProvider.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumFeeProviderFactory.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumListener.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumStatusMonitor.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumSyncSummaryProvider.csPlugins/BTCPayServer.Plugins.Electrum/Views/Electrum/Settings.cshtmlPlugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/ElectrumSyncSummary.cshtmlPlugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/NavExtension.cshtmlPlugins/BTCPayServer.Plugins.Electrum/Views/_ViewImports.cshtmldocs/superpowers/plans/2026-04-06-electrum-plugin.mddocs/superpowers/specs/2026-04-06-electrum-plugin-design.md
Comment on lines
+39
to
+54
| try | ||
| { | ||
| using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | ||
| var testClient = new ElectrumClient( | ||
| Microsoft.Extensions.Options.Options.Create(settings), | ||
| Microsoft.Extensions.Logging.Abstractions.NullLogger<ElectrumClient>.Instance); | ||
| await testClient.ConnectAsync(cts.Token); | ||
| var (sw, pv) = await testClient.ServerVersionAsync("BTCPayServer-Electrum", "1.4", cts.Token); | ||
| await testClient.DisposeAsync(); | ||
| ViewBag.StatusMessage = $"Connection successful! Server: {sw}, Protocol: {pv}"; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| ViewBag.StatusMessage = $"Connection failed: {ex.Message}"; | ||
| } | ||
| return View(settings); |
There was a problem hiding this comment.
Resource leak if ServerVersionAsync throws.
The testClient is disposed explicitly on line 47, but if an exception occurs after ConnectAsync and before DisposeAsync, the client leaks. Use await using for deterministic cleanup.
🛡️ Proposed fix
try
{
using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10));
- var testClient = new ElectrumClient(
+ await using var testClient = new ElectrumClient(
Microsoft.Extensions.Options.Options.Create(settings),
Microsoft.Extensions.Logging.Abstractions.NullLogger<ElectrumClient>.Instance);
await testClient.ConnectAsync(cts.Token);
var (sw, pv) = await testClient.ServerVersionAsync("BTCPayServer-Electrum", "1.4", cts.Token);
- await testClient.DisposeAsync();
ViewBag.StatusMessage = $"Connection successful! Server: {sw}, Protocol: {pv}";
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try | |
| { | |
| using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | |
| var testClient = new ElectrumClient( | |
| Microsoft.Extensions.Options.Options.Create(settings), | |
| Microsoft.Extensions.Logging.Abstractions.NullLogger<ElectrumClient>.Instance); | |
| await testClient.ConnectAsync(cts.Token); | |
| var (sw, pv) = await testClient.ServerVersionAsync("BTCPayServer-Electrum", "1.4", cts.Token); | |
| await testClient.DisposeAsync(); | |
| ViewBag.StatusMessage = $"Connection successful! Server: {sw}, Protocol: {pv}"; | |
| } | |
| catch (Exception ex) | |
| { | |
| ViewBag.StatusMessage = $"Connection failed: {ex.Message}"; | |
| } | |
| return View(settings); | |
| try | |
| { | |
| using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(10)); | |
| await using var testClient = new ElectrumClient( | |
| Microsoft.Extensions.Options.Options.Create(settings), | |
| Microsoft.Extensions.Logging.Abstractions.NullLogger<ElectrumClient>.Instance); | |
| await testClient.ConnectAsync(cts.Token); | |
| var (sw, pv) = await testClient.ServerVersionAsync("BTCPayServer-Electrum", "1.4", cts.Token); | |
| ViewBag.StatusMessage = $"Connection successful! Server: {sw}, Protocol: {pv}"; | |
| } | |
| catch (Exception ex) | |
| { | |
| ViewBag.StatusMessage = $"Connection failed: {ex.Message}"; | |
| } | |
| return View(settings); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.cs`
around lines 39 - 54, The ElectrumClient instance (testClient) can leak if
ServerVersionAsync throws because DisposeAsync is only called on the happy path;
change the creation/usage of testClient to use await using (or otherwise ensure
DisposeAsync runs in a finally) so ConnectAsync, ServerVersionAsync and
DisposeAsync are all covered — specifically update the block that constructs
ElectrumClient and calls ConnectAsync/ServerVersionAsync/DisposeAsync
(testClient, ConnectAsync, ServerVersionAsync, DisposeAsync) to use await using
or a try/finally to guarantee deterministic cleanup.
Comment on lines
+86
to
+90
| if (_settings.UseTls) | ||
| { | ||
| var sslStream = new SslStream(stream, false, (_, _, _, _) => true); | ||
| await sslStream.AuthenticateAsClientAsync(host); | ||
| stream = sslStream; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f "ElectrumClient.cs" --exec head -150 {}Repository: Kukks/BTCPayServerPlugins
Length of output: 4677
🏁 Script executed:
rg -i "sslstream\|certificate\|pinning\|validation" -A 2 -B 2 Plugins/BTCPayServer.Plugins.Electrum/Repository: Kukks/BTCPayServerPlugins
Length of output: 51
🏁 Script executed:
rg "AuthenticateAsClient" -A 5 -B 5 Plugins/BTCPayServer.Plugins.Electrum/Repository: Kukks/BTCPayServerPlugins
Length of output: 1126
Remove the custom TLS certificate validation callback to prevent MITM attacks.
The callback (_, _, _, _) => true accepts any certificate unconditionally, allowing an attacker to spoof the Electrum server and modify transaction history, fee estimates, or broadcast responses. Use the default validation by removing the callback:
Suggested fix
- var sslStream = new SslStream(stream, false, (_, _, _, _) => true);
+ var sslStream = new SslStream(stream, false);
await sslStream.AuthenticateAsClientAsync(host);If self-signed certificates must be supported, implement certificate pinning instead of bypassing validation entirely.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (_settings.UseTls) | |
| { | |
| var sslStream = new SslStream(stream, false, (_, _, _, _) => true); | |
| await sslStream.AuthenticateAsClientAsync(host); | |
| stream = sslStream; | |
| if (_settings.UseTls) | |
| { | |
| var sslStream = new SslStream(stream, false); | |
| await sslStream.AuthenticateAsClientAsync(host); | |
| stream = sslStream; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumClient.cs` around
lines 86 - 90, The TLS code in ElectrumClient.cs is bypassing certificate
validation by passing a custom callback to SslStream; remove the insecure
callback so SslStream uses the system/default certificate validation (i.e.,
construct SslStream(stream, false) and call AuthenticateAsClientAsync(host) on
it), and if you must support self-signed servers implement certificate pinning
or explicit validation logic instead of returning true; look for the
_settings.UseTls branch, the SslStream creation and
AuthenticateAsClientAsync(host) usage and update those lines accordingly.
Comment on lines
+104
to
+124
| public async Task DisconnectAsync() | ||
| { | ||
| IsConnected = false; | ||
| _cts?.Cancel(); | ||
|
|
||
| foreach (var kvp in _pending) | ||
| { | ||
| kvp.Value.TrySetCanceled(); | ||
| } | ||
| _pending.Clear(); | ||
|
|
||
| try { _reader?.Dispose(); } catch { } | ||
| try { _writer?.Dispose(); } catch { } | ||
| try { _stream?.Dispose(); } catch { } | ||
| try { _tcpClient?.Dispose(); } catch { } | ||
|
|
||
| if (_readLoop != null) | ||
| { | ||
| try { await _readLoop; } catch { } | ||
| } | ||
| } |
There was a problem hiding this comment.
Gate reconnects and skip them during shutdown/dispose.
DisconnectAsync()/DisposeAsync() cancel the read loop, but ReadLoopAsync() always schedules ReconnectLoopAsync() in finally. That makes intentional shutdown reconnect again, and it can also race ElectrumStatusMonitor’s own ConnectAsync() calls into overlapping transports on the same singleton.
Also applies to: 315-319, 360-399
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumClient.cs` around
lines 104 - 124, DisconnectAsync currently cancels the read loop but
ReadLoopAsync's finally always starts ReconnectLoopAsync, causing unwanted
reconnects during shutdown and racing ConnectAsync; fix by adding a rendezvous
flag (e.g. private volatile bool _allowReconnect or _isShuttingDown) that you
set to false in DisconnectAsync/DisposeAsync and check in ReadLoopAsync before
scheduling ReconnectLoopAsync (only call ReconnectLoopAsync when _allowReconnect
is true and _cts is not cancelled), and also ensure ReconnectLoopAsync is
re-entrant-safe (guard with a single _reconnectTask or skip if cancellation
requested) to prevent overlapping transports when ElectrumStatusMonitor calls
ConnectAsync.
Comment on lines
+317
to
+338
| var addr = await ctx.TrackedAddresses | ||
| .Where(a => a.WalletId == strategyStr && a.IsChange == isChange && !a.IsUsed) | ||
| .OrderBy(a => a.KeyPath) | ||
| .FirstOrDefaultAsync(ct); | ||
|
|
||
| if (addr == null) | ||
| { | ||
| // Might need to track first | ||
| await TrackWalletAsync(strategyStr, ct); | ||
| addr = await ctx.TrackedAddresses | ||
| .Where(a => a.WalletId == strategyStr && a.IsChange == isChange && !a.IsUsed) | ||
| .OrderBy(a => a.KeyPath) | ||
| .FirstOrDefaultAsync(ct); | ||
| } | ||
|
|
||
| if (addr == null) return null; | ||
|
|
||
| if (reserve) | ||
| { | ||
| addr.IsUsed = true; | ||
| await ctx.SaveChangesAsync(ct); | ||
| } |
There was a problem hiding this comment.
Make address allocation atomic.
This query/update path is outside _lock, so two concurrent callers can reserve the same “unused” address. It also consumes IsUsed without extending the gap window, so once the initial receive pool is fully reserved TrackWalletAsync() on an existing wallet adds nothing and address generation starts returning null. OrderBy(a => a.KeyPath) is also lexicographic, so 0/10 sorts before 0/2.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`
around lines 317 - 338, The allocation code in ElectrumWalletTracker (using
ctx.TrackedAddresses, TrackWalletAsync, IsUsed, KeyPath and the reserve flag) is
racy and orders KeyPath lexicographically; make selection-and-reserve atomic and
use numeric ordering: perform the find-and-update inside a DB transaction (or
use a single UPDATE ... WHERE WalletId==strategyStr AND IsChange==isChange AND
IsUsed==false ORDER BY parsed KeyPath LIMIT 1 RETURNING *) so the row is marked
IsUsed=true atomically (or use EF Core ExecuteSqlRaw with OUTPUT/RETURNING), and
if none found call TrackWalletAsync then retry; also replace OrderBy(a =>
a.KeyPath) with ordering by parsed numeric index (e.g., parse the key path
segment as int or use an indexed numeric column) and after reserving ensure you
extend the gap by invoking TrackWalletAsync for that wallet if reservation
consumed into the gap window.
Comment on lines
+170
to
+177
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "Error handling {Method} {Path}", method, path); | ||
| return new HttpResponseMessage(HttpStatusCode.InternalServerError) | ||
| { | ||
| Content = new StringContent(ex.Message) | ||
| }; | ||
| } |
There was a problem hiding this comment.
Avoid exposing internal exception messages to HTTP responses.
Returning ex.Message in the response body could leak sensitive internal details (stack traces, connection strings, etc.). Consider returning a generic error message.
🛡️ Proposed fix
catch (Exception ex)
{
_logger.LogError(ex, "Error handling {Method} {Path}", method, path);
return new HttpResponseMessage(HttpStatusCode.InternalServerError)
{
- Content = new StringContent(ex.Message)
+ Content = new StringContent("Internal server error")
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error handling {Method} {Path}", method, path); | |
| return new HttpResponseMessage(HttpStatusCode.InternalServerError) | |
| { | |
| Content = new StringContent(ex.Message) | |
| }; | |
| } | |
| catch (Exception ex) | |
| { | |
| _logger.LogError(ex, "Error handling {Method} {Path}", method, path); | |
| return new HttpResponseMessage(HttpStatusCode.InternalServerError) | |
| { | |
| Content = new StringContent("Internal server error") | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs` around
lines 170 - 177, The catch block in ElectrumHttpHandler currently returns
ex.Message to the client which can leak sensitive info; keep the
_logger.LogError(ex, ...) as-is but change the HttpResponseMessage Content to a
generic message (e.g., "Internal server error" or "An error occurred") and
optionally set a safe content type, ensuring the response remains
HttpStatusCode.InternalServerError; locate the catch that wraps
_logger.LogError(ex, "Error handling {Method} {Path}", method, path) in
ElectrumHttpHandler (ElectrumHttpHandler.cs) and replace ex.Message with the
generic text.
Comment on lines
+251
to
+305
| var invoices = await _invoiceRepository.GetMonitoredInvoices(pmi); | ||
| var paymentCount = 0; | ||
|
|
||
| foreach (var invoice in invoices) | ||
| { | ||
| try | ||
| { | ||
| var prompt = invoice.GetPaymentPrompt(pmi); | ||
| if (prompt?.Details == null) continue; | ||
|
|
||
| var promptDetails = handler.ParsePaymentPromptDetails(prompt.Details); | ||
| if (promptDetails?.AccountDerivation == null) continue; | ||
|
|
||
| var strategy = promptDetails.AccountDerivation; | ||
| var coins = await wallet.GetUnspentCoins(strategy, cancellation: ct); | ||
|
|
||
| var alreadyAccounted = invoice.GetPayments(false) | ||
| .Select(p => | ||
| { | ||
| var d = handler.ParsePaymentDetails(p.Details); | ||
| return d.Outpoint; | ||
| }).ToHashSet(); | ||
|
|
||
| foreach (var coin in coins) | ||
| { | ||
| if (alreadyAccounted.Contains(coin.OutPoint)) | ||
| continue; | ||
|
|
||
| var tx = await wallet.GetTransactionAsync(coin.OutPoint.Hash, cancellation: ct); | ||
| if (tx == null) continue; | ||
|
|
||
| wallet.InvalidateCache(strategy); | ||
|
|
||
| var paymentData = new PaymentData | ||
| { | ||
| Id = coin.OutPoint.ToString(), | ||
| InvoiceDataId = invoice.Id, | ||
| PaymentMethodId = pmi.ToString(), | ||
| Status = tx.Confirmations > 0 ? PaymentStatus.Settled : PaymentStatus.Processing, | ||
| Amount = ((Money)coin.Value).ToDecimal(MoneyUnit.BTC), | ||
| Currency = network.CryptoCode, | ||
| Created = tx.Timestamp | ||
| }; | ||
|
|
||
| var details = new BitcoinLikePaymentData( | ||
| coin.OutPoint, | ||
| tx.Transaction?.RBF ?? false, | ||
| coin.KeyPath, | ||
| coin.KeyIndex); | ||
|
|
||
| paymentData.Set(invoice, handler, details); | ||
|
|
||
| var payment = await _paymentService.AddPayment(paymentData, [coin.OutPoint.Hash.ToString()]); | ||
| if (payment != null) paymentCount++; | ||
| } |
There was a problem hiding this comment.
Scope the polling scan to the invoice’s own address/keypath.
This loop iterates invoices, but wallet.GetUnspentCoins(strategy) returns coins for the whole derivation strategy. Because the dedupe is only invoice-local, the same UTXO can be recorded on every open invoice that shares that xpub. Filter the wallet coins back down to the invoice’s destination address/keypath before creating PaymentData.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumListener.cs` around
lines 251 - 305, The loop is scanning all coins for the full derivation strategy
(wallet.GetUnspentCoins(strategy)) which lets the same UTXO be assigned to every
invoice that shares the xpub; limit the scan to the invoice's own
destination/keypath before creating payments. After you call promptDetails =
handler.ParsePaymentPromptDetails(prompt.Details) use the promptDetails (or
invoice/prompt) field that identifies the destination (address/script or
keypath/index) to filter the returned coins (or, if wallet.GetUnspentCoins
supports a more specific call, pass the specific keypath/index) so you only
process coins whose coin.KeyPath/coin.KeyIndex (or derived scriptPubKey/address)
matches the invoice destination, then proceed to build PaymentData and call
_paymentService.AddPayment as before.
Comment on lines
+103
to
+113
| try | ||
| { | ||
| await _client.PingAsync(ct); | ||
|
|
||
| var features = await _client.ServerFeaturesAsync(ct); | ||
|
|
||
| // We consider ourselves synced if connected and responding | ||
| State = NBXplorerState.Ready; | ||
|
|
||
| var status = BuildStatusResult(); | ||
| PublishDashboard(status, oldState); |
There was a problem hiding this comment.
Validate the remote chain before publishing Ready.
server.features is fetched and then ignored. If a mainnet BTCPay instance points at testnet/regtest Electrum, this still transitions to NBXplorerState.Ready, and the plugin can start tracking activity on the wrong chain. Compare the reported chain/genesis against the configured BTCPayNetwork before setting State = NBXplorerState.Ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumStatusMonitor.cs`
around lines 103 - 113, The code sets State = NBXplorerState.Ready after calling
_client.ServerFeaturesAsync(ct) but never validates the remote chain; update the
logic in ElectrumStatusMonitor to inspect the returned features (from
_client.ServerFeaturesAsync) and compare the remote chain/genesis identifier
against the configured BTCPayNetwork (e.g., BTCPayNetwork.Genesis or other
unique chain id exposed on the network object) and only set State =
NBXplorerState.Ready (and call BuildStatusResult/PublishDashboard) when they
match; if they don’t match, transition to a non-ready/error state and log the
mismatch for diagnostics.
| <span class="badge bg-warning">Disconnected</span> | ||
| } | ||
| </td> | ||
| <td>@status.ChainHeight / @status.SyncHeight</td> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the ElectrumSyncStatus class to understand property semantics
rg -n "class ElectrumSyncStatus" -A 20 --type csRepository: Kukks/BTCPayServerPlugins
Length of output: 1344
🏁 Script executed:
#!/bin/bash
# Find where ChainHeight and SyncHeight are assigned/set in ElectrumSyncStatus
rg -n "ChainHeight\s*=" -B 2 -A 1 --type cs
rg -n "SyncHeight\s*=" -B 2 -A 1 --type csRepository: Kukks/BTCPayServerPlugins
Length of output: 2998
🏁 Script executed:
#!/bin/bash
# Check the underlying Electrum status to understand the naming convention
# Look for where summary?.Status comes from
rg -n "class.*Status" -A 10 --type cs | grep -E "(ChainHeight|SyncHeight)" -B 3Repository: Kukks/BTCPayServerPlugins
Length of output: 639
Swap the height display order for clearer sync progress representation.
The current display shows ChainHeight / SyncHeight, which represents "chain tip / sync progress"—a backwards order for progress indication. Standard UX convention displays progress as "current / target". Since ChainHeight is the blockchain's tip height (target) and SyncHeight is the wallet's sync progress (current), the display should be reversed:
Suggested change
- <td>@status.ChainHeight / `@status.SyncHeight`</td>
+ <td>@status.SyncHeight / `@status.ChainHeight`</td>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <td>@status.ChainHeight / @status.SyncHeight</td> | |
| <td>@status.SyncHeight / `@status.ChainHeight`</td> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Plugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/ElectrumSyncSummary.cshtml`
at line 18, The sync height display currently shows status.ChainHeight /
status.SyncHeight (target / current); update the view in
ElectrumSyncSummary.cshtml to reverse the order so it shows status.SyncHeight /
status.ChainHeight to present progress as "current / target" (swap the two
status properties in the <td> that renders the heights).
…pHandler Network was injected but never used — remove it to fix DI validation failure at startup.
…NullRef AddScheduledTask<T> registers both a singleton and a ScheduledTask descriptor. We removed the FeeProviderFactory singleton but left the ScheduledTask, causing PeriodicTaskLauncherHostedService to resolve null and crash.
The SyncModal layout renders partials without passing a model, so they inherit the parent page's model (e.g. StoreDashboardViewModel). Using @model IEnumerable<ISyncStatus> caused a type mismatch crash. Switched to @Inject ElectrumSyncSummaryProvider pattern matching how NBXSyncSummary.cshtml works. Also registered the concrete type in DI so the inject can resolve it.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs`:
- Around line 96-98: The methods in ElectrumExplorerClientProvider are hiding
base implementations using "public new" which prevents polymorphic dispatch;
change the method declarations for GetExplorerClient, IsAvailable, and GetAll
from "public new" to "public override" so calls on ExplorerClientProvider or
Common.IExplorerClientProvider will dispatch to the Electrum implementations; if
the base class method signatures are not marked virtual/abstract, mark them
virtual/abstract in the base class so the overrides compile and keep method
signatures identical (including return types and parameters).
- Around line 142-160: Remove the bogus null IServiceProvider usage in
RemoveScheduledTask<T>: instead of calling d.ImplementationFactory(null!), build
a short-lived IServiceProvider from the IServiceCollection (e.g. var provider =
services.BuildServiceProvider()) and pass that provider into
d.ImplementationFactory(provider) to create the ScheduledTask instance,
disposing the provider when done; use a single temporary provider for the loop
(or one per outer call) so factories that require DI resolve correctly, then
check instance.PeriodicTaskType == typeof(T) and call services.Remove(d) as
before, and keep the try/catch around the factory invocation but do not pass
null to ImplementationFactory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b552d56c-4370-41fb-aab9-2bd30cad12e6
📒 Files selected for processing (1)
Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs
Comment on lines
+142
to
+160
| private static void RemoveScheduledTask<T>(IServiceCollection services) | ||
| { | ||
| var descriptors = services.Where(d => | ||
| d.ServiceType == typeof(ScheduledTask) && | ||
| d.ImplementationFactory != null).ToList(); | ||
| foreach (var d in descriptors) | ||
| { | ||
| // ScheduledTask factories capture the type in their closure. | ||
| // Instantiate to check PeriodicTaskType. | ||
| try | ||
| { | ||
| var instance = (ScheduledTask)d.ImplementationFactory(null!); | ||
| if (instance.PeriodicTaskType == typeof(T)) | ||
| services.Remove(d); | ||
| } | ||
| catch | ||
| { | ||
| // Factory might need a real provider — skip | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the ElectrumPlugin.cs file and understand the context
fd -t f "ElectrumPlugin.cs" --exec cat -n {} \;Repository: Kukks/BTCPayServerPlugins
Length of output: 9280
🏁 Script executed:
# Also search for FeeProviderFactory in the codebase to understand its implementation
rg "FeeProviderFactory" --type cs -A 10 -B 2Repository: Kukks/BTCPayServerPlugins
Length of output: 8676
🏁 Script executed:
# Check how ScheduledTask instances are registered in this file
rg "ScheduledTask" "Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs" -A 5 -B 2Repository: Kukks/BTCPayServerPlugins
Length of output: 1095
🏁 Script executed:
# Search for ScheduledTask registrations to understand typical factory patterns
rg "ScheduledTask" --type cs -B 5 -A 10 | head -100Repository: Kukks/BTCPayServerPlugins
Length of output: 6195
🏁 Script executed:
# Look for typical ImplementationFactory usage with ScheduledTask
rg "services\.Add.*ScheduledTask" --type cs -B 2 -A 8Repository: Kukks/BTCPayServerPlugins
Length of output: 51
🏁 Script executed:
# Check if there are real ScheduledTask implementations that use IServiceProvider
rg "class.*ScheduledTask" --type cs -A 20 | head -80Repository: Kukks/BTCPayServerPlugins
Length of output: 51
🌐 Web query:
Microsoft.Extensions.DependencyInjection ServiceDescriptor ImplementationFactory contract
💡 Result:
ServiceDescriptor.ImplementationFactory is the factory delegate the default Microsoft DI container invokes to create the service instance.
Contract / expectations
- Signature:
Func<IServiceProvider, object>(for non-keyed services). The container will pass anIServiceProviderthat is actually the current scope (ServiceProviderEngineScope). The factory is invoked asfactory(scope). [1], [2] - Must be non-null when used: the registration helpers (
AddSingleton/AddScoped/...overloads that accept a factory) throw ifimplementationFactoryisnull. [3] - Return value: should be an instance assignable to the registered
ServiceType. There’s no built-in null check when the factory is invoked, so returningnullis technically allowed by the container (it will flow through like any other resolved value), but it’s usually a bad idea unless you intentionally want “service resolves to null”. [2] - Caching / how often it runs (lifetime semantics):
- Transient: factory runs every resolve.
- Scoped: factory runs at most once per scope (result cached in the scope).
- Singleton: factory runs at most once for the root container (result cached at root).
This is implemented via the runtime resolver’s caching paths, which call the factory and then store the returned value in the appropriate cache. [2]
- Disposal: if the created instance is
IDisposable/IAsyncDisposable, the container will track it and dispose it when the owning scope (or root provider for singletons) is disposed. This happens right after creation (CaptureDisposable(resolved)). [2] - Keyed services note (.NET 8+): for keyed registrations,
ImplementationFactoryisnull; you useKeyedImplementationFactoryinstead (conceptually aFunc<IServiceProvider, object, object>). [1]
Common guidance implied by the contract
- Don’t assume the
IServiceProvideryou receive is the root; it’s the current scope. - Avoid resolving lots of “random” services from
spinside the factory unless that’s explicitly what you want (it becomes a service-locator style registration).
Sources
- Microsoft Learn API docs:
ServiceDescriptor(properties incl.ImplementationFactory/ keyed variants). [1] CallSiteRuntimeResolver.VisitFactoryand caching/disposal paths (source). [2]ServiceCollectionServiceExtensions.AddScoped/AddSingleton(..., Func<IServiceProvider, object>)null-guard (source). [3]
Don't invoke DI factories with null — this violates the contract and may silently fail to remove the task.
Line 154 calls ImplementationFactory(null!), but the DI factory contract requires a non-null IServiceProvider (the current scope). If the upstream factory depends on the provider to resolve its dependencies, passing null will throw, and the exception is swallowed at line 158—leaving the old FeeProviderFactory scheduled task still registered. This defeats the purpose of removal and can reintroduce the PeriodicTaskLauncherHostedService startup failure this change aims to fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs` around lines 142 -
160, Remove the bogus null IServiceProvider usage in RemoveScheduledTask<T>:
instead of calling d.ImplementationFactory(null!), build a short-lived
IServiceProvider from the IServiceCollection (e.g. var provider =
services.BuildServiceProvider()) and pass that provider into
d.ImplementationFactory(provider) to create the ScheduledTask instance,
disposing the provider when done; use a single temporary provider for the loop
(or one per outer call) so factories that require DI resolve correctly, then
check instance.PeriodicTaskType == typeof(T) and call services.Remove(d) as
before, and keep the try/catch around the factory invocation but do not pass
null to ImplementationFactory.
Razor convention looks for views in a folder matching the controller name minus the "Controller" suffix. UIElectrumController expects Views/UIElectrum/, not Views/Electrum/.
- Move nav link to server-nav section (alongside other server settings) - Use SetLayoutModel with WellKnownCategories.Server for proper layout - Add "Configure Electrum server" link in sync summary modal - Add 7 trusted public Electrum servers from Sparrow Wallet as defaults (blockstream.info, bitcoin.lu.ke, electrum.emzy.de, etc.) - Trusted server dropdown auto-fills host:port and enables TLS
… IOptions The async Configure<ElectrumSettings> delegate was silently failing because Configure expects Action, not Func<Task>. The async lambda became async void, leaving settings empty at connect time. Now ElectrumClient and ElectrumWalletTracker load settings directly from SettingsRepository, which also means settings changes take effect on reconnect without restarting the server.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs (4)
748-781: Synchronous database queries block the thread pool.
ComputeBalanceChangeexecutes synchronous LINQ queries (ToHashSet(),FirstOrDefault()) inside an async method path. These block the thread while waiting for database I/O, potentially starving the thread pool under load.♻️ Proposed fix to use async queries
- private long ComputeBalanceChange(ElectrumDbContext ctx, Transaction tx, string walletId) + private async Task<long> ComputeBalanceChange(ElectrumDbContext ctx, Transaction tx, string walletId) { // Look up which of our addresses are involved - var ourScripts = ctx.TrackedAddresses + var ourScripts = (await ctx.TrackedAddresses .Where(a => a.WalletId == walletId) .Select(a => a.ScriptPubKey) - .ToHashSet(new ByteArrayComparer()); + .ToListAsync()) + .ToHashSet(new ByteArrayComparer()); long change = 0; // Outputs to us are positive foreach (var output in tx.Outputs) { if (ourScripts.Contains(output.ScriptPubKey.ToBytes())) { change += output.Value.Satoshi; } } // Inputs from us are negative foreach (var input in tx.Inputs) { var prevOutpoint = $"{input.PrevOut.Hash}:{input.PrevOut.N}"; var ourUtxo = ctx.Utxos.Local.FirstOrDefault(u => u.Outpoint == prevOutpoint && u.WalletId == walletId) - ?? ctx.Utxos.FirstOrDefault(u => u.Outpoint == prevOutpoint && u.WalletId == walletId); + ?? await ctx.Utxos.FirstOrDefaultAsync(u => u.Outpoint == prevOutpoint && u.WalletId == walletId); if (ourUtxo != null) { change -= ourUtxo.Value; } } return change; }Update callers to await the async version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs` around lines 748 - 781, ComputeBalanceChange performs synchronous EF queries causing thread-blocking; convert it to an async method (e.g., Task<long> ComputeBalanceChangeAsync) and replace blocking calls with their EF Core async counterparts: use await ctx.TrackedAddresses.Where(...).Select(...).ToHashSetAsync(new ByteArrayComparer()) for ourScripts and replace FirstOrDefault on ctx.Utxos with await ctx.Utxos.Local.FirstOrDefaultAsync(...) or (if Local is in-memory) check Local synchronously then await ctx.Utxos.Where(...).FirstOrDefaultAsync(...) for DB lookups; update all callers to call and await ComputeBalanceChangeAsync. Ensure to import necessary async extensions and propagate async/await through the call chain.
436-441: Empty catch block hides all errors including unexpected failures.The catch block at line 438 swallows all exceptions, including potential
OperationCanceledExceptionfrom the cancellation token. Consider catching only expected exceptions (e.g.,ElectrumException) or at minimum logging the failure.♻️ Proposed fix
catch + catch (Exception ex) when (ex is not OperationCanceledException) { + _logger.LogDebug(ex, "Failed to fetch transaction {TxId} from Electrum", txId); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs` around lines 436 - 441, The empty catch in ElectrumWalletTracker (in the method that builds/returns the object where the current catch { return null; } exists) swallows all exceptions; change it to explicitly handle cancellation and either catch only expected Electrum-related exceptions or log unexpected ones: rethrow OperationCanceledException (catch OperationCanceledException and throw), then catch expected ElectrumException (or specific domain exceptions) and return null after logging, and for any other Exception log the error via the class logger (include ex) before returning null or rethrowing depending on expected behavior. Ensure you reference the ElectrumWalletTracker class and the method containing the current empty catch when making the change.
259-280: Inefficient confirmation checking: O(unconfirmed_txs × addresses) RPC calls per block.For each unconfirmed transaction, this loops through all wallet addresses and makes a
ScripthashGetHistoryAsynccall for each. With many addresses or unconfirmed transactions, this creates significant RPC overhead on every new block.Consider maintaining a reverse index (txid → scripthash) or batch-querying only the scripthashes that originated the unconfirmed transactions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs` around lines 259 - 280, The current nested loop in ElectrumWalletTracker (processing ctx.Transactions unconfirmed and calling _client.ScripthashGetHistoryAsync per address) causes O(unconfirmed × addresses) RPC calls; instead collect the set of affected wallet IDs and load all TrackedAddresses for those wallets once (use ctx.TrackedAddresses.Where(a => walletIds.Contains(a.WalletId))). Build a mapping of scripthash → addresses/WalletId and deduplicate scripthashes, then call ScripthashGetHistoryAsync only once per unique scripthash and reconcile its returned history against unconfirmed tx Txid to set tx.BlockHeight; alternatively add/maintain a persistent reverse index (txid → scripthash) at creation time to map txids to scripthashes and avoid per-block address scans.
823-827:GetHashCodethrows if byte array is shorter than 4 bytes.
BitConverter.ToInt32(obj, 0)requires at least 4 bytes. While Bitcoin script public keys are always longer, this comparer is fragile if reused elsewhere or ifobjis null.♻️ Proposed defensive fix
private class ByteArrayComparer : IEqualityComparer<byte[]> { - public bool Equals(byte[] x, byte[] y) => x.SequenceEqual(y); - public int GetHashCode(byte[] obj) => BitConverter.ToInt32(obj, 0); + public bool Equals(byte[] x, byte[] y) => + x == null ? y == null : y != null && x.SequenceEqual(y); + public int GetHashCode(byte[] obj) => + obj == null || obj.Length < 4 ? 0 : BitConverter.ToInt32(obj, 0); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs` around lines 823 - 827, ByteArrayComparer currently throws for null or arrays shorter than 4 bytes because GetHashCode uses BitConverter.ToInt32 and Equals uses SequenceEqual without null checks; update ByteArrayComparer.Equals to handle ReferenceEquals and nulls before calling SequenceEqual, and change GetHashCode to defensively handle null and short arrays (return 0 for null, and compute a safe hash by iterating bytes or using System.HashCode/xxHash over the available bytes instead of BitConverter.ToInt32) so it never reads past the array bounds.Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs (1)
162-173: String-based factory matching is fragile and could remove unintended services.Line 169 uses
factoryMethod.ToString()?.Contains(typeof(T).Name)to detect factory targets. This could match unrelated methods if their string representation happens to contain the type name (e.g., a method parameter type, a comment in the IL, etc.).💡 Consider more precise matching
If the factories follow a consistent pattern like
sp => sp.GetRequiredService<T>(), you could inspect the generic arguments of the target method instead:// For GetRequiredService<T> pattern var genericArgs = factoryMethod.GetGenericArguments(); if (genericArgs.Any(g => g == typeof(T))) { services.Remove(d); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs` around lines 162 - 173, The string-based check on d.ImplementationFactory (using factoryMethod.ToString()?.Contains(typeof(T).Name)) is fragile; instead inspect the factory delegate's Method metadata for actual generic type information and known call patterns: for the factoryMethod obtained from d.ImplementationFactory.Method, check its MethodInfo.GetGenericArguments() and/or examine factoryMethod.GetGenericMethodDefinition() and its generic parameters for a match to typeof(T), and also detect direct calls to IServiceProvider extensions like GetRequiredService by checking factoryMethod.Name/DeclaringType and its generic args; only call services.Remove(d) when a precise generic-argument match to typeof(T) (or a recognized IServiceProvider generic method targeting T) is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`:
- Around line 893-896: The reflection fallback used to access
ElectrumClient._subscribedScripthashes in ElectrumWalletTracker
(_subscribedScripthashes property) can silently return null and hide breakage;
change to use a proper public API on ElectrumClient (e.g. add a public
IReadOnlyDictionary<string,string> SubscribedScripthashes or a getter method
exposing the internal ConcurrentDictionary) and update ElectrumWalletTracker to
call that API instead of using reflection; if you cannot modify ElectrumClient,
detect reflection failure (when GetField/GetValue returns null), log an explicit
error via the plugin logger including the failure context and throw or surface
an exception so the problem isn't silently ignored.
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs`:
- Around line 78-81: ElectrumExplorerClientProvider is hiding base methods with
the new keyword causing incorrect polymorphic behavior; change each method
declaration for GetExplorerClient, IsAvailable, GetAll (the five methods
currently using new) to use override instead, and if the corresponding methods
in ExplorerClientProvider are not virtual/abstract, update those base method
signatures to virtual (or abstract) so the overrides compile and dispatch
correctly; ensure method signatures exactly match the base class (return types
and parameters) so the override is valid and run tests to confirm callers
holding ExplorerClientProvider now invoke ElectrumExplorerClientProvider
implementations.
In `@Plugins/BTCPayServer.Plugins.Electrum/Views/UIElectrum/Settings.cshtml`:
- Around line 18-24: The option tag currently renders selected="True"/"False"
because of selected="@(val == currentServer)"; change it to emit the boolean
attribute only when the values match: when iterating
ElectrumSettings.TrustedServers and building val (var val =
$"{server.Host}:{server.Port}";), render the selected attribute conditionally
(e.g., output selected="selected" or the bare selected attribute) only if val ==
currentServer, otherwise omit the attribute so only the matching option is
actually selected.
- Around line 18-25: The "Custom server..." option is never selected when using
a custom server; update the select markup so the first <option value="">Custom
server...</option> includes the selected attribute when the computed isTrusted
is false (i.e. when currentServer is not in ElectrumSettings.TrustedServers).
Keep the per-server option selection logic (option value="@val" selected="@(val
== currentServer)") for trusted entries and ensure the select element (id
"trustedServer") uses isTrusted to conditionally render selected on the
empty/custom option so the dropdown shows "Custom server..." for non-trusted
currentServer values.
---
Nitpick comments:
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`:
- Around line 748-781: ComputeBalanceChange performs synchronous EF queries
causing thread-blocking; convert it to an async method (e.g., Task<long>
ComputeBalanceChangeAsync) and replace blocking calls with their EF Core async
counterparts: use await
ctx.TrackedAddresses.Where(...).Select(...).ToHashSetAsync(new
ByteArrayComparer()) for ourScripts and replace FirstOrDefault on ctx.Utxos with
await ctx.Utxos.Local.FirstOrDefaultAsync(...) or (if Local is in-memory) check
Local synchronously then await ctx.Utxos.Where(...).FirstOrDefaultAsync(...) for
DB lookups; update all callers to call and await ComputeBalanceChangeAsync.
Ensure to import necessary async extensions and propagate async/await through
the call chain.
- Around line 436-441: The empty catch in ElectrumWalletTracker (in the method
that builds/returns the object where the current catch { return null; } exists)
swallows all exceptions; change it to explicitly handle cancellation and either
catch only expected Electrum-related exceptions or log unexpected ones: rethrow
OperationCanceledException (catch OperationCanceledException and throw), then
catch expected ElectrumException (or specific domain exceptions) and return null
after logging, and for any other Exception log the error via the class logger
(include ex) before returning null or rethrowing depending on expected behavior.
Ensure you reference the ElectrumWalletTracker class and the method containing
the current empty catch when making the change.
- Around line 259-280: The current nested loop in ElectrumWalletTracker
(processing ctx.Transactions unconfirmed and calling
_client.ScripthashGetHistoryAsync per address) causes O(unconfirmed × addresses)
RPC calls; instead collect the set of affected wallet IDs and load all
TrackedAddresses for those wallets once (use ctx.TrackedAddresses.Where(a =>
walletIds.Contains(a.WalletId))). Build a mapping of scripthash →
addresses/WalletId and deduplicate scripthashes, then call
ScripthashGetHistoryAsync only once per unique scripthash and reconcile its
returned history against unconfirmed tx Txid to set tx.BlockHeight;
alternatively add/maintain a persistent reverse index (txid → scripthash) at
creation time to map txids to scripthashes and avoid per-block address scans.
- Around line 823-827: ByteArrayComparer currently throws for null or arrays
shorter than 4 bytes because GetHashCode uses BitConverter.ToInt32 and Equals
uses SequenceEqual without null checks; update ByteArrayComparer.Equals to
handle ReferenceEquals and nulls before calling SequenceEqual, and change
GetHashCode to defensively handle null and short arrays (return 0 for null, and
compute a safe hash by iterating bytes or using System.HashCode/xxHash over the
available bytes instead of BitConverter.ToInt32) so it never reads past the
array bounds.
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs`:
- Around line 162-173: The string-based check on d.ImplementationFactory (using
factoryMethod.ToString()?.Contains(typeof(T).Name)) is fragile; instead inspect
the factory delegate's Method metadata for actual generic type information and
known call patterns: for the factoryMethod obtained from
d.ImplementationFactory.Method, check its MethodInfo.GetGenericArguments()
and/or examine factoryMethod.GetGenericMethodDefinition() and its generic
parameters for a match to typeof(T), and also detect direct calls to
IServiceProvider extensions like GetRequiredService by checking
factoryMethod.Name/DeclaringType and its generic args; only call
services.Remove(d) when a precise generic-argument match to typeof(T) (or a
recognized IServiceProvider generic method targeting T) is found.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1136ed7-0369-4d71-87f3-5549df0d45ff
📒 Files selected for processing (8)
Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.csPlugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumClient.csPlugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.csPlugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.csPlugins/BTCPayServer.Plugins.Electrum/ElectrumSettings.csPlugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/ElectrumSyncSummary.cshtmlPlugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/NavExtension.cshtmlPlugins/BTCPayServer.Plugins.Electrum/Views/UIElectrum/Settings.cshtml
✅ Files skipped from review due to trivial changes (1)
- Plugins/BTCPayServer.Plugins.Electrum/ElectrumSettings.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Plugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/NavExtension.cshtml
- Plugins/BTCPayServer.Plugins.Electrum/Views/Shared/Electrum/ElectrumSyncSummary.cshtml
- Plugins/BTCPayServer.Plugins.Electrum/Controllers/UIElectrumController.cs
- Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumClient.cs
Comment on lines
+78
to
+81
| // ExplorerClientProvider replacement | ||
| services.AddSingleton<ElectrumExplorerClientProvider>(); | ||
| services.AddSingleton<ExplorerClientProvider>(sp => sp.GetRequiredService<ElectrumExplorerClientProvider>()); | ||
| services.AddSingleton<Common.IExplorerClientProvider>(sp => sp.GetRequiredService<ElectrumExplorerClientProvider>()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ElectrumExplorerClientProvider methods use 'new' vs 'override'
rg -n "public (new|override)" Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.csRepository: Kukks/BTCPayServerPlugins
Length of output: 404
Fix ElectrumExplorerClientProvider to use override instead of new for all method overrides.
ElectrumExplorerClientProvider hides base methods from ExplorerClientProvider using new keyword on 5 methods (GetExplorerClient, IsAvailable, GetAll at lines 46, 53, 59, 64, 70), breaking polymorphic dispatch. Callers holding a base ExplorerClientProvider reference will invoke the wrong implementation. Change all new keywords to override.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/ElectrumPlugin.cs` around lines 78 -
81, ElectrumExplorerClientProvider is hiding base methods with the new keyword
causing incorrect polymorphic behavior; change each method declaration for
GetExplorerClient, IsAvailable, GetAll (the five methods currently using new) to
use override instead, and if the corresponding methods in ExplorerClientProvider
are not virtual/abstract, update those base method signatures to virtual (or
abstract) so the overrides compile and dispatch correctly; ensure method
signatures exactly match the base class (return types and parameters) so the
override is valid and run tests to confirm callers holding
ExplorerClientProvider now invoke ElectrumExplorerClientProvider
implementations.
Comment on lines
+18
to
+24
| <select id="trustedServer" class="form-select"> | ||
| <option value="">Custom server...</option> | ||
| @foreach (var server in ElectrumSettings.TrustedServers) | ||
| { | ||
| var val = $"{server.Host}:{server.Port}"; | ||
| <option value="@val" selected="@(val == currentServer)">@val</option> | ||
| } |
There was a problem hiding this comment.
The selected attribute renders incorrectly as a string.
The expression selected="@(val == currentServer)" outputs selected="True" or selected="False" as string values. In HTML, selected is a boolean attribute—its presence selects the option, regardless of value. selected="False" will still select the option in some browsers.
🐛 Proposed fix
<select id="trustedServer" class="form-select">
<option value="">Custom server...</option>
`@foreach` (var server in ElectrumSettings.TrustedServers)
{
var val = $"{server.Host}:{server.Port}";
- <option value="@val" selected="@(val == currentServer)">@val</option>
+ `@if` (val == currentServer)
+ {
+ <option value="@val" selected>@val</option>
+ }
+ else
+ {
+ <option value="@val">@val</option>
+ }
}
</select>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <select id="trustedServer" class="form-select"> | |
| <option value="">Custom server...</option> | |
| @foreach (var server in ElectrumSettings.TrustedServers) | |
| { | |
| var val = $"{server.Host}:{server.Port}"; | |
| <option value="@val" selected="@(val == currentServer)">@val</option> | |
| } | |
| <select id="trustedServer" class="form-select"> | |
| <option value="">Custom server...</option> | |
| `@foreach` (var server in ElectrumSettings.TrustedServers) | |
| { | |
| var val = $"{server.Host}:{server.Port}"; | |
| `@if` (val == currentServer) | |
| { | |
| <option value="@val" selected>@val</option> | |
| } | |
| else | |
| { | |
| <option value="@val">@val</option> | |
| } | |
| } | |
| </select> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Views/UIElectrum/Settings.cshtml`
around lines 18 - 24, The option tag currently renders selected="True"/"False"
because of selected="@(val == currentServer)"; change it to emit the boolean
attribute only when the values match: when iterating
ElectrumSettings.TrustedServers and building val (var val =
$"{server.Host}:{server.Port}";), render the selected attribute conditionally
(e.g., output selected="selected" or the bare selected attribute) only if val ==
currentServer, otherwise omit the attribute so only the matching option is
actually selected.
Comment on lines
+18
to
+25
| <select id="trustedServer" class="form-select"> | ||
| <option value="">Custom server...</option> | ||
| @foreach (var server in ElectrumSettings.TrustedServers) | ||
| { | ||
| var val = $"{server.Host}:{server.Port}"; | ||
| <option value="@val" selected="@(val == currentServer)">@val</option> | ||
| } | ||
| </select> |
There was a problem hiding this comment.
"Custom server..." option is never pre-selected when using a custom server.
When isTrusted is false (the current server doesn't match any trusted server), the dropdown will show the first trusted server as selected instead of "Custom server...". The isTrusted variable computed on line 10 should be used to conditionally select the custom option.
🐛 Proposed fix
<select id="trustedServer" class="form-select">
- <option value="">Custom server...</option>
+ <option value="" selected="@(!isTrusted)">Custom server...</option>Or using proper boolean attribute handling:
<select id="trustedServer" class="form-select">
- <option value="">Custom server...</option>
+ `@if` (!isTrusted)
+ {
+ <option value="" selected>Custom server...</option>
+ }
+ else
+ {
+ <option value="">Custom server...</option>
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <select id="trustedServer" class="form-select"> | |
| <option value="">Custom server...</option> | |
| @foreach (var server in ElectrumSettings.TrustedServers) | |
| { | |
| var val = $"{server.Host}:{server.Port}"; | |
| <option value="@val" selected="@(val == currentServer)">@val</option> | |
| } | |
| </select> | |
| <select id="trustedServer" class="form-select"> | |
| <option value="" selected="@(!isTrusted)">Custom server...</option> | |
| `@foreach` (var server in ElectrumSettings.TrustedServers) | |
| { | |
| var val = $"{server.Host}:{server.Port}"; | |
| <option value="@val" selected="@(val == currentServer)">@val</option> | |
| } | |
| </select> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Views/UIElectrum/Settings.cshtml`
around lines 18 - 25, The "Custom server..." option is never selected when using
a custom server; update the select markup so the first <option value="">Custom
server...</option> includes the selected attribute when the computed isTrusted
is false (i.e. when currentServer is not in ElectrumSettings.TrustedServers).
Keep the per-server option selection logic (option value="@val" selected="@(val
== currentServer)") for trusted entries and ensure the select element (id
"trustedServer") uses isTrusted to conditionally render selected on the
empty/custom option so the dropdown shows "Custom server..." for non-trusted
currentServer values.
Hand-written initial migration (dotnet ef tooling can't run against plugin assemblies due to BTCPayServer.Abstractions load issues). Creates 5 tables in the 'electrum' schema: tracked_wallets, tracked_addresses, utxos, transactions, sync_state. Also adds MigrationInterceptor to the DbContextFactory following the MicroNode plugin pattern.
ExplorerClientProvider methods (GetExplorerClient, IsAvailable, GetAll) are NOT virtual, so the 'new' keyword shadowing approach didn't work — callers holding a base class reference always called the base methods which returned null (empty _Clients dict), causing wallet 404s. Now we inject our Electrum-backed ExplorerClient instances directly into the base class's private _Clients dictionary via reflection.
POST /v1/cryptos/{code}/derivations without a strategy segment is
NBXplorer's GenerateWallet endpoint. We now generate HD wallets
client-side using NBitcoin: create/import mnemonic, derive account
key via BIP44/49/84/86 path, create derivation strategy, track it,
and return a GenerateWalletResponse-compatible JSON.
This fixes the 501 Not Implemented error when creating wallets
through the BTCPay Server UI.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs (2)
213-218:⚠️ Potential issue | 🟡 MinorGuard the broadcast path against a missing body.
request.Content!still dereferences null here. A POST without a body turns into a 500 instead of a clean client error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs` around lines 213 - 218, The POST broadcast handler in ElectrumHttpHandler.cs dereferences request.Content without checking for null/empty causing 500s; before calling request.Content!.ReadAsStringAsync in the block that matches "/v1/cryptos/\\w+/transactions" check if request.Content is null or that the read string is null/whitespace and return a client error (e.g., BadRequestResponse or equivalent) with a helpful message instead of calling _tracker.BroadcastAsync; update the branch that currently uses request.Content!.ReadAsStringAsync(...) and OkResponse(result) to validate content first and only call _tracker.BroadcastAsync(body, cancellationToken) when the body is present.
239-245:⚠️ Potential issue | 🟡 MinorDon’t return raw exception text to clients.
This still sends
ex.Messageback over HTTP, which can leak internal Electrum/NBXplorer details. Logging the exception is enough; the response body should stay generic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs` around lines 239 - 245, The catch block in ElectrumHttpHandler.cs currently returns raw exception text to clients; change the HttpResponseMessage construction in the catch for Exception ex so it does not expose ex.Message but instead returns a generic response body (e.g. "Internal Server Error" or empty) while keeping the existing _logger.LogError(ex, "Error handling {Method} {Path}", method, path) call; update the HttpResponseMessage created in that catch (the code that sets Content = new StringContent(...)) to use a non-sensitive generic message and preserve HttpStatusCode.InternalServerError.Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs (2)
872-886:⚠️ Potential issue | 🔴 CriticalPopulate each matched output from its own tracked address.
When
scriptHashmatches another subscribed address, this still copiesmatchedAddrintoOutputInfo. Multi-output wallet transactions then report the wrong address/keypath for at least one output, which can misattribute downstream payment handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs` around lines 872 - 886, The code is copying the earlier matchedAddr into each OutputInfo even when scriptHash matches a different subscribed address; update the block in ElectrumWalletTracker.cs so you resolve the tracked address for the current scriptHash (use _subscribedScripthashes.TryGetValue(scriptHash, out var tracked) or otherwise look up the entry for scriptHash) and then populate OutputInfo.Address, KeyPath and compute KeyIndex from that tracked entry (instead of reusing matchedAddr); keep the existing parsing logic for KeyPath/KeyIndex but apply it to the per-output tracked variable (e.g., tracked or matchedForOutput) so each output gets its correct address and key path.
317-340:⚠️ Potential issue | 🔴 CriticalMake unused-address allocation atomic.
This still does a read-then-write outside the tracker lock, orders
KeyPathlexicographically, and never extends the gap when reserving the last slot. Two concurrent callers can reserve the same address, and once the initial pool is exhausted the retry throughTrackWalletAsync()still returnsnullbecause existing wallets short-circuit there.
🧹 Nitpick comments (2)
Plugins/BTCPayServer.Plugins.Electrum/Data/ElectrumDbContextFactory.cs (1)
28-35: Keep the design-time EF pipeline aligned with runtime.
DesignTimeElectrumDbContextFactorybypasses bothMigrationInterceptor.InstanceandConfigureBuilder(...), sodotnet efcan exercise a different provider setup than the app does at runtime. Reusing the same builder path here will make migration generation/update behavior much less fragile.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Data/ElectrumDbContextFactory.cs` around lines 28 - 35, DesignTimeElectrumDbContextFactory currently creates DbContextOptions directly and skips the runtime configuration; update CreateDbContext in DesignTimeElectrumDbContextFactory to reuse the same builder path as the app by calling the shared ConfigureBuilder(...) used at runtime and by adding the MigrationInterceptor.Instance to the options (e.g., invoke ConfigureBuilder(builder) and builder.AddInterceptors(MigrationInterceptor.Instance)) before constructing new ElectrumDbContext(builder.Options) so migrations run against the same provider/configuration as runtime.Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.cs (1)
28-31: Fail explicitly if the base_Clientsfield stops matching.This reflection hook is now a hard dependency on NBXplorer internals, but
clientsField!/the blind cast will only surface as a generic startup failure on upgrade. An explicit null/type check with a descriptive exception would make version drift much easier to diagnose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.cs` around lines 28 - 31, The reflection access to the base class's private _Clients field must fail fast with a clear message: after calling typeof(ExplorerClientProvider).GetField("_Clients", BindingFlags.NonPublic | BindingFlags.Instance) check that clientsField is not null and throw an InvalidOperationException naming ExplorerClientProvider and "_Clients" if it is null; then call clientsField.GetValue(this) and verify the result is a Dictionary<string, ExplorerClient> (or not null) before casting, throwing another descriptive InvalidOperationException if the value is null or not of the expected type to indicate NBXplorer internal changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`:
- Around line 713-745: Load existing UTXO rows for the address regardless of
IsSpent (remove the .Where(... && !u.IsSpent) filter) or additionally query for
any existing rows matching Outpoint before inserting; then when processing utxos
check ctx.Utxos for a matching Outpoint and if found and IsSpent==true, set
IsSpent=false and update relevant fields (Txid, Vout, Value, BlockHeight,
SeenAt, etc.) instead of calling ctx.Utxos.Add, otherwise add a new TrackedUtxo
as before; reference existingUtxos, currentOutpoints, ctx.Utxos, TrackedUtxo and
Outpoint to locate where to change the logic.
- Around line 650-681: The sync loads existingTxids from the DB but doesn't
account for transactions already added in this DbContext, causing duplicate
Add() for txs touching multiple addresses; fix by maintaining an in-memory
HashSet (e.g., stagedTxids) alongside existingTxids and check
stagedTxids.Contains(item.TxHash) in the history loop before adding, add
item.TxHash to stagedTxids immediately after ctx.Transactions.Add(new
TrackedTransaction { ... }) (or after SaveChanges if you prefer), and also
consult stagedTxids when updating the "existing" lookup so functions like
ComputeBalanceChange, the existingTxids set, the history loop, and the
ctx.Transactions.Add/TrackedTransaction logic treat staged transactions as
existing within the same wallet-sync pass.
- Around line 683-689: When marking historical addresses as used in
ElectrumWalletTracker (the block that sets addr.IsUsed = true and updates
ctx.TrackedAddresses via FindAsync), also call and await ExtendGapIfNeeded for
that address so the gap is extended and later addresses are derived/subscribed;
specifically, after setting addr.IsUsed (and tracked.IsUsed if found) invoke
await ExtendGapIfNeeded(addr.Scripthash, ct) (or the appropriate overload that
accepts the scripthash/index and CancellationToken), and ensure the call is
awaited so derivation/subscription happens before continuing.
- Around line 748-773: ComputeBalanceChange currently only subtracts inputs by
looking up prevOutpoints in the live ctx.Utxos (and Utxos.Local), which misses
historical spends; update ComputeBalanceChange to resolve input debits by
locating the spent output record in the persisted outputs/UTXO history instead
of only ctx.Utxos: when iterating tx.Inputs and building prevOutpoint from
input.PrevOut, first query the persisted outputs table (e.g., a table analogous
to TransactionOutputs/AllOutputs or any spend-history table you have in the
DbContext) for a matching Outpoint and WalletId and subtract that
output.Value.Satoshi if found; only fall back to ctx.Utxos (and ctx.Utxos.Local)
if no persisted output record exists. Ensure you reference ComputeBalanceChange,
ctx.TrackedAddresses, tx.Inputs/input.PrevOut, and ctx.Utxos when implementing
the lookup and subtraction logic.
---
Duplicate comments:
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`:
- Around line 872-886: The code is copying the earlier matchedAddr into each
OutputInfo even when scriptHash matches a different subscribed address; update
the block in ElectrumWalletTracker.cs so you resolve the tracked address for the
current scriptHash (use _subscribedScripthashes.TryGetValue(scriptHash, out var
tracked) or otherwise look up the entry for scriptHash) and then populate
OutputInfo.Address, KeyPath and compute KeyIndex from that tracked entry
(instead of reusing matchedAddr); keep the existing parsing logic for
KeyPath/KeyIndex but apply it to the per-output tracked variable (e.g., tracked
or matchedForOutput) so each output gets its correct address and key path.
In `@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs`:
- Around line 213-218: The POST broadcast handler in ElectrumHttpHandler.cs
dereferences request.Content without checking for null/empty causing 500s;
before calling request.Content!.ReadAsStringAsync in the block that matches
"/v1/cryptos/\\w+/transactions" check if request.Content is null or that the
read string is null/whitespace and return a client error (e.g.,
BadRequestResponse or equivalent) with a helpful message instead of calling
_tracker.BroadcastAsync; update the branch that currently uses
request.Content!.ReadAsStringAsync(...) and OkResponse(result) to validate
content first and only call _tracker.BroadcastAsync(body, cancellationToken)
when the body is present.
- Around line 239-245: The catch block in ElectrumHttpHandler.cs currently
returns raw exception text to clients; change the HttpResponseMessage
construction in the catch for Exception ex so it does not expose ex.Message but
instead returns a generic response body (e.g. "Internal Server Error" or empty)
while keeping the existing _logger.LogError(ex, "Error handling {Method}
{Path}", method, path) call; update the HttpResponseMessage created in that
catch (the code that sets Content = new StringContent(...)) to use a
non-sensitive generic message and preserve HttpStatusCode.InternalServerError.
---
Nitpick comments:
In `@Plugins/BTCPayServer.Plugins.Electrum/Data/ElectrumDbContextFactory.cs`:
- Around line 28-35: DesignTimeElectrumDbContextFactory currently creates
DbContextOptions directly and skips the runtime configuration; update
CreateDbContext in DesignTimeElectrumDbContextFactory to reuse the same builder
path as the app by calling the shared ConfigureBuilder(...) used at runtime and
by adding the MigrationInterceptor.Instance to the options (e.g., invoke
ConfigureBuilder(builder) and
builder.AddInterceptors(MigrationInterceptor.Instance)) before constructing new
ElectrumDbContext(builder.Options) so migrations run against the same
provider/configuration as runtime.
In
`@Plugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.cs`:
- Around line 28-31: The reflection access to the base class's private _Clients
field must fail fast with a clear message: after calling
typeof(ExplorerClientProvider).GetField("_Clients", BindingFlags.NonPublic |
BindingFlags.Instance) check that clientsField is not null and throw an
InvalidOperationException naming ExplorerClientProvider and "_Clients" if it is
null; then call clientsField.GetValue(this) and verify the result is a
Dictionary<string, ExplorerClient> (or not null) before casting, throwing
another descriptive InvalidOperationException if the value is null or not of the
expected type to indicate NBXplorer internal changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 35c3bb73-1ae5-43cc-ab6e-44259e75466d
📒 Files selected for processing (8)
Plugins/BTCPayServer.Plugins.Electrum/BTCPayServer.Plugins.Electrum.csprojPlugins/BTCPayServer.Plugins.Electrum/Data/ElectrumDbContextFactory.csPlugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.csPlugins/BTCPayServer.Plugins.Electrum/Migrations/20260408000000_Initial.Designer.csPlugins/BTCPayServer.Plugins.Electrum/Migrations/20260408000000_Initial.csPlugins/BTCPayServer.Plugins.Electrum/Migrations/ElectrumDbContextModelSnapshot.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumExplorerClientProvider.csPlugins/BTCPayServer.Plugins.Electrum/Services/ElectrumHttpHandler.cs
✅ Files skipped from review due to trivial changes (1)
- Plugins/BTCPayServer.Plugins.Electrum/Migrations/ElectrumDbContextModelSnapshot.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Plugins/BTCPayServer.Plugins.Electrum/BTCPayServer.Plugins.Electrum.csproj
Comment on lines
+713
to
+745
| var existingUtxos = await ctx.Utxos | ||
| .Where(u => u.Scripthash == addr.Scripthash && !u.IsSpent) | ||
| .ToListAsync(ct); | ||
|
|
||
| foreach (var existing in existingUtxos) | ||
| { | ||
| if (!currentOutpoints.Contains(existing.Outpoint)) | ||
| { | ||
| existing.IsSpent = true; | ||
| } | ||
| } | ||
|
|
||
| // Add new UTXOs | ||
| var existingOutpoints = existingUtxos.Select(u => u.Outpoint).ToHashSet(); | ||
| foreach (var utxo in utxos) | ||
| { | ||
| var outpoint = $"{utxo.TxHash}:{utxo.TxPos}"; | ||
| if (existingOutpoints.Contains(outpoint)) continue; | ||
|
|
||
| ctx.Utxos.Add(new TrackedUtxo | ||
| { | ||
| Outpoint = outpoint, | ||
| WalletId = addr.WalletId, | ||
| Scripthash = addr.Scripthash, | ||
| Txid = utxo.TxHash, | ||
| Vout = utxo.TxPos, | ||
| Value = utxo.Value, | ||
| ScriptPubKey = addr.ScriptPubKey, | ||
| KeyPath = addr.KeyPath, | ||
| BlockHeight = utxo.Height > 0 ? utxo.Height : null, | ||
| SeenAt = DateTimeOffset.UtcNow | ||
| }); | ||
| } |
There was a problem hiding this comment.
Revive reappearing outpoints instead of inserting duplicates.
existingUtxos only loads rows where IsSpent == false. If a previously spent outpoint comes back after a reorg or reconnect diff, this code inserts a second row with the same Outpoint primary key instead of flipping the existing record back to unspent, so SaveChangesAsync() fails and sync stops.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`
around lines 713 - 745, Load existing UTXO rows for the address regardless of
IsSpent (remove the .Where(... && !u.IsSpent) filter) or additionally query for
any existing rows matching Outpoint before inserting; then when processing utxos
check ctx.Utxos for a matching Outpoint and if found and IsSpent==true, set
IsSpent=false and update relevant fields (Txid, Vout, Value, BlockHeight,
SeenAt, etc.) instead of calling ctx.Utxos.Add, otherwise add a new TrackedUtxo
as before; reference existingUtxos, currentOutpoints, ctx.Utxos, TrackedUtxo and
Outpoint to locate where to change the logic.
Comment on lines
+748
to
+773
| private long ComputeBalanceChange(ElectrumDbContext ctx, Transaction tx, string walletId) | ||
| { | ||
| // Look up which of our addresses are involved | ||
| var ourScripts = ctx.TrackedAddresses | ||
| .Where(a => a.WalletId == walletId) | ||
| .Select(a => a.ScriptPubKey) | ||
| .ToHashSet(new ByteArrayComparer()); | ||
|
|
||
| long change = 0; | ||
|
|
||
| // Outputs to us are positive | ||
| foreach (var output in tx.Outputs) | ||
| { | ||
| if (ourScripts.Contains(output.ScriptPubKey.ToBytes())) | ||
| { | ||
| change += output.Value.Satoshi; | ||
| } | ||
| } | ||
|
|
||
| // Inputs from us are negative (we'd need to look up the previous output) | ||
| // For simplicity, we check if the spent outpoint matches our UTXOs | ||
| foreach (var input in tx.Inputs) | ||
| { | ||
| var prevOutpoint = $"{input.PrevOut.Hash}:{input.PrevOut.N}"; | ||
| var ourUtxo = ctx.Utxos.Local.FirstOrDefault(u => u.Outpoint == prevOutpoint && u.WalletId == walletId) | ||
| ?? ctx.Utxos.FirstOrDefault(u => u.Outpoint == prevOutpoint && u.WalletId == walletId); |
There was a problem hiding this comment.
Historical spends are reconstructed from the live UTXO set.
Input debits are derived by looking up PrevOut in ctx.Utxos, but this tracker only persists the current unspent set plus later spend markers. During initial/reconnect sync, any transaction spending an output that is already spent today will miss its negative side here, so BalanceChange is overstated for restored wallets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Plugins/BTCPayServer.Plugins.Electrum/Electrum/ElectrumWalletTracker.cs`
around lines 748 - 773, ComputeBalanceChange currently only subtracts inputs by
looking up prevOutpoints in the live ctx.Utxos (and Utxos.Local), which misses
historical spends; update ComputeBalanceChange to resolve input debits by
locating the spent output record in the persisted outputs/UTXO history instead
of only ctx.Utxos: when iterating tx.Inputs and building prevOutpoint from
input.PrevOut, first query the persisted outputs table (e.g., a table analogous
to TransactionOutputs/AllOutputs or any spend-history table you have in the
DbContext) for a matching Outpoint and WalletId and subtract that
output.Value.Satoshi if found; only fall back to ctx.Utxos (and ctx.Utxos.Local)
if no persisted output record exists. Ensure you reference ComputeBalanceChange,
ctx.TrackedAddresses, tx.Inputs/input.PrevOut, and ctx.Utxos when implementing
the lookup and subtraction logic.
…tpHandler Implements the remaining ExplorerClient endpoints needed for full wallet functionality: - GET/POST metadata: store and retrieve wallet metadata (MasterHDKey, AccountHDKey, Mnemonic, etc.) using SyncState table - POST psbt/create: build PSBTs from available UTXOs with coin selection, fee estimation, and change address handling - POST psbt/update: rebase key paths on existing PSBTs Also stores metadata (keys, mnemonic, account path) during wallet generation when SavePrivateKeys is enabled.
Three issues prevented the wallet creation flow from completing:
1. ExtPubKey.ToString() returns type name instead of xpub string in
accountDescriptor - fixed by using GetWif(network)
2. Track endpoint returned JSON {} which failed ExplorerClient's
SendAsync<string> deserialization - fixed by returning empty 200
3. Wallet generation used JObject instead of typed GenerateWalletResponse,
causing NBXplorer custom JsonConverters to be skipped - fixed by
constructing proper GenerateWalletResponse with typed properties
Also: run TrackWalletAsync in background to avoid HTTP timeout, use
NBXplorer serializer settings for all JSON responses, remove diagnostic
Console.WriteLine logging.
…and status capabilities - Fix duplicate TrackedTransaction entities in SyncWalletStateAsync by hoisting existingTxids before the address loop and updating it on insert - Restore _tipHeight from DB on startup and set from initial HeadersSubscribe, fixing transactions appearing as unconfirmed - Add BitcoinStatus with CanScanTxoutSet to ElectrumStatusMonitor so rescan wallet UI shows as supported - Clamp GapLimit to 1-1000 with default 20 - Add rescan (scan/wipe) endpoints to ElectrumHttpHandler - Keep NBXplorerConnectionFactory.Available = false to avoid cascading SQL compat requirements across BTCPay's direct-query paths
Leverage upstream btcpayserver/btcpayserver#7308 which added a protected constructor and protected _Clients to ExplorerClientProvider. Removes reflection, NullHttpClientFactory, and dummy constructor arguments.
Registers a "Conference" app type that provisions user accounts, stores (with Lightning, currency, spread), and Light POS apps for each merchant from a single admin interface. Supports CSV import, per-merchant overrides, bulk re-apply of settings, health checks with repair, and sales reporting aggregated by time range and currency. Login code QR generation is handled entirely server-side — userId never reaches the client HTML — preventing privilege escalation via DOM manipulation.
When a merchant email matches an existing BTCPay account (e.g., a server admin), the provisioning service now tracks that the user was found, not created. Login code QR generation is gated on UserCreatedByPlugin=true, so an attacker cannot create a Conference app, add an admin's email as a merchant, and obtain a login code that grants admin access. Pre-existing user merchants show "N/A" in the Login column with a tooltip explaining they must use their own credentials.
Conference: support adding multiple merchants at once, individual provisioning, persist active tab on redirects, add POS/login QR modals to settings page, and restyle dashboard to use native BTCPay widget components. Electrum: add "random" server mode that picks a trusted server on each connection, and handle missing DB on first-run settings load.
…out-banner Conference: Merge dashboard into settings, add Reports tab with time-range filtering and per-merchant breakdowns, lazy AJAX login code generation, inline add-merchant rows with integrated CSV import, use layout-menu-item tag helper. Remove Dashboard view and viewmodel. Electrum: Show "Connecting to..." with configured server in sync status instead of just "not configured", auto-refresh every 10s via AJAX partial. ServerMessage: Use layout-banner extension point instead of header-nav for proper mobile support. Bump min BTCPay to >=2.3.8. All plugins bumped to v1.1.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
scripthash.subscribe+headers.subscribenotificationsElectrumHttpHandler, returning NBXplorer-compatible JSON responses from the Electrum engineelectrumschema) for tracked wallets, addresses, UTXOs, transactions, and sync state via EF CoreArchitecture
The plugin uses a "Shadow & Replace" pattern — it removes 8 NBXplorer service registrations from DI and re-registers Electrum-backed replacements:
Key insight: BTCPayWallet methods are NOT virtual, so we can't subclass. Instead,
ElectrumHttpHandlerintercepts the HTTP requests thatExplorerClientmakes, and returns NBXplorer-compatible responses built from Electrum data.Files (23 new, 0 modified)
electrumschema)Test plan
dotnet build(verified locally — 0 errors)Summary by CodeRabbit
New Features
Documentation