Persist payjoin sender session and resume on cold restart#809
Persist payjoin sender session and resume on cold restart#809Sandipmandal25 wants to merge 8 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughWallet data now stores payjoin sender sessions, the payjoin actor persists session state and handles terminal outcomes, and wallet startup can resume stored sessions or replay their saved fallback transaction. ChangesPersisted Payjoin Session Flow
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rust/src/manager/wallet_manager/payjoin.rs (1)
412-413: 🗄️ Data Integrity & Integration | 🟠 MajorPersist the POST transition before sending the request
create_v2_post_request()runs beforesender.process_response(&post_response, post_ctx).save(&persister)commits the new state, so a crash in that window leaves only the pre-POST session on disk and a restart can resend the original PSBT. Persist an in-flight/pre-send state first, or handle this restart path without re-posting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rust/src/manager/wallet_manager/payjoin.rs` around lines 412 - 413, The POST transition in payjoin flow is being persisted too late, after the request is already sent, which can leave only the pre-POST session on disk if the process crashes. Update the flow around create_v2_post_request() and sender.process_response(&post_response, post_ctx).save(&persister) so the in-flight/pre-send state is saved before the POST is issued, or make the restart logic in this path safe to resume without resending the original PSBT.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/database/wallet_data.rs`:
- Around line 84-89: Persist the session deadline in PayjoinSenderSession so
restarts don’t reset the timeout. Update the PayjoinSenderSession record in
wallet_data.rs to include an absolute created-at or expires-at timestamp
alongside events and fallback_tx, then change the resume logic that currently
rebuilds the deadline from Instant::now() + PAYJOIN_SESSION_TIMEOUT to derive
remaining time from the stored timestamp and immediately fall back when the
session is already expired.
In `@rust/src/manager/wallet_manager/actor/transactions.rs`:
- Around line 662-665: Make the payjoin cleanup in handle_payjoin_success
idempotent by ensuring the persisted session is cleared on every terminal
success path, not just after the main broadcast succeeds. Update the
broadcast_and_notify fallback flow and the proposal-broadcast result handling in
transactions.rs so that “already known/already broadcast” proposal outcomes are
treated as terminal success before any fallback is attempted, and
delete_payjoin_sender_session is still invoked for those accepted terminal
cases.
---
Outside diff comments:
In `@rust/src/manager/wallet_manager/payjoin.rs`:
- Around line 412-413: The POST transition in payjoin flow is being persisted
too late, after the request is already sent, which can leave only the pre-POST
session on disk if the process crashes. Update the flow around
create_v2_post_request() and sender.process_response(&post_response,
post_ctx).save(&persister) so the in-flight/pre-send state is saved before the
POST is issued, or make the restart logic in this path safe to resume without
resending the original PSBT.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e6d4e1ed-8eb4-4ea6-86ef-8a493853342f
📒 Files selected for processing (4)
rust/src/database/wallet_data.rsrust/src/manager/wallet_manager/actor.rsrust/src/manager/wallet_manager/actor/transactions.rsrust/src/manager/wallet_manager/payjoin.rs
d4fa3d4 to
47b72ad
Compare
89ac6d5 to
f45a931
Compare
9290c80 to
f4e736b
Compare
|
@coderabbitai re review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/src/manager/wallet_manager/actor.rs`:
- Around line 190-191: The `resume_session`/`SessionResumption` flow currently
collapses missing session and recovery failures into `SessionResumption::None`,
so `WalletManagerActor` silently no-ops in `actor.rs`. Update `resume_session`
to distinguish “no persisted session” from read/corruption failures by returning
a typed error outcome for failures, then handle that error in the `match` in
`WalletManagerActor` and route it through `notify_payjoin_error` instead of
treating it like `None`.
In `@rust/src/manager/wallet_manager/actor/transactions.rs`:
- Around line 716-728: Don’t emit the payjoin “complete” signal when cleanup
fails: in the transaction flow around self.db.delete_payjoin_sender_session(), a
failed cleanup currently still leads to sending Msg::PayjoinTxBroadcast even
though the session record remains and will block future sends. Update the
handling in this branch so the failure is surfaced as a cleanup/retry error or
the completion message is withheld until the retained session is actually
cleared, using the existing self.db.delete_payjoin_sender_session() and
Msg::PayjoinTxBroadcast path to locate the fix.
🪄 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: c4e5fd8b-ea81-4f00-93c5-083401996c0f
⛔ Files ignored due to path filters (1)
ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**
📒 Files selected for processing (8)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktrust/src/database/wallet_data.rsrust/src/manager/wallet_manager/actor.rsrust/src/manager/wallet_manager/actor/transactions.rsrust/src/manager/wallet_manager/payjoin.rsrust/src/node/client.rsrust/src/node/client/electrum.rsrust/src/node/client/esplora.rs
a535f2e to
95f4499
Compare
95f4499 to
c379946
Compare
44b37ce to
537014c
Compare
7eb718d to
75a985d
Compare
|
|
||
| if !can_cleanup { | ||
| return Produces::ok(Err(Error::SignAndBroadcastError( | ||
| "a previous payjoin session is pending recovery; please try again later" |
There was a problem hiding this comment.
Longer-term, this probably needs a user-visible recovery/abandon path. If the committed terminal tx is permanently rejected by the network, startup will keep retrying, get_transaction will keep saying it is not known, and this gate leaves the wallet unable to send with no way out other than manual database surgery.
There was a problem hiding this comment.
will track this as a follow up for a cancel button.
f76562a to
04a49d7
Compare
704037a to
af441ca
Compare
…join recovery paths
af441ca to
a538fd8
Compare
summary
this pr makes payjoin sender sessions recoverable after a cold restart.
the payjoin sender pr (#772) used
NoopSessionPersister, so any in flight payjoin session was lost if the app restarted before the negotiation finished. this implements a realSessionPersisterbacked byredb.changes
the session event log is now persisted before the first network request, along with the consensus-encoded fallback transaction. on startup, the stored event log is replayed so the app can continue the payjoin flow safely.
depending on the recovered state, the sender can now resume polling, broadcast an already completed proposal, or fall back to the original transaction if the session cannot be reconstructed.
Checklist
Summary by CodeRabbit
New Features
Bug Fixes