Uh oh!
There was an error while loading. Please reload this page.
Refactor sync_kyoto_client - #230
Conversation
Pull Request Test Coverage Report for Build 21587356847Details
💛 - Coveralls |
I tried to do a test a payjoin with CBF on I also got this error on the receiver side after trying the send a second time: The URI was using to send to is: I suspect the error is not related to this PR but maybe it could be? I was able to successfully do a payjoin tx when I tested #200 but that was on regtest and not signet. Is this something you can look into or @mehmetefeumit can help investigate? If you want to test with my already funded signet wallets the descriptors are below. I ended up setting up my own local signet node, syncing it, and then setting my CBF number of connections to 1. I also had to patch the code to manually set a peer pointing to my local signet node "127.0.0.1:38332". wallet "pj1" the sender: wallet "pj2" the receiver: |
notmandatory
commented
Jan 10, 2026
My patch to add the |
notmandatory
commented
Jan 10, 2026
Otherwise this PR looks good to me, especially good commit messages and PR description. |
Mshehu5
commented
Jan 11, 2026
Thank you very much for the review while woking on the PR I tested a CBF(kyoto) payjoin on regtest and it works using this patch: One thing to note is that on CBF (Kyoto) we cannot sync in monitor_payjoin_proposal. This is because it is designed to wait for blockchain events (new blocks, filters) not for mempool transactions unlike RPC which can directly query the mempool.(You can manually check the transaction though with So it seems the issue might be from signet I will try to access a signet server and test whether I also get the same results you are seeing. |
@notmandatory I made a Payjoin transaction on public signet i did not face any errors I also made transactions using command in the PR body and no errors firstly i will drop a script that i use to export variables needed for transactions i mainly just do The commands for payjoin : Make sure to sync wallets first sender sync: receive payjoin transaction: send payjoin transaction: for PAYJOIN_URI i usually export it to sender CLI after copying it once its displayed in receiver CLI these are the steps and config I followed to get a successful payjoin transaction on public signet |
tvpeter
left a comment
There was a problem hiding this comment.
Thank you, @Mshehu5, for your work on this. In addition to the comments I’ve left below, you can test the PR on Signet by using a VPN. That has been working for me if you are located in Nigeria.
Also, kindly rebase the code so that the wallet configuration changes are reflected, which will make testing this PR easier.
Thank you!
| ) | ||
| .save(persister) | ||
| .map_err(|e| { | ||
| Error::Generic(format!("Error occurred when saving after checking that sender has broadcasted the Payjoin transaction: {e}")) |
There was a problem hiding this comment.
While this was widely used in the previous payjoin PR, you can optimize the code a bit by adding Payjoin variant errors to the BDKCliError
There was a problem hiding this comment.
Yes, adding a variant would help optimize this but this particular error in the codebase is related to persistence. It would be better to add this variant in the intended persitence addition follow-up PR,where it fits the scope more appropriately.
I have however, added a fourth commit that introduces error variants for functions that return errors in the Payjoin crate. This slightly optimizes the code and removes the need for .map_err() calls.
Uh oh!
There was an error while loading. Please reload this page.
Refactor KyotoClient to use a KyotoClientHandle struct instead of Box<LightClient>. Previously, the Box<LightClient> was consumed when destructured in sync_kyoto_client, preventing the function from being called multiple times or borrowed. With the handle pattern, sync_kyoto_client now takes &mut KyotoClientHandle, allowing it to be borrowed and reused. This also allows the node to be started at creation time in new_blockchain_client rather than during sync, making the client ready for use immediately after creation. - Add KyotoClientHandle struct containing requester and update_subscriber - Move node startup and logger spawning to new_blockchain_client - Update sync_kyoto_client to accept &mut KyotoClientHandle instead of consuming Box<LightClient>
@tvpeter
the code has also been rebased on master which has the wallet configuration changes |
tvpeter
left a comment
There was a problem hiding this comment.
I've added one more suggestion. Please fix it so I can test.
Uh oh!
There was an error while loading. Please reload this page.
Update function signatures to accept &BlockchainClient instead of taking ownership. This refactoring to allow the client to be Referenced across multiple operations including repeated calls to sync_kyoto_client. - Update handle_online_wallet_subcommand signature - Update all PayjoinManager methods to use &BlockchainClient - Fix parameter dereferencing in full_scan calls - Update all call sites to pass references
Replace single sync-and-check with periodic polling loop. This allows multiple sync operations since sync_wallet now accepts a reference to BlockchainClient, enabling proper long-running monitoring instead of a one-time check.
Replace Error::Generic with specific Payjoin error variants using #[from] derives for applicable places, enabling automatic conversion and eliminating verbose .map_err() calls.
@Mshehu5 thanks for the detailed testing steps. I modified them a little since rebase that added new config feature: cargo run --features cbf -- wallet --wallet signet_receiver_wallet \
config --ext-descriptor "$RECEIVER_EXT_DESC"\
--int-descriptor "$RECEIVER_INT_DESC" \
--database-type $DATABASE_TYPE \
--client-type $CLIENT_TYPE
cargo run --features cbf -- wallet --wallet signet_receiver_wallet sync
cargo run --features cbf -- wallet --wallet signet_sender_wallet \
config --ext-descriptor "$SENDER_EXT_DESC" \
--int-descriptor "$SENDER_INT_DESC" \
--database-type $DATABASE_TYPE \
--client-type $CLIENT_TYPE
cargo run --features cbf -- wallet --wallet signet_sender_wallet sync
cargo run --features cbf -- wallet \
--wallet signet_receiver_wallet \
receive_payjoin \
--amount 1000 \
--directory $PAYJOIN_DIRECTORY \
--ohttp_relay $OHTTP_RELAY \
--ohttp_relay $OHTTP_RELAY2 \
--ohttp_relay $OHTTP_RELAY3<manually copy payjoin URI into PAYJOIN_URI>
cargo run --features cbf -- wallet \
--wallet signet_sender_wallet send_payjoin \
--uri $PAYJOIN_URI \
--ohttp_relay $OHTTP_RELAY \
--ohttp_relay $OHTTP_RELAY2 \
--ohttp_relay $OHTTP_RELAY3 \
--fee_rate 10 |
notmandatory
left a comment
There was a problem hiding this comment.
ACK 974c8d5
Great job! exciting to see this capability in bdk-cli.
2f14a36 Retry Payjoin requests with other relays (Mshehu5) 6fb0ecd Restrict payjoin inputs to confirmed UTXOs (Mshehu5) 262ce86 Add payjoin persistence test (Mshehu5) e85a92c Prune stale payjoin sessions on DB open (Mshehu5) 181d132 chore(deps): Bump payjoin and enable reqwest rustls (Mshehu5) b9ca88f Document payjoin recovery commands and SQLite setup (Mshehu5) 559ba76 Add commands to resume and view payjoin history (Mshehu5) 4f71144 Integrate payjoin session persistence into workflow (Mshehu5) c2bdb3d Add SQLite backing store for payjoin sessions (Mshehu5) Pull request description: ### 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 https://github.com/bitcoindevkit/bdk-cli/blob/b9cf2acc5f10db46fa30777ff0910b8610a5fc33/README.md?plain=1#L121-L141 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: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk-cli/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [x] I've added docs for the new feature Top commit has no ACKs. Tree-SHA512: 1e8be24fbf7d894687389192381242cebafcaffd8601418e197cfb7942917cbcd5ae308ed5e9fcc644c1dacb78abed9aa23c49c5ff08151b4657956e89d14217
Description
This PR addresses issues encountered while implementing persistence for Payjoin specifically around the BlockchainClient only being an owned variable rather than being able to be borrowed/referenced as &Blockchainclient.
While working on persistence I ran into problems while working on resume command which needs a blockchain client to resume states such as monitor_payjoin_proposal (receiver) and process_payjoin_proposal (sender)
Because BlockchainClient can only be owned the current design will require a function signature of passing two separate clients to resume sender and receiver states. I initially considered splitting the command into resume_send and resume_receive but this does not fully solve the issue. In particular the sender’s process_payjoin_proposal may call broadcast_transaction and potentially broadcast multiple transactions for persisted send entries stored in the database which still requires reusable access to the client.
This Ownership issue was previously mentioned in #200 and is also noted in a comment at the top of monitor_payjoin_proposal. It prevents the function from being able to resync multiple times and reliably detect when a transaction appears in the mempool. The root cause is that the Kyoto client Box is destructured and spawned into other tasks when passed through sync_kyoto_client making it unusable afterward.
What this PR changes
This PR fixes the issue by refactoring sync_kyoto_client
The logic responsible for running the Kyoto node and logger is moved into new_blockchain_client. This makes it that node is started at start of command and not during every sync
Instead of returning a Box the function now returns a KyotoClientHandle. Previously the boxed client takes ownership when destructured inside sync_kyoto_client, preventing reuse/reference.
With the new design sync_kyoto_client takes &KyotoClientHandle, allowing the client to be Refrenced which can be used for syncing and broadcasting transactions without being owned
Additionally monitor_payjoin_proposal is refactored to support resyncing demonstrating that the Kyoto client refactor successfully resolves the original limitations
Notes to the reviewers
After refactor I tested the kyoto client on regtest (Cause I do not have access to a signet) I had to set a trusted peer in the code to connect with a cbf count of 1 this worked and I also made transaction using the steps below:
N.B Payjoin was also tested for the monitor_payjoin_proposal refactor using steps in project readme
Step 1: Create transaction
Step 2: Sign transaction
Step 3: Broadcast transaction
Mine a block to confirm
bitcoin-cli -regtest generatetoaddress 1 $(bitcoin-cli -regtest getnewaddress)Checking Transaction Status
After broadcasting, wait a moment and sync your wallet:
Sync wallet
Check balance
List recent transactions
Checklists
All Submissions:
cargo fmtandcargo clippybefore committing