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% 8.75% -2.22%
=========================================
Files 8 9 +1
Lines 2526 3165 +639
=========================================
Hits 277 277
- Misses 2249 2888 +639
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
force-pushed
the
payjion_persistance
branch
2 times, most recently
from
52c05a9 to
d22d8b0
Compare
|
@Mshehu5 please rebase |
- Create db.rs with Database, SenderPersister and ReceiverPersister - Implement SessionPersister trait for both sender and receiver - Add session ID management and query methods - Add input deduplication tracking to prevent probing attacks - Add timestamp formatting utilities
- Add database initialization in handlers - Replace NoopSessionPersister with real persisters - Implement session resumption for existing sessions - Add input-seen-before tracking in receiver flow - Remove verbose error wrapping (use ? operator)
- Implement resume_payjoins() to continue pending sessions - Add history() to display all session states - Add session status text helpers for UI display - Support filtering by session ID
- Document resume and history commands - Add sqlite dependecy for payjoin
Update the lockfile and refine the skipped monitoring status.
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
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.
| // TODO: Implement proper persister. | ||
| let persister = | ||
| payjoin::persist::NoopSessionPersister::<SenderSessionEvent>::default(); | ||
| use payjoin::send::v2::replay_event_log as replay_sender_event_log; |
There was a problem hiding this comment.
This import is already on line 22.
| use payjoin::send::v2::replay_event_log as replay_sender_event_log; |
| impl core::ops::Deref for SessionId { | ||
| type Target = i64; | ||
| fn deref(&self) -> &Self::Target { | ||
| &self.0 | ||
| } | ||
| } |
There was a problem hiding this comment.
impl Deref is only used to unwrap the inner i64 for params!. Per C-DEREF, Deref should be implemented only for smart pointers - not for newtypes.
Cleaner: impl ToSql for SessionId (delegating to self.0), then params![self.session_id, ...] works directly. Drop the Deref impl and remove * from every params![*session_id, ...] call site.
| impl core::ops::Deref for SessionId { | |
| type Target = i64; | |
| fn deref(&self) -> &Self::Target { | |
| &self.0 | |
| } | |
| } | |
| impl ToSql for SessionId { | |
| fn to_sql(&self) -> bdk_wallet::rusqlite::Result<ToSqlOutput<'_>> { | |
| self.0.to_sql() | |
| } | |
| } |
| /// TODO: At the time of writing, this struct is written to make a Persister implementation easier | ||
| /// but the persister is not implemented yet! For instance [`PayjoinManager::proceed_sender_session`] and | ||
| /// [`PayjoinManager::proceed_receiver_session`] are designed such that the manager can enable | ||
| /// resuming ongoing payjoins are well. So... this is a TODO for implementing persister. |
There was a problem hiding this comment.
Stale TODO.
| let relay_manager = Arc::new(Mutex::new(RelayManager::new())); | ||
| let db = open_payjoin_db(datadir, &wallet_name)?; | ||
| let mut payjoin_manager = PayjoinManager::new(wallet, relay_manager, db); | ||
| return payjoin_manager | ||
| .resume_payjoins(directory, ohttp_relay, session_id, client) | ||
| .await; |
There was a problem hiding this comment.
The same 3-line boilerplate is repeated before every PayjoinManager::new:
let relay_manager = Arc::new(Mutex::new(RelayManager::new()));
let db = open_payjoin_db(datadir.clone(), &wallet_name)?;
let mut payjoin_manager = PayjoinManager::new(wallet, relay_manager, db);Both db and relay_manager are used only by PayjoinManager - callers don't touch them. Move construction inside new:
pub fn new(
wallet: &'a mut Wallet,
datadir: Option<PathBuf>,
wallet_name: &str,
) -> Result<Self, Error> {
let db = open_payjoin_db(datadir, wallet_name)?;
let relay_manager = Arc::new(Mutex::new(RelayManager::new()));
Ok(Self { wallet, relay_manager, db })
}Callers collapse to one line.
| Ok(dir) | ||
| } | ||
| #[cfg(feature = "payjoin")] | ||
| pub fn open_payjoin_db( |
There was a problem hiding this comment.
This function should be moved to payjoin/db.rs.
6 participants
Description
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: