Migrate payjoin-cli from sled to rusqlite#873
Conversation
Pull Request Test Coverage Report for Build 17267118353Details
💛 - Coveralls |
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
This looks great. I appreciate the attention to detail updating the docs and examples.
I have a few suggestions for the DB schema, mainly I don't think SessionId::generate() is the right approach, although hashing low entropy values like time and thread IDs is unlikely to collide since a hash function is applied it's not guaranteed. If we go that route UUIDs would be better, but I would much prefer if we relied on the database to generate sequence numbers, i.e. INTEGER PRIMARY KEY AUTOINCREMENT
This is only an initial round of comments, I only skimmed the code and focused on the DB schema for now
|
Also, since we're before a 1.0.0 release, I think we can skip any kind of migration by supporting both databases, but then we should probably document somewhere that any ongoing sessions with the old DB need to be completed with the old version Alternatively something to dump session events out of sled and into sqlite could be considered, but i would prefer the simpler approach of not supporting that, since sessions expire it's not a big deal IMO. |
|
Agree on deciding not to bother with migration |
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
Couple code related comments. Overall this looks like the right approach.
Two other things:
- Could you please squash your commits into one. I noticed CI was failing for some of them. And Please try to make formatting/linting changes historically in the commit that introduce the problem.
- I am not sure rustqlite latest supports our MSRV(1.63) but version 29.0 does (rusqlite/rusqlite@cbf49b4)
|
bbd523e to
69f40ef
Compare
|
Thanks for making the updates @Mshehu5 . I will take a look at this tomorrow. In the mean time can you please rebase agianst master and ensure the r2d2 dependencies does not break MSRV. Thanks |
|
On Tue, Jul 22, 2025, 18:00 spacebear ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In payjoin-cli/src/db/mod.rs
<#873 (comment)>:
> + conn.execute(
+ "CREATE TABLE IF NOT EXISTS send_sessions (
+ session_id INTEGER PRIMARY KEY AUTOINCREMENT,
+ completed_at INTEGER
+ )",
+ [],
+ )?;
+
+ conn.execute(
+ "CREATE TABLE IF NOT EXISTS receive_sessions (
+ session_id INTEGER PRIMARY KEY AUTOINCREMENT,
+ completed_at INTEGER
+ )",
+ [],
+ )?;
+
+ conn.execute(
+ "CREATE TABLE IF NOT EXISTS session_events (
+ id INTEGER PRIMARY KEY AUTOINCREMENT,
+ session_id INTEGER NOT NULL,
+ session_type TEXT NOT NULL,
+ event_data TEXT NOT NULL,
+ created_at INTEGER NOT NULL
+ )",
+ [],
+ )?;
Ah my bad, I missed that first round of comments. @nothingmuch
<https://github.com/nothingmuch> what's the rationale for separate tables?
I was facing a similar decision in BullBitcoinMobile where they already
use separate send/receive session tables from the pre-SEL implementation,
so in that case I'd just keep the existing separation. But writing a schema
from scratch I would think a shared table is cleaner?
Two tables feels like static type separation
Session type as string column feels like dynamic typing, opens up the
possibility for invalid data to be representable
NS1R multiparty might seem like a an argument in favour of not yet more
tables, but I suspect we might need more information for multiparty
sessions in their full generality (e.g. parameters set before any events),
separation by table means wouldn't have to deal with nullable columns etc,
since a single tble/relation can't represent sum types in sql, rows are
always product type
Dealing with multiple tables by name is not very different than a column
denoting the type, so i don't see a big downside
|
This is still an issue with the current approach, except the dynamic type is now the Events instead of the Sessions (Event needs to know whether the session ID is from the receiver or sender table). Or were you implying we use two mutually exclusive, nullable foreign key columns in the Events table, i.e. receiver_session and sender_session? Or even separate event tables? |
Yes, I haven't caught up reviewing this yet since I posted that feedback, sounds like it's one events table with multiple session tables in the current state? Yes, I meant to imply by that comment that each session table should have its own events table, too |
cbb0e6d to
fc7098e
Compare
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
Sorry for my unclear suggestion in the last round, the session_type columns should be eliminated entirely, otherwise session_events.session_id can't really be a foreign key
fc7098e to
c6a5511
Compare
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
This PR is in really good shape. I just had a couple nits and questions
| let mut events = Vec::new(); | ||
| for event_row in event_rows { | ||
| let event_data = event_row?; | ||
| let event: ReceiverSessionEvent = | ||
| serde_json::from_str(&event_data).map_err(Error::Deserialize)?; | ||
| events.push(event); | ||
| } | ||
|
|
||
| Ok(Box::new(events.into_iter())) |
There was a problem hiding this comment.
Is there anyway to lazily deserialize this?
Ok(Box::new(event_rows.map(|row| {
row.and_then(|data| {
serde_json::from_str::<ReceiverSessionEvent>(&data).map_err(Error::Deserialize)
})
})))This might not work bc the return type for this method is not Item=Result<...> in which case is it fine to just use expect() instead map_err(Error::Deserialize)? If we cannot deserialize an event the database is corrupted and I think that is situtation where a panic is appropirate.
There was a problem hiding this comment.
I made changes to the code and did this :
`
let events: Vec<SenderSessionEvent> = event_rows
.map(|row| {
let event_data = row.expect("Failed to read event data from database");
serde_json::from_str::<SenderSessionEvent>(&event_data)
.expect("Database corruption: failed to deserialize session event")
})
.collect();
Ok(Box::new(events.into_iter()))`
is this implementation oKay?
7bb1750 to
77c70ec
Compare
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
@Mshehu5 e2e tests seem to be passing but my manual smoke testing is failing
Got a request from the sender. Responding with a Payjoin proposal.
Fallback transaction received. Consider broadcasting this to get paid if the Payjoin fails:
020000000001015ef1cd09f4429aa5474d29f2bd5eb24beec945ee06f3619cd75d35019d5324090000000000fdffffff02b3bd402500000000160014146eaf160b122f82249868f48f25dccf5c53d0a2a08601000000000016001404281346f0fde7c11261b9311c42ade13ec318dc0247304402205c59ad8f13cd31803e18e0151e15d589d9ee8ec6c4038503ed4c1c24df5535c402203040fb1a876c496a28194546a8f4646504896cf57858b5949fd5e12d55fd886e0121034d97fb9811f63b1f194c67bbedd85dde211289eb85539d8c72d08b40183de10e00000000
[2025-08-19T15:49:12Z DEBUG reqwest::connect] starting new connection: https://pj.bobspacebkk.com/
[2025-08-19T15:49:12Z DEBUG rustls::client::hs] No cached session for DnsName("pj.bobspacebkk.com")
[2025-08-19T15:49:12Z DEBUG rustls::client::hs] Not resuming any session
[2025-08-19T15:49:12Z DEBUG rustls::client::hs] ALPN protocol is None
[2025-08-19T15:49:12Z DEBUG rustls::client::hs] Using ciphersuite TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384
[2025-08-19T15:49:12Z DEBUG rustls::client::tls12::server_hello] Server supports tickets
[2025-08-19T15:49:12Z DEBUG rustls::client::tls12] ECDHE curve is EcParameters { curve_type: NamedCurve, named_group: X25519 }
[2025-08-19T15:49:12Z DEBUG rustls::client::tls12] Server DNS name is DnsName("pj.bobspacebkk.com")
[2025-08-19T15:49:13Z DEBUG rustls::common_state] Sending warning alert CloseNotify
Error: Replied with error: {"errorCode":"original-psbt-rejected","message":"The receiver rejected the original PSBT."}
Are you able to re-create this? Just sending and receiving via payjoin-cli.
| pub(crate) const DB_PATH: &str = "payjoin.sqlite"; | ||
|
|
||
| pub(crate) struct Database(sled::Db); | ||
| pub(crate) struct Database { |
There was a problem hiding this comment.
Nit:
| pub(crate) struct Database { | |
| struct Database(Pool<SqliteConnectionManager>) |
There was a problem hiding this comment.
it seems the struct needs to be accessible within the crate so pub(crate) needs to be available but i have changed it to a tuple struct as you suggested
|
Also heads up there are some conflicts |
@arminsabouri Are there any conditions I need to recreate apart from sending and receiving? |
77c70ec to
39f1144
Compare
4eee871 to
01d44a6
Compare
|
@Mshehu5 I will give it another try either today or tommorrow |
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
tACK 01d44a6
Tested payjoin-cli locally and double checked the schemas manually. I pushed a commit updating the .gitignore. And there a few follow up items I'd like to see addressed. Lastly, there is a 1-2 conflicts @Mshehu5 can you please rebase your branch and resolve those conflicts and I will drop a re-ack. Thanks
| let session_id = session_row?; | ||
| session_ids.push(session_id); | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: noticing new lines added here and there. Please becareful to keep formatting changes outside of non formatting commits.
| match id { | ||
| SessionId::Send(_) => Ok(Self { db, session_id: id }), | ||
| SessionId::Receive(_) => | ||
| panic!("Attempted to create SenderPersister with Receive session ID"), |
There was a problem hiding this comment.
Something about this does not seem right to me. Perhaps we should have had stronger types for receiver and sender ids. And the sum type SessionId just wraps those. This can be left as a follow up.
There was a problem hiding this comment.
agreed, this seems like a holdover from when there was only one sessions table and a session type column
| }) | ||
| .collect(); | ||
|
|
||
| Ok(Box::new(events.into_iter())) |
There was a problem hiding this comment.
Another follow up item. I'd like to see the deserialization be done lazily.
This commit replaces the sled-based embedded database with SQLite, addressing the need for more robust, SQL-compatible persistence across payjoin-cli. This aligns with common practices in other projects (e.g., Liana, BullBitcoin) and makes payjoin-cli a better reference for downstream wallet developers. Key changes: Replace sled with rusqlite (v0.29.0) and r2d2_sqlite (v0.22.0) for pooled connection support Introduce a relational schema with three tables: sessions: tracks session_id and completed_at session_events: logs events, sorted by created_at, FK to sessions inputs_seen: stores seen inputs with timestamps Redesign SessionId as a fixed-length 8-byte identifier using a deterministic hash from system time and thread ID Improve error handling by simplifying enum variants and supporting JSON deserialization errors via From<serde_json::Error> Update config defaults and documentation: payjoin.sled → payjoin.sqlite Remove obsolete sled-based files and dependencies This migration preserves interface compatibility where possible and makes only the necessary changes to ensure smooth integration.
And remove sled dirs as they have now been replaced.
ab2511d to
114eb89
Compare
arminsabouri
left a comment
arminsabouri
left a comment
There was a problem hiding this comment.
Rebased against master
Re-ack
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
utACK, concurring with Armin's review feedback
| let session_id: i64 = row.get(0)?; | ||
| Ok(SessionId::Receive(session_id)) |
There was a problem hiding this comment.
nit: i think this can be simplified, although I'm confused about why i64 needs to be annoted explicitly in the line i suggest removing, i expected the type constructor to suffice for inference
| let session_id: i64 = row.get(0)?; | |
| Ok(SessionId::Receive(session_id)) | |
| row.get(0).map(SessionId::Receive) |
| match id { | ||
| SessionId::Send(_) => Ok(Self { db, session_id: id }), | ||
| SessionId::Receive(_) => | ||
| panic!("Attempted to create SenderPersister with Receive session ID"), |
There was a problem hiding this comment.
agreed, this seems like a holdover from when there was only one sessions table and a session type column
| let mut session_ids = Vec::new(); | ||
| for item in send_tree.iter() { | ||
| let (key, value) = item?; | ||
| let wrapper: SessionWrapper<SenderSessionEvent> = | ||
| serde_json::from_slice(&value).map_err(Error::Deserialize)?; | ||
| if wrapper.completed_at.is_some() { | ||
| continue; | ||
| } | ||
| session_ids.push(SessionId::new(u64::from_be_bytes( | ||
| key.as_ref().try_into().map_err(Error::TryFromSlice)?, | ||
| ))); | ||
| for session_row in session_rows { | ||
| let session_id = session_row?; | ||
| session_ids.push(session_id); | ||
| } | ||
|
|
||
| Ok(session_ids) |
There was a problem hiding this comment.
i believe this can just be a call to collect, collecting an Iter<Result<_>> into a Result<Vec<_>> is supported: https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.collect
| let mut session_ids = Vec::new(); | |
| for item in send_tree.iter() { | |
| let (key, value) = item?; | |
| let wrapper: SessionWrapper<SenderSessionEvent> = | |
| serde_json::from_slice(&value).map_err(Error::Deserialize)?; | |
| if wrapper.completed_at.is_some() { | |
| continue; | |
| } | |
| session_ids.push(SessionId::new(u64::from_be_bytes( | |
| key.as_ref().try_into().map_err(Error::TryFromSlice)?, | |
| ))); | |
| for session_row in session_rows { | |
| let session_id = session_row?; | |
| session_ids.push(session_id); | |
| } | |
| Ok(session_ids) | |
| session_rows.collect() |
6 participants
Description
This PR migrates the persistence layer in
payjoin-clifrom thesledembedded database torusqlite(v0.31.0), usingr2d2for connection pooling. The migration aims to improve reliability, enable structured relational data handling, and align the project with other ecosystem tools such as Liana and BullBitcoin.This change addresses issue #867, making
payjoin-clia more robust reference implementation for downstream wallet developers.Key Changes
Persistence Layer
sledwithrusqlite(v0.31.0) andr2d2_sqlite(v0.24.0).sledand its dependent types.Relational Schema (SQLite)
sessions: Tracks session ID (BLOB primary key) andcompleted_attimestamp.session_events: Logs session events; includes a sortablecreated_attimestamp and a foreign key tosessions.inputs_seen: Stores seen UTXOs withcreated_at, to support input deduplication.Session ID Redesign
Error Handling
NotFound,TryFromSlice.From<serde_json::Error>for better classification.Configuration Updates
payjoin.sledtopayjoin.sqlite.Impact
Testing
Closes #867