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.28% -2.69%
=========================================
Files 8 9 +1
Lines 2526 3344 +818
=========================================
Hits 277 277
- Misses 2249 3067 +818
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 |
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() | |
| } | |
| } |
| @@ -35,13 +40,77 @@ pub mod ohttp; | |||
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.
Mshehu5
force-pushed
the
payjion_persistance
branch
from
4a62566 to
2af68a8
Compare
- 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
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
| /// Error type for payjoin database operations | ||
| #[cfg(feature = "payjoin")] | ||
| #[derive(Debug)] | ||
| pub enum PayjoinDbError { |
There was a problem hiding this comment.
Unless these new error types are needed at this top level visibility, I'd move them into the payjoin::db module. it's in the name. payjoin::db::Error seems like the appropriate place/name for this type.
| bdk_redb = { version = "0.1.1", optional = true } | ||
| shlex = { version = "1.3.0", optional = true } | ||
| payjoin = { version = "=1.0.0-rc.1", features = ["v1", "v2", "io", "_test-utils"], optional = true} | ||
| payjoin = { version = "1.0.0-rc.2", features = ["v1", "v2", "io", "_test-utils"], optional = true} |
There was a problem hiding this comment.
Using the latest, payjoin version="0.25.0" for this and keep updating proper 1.0 is going to stay closer to the released api.
| // 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?
| .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
| relay: impl payjoin::IntoUrl, | ||
| persister: &impl SessionPersister<SessionEvent = SenderSessionEvent>, | ||
| blockchain_client: &BlockchainClient, | ||
| relay: String, |
There was a problem hiding this comment.
This change that loosens the contract doesn't make sense to me. Still want to enforce that the param is of the correct type even if you convert it to a string in the method body.
| 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
| 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?
| cli_opts.datadir.clone(), | ||
| wallet_name.clone(), |
There was a problem hiding this comment.
passing wallet and wallet_name which are cloned versions of the same type to this function seems very definitely like a hack. What the heck is going on here?
| let sender = payjoin::send::v2::SenderBuilder::new(original_psbt.clone(), uri) | ||
| .build_recommended(fee_rate)? | ||
| .save(&persister) | ||
| .map_err(|e| { | ||
| Error::Generic(format!( | ||
| "Failed to save the Payjoin v2 sender in the persister: {e}" | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
This kind of spacing-only change makes me think you haven't reviewed this line-by-line yourself before asking for review here. Please make sure these kinds of formatting issues you can find yourself are addressed beforehand.
| 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,
} 
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: