Conversation
40b5cbf to
abd47d4
Compare
|
Added cold wallet support to the sender. Hardware devices and multisig groups using the same path. |
ValeraFinebits
left a comment
There was a problem hiding this comment.
Good feature, but as a separate page I think it will be inconvenient in production. The dedicated page loses coin selection, labels, fee-rate presets, balance, fiat conversion and other.
Core already models this: Paste BIP21 posts the URI to the server, and UIWalletsController.LoadFromBIP21 sets vm.PayJoinBIP21 when the URI carries a pj= endpoint, which drives the v1 payjoin. Async payjoin is the v2 variant of the same thing and belongs in that same flow.
If that isn't reachable from a plugin without a new extension point in core, then I'd suggest scoping this page explicitly as a testing tool rather than a production user flow, it is genuinely handy for making test payments while working on the plugin.
At first, I thought Core had a limitation, but you have mentioned something that I didn't first consider. I assumed there's a limit; let's see. I'm going to try folding it into the main page as you suggest, which was also my preferred shape |
ValeraFinebits
left a comment
There was a problem hiding this comment.
Requesting changes
Send.cshtml:12, :64 wallet links 403. They build $"{Model.StoreId}-BTC", but WalletId.TryParse requires ^S-… (WalletId.cs:11), so binding fails first. Hit both in a browser, including "Sign the transaction", the only way into the off-server flow. Use new WalletId(...).ToString(), as RedirectAfterStart does.
PayjoinSenderSessionProcessor.cs:386 round two never expires. expiry: null never matches core's sweep (PendingTransactionService.cs:43), and the poller skips AwaitingSignature, so an unsigned proposal waits forever with its coins reserved. Round one passes 7 days, please pass a window here too, and decide what lapsing should do.
PayjoinSenderSessionProcessor.cs:547, PayjoinSenderSignatureHandler.cs:213 the signed fallback is dropped. Both record Failed without touching OriginalTransactionHex. Abandon fires when the operator cancels the pending transaction, which is the same intent as Stop and Stop broadcasts. Deliberate for FailSession?
PayjoinSenderSessionStore.cs:260 the first-round pending transaction is orphaned. Clearing the id drops your handle, and core never retires a Signed row (PendingTransactionService.cs:342) while still excluding its outpoints (UIWalletsController.PSBT.cs:57). If the session later fails, those coins stay blocked permanently. Retire the row before clearing.
PayjoinSenderSignatureListener.cs:61 a cancel can race a collected signature. No status check here, no terminal guard in CompleteSession, and PendingTransactionId survives completion. Core blocks the wide case (PendingTransactionService.cs:182), but events are delivered async, so a signature collected just before Stop can land after it and broadcast the proposal on top of the original. Status check plus clearing the id on completion.
PayjoinWalletSendExtension.cshtml:54 core's v1 field is hidden, not cleared. PayJoinBIP21 is a string rendering as type="text" (WalletSend.cshtml:220), so the checkbox branch never runs. Verified live: the value still submits, so core's own Sign button still fires v1 at a v2 endpoint. toggle.value = '' unconditionally.
PayjoinSenderSessionProcessor.cs:462, PayjoinReceiverRelayTimeoutException : TaskCanceledException matches no catch in ProcessTickAsync, so a stalled relay skips every remaining session that tick.
ReconcileAsync sits outside the per-session guard, so one throwing session stops the rest.
PayjoinSenderSignatureHandler.cs:134 any InvalidOperationException is terminal, so a rejected broadcast discards a fully signed payjoin with no retry.
PayjoinSenderSessionStore.cs:172 core can't see SenderSessions, so an ordinary send can still spend a live session's coins.
Suggestion: reuse IPayjoinReceiverRelayRequestSender
SendThroughRelayAsync re-implements it, and the existing one already handles relay quarantine and rotation, catches the timeout above, disposes the context on every failure path (the copy never disposes it on success at all), and errors out instead of returning null when a store has no relays. It's already generic over the context type and RequestOhttpContext fits as-is. Needs invoiceId renamed and that timeout added to the transient list. Happy to take it.
Tests
Four that turn on timing or an external string: ConcurrentStartSignedSessionLetsExactlyOneWinnerThrough, TwoSubmissionsOfOneUriCreateOneSession, ConcurrentAppendsLeaveTheLogReplayable, BroadcastTreatsCoreMempoolDuplicateAsSuccess.
The first three need real constraint enforcement, the unit project's InMemory provider enforces no unique indexes, so PostgresPayjoinUniqueConstraintViolationDetector never fires and they'd pass against broken code. They can go straight into PayjoinPluginConcurrencyIntegrationTests beside the receiver's equivalents; no new fixture needed. The fourth wants the regtest node instead: broadcast one transaction twice and assert the second call succeeds, so it checks the real Core version's reason string. For the two sequence-conflict tests, pre-seeding the conflicting row is more reliable than racing tasks, the window between reading max(Sequence) and SaveChanges is small.
Two more with no new seams: an end-to-end test through SendFromWallet everything currently calls StartAsync directly, which is how the link bug got through and a sender equivalent of InFlightReceiverSessionSurvivesServerRestartAndCompletesPayjoin.
|
6fc3cf4 answers the remaining findings. Coin visibility did not need core. Each live session keeps a Signed pending transaction that holds the signed original, so core excludes its coins. The session releases the row when it ends.
I think we found a core bug. Fixed two more faults: the cleared v1 field also emptied the URI our own button posts, and a double submission of one URI could make two sessions. @ValeraFinebits Please take the relay sender. The four named tests are not in this branch. Their subjects now run in the integration project against Postgres. The restart test is skipped, with the same harness limit as the receiver's. |
Taking it. I'll keep this to one commit on top of bfe4742: wire |
ValeraFinebits
left a comment
There was a problem hiding this comment.
Requesting changes
Thank you for addressing all findings from my previous review. After reviewing and testing the follow-up changes on a98ef4e, I found the following additional issues. Please add regression coverage for each fix using scenarios like these:
-
Live outpoint uniqueness
PayjoinSenderSessionConfiguration.cs/20260819190647_AddSenderSessions
Enforce outpoint ownership per store at the database level.
Test: create two live sessions in the same store with different BIP21 URIs but the same outpoint. The second must be rejected; after the first becomes terminal, the outpoint should be reusable. -
Second-signature reservation sweep
PayjoinSenderSessionStore.GetPendingSessionsWithCoinReservations
The query excludesAwaitingSignature, so manual fallback activity can be missed during the second signing round.
Test: create a session with a coin reservation, transition it toAwaitingSignature, and verify the reservation sweep still returns it. -
Atomic state transitions
PayjoinSenderSessionStore.AwaitSignature/CompleteSession
Both methods remain read-check-write operations without a conditional update. A late writer can overwrite a terminal state.
Test: force both callers to readPending, letCompleteSessioncommit first, then releaseAwaitSignature; the late transition must fail and must not resurrect the session. Also verify that two concurrent completions produce exactly one winner. -
Proposal pending-transaction cleanup
PayjoinSenderSignatureHandler.BroadcastProposalAsync
A successfully broadcast proposal can leave its signing request looking actionable.
Test: complete the cold-wallet flow with two off-server signatures, wait forCompletedPayjoin, then verify the proposal pending transaction is absent or terminal, notPendingorSigned. -
Permanent broadcast rejection
PayjoinSenderBroadcaster,PayjoinSenderSessionProcessor.cs:133,PayjoinSenderSignatureHandler.cs:279
All broadcast failures are treated as transient, including permanent spent-input rejection.
Test: obtain a valid proposal, spend the receiver’s contributed input first, and process the sender again. It must broadcast the original transaction and finish asCompletedFallback, rather than remainPending. -
Head-of-line blocking
PayjoinSenderSessionProcessor.ProcessTickAsync
Sessions are awaited sequentially, so one relay long-poll blocks every following session.
Test: create two pending sessions and use a coordinating relay sender that blocks the first request. Verify the second request starts before the first is released. -
Store-scoped BIP21 uniqueness
PayjoinSenderSessionConfiguration.cs/20260819190647_AddSenderSessions
The filtered unique index is global instead of store-scoped.
Test: allow two different stores to create live sessions for the same BIP21 URI, while still rejecting a duplicate URI within the same store. -
Cumulative authorization
UIPayjoinSenderController.cs:22and:51
ASP.NET Core combines class- and action-level authorization; the action policy does not replaceCanModifyStoreSettings.
Test: inspect the effective policies forSendFromWallet. They should contain create/sign/broadcast wallet permissions and must not containCanModifyStoreSettings. -
Invalid model submission
UIPayjoinSenderController.SendFromWallet
The action does not checkModelState.IsValid.
Test: submit an otherwise usable model with a binding error and verify that no sender session or pending transaction is created.
Non-blocking: please derive the Status IN (0, 4) filter in PayjoinSenderSessionConfiguration.cs:33 from PayjoinSenderSessionStatus instead of using magic values.
|
@ValeraFinebits Thank you for the relay sender; 8c9b758 rebases on it and answers all nine findings and the style note, each with the test you asked for. Two design notes. The coin guard is a table keyed by the outpoint itself, and it is global on purpose: two stores can share a wallet, so the same coin must not fund two live sessions anywhere. The URI index is store-scoped, as you asked. The status column is now a concurrency token, so every transition is conditional: a late writer loses instead of overwriting a terminal state. Release of the signing request and the coin reservation runs on every terminal transition, with a sweep for runs that crash between the two steps. Broadcast classification taught us two things on regtest. NBXplorer's One limit to record: stores that share a derivation scheme do not see each other's live coins when they build a transaction. Core's own pending-transaction exclusion has the same store scope, and the new outpoint table turns that race into a refusal at start. |
ValeraFinebits
left a comment
There was a problem hiding this comment.
Thanks for this. I gave the wallet-driven sender a run on mainnet and hit three things.
All of them reproduce on regtest, so they should be easy to pin down.
1. Unhandled exceptions take the host down. UIPayjoinSenderController has no
try/catch, so anything escaping SendFromWallet or Cancel makes core disable the
plugin and stop the process. Easy to miss, since every other path is guarded.
2. Not enough funds for the fee throws instead of returning an error.
PayjoinSenderService.cs:233 calls CreatePSBTAsync unguarded -> NBXplorerException: Not enough funds for doing this transaction -> crash via (1). Core and our own
RunTestPaymentService.cs:197 both catch it, so it is just this one call site.
3. Cancel should actually cancel. I see from the doc comment that broadcasting the
original is deliberate, but from the operator's side pressing cancel and having the
payment go out anyway is the opposite of what the button promises. Could cancel drop the
payment entirely: nothing broadcast; coins released: the way an AwaitingSignature
session already behaves? If sending the plain payment is still worth offering, a separate
explicit action for it would read much more clearly.
Two more things I wanted to ask about rather than assume:
- Would it make sense to move the "Send as async payjoin" button to the confirmation
screen aftercommand="sign"? Core puts the v1 equivalent there
(WalletPSBTDecoded.cshtml:125-131), and the operator would see the fee first. - Could the sent-payments list move from the store settings menu
(PayJoinStoreNavExtension.cshtml) onto the plugin's main page? It reads more like a
payment log than a setting.
One last thing: CancelAsync has no test yet. Could you cover an insufficient-funds
start (redirect with an error rather than an exception), a cancel on a hot-wallet
session, and a cancel on AwaitingSignature with nothing broadcast?
PayjoinSenderWalletSendIntegrationTests looks like the natural home for them.
|
I found a few additional cases that I believe should be covered before merging:
Could you please add regression coverage for these scenarios as part of the fixes? |
842bb64 to
0948422
Compare
|
These should address much of the list...
|
Master selects Microsoft.Testing.Platform through global.json. Its test commands must pass project paths with --project; positional paths fail before tests run. Update both unit and integration workflows and keep the explicit-test policy unchanged. Reference core directly from the unit project so tests can compile against core types and load the HTTP host's runtime dependencies. Keep this test infrastructure before the sender commits that depend on it.
The existing relay transport already owns endpoint rotation, timeouts and native request-context disposal. Allow the sender to reuse it without introducing a second transport implementation. Name the request identifier sessionId rather than invoiceId, and report it as a payjoin session in timeout errors. Align the receiver test doubles and retain their existing settlement and proposal-finalizer coverage. Co-authored-by: Valera <50830352+ValeraFinebits@users.noreply.github.com>
An unexpected plugin action exception can reach core's plugin-failure handler and stop the host. Handle it at the MVC boundary instead. Return a stable JSON error or local UI redirect, and keep the full exception in server logs. Apply the filter to existing controllers before introducing the sender routes. Cover JSON responses, redirect origin checks and already handled exceptions. Sender HTTP coverage follows with the wallet UI.
Establish the durable sender contract before adding payment workers. Store protocol events, signing handles, terminal outcomes and reserved outpoints. Reject duplicate live URIs and globally conflicting outpoints. Bind each append to the event version loaded for replay; never renumber a stale transition. Use conditional status/resource updates and a durable PaymentExposed marker so unshared cancellation cannot win after exposure. Coordinate cooperating workers with per-session PostgreSQL advisory locks. Keep both migration identities and their legacy-exposure backfill intact. Add broadcast classification and terminal resource cleanup: cancel current handles, compare-and-clear them, and retain interrupted cleanup for recovery. Include store regressions and PostgreSQL migration, locking, replay and concurrency tests. Workflow-dependent signature regressions follow later.
Build the original payment from a validated BIP21 URI and the wallet's selected inputs and fee rate. Exclude reserved coins and reject duplicate payments before recording a session. Resolve signing keys through one wallet helper. A hot wallet signs the original and keeps a core-visible coin reservation. An off-server wallet creates a pending signing request and waits for its first signature. Register the funding service without exposing an HTTP route yet. The next commit supplies the background lifecycle and hot/cold payment acceptance tests; the final UI commit supplies the operation-permission boundary.
Use one poller for protocol progress, both off-server signing rounds and resource recovery. Reload sessions under ownership before network effects, and match each signature to the current pending request rather than status alone. This avoids a second event-listener execution path. Record exposure before POST or broadcast. Keep Cancel separate from PayNow: a late cancellation must not turn into a payment or release exposed coins. Retry storage and transient failures without forcing fallback; retain the signed original for explicit payment and permanent proposal rejection. Add deterministic dispatch/cancel and stale-signature regressions, plus hot/cold, selected-input, fallback and stopped-poller integration coverage. The existing full-host restart test remains skipped for its harness defect; stopped-poller recovery is not a substitute for that missing coverage. Co-authored-by: Valera <50830352+ValeraFinebits@users.noreply.github.com>
Use core's wallet send form so selected inputs, labels, fees and wallet navigation stay in the normal payment flow. Add the sender status page and separate Cancel and PayNow forms so stale UI cannot change operator intent. Require create, sign and broadcast permissions to start this automatic payment workflow. Gate cancellation and pay-now independently; require only wallet-view permission for the status page. Use public MVC activation. Cover model validation, activation, permission combinations, redirects, error disclosure and current core-row cleanup. The HTTP tests use test permission claims and JSON view output, not core role storage or full browser rendering. All source and tests now match the original PR.
0948422 to
cf8ea68
Compare
|
I've made some changes to git history to make this easy to follow. The 19 commits are now seven, ordered by dependency. Fixes and tests sit with the code they protect. This remains a large PR. About half the added lines are tests and migrations. The rewrite changes the review order, not the scope or final code. The previous history is preserved in the backup branch. Its Git tree exactly matches the new head, so all current fixes and coverage remain. Build, integration, and CodeQL pass on |
ValeraFinebits
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier feedback. A few issues remain on cf8ea68:
-
Cancel through core still broadcasts.
PayjoinSenderSignatureHandler.cs:166–168
Cancelling the core coin-reservation transaction callsEndSessionAsyncand broadcasts the original, even before it has been shared. Please keep cancellation separate from PayNow across both entry points. -
Empty coin selection is ignored.
PayjoinSenderService.cs:212
With coin selection enabled but no inputs selected, the sender switches to automatic selection. Please reject the empty selection instead of spending other wallet coins. -
NoChangeis ignored.
UIPayjoinSenderController.SendFromWallet
The sender can create change despite this option being selected. Please support it or reject it before creating the payment. -
Labels bypass the management permission.
UIPayjoinSenderController.cs:88
Destination labels are saved without checkingCanManageWalletTransactions. Please apply the same permission check as core. -
A failed second-round save can orphan a signing request.
PayjoinSenderSessionProcessor.cs:380–404
IfAwaitSignaturethrows after the pending transaction is created, that request remains unlinked. A retry creates another, and cleanup misses the first. Please make this handoff recoverable. -
Long polls can exhaust the ownership pool.
PayjoinSenderSessionLock.cs:36
Sessions hold connections throughout relay requests, but the pool allows only 16. Additional sessions can time out outside the per-session guard. Please bound concurrent processing and safely defer excess work.
Could you also add regression coverage for these cases? The previously requested overlapping-read AwaitSignature/CompleteSession race and concurrent-completion tests are still missing; the existing tests exercise sequential calls.
Adds async payjoin sending through the existing wallet send form. Supports hot wallets and two off-server signing rounds.
Persisted sessions track negotiation, signing, broadcast, and recovery. The signed original remains available for fallback. Cancel never broadcasts. Once payment exposure is possible, Cancel cannot release the coins. PayNow requires a separate request.
The seven commits follow dependency order: test setup, shared relay transport, controller error handling, persistence, funding, processing, and wallet UI. Fixes and regression tests are included with the code they protect.
Disclosure: developed with assistance from Claude Code and Codex.