Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unresumable Sender #443

Merged
merged 3 commits into from
Dec 23, 2024
Merged

Fix unresumable Sender #443

merged 3 commits into from
Dec 23, 2024

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Dec 23, 2024

The Sender was generating new hpke keys on resume since keygen was being done in extract_v2. This caused a problem where when the Sender was persisted, stopped, and resumed, the receiver would have already pushed a response to a different ShortId than the one the Sender would look for it in the resumed state.

By generating a key on Sender creation and persisting that the Sender can produce a consistent HpkeContext every single run.

I am NOT sure why the e2e test did not catch this. I believed its behavior was starting a receiver, stopping it, starting a sender, stopping it, resuiming a receiver, stopping it, and resuming a sender, and stopping it which is where this bug propagated. It didn't fail before this change and I'm not sure why, it should have.

@coveralls
Copy link
Collaborator

coveralls commented Dec 23, 2024

Pull Request Test Coverage Report for Build 12465664083

Details

  • 10 of 10 (100.0%) changed or added relevant lines in 2 files are covered.
  • 252 unchanged lines in 8 files lost coverage.
  • Overall coverage decreased (-5.3%) to 61.852%

Files with Coverage Reduction New Missed Lines %
payjoin-directory/src/lib.rs 4 79.19%
payjoin-directory/src/db.rs 4 89.55%
payjoin-cli/src/db/mod.rs 4 0.0%
payjoin-cli/src/db/v2.rs 6 0.0%
payjoin-cli/src/app/mod.rs 15 0.0%
payjoin-cli/src/app/config.rs 54 0.0%
payjoin-cli/src/app/v2.rs 64 0.0%
payjoin-cli/src/main.rs 101 0.0%
Totals Coverage Status
Change from base Build 12425801101: -5.3%
Covered Lines: 2899
Relevant Lines: 4687

💛 - Coveralls

The Sender was generating new hpke keys on resume since keygen was being
done in `extract_v2`. This caused a problem where when the Sender was
persisted, stopped, and resumed, the receiver would have already pushed a
response to a different ShortId than the one the Sender would look for it
in the resumed state.

By generating a key on Sender creation and persisting that the Sender can
produce a consistent HpkeContext every single run.
@DanGould DanGould force-pushed the fix-send-resume branch 3 times, most recently from c8fd0a3 to 226816a Compare December 23, 2024 05:42
@spacebear21
Copy link
Collaborator

It looks like the e2e test is silently swallowing errors somehow. This is in master:

running 1 test
Using OHTTP Keys from config
Receive session established
Request Payjoin by sharing this Payjoin Uri:
bitcoin:bcrt1q520j8a7zh4xg58eajj25ccwpq98x8u9d70ahcy?amount=0.00054321&pjos=0&pj=HTTPS://LOCALHOST:53404/QE3WD63UC0FT2%23RK1QWWGRLNPZX7FCW0PGNNZ2TR5PUE6FQ6GJTRPKMXAW6UKQF83Q9XAK+OH1QYPW96LWVYX0EYRKPN39CK87N6SHUL6WTLATYZXC977WHTSLZ3P2RSS+EX1CX9K5EC
Posting Original PSBT Payload request...
Sent fallback transaction
No response yet.
resume
Receive session established
Request Payjoin by sharing this Payjoin Uri:
bitcoin:bcrt1q520j8a7zh4xg58eajj25ccwpq98x8u9d70ahcy?pjos=0&pj=HTTPS://LOCALHOST:53404/QE3WD63UC0FT2%23RK1QWWGRLNPZX7FCW0PGNNZ2TR5PUE6FQ6GJTRPKMXAW6UKQF83Q9XAK+OH1QYPW96LWVYX0EYRKPN39CK87N6SHUL6WTLATYZXC977WHTSLZ3P2RSS+EX1CX9K5EC
Polling receive request...
Fallback transaction received. Consider broadcasting this to get paid if the Payjoin fails:
02000000000101e195c1d2d1d490fa9682259c2096929ef4977440e0c9f3fa184febd19d7ecb830000000000feffffff0231d4000000000000160014a29f23f7c2bd4c8a1f3d94954c61c1014e63f0ad421d052a01000000160014291e848a3f81099b9dabba25a5802f1f004765810247304402204010bc2326ffa93982a7ac686190b8f55fed7d00347c9b9b6427a8a43d8d28a202206d697191848fc2f6e81707335b59a86b5b8a66db1afec3b5aa51aee0f10f8d26012103293e88f418349f2f2fc2cca489deeb1c7afcd5e01e1466610f6e816d5e5763c100000000
Got a request from the sender. Responding with a Payjoin proposal.
Response successful. Watch mempool for successful Payjoin. TXID: d6e44b930408afbc553c370b85a81866b5d930e4e2ddf5d6ca310a8218f29f03
Posting Original PSBT Payload request...
Sent fallback transaction
No response yet.
No response yet.
No response yet.
No response yet.
The receiver sent an invalid response.
Error: The receiver sent an invalid response.

Caused by:
    Response error
test e2e::send_receive_payjoin ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 16.76s

Copy link
Collaborator

@spacebear21 spacebear21 left a comment

Choose a reason for hiding this comment

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

tACK 226816a

@spacebear21
Copy link
Collaborator

I pushed a fix for the e2e test to correctly bubble up payjoin-cli errors, and confirmed that this commit currently fails on master.

Bubble up payjoin-cli errors instead of silently swallowing them.
@DanGould
Copy link
Contributor Author

Also tested 86a9a8a cherry-picked on top of master to make sure it fails. good catch.

@DanGould DanGould merged commit f202098 into payjoin:master Dec 23, 2024
6 checks passed
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.

3 participants