Payjoin persistence#242
Conversation
Pull Request Test Coverage Report for Build 22068931916Details
💛 - Coveralls |
Mshehu5
force-pushed
the
payjion_persistance
branch
from
7e4ffd1 to
ef73b66
Compare
Mshehu5
force-pushed
the
payjion_persistance
branch
4 times, most recently
from
a5e2482 to
c79bd95
Compare
tvpeter
left a comment
tvpeter
left a comment
There was a problem hiding this comment.
Thank you @Mshehu5 for working on this feature.
Below are some of the observations that I think will make the implementation better:
- I noticed that the implementation used only the
sqlitedb, and sincesqliteis a db option in the project, I think it will be ideal if you also include implementation forredbdb so users are free to choose any to use. - The implementation does not provide for pruning/cleanup of data. Given that the db will tend to grow linearly and become sizable over time, I think it will be great to consider adding a pruning subcommand or mechanism that deletes irrelevant data.
- Since the
save_eventis generated at multiple transitions in a session, I think it will be great if you consider grouping such updates in a db transaction to ensure that entries are saved successfully.
I have also left some comments in the code.
Thank you.
Mshehu5
force-pushed
the
payjion_persistance
branch
2 times, most recently
from
8f4b815 to
b6b7cb4
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #242 +/- ##
===========================================
+ Coverage 10.96% 26.28% +15.31%
===========================================
Files 8 9 +1
Lines 2526 3584 +1058
===========================================
+ Hits 277 942 +665
- Misses 2249 2642 +393
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Mshehu5
force-pushed
the
payjion_persistance
branch
from
b6b7cb4 to
c012c3a
Compare
|
Thank you for the review! Just wanted some clarifications
For this feature, how would you like it to be implemented? For example should we allow selecting a specific date or session to start pruning from? Or should it apply to all sessions older than a certain date? Alternatively should it target only expired or unsuccessful sessions?
Also for the redb implementation most of the existing integrations have been built around SQLite so it made sense as the initial approach for this implementation. The Redb support may require some additional consideration and it would be good to align with the PayJoin team to ensure the approach is correct and consistent. It would also be helpful to understand what you consider a hard requirement for this PR versus what can be iterated on in subsequent changes. |
va-an
left a comment
va-an
left a comment
There was a problem hiding this comment.
Hi @Mshehu5, thanks for this!
Could you update payjoin dependency to 1.0.0-rc.2 and run just pre-push?
I've got some errors from just pre-push like:
error[E0061]: this method takes 1 argument but 2 arguments were supplied
--> src/payjoin/mod.rs:691:30
|
691 | ... .check_payment(
| ^^^^^^^^^^^^^
...
704 | / ... |outpoint| {
705 | | ... let utxo = self.wallet.get_utxo(outpoint);
706 | | ... match utxo {
707 | | ... Some(_) => Ok(false),
... |
710 | | ... }
| |___________________________- unexpected argument #2 of type `{closure@src/payjoin/mod.rs:704:33: 704:43}`
Mshehu5
force-pushed
the
payjion_persistance
branch
3 times, most recently
from
7d7be05 to
6b06cf7
Compare
|
@va-an Thanks for the review! I bumped Payjoin to 1.0.0-rc.2 and just pre-push passes locally. Mind giving it a try on your end? Would appreciate your feedback |
|
@tvpeter Would really appreciate your feedback on this: #242 (comment) It would help move this PR forward. |
I think it should apply to all sessions within a specific timeframe, say anything older than 30 days, and if it can be implemented without adding another command, that would be great. This can even go into another PR.
That is still fine. It can form another PR, but it's one of the things you can consider adding.
The current implementation covering SQLite is good enough (and the most important). Those others can serve as improvements to the persistence feature. |
Mshehu5
force-pushed
the
payjion_persistance
branch
from
db0ccec to
9a62b1e
Compare
|
@Mshehu5 please rebase |
Mshehu5
force-pushed
the
payjion_persistance
branch
from
d22d8b0 to
4a62566
Compare
|
Hello @notmandatory A review/feedback from you on this PR will really help push this forward |
|
@evanlinjin this might be a PR for us to look at together this week |
va-an
left a comment
va-an
left a comment
There was a problem hiding this comment.
Tried end-to-end on regtest with pj.bobspacebkk.com. The URI is generated and the persister state is saved correctly, but the first poll request fails:
-> % RUST_LOG=debug cargo run --features rpc -- \
wallet --wallet payjoin_wallet1 \
receive_payjoin \
--amount 400000 \
--max_fee_rate 1000 \
--directory "https://payjo.in" \
--ohttp_relay "https://pj.bobspacebkk.com"
...
[2026-05-26T20:05:14Z DEBUG bdk_cli] network: Testnet
[2026-05-26T20:05:14Z DEBUG bdk_cli::handlers] Sqlite database opened successfully
[2026-05-26T20:05:14Z DEBUG reqwest::connect] starting new connection: https://payjo.in/
[2026-05-26T20:05:14Z DEBUG reqwest::connect] proxy(https://pj.bobspacebkk.com/) intercepts 'https://payjo.in/'
Request Payjoin by sharing this Payjoin Uri:
bitcoin:bcrt1qn0wg553k3yfeg56535tej03wxv5yftzx4gpvpe?amount=0.004&pjos=0&pj=HTTPS://PAYJO.IN/SM6XG5LDPYGLC%23EX10D8PW6S-OH1QYPFLM8XL59R0XV4VGPLS7FRDSSM4TUXL07TXCWC4S0GLVLNK2SE4NQ-RK1QFF9Z8Y93K4TUVGJAHR9AX27SDPSJ88FY8HAC7SQ9DFCMCTJ5LSTZ
[2026-05-26T20:05:15Z DEBUG reqwest::connect] starting new connection: https://pj.bobspacebkk.com/
[2026-05-26T20:05:15Z ERROR bdk_cli] Reqwest error: error sending request for url (https://pj.bobspacebkk.com/https://payjo.in/)
After downgrading Cargo.toml to reqwest = "0.12.23" (the version used by the reference payjoin-cli), the polling succeeds and the CLI hangs waiting for the sender as expected:
-> % RUST_LOG=debug cargo run --features rpc -- \
wallet --wallet payjoin_wallet1 \
receive_payjoin \
--amount 400000 \
--max_fee_rate 1000 \
--directory "https://payjo.in" \
--ohttp_relay "https://pj.bobspacebkk.com"
...
[2026-05-26T20:04:24Z DEBUG bdk_cli] network: Testnet
[2026-05-26T20:04:24Z DEBUG bdk_cli::handlers] Sqlite database opened successfully
[2026-05-26T20:04:24Z DEBUG reqwest::connect] starting new connection: https://payjo.in/
[2026-05-26T20:04:24Z DEBUG reqwest::connect] proxy(https://pj.bobspacebkk.com/) intercepts 'https://payjo.in/'
Request Payjoin by sharing this Payjoin Uri:
bitcoin:bcrt1qn0wg553k3yfeg56535tej03wxv5yftzx4gpvpe?amount=0.004&pjos=0&pj=HTTPS://PAYJO.IN/7X88PY7FGVM07%23EX1F98PW6S-OH1QYPFLM8XL59R0XV4VGPLS7FRDSSM4TUXL07TXCWC4S0GLVLNK2SE4NQ-RK1Q2DC9EW9ECFDEZJ964H9ELY83F8HM7P3AFSJSFNW6VY9D3RPXE80X
[2026-05-26T20:04:25Z DEBUG reqwest::connect] starting new connection: https://pj.bobspacebkk.com/
^C
The reqwest 0.13.2 bump wasn't introduced by this PR (likely came in via a rebase on master), but as it stands the code doesn't work end-to-end.
Mshehu5
force-pushed
the
payjion_persistance
branch
2 times, most recently
from
2af68a8 to
e544ac2
Compare
DanGould
left a comment
DanGould
left a comment
There was a problem hiding this comment.
Went through this in prep for mob review, although I don't have.
The lack of any explained decision motivation in the commit messages is the fundamental to change first whenever asking for review. Whoever reads the history can see what is done. The commit message exists to tell the history of WHY you made changes. Tell me what you considered so I don't have to imagine.
Some things we'll talk about:
- appropriate use of modules
- rebasing
- Encapsulating payjoin business from cli business
- using the right version
- The right abstraction level for db vs a payjoin-specific storage/manager type
| // TODO: Implement proper persister. | ||
| let persister = payjoin::persist::NoopSessionPersister::<ReceiverSessionEvent>::default(); | ||
|
|
||
| let persister = crate::payjoin::db::ReceiverPersister::new(self.db.clone())?; |
There was a problem hiding this comment.
Seems like the PayjoinManager could encapsulate the ReceiverPersister around the db itself. Why not?
| Error::Generic(format!( | ||
| "Failed to replay sender event log: {:?}", | ||
| e | ||
| )) |
There was a problem hiding this comment.
Why generic error instead of panic if session nonexistence otherwise panics?
There was a problem hiding this comment.
saw it in the reference implementation so I just went that direction. looked like the preferred
https://github.com/payjoin/rust-payjoin/blob/fb884d11996acea9d9bd5573b32b549b1bfdc334/payjoin-cli/src/app/v2/mod.rs#L230
| .expect("A relay should already be selected") | ||
| let (req, ctx) = receiver | ||
| .create_post_request( | ||
| self.unwrap_relay_or_else_fetch(vec![], None::<&str>) |
There was a problem hiding this comment.
No relay no directory? What's the target of this request? Why not use the fetch function directly if not relay manager?
There was a problem hiding this comment.
This is how the reference does it payjoin/rust-payjoin#1582, though I note the payjoin-cli reference has no vec![] which adds further confusion to my eye
There was a problem hiding this comment.
vec![] here is not the request target. In this receiver flow the relay was already selected earlier when we fetched OHTTP keys and that relay is stored in relay_manager so this call just reuses it.
The reason payjoin-cli does not need the extra Vec is that it reads relay candidates from config inside the helper. In bdk-cli, relay candidates come from CLI input so the shared helper takes them as a parameter even though this specific receiver call site does not use them.
I could break down or add a function to just do select relay instead of trying to use unwrap_relay_or_else_fetch and having to settle the vec![] for thr function signature
| Ok((receiver_state, _)) => { | ||
| println!("Resuming receiver session {}", session_id); | ||
| match tokio::time::timeout( | ||
| std::time::Duration::from_secs(30), |
There was a problem hiding this comment.
why time out after 30 seconds? Seems brief since payjoins are long-running and don't expire by default for 24h
There was a problem hiding this comment.
I forgot to talk about this during mob review because it was one of the tradeoff/design decisions in this PR.
This timeout is for a single session resume attempt not the session lifetime. I kept resume synchronous unlike payjoin-cli because the wallet in this codebase is currently handled as a single mutable resource. Making resume concurrent would require a broader refactor around shared wallet access and locking which felt too large (may change alot of things and how the maintainers intend codebase to be ) and beyond the scope of this PR (This is already a big PR ).
Given that, the 30-second outer timeout is there to prevent one resumed session from hanging indefinitely and blocking the rest of the queue. The payjoin session itself is still persisted and can be resumed again later this does not replace the protocol’s longer expiration window.
There was a problem hiding this comment.
I forgot to talk about this during mob review because it was one of the tradeoff/design decisions in this PR.
This timeout is for a single session resume attempt not the session lifetime. I kept resume synchronous unlike payjoin-cli because the wallet in this codebase is currently handled as a single mutable resource. Making resume concurrent would require a broader refactor around shared wallet access and locking which felt too large (may change alot of things and how the maintainers intend codebase to be ) and beyond the scope of this PR (This is already a big PR ).
Given that, the 30-second outer timeout is there to prevent one resumed session from hanging indefinitely and blocking the rest of the queue. The payjoin session itself is still persisted and can be resumed again later this does not replace the protocol’s longer expiration window.
| feature = "cbf", | ||
| feature = "rpc" | ||
| ))] | ||
| #[cfg(any(feature = "redb", feature = "compiler"))] |
There was a problem hiding this comment.
This seems like it's touching things outside of the scope of this PR. Are you sure these feature gates can be removed?
There was a problem hiding this comment.
This is related to #242 (comment) . handlers.rs no longer constructs Arc<Mutex> or opens the Payjoin DB before each PayjoinManager::new call. That setup now lives inside PayjoinManager::new.
After that refactor, Arc is no longer used in handlers.rs for rpc/cbf/electrum/esplora and Mutex is not used there at all. The remaining Arc uses are only under redb and compiler so the narrowed cfg matches the actual users.
| pub struct Database { | ||
| conn: Mutex<Connection>, | ||
| } |
There was a problem hiding this comment.
This struct does not earn it's keep and neither does the Mutex.
I think it would make more sense just to have separate functions instead of methods on Database (that doesn't need to be there).
The ...Persister types should just be:
stuct XxxPersister {
conn: Rc<Connection>,
session_id: SessionId,
} 
Mshehu5
force-pushed
the
payjion_persistance
branch
5 times, most recently
from
41348d3 to
d80c63a
Compare
Persist payjoin sender and receiver state in SQLite so interrupted payjoin sessions can be resumed after the CLI exits. Add dedicated tables for send and receive sessions, append-only event logs for state replay, receiver pubkey lookup for sender sessions, and seen-input tracking for replay protection. This follows the intended async payjoin design by persisting session state across interruptions. SQLite keeps the initial persistence backend simple and builds on existing rusqlite support, at the cost of a small payjoin-specific schema and serialization layer.
Mshehu5
force-pushed
the
payjion_persistance
branch
2 times, most recently
from
1239f7b to
071c5bb
Compare
Wire the payjoin persistence layer into the existing send and receive flows so session state is saved during normal operation and can be replayed later. Initialize the database from the handlers, replace the noop persisters with SQLite-backed persisters, resume existing sessions, and record seen inputs in the receiver flow for replay protection. This moves persistence from a standalone storage layer into the runtime payjoin workflow so interrupted sessions can continue from saved state instead of starting over. It also simplifies error handling by relying on `?` once the storage errors map cleanly into the CLI error type.
Expose persisted payjoin session state through CLI commands so users can recover interrupted sessions and review prior session progress. Add `resume` to continue pending sender and receiver sessions, `history` to list saved sessions, status text helpers for clearer output, and session ID filtering to target a specific session. This improves recovery and troubleshooting for long-running async payjoin flows by making persisted session state available from the CLI.
Update the README to cover the new `resume` and `history` commands so users can recover interrupted payjoin sessions and inspect saved session state. Also add the SQLite dependency needed for the payjoin persistence workflow. This keeps the user-facing setup and command documentation aligned with the new persistence and recovery behavior.
Move to payjoin 0.25.0 to stay closer to the latest release. Enable rustls for the direct reqwest client because Payjoin polling uses bdk-cli's own HTTPS client and without a TLS backend the first relay poll fails. Refine the skipped monitoring status message for the receiver flow.
Delete payjoin sessions older than 30 days when the payjoin database is accessed. Remove related event rows in the same cleanup pass.
Mshehu5
force-pushed
the
payjion_persistance
branch
2 times, most recently
from
418bd64 to
06c4ae7
Compare
Cover the new payjoin persistence paths with focused unit tests. Exercise replay protection, sender and receiver event persistence, session pruning and history rendering. These cases were chosen because the PR's new behavior is concentrated in the persistence layer and session history path
Mshehu5
force-pushed
the
payjion_persistance
branch
from
06c4ae7 to
54ae896
Compare
7 participants
Description
#230 needed to be merged for this to go through
Address #149 also follow up to #200
This PR adds persistance to existing async payjoin integration
This introduces neccessary database model and tables also add commad for resume to allow interrupted sessions to be continued also a particular session either send or receive.
A history commad to view payjoin history and status has been added
Notes to the reviewers
Step to review this include making a payjoin transaction
Run a receiver to get a BIP21 URI then pass it to the sender as seen in docs
bdk-cli/README.md
Lines 121 to 141 in b9cf2ac
To test resumption, interrupt either side with Ctrl+C mid-session then run resume on that side to continue. A few scenarios worth covering: receiver resuming after interrupt and sender resuming after interrupt. Use history after each scenario to confirm the session state was persisted correctly.
docs for this can be seen in 7e4ffd1
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features: