Skip to content

Fix send success log error and order#1498

Merged
spacebear21 merged 1 commit into
payjoin:masterfrom
Arowolokehinde:v2-proposal-error-fix
Apr 22, 2026
Merged

Fix send success log error and order#1498
spacebear21 merged 1 commit into
payjoin:masterfrom
Arowolokehinde:v2-proposal-error-fix

Conversation

@Arowolokehinde

Copy link
Copy Markdown
Contributor
This PR addresses the issue in #1394 where payjoin-cli "send" command silently exits when process_response fails. Two bugs were identified and fixed:

println!("Posted original proposal...") fired before process_response validated the directory response, showing a false success banner even on failure.
tokio::select! in send_payjoin discarded the Result from process_sender_session with _ = ... => return Ok(()), silently swallowing all errors.
The fix moves the print statement after validation and propagates the result with res = ... => return res, so errors reach main.rs where anyhow prints them.

Manually tested with self-loop configuration (relay = directory) confirming the error Unexpected response size 0, expected 8192 bytes is now surfaced and the process exits non-zero.

I brainstormed the implementation approach with Claude while working through this fix.

Please confirm the following before requesting review:

Print "Posted original proposal..." only after process_response
succeeds so users don't see a false success banner when the
directory rejects or truncates the OHTTP response.

Also propagate errors from process_sender_session through
tokio::select! instead of silently returning Ok(()).
@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 24774218494

Coverage remained the same at 84.92%

Details

  • Coverage remained the same as the base build.
  • Patch coverage: 1 of 1 lines across 1 file are fully covered (100%).
  • No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 13375
Covered Lines: 11358
Line Coverage: 84.92%
Coverage Strength: 400.65 hits per line

💛 - Coveralls

@Arowolokehinde Arowolokehinde changed the title Fix v2 send success log order Fix send success log order Apr 22, 2026
@Arowolokehinde Arowolokehinde changed the title Fix send success log order Fix send success log error and order Apr 22, 2026
@zealsham

zealsham commented Apr 22, 2026

Copy link
Copy Markdown
Collaborator

Not on my PC for now so i can't really test things out. However this seem to hinge on what our definition of "posting the original proposal " means.

  1. are we just telling the sender that the orginal proposal has been posted irrespective of if the proposal hit the directory
  2. do we want to use this log to tell the sender that his proposal has been posted and it got to the directory and was accpeted.

since process_response will only return success when the original proposal has been accepted , i'll say the log message was aimed at point 1 above which imho is still correct.

@Arowolokehinde

Arowolokehinde commented Apr 22, 2026

Copy link
Copy Markdown
Contributor Author

Before this fix, while playing around the payjoin-cli, i encountered the issue raised in the PR which was "Posted original proposal..." always printed even when the send failed and exited quietly. The user would see a success message followed by a silent exit with no error. That's exactly the problem that i faced

Moving the print after process_response ensures it only appears when the proposal actually reaches the directory. The tokio::select! fix ensures that when something does go wrong, the error message is shown to the user instead of being silently discarded.

So i think based on the Expected behavior
It should not print "Posted original proposal.." until a successful response is confirmed, and in the case of failure it should print an error message mentioned by spacebear in the issue. so the second option is what i implemented

i would also like to get your feedback too

@spacebear21

Copy link
Copy Markdown
Collaborator

are we just telling the sender that the orginal proposal has been posted irrespective of if the proposal hit the directory

If it hasn't hit the directory then it hasn't been posted, and the message is misleading.

@spacebear21 spacebear21 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK 8cc3bc2

on failure pointing to the same ohttp relay as the directory:

❯ cargo run -- send $BIP21 --fee-rate 1
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.12s
     Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-cli send 'bitcoin:bcrt1qjf69494jh08hgu32srynxfpznatmn35pycxfh0?amount=0.00001&pjos=0&pj=HTTPS://PAYJO.IN/FWGEGWG8H27CY%23EX1MFQW56G-OH1QYPFLM8XL59R0XV4VGPLS7FRDSSM4TUXL07TXCWC4S0GLVLNK2SE4NQ-RK1QDSC8SJ8ZVNNPPVZ4LUVY0WYDJ9STVUALM7RNLC9C0TK6LKGZN6MW' --fee-rate 1`
Bootstrapping private network transport over Oblivious HTTP
Error: Transient error: Directory response error: Unexpected response size 0, expected 8192 bytes

and on success with a different ohttp relay:

❯ cargo run -- send $BIP21 --fee-rate 1
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.15s
     Running `/Users/spacebear/Projects/rust-payjoin/target/debug/payjoin-cli send 'bitcoin:bcrt1qjf69494jh08hgu32srynxfpznatmn35pycxfh0?amount=0.00001&pjos=0&pj=HTTPS://PAYJO.IN/FWGEGWG8H27CY%23EX1MFQW56G-OH1QYPFLM8XL59R0XV4VGPLS7FRDSSM4TUXL07TXCWC4S0GLVLNK2SE4NQ-RK1QDSC8SJ8ZVNNPPVZ4LUVY0WYDJ9STVUALM7RNLC9C0TK6LKGZN6MW' --fee-rate 1`
Bootstrapping private network transport over Oblivious HTTP
Posted original proposal...
Proposal received. Processing...
Payjoin sent. TXID: b4c7c49ebb9790f38dec059dd3e29e858f74f9735c9835fe0288149aacaf8717

@spacebear21
spacebear21 merged commit 7f8ec33 into payjoin:master Apr 22, 2026
14 checks passed
@Arowolokehinde

Copy link
Copy Markdown
Contributor Author

Thank you @spacebear21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants