Guard concurrent sends with exclusive DB lock and URI/RK checks by Mshehu5 · Pull Request #1376 · payjoin/rust-payjoin

Mshehu5 · 2026-03-02T15:13:40Z

This PR Closes #1032

As discussed in the issue two concurrent send commands targeting the same URI could select different coins. PRAGMA locking_mode = EXCLUSIVE has been set as stated to make only one process holds write access at a time.

And also check to make sure URI and recceiver pubkey which are in use for another session is not reused

Co-authored by claude sonnet 4.6

Pull Request Checklist

Please confirm the following before requesting review:

I have disclosed my use of
AI
in the body of this PR.
I have read CONTRIBUTING.md and rebased my branch to produce hygienic commits.

coveralls · 2026-03-02T15:17:09Z

Pull Request Test Coverage Report for Build 23953519233

Details

65 of 71 (91.55%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.04%) to 84.381%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
payjoin-cli/src/app/v2/mod.rs	0	1	0.0%
payjoin-cli/src/db/v2.rs	62	63	98.41%
payjoin-cli/src/db/error.rs	0	4	0.0%

Totals
Change from base Build 23927050434:	0.04%
Covered Lines:	10859
Relevant Lines:	12869

💛 - Coveralls

zealsham

tAck!
Can you rebase as this branch is almost a month old.

zealsham · 2026-03-24T00:25:02Z

payjoin-cli/src/db/v2.rs

        // Create a new session in send_sessions and get its ID
        let session_id: i64 = conn.query_row(
-            "INSERT INTO send_sessions (session_id, receiver_pubkey) VALUES (NULL, ?1) RETURNING session_id",
-            params![receiver_pubkey.to_compressed_bytes()],
+            "INSERT INTO send_sessions (pj_uri, receiver_pubkey) VALUES (?1, ?2) RETURNING session_id",
+            params![pj_uri, &receiver_pubkey_bytes.as_slice()],


I don't think .as_slice() does anything here as the receiver_pubkey_bytes is already [u8]

payjoin-cli/src/db/v2.rs

zealsham

ACK! , it still needs another pair of eyes before we can get it in

tested and got this

✦ ❯ ./../../target/debug/payjoin-cli send  "bitcoin:bcrt1qdlfxfpmyn08g6wytw0qqq623mn7463ezplreaq?amount=0.00001&pjos=0&pj=HTTPS://PAYJO.IN/FZRC3CRDXV826%23EX1VEHUG6G-OH1QYPFLM8XL59R0XV4VGPLS7FRDSSM4TUXL07TXCWC4S0GLVLNK2SE4NQ-RK1QV6EYM8CPS9DL5CZKYGC2WWNLNL8D00PCPZ3M83K70ATLGC2GAD6Q " --fee-rate=1 &  ./../../target/debug/payjoin-cli send  "bitcoin:bcrt1qdlfxfpmyn08g6wytw0qqq623mn7463ezplreaq?amount=0.00001&pjos=0&pj=HTTPS://PAYJO.IN/FZRC3CRDXV826%23EX1VEHUG6G-OH1QYPFLM8XL59R0XV4VGPLS7FRDSSM4TUXL07TXCWC4S0GLVLNK2SE4NQ-RK1QV6EYM8CPS9DL5CZKYGC2WWNLNL8D00PCPZ3M83K70ATLGC2GAD6Q " --fee-rate=1
[1] 62106
Error: Database operation failed: database is locked

Caused by:
    0: database is locked
    1: Error code 5: The database file is locked

rust-payjoin/payjoin-cli/sender on  guard_send [?] took 5s 
✦ ❯ Bootstrapping private network transport over Oblivious HTTP
Posted original proposal...
Proposal received. Processing...
Payjoin sent. TXID: af8dee914cbb9608723e49a88be8a5e70e0f62a442db9d6b79c53740bd56172a

[1]  + 62106 done       ./../../target/debug/payjoin-cli send  --fee-rate=1

with only one of the transaction going through which is intended as per implementation

nothingmuch

i think completed sessions should still not allow reuse

Specific carveouts for canceled or failed sessions can be added, but we should still do that carefully, for example if two non-double-spending fallback transactions are given, that can result in the sender overpaying. this PR currently makes doing that unintentionally more difficult, but not impossible, IMO it should be impossible

nothingmuch · 2026-03-29T16:08:29Z

payjoin-cli/src/db/mod.rs

    pub(crate) fn create(path: impl AsRef<Path>) -> Result<Self> {
-        let manager = SqliteConnectionManager::file(path.as_ref());
+        let manager = SqliteConnectionManager::file(path.as_ref())
+            .with_init(|conn| conn.execute_batch("PRAGMA locking_mode = EXCLUSIVE;"));


isn't it more appropriate to put this in init_schema? or was this done deliberately to upgrade existing databases? if so some explanatory comment should be given

thanks for the clarification, i thought it was a schema level pragma

nothingmuch · 2026-03-29T16:10:54Z

payjoin-cli/src/db/v2.rs

+                EXISTS(SELECT 1 FROM send_sessions WHERE completed_at IS NULL AND pj_uri = ?1), \
+                EXISTS(SELECT 1 FROM send_sessions WHERE completed_at IS NULL AND receiver_pubkey = ?2)",


what's the rationale for completed_at IS NULL? if a completed session with a duplicate session exists shouldn't this also ensure that it was canceled in order to allow reusing the URI?

true, this has been removed

nothingmuch · 2026-03-29T16:12:00Z

payjoin-cli/Cargo.toml

 anyhow = "1.0.99"
 async-trait = "0.1.89"
-bitcoind-async-client = { git = "https://github.com/benalleng/bitcoind-async-client.git", rev = "a6292420de5cfd666d6185c534bea3f22736452b" }
+bitcoind-async-client = { git = "https://github.com/benalleng/bitcoind-async-client.git", rev = "96a4bde287c397981faba74896dd3ceff6bd58e4" }


is this change intended?

also for this bitcoind-async-client had an issues so we had to use a fork of ben allen at the time so i had to make the change to pass CI this has been fixed in #1441

nothingmuch · 2026-03-29T16:14:09Z

payjoin-cli/src/db/v2.rs

+        );
+    }
+
+    /// After a session is marked completed, a new session with the same URI should be allowed.


i disagree, that may result in address reuse, which is very bad, as well as key reuse of the HPKE receiver key, which is arguably not as bad but still undesirable

changes have been so even after a completed session, a new session with the same URI will still be rejected

nothingmuch

utACK except for minor nits

i'm confused about the weird formatting of that removed line, i would have expected CI to complain about that

nothingmuch · 2026-04-03T16:01:27Z

payjoin-cli/src/db/v2.rs


    pub fn from_id(db: Arc<Database>, id: SessionId) -> Self { Self { db, session_id: id } }
 }
-


i think this needs rustfmt

seems according to rustfmt rules this is the right way to write it
also tried adding the line but rust fmt removed it

nothingmuch · 2026-04-03T16:01:37Z

payjoin-cli/src/db/v2.rs

 }

-#[derive(Clone)]
+#[derive(Clone, Debug)]


is the debug still necessary?

still needed for test, when removed test error

nothingmuch · 2026-04-03T16:03:35Z

payjoin-cli/src/app/v2/mod.rs

        for session_id in send_session_ids {
-            let sender_persiter = SenderPersister::from_id(self.db.clone(), session_id.clone());
-            match replay_sender_event_log(&sender_persiter) {
+            let sender_persister = SenderPersister::from_id(self.db.clone(), session_id.clone());


in this case it doesn't matter but as i'm lookinmg at this change for the 3rd time and remembering it's just a typo fix that's unrelated, it'd be helpful if in the future this kind of change was in a separate commit

Two concurrent send commands targeting the same URI could select different coins. Set PRAGMA locking_mode = EXCLUSIVE so only one process holds write access at a time. Check uniqueness of URI and recceiver pubkey with duplicate checks

Mshehu5 mentioned this pull request Mar 2, 2026

Standup Input: Week of 2026-03-02 #1372

Closed

Mshehu5 marked this pull request as ready for review March 3, 2026 13:57

zealsham reviewed Mar 24, 2026

View reviewed changes

Mshehu5 force-pushed the guard_send branch 2 times, most recently from 39a1c67 to cc8e38d Compare March 24, 2026 13:15

zealsham approved these changes Mar 25, 2026

View reviewed changes

nothingmuch suggested changes Mar 29, 2026

View reviewed changes

Mshehu5 force-pushed the guard_send branch from cc8e38d to 3e00082 Compare March 30, 2026 00:04

Mshehu5 requested a review from nothingmuch April 3, 2026 15:46

nothingmuch reviewed Apr 3, 2026

View reviewed changes

Mshehu5 force-pushed the guard_send branch from 3e00082 to 4d37434 Compare April 3, 2026 16:25

Mshehu5 force-pushed the guard_send branch from 4d37434 to 767fb3e Compare April 3, 2026 16:28

nothingmuch approved these changes Apr 3, 2026

View reviewed changes

		EXISTS(SELECT 1 FROM send_sessions WHERE completed_at IS NULL AND pj_uri = ?1), \
		EXISTS(SELECT 1 FROM send_sessions WHERE completed_at IS NULL AND receiver_pubkey = ?2)",


		pub fn from_id(db: Arc<Database>, id: SessionId) -> Self { Self { db, session_id: id } }
		}

Conversation

Mshehu5 commented Mar 2, 2026

Uh oh!

coveralls commented Mar 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 23953519233

Details

💛 - Coveralls

Uh oh!

zealsham left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zealsham left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nothingmuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mshehu5 Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coveralls commented Mar 2, 2026 •

edited

Loading

zealsham left a comment •

edited

Loading

Mshehu5 Apr 3, 2026 •

edited

Loading