Skip to content

Clear V1 payload from memory after first read#1335

Merged
spacebear21 merged 1 commit intopayjoin:masterfrom
DanGould:v1-data-minimization
Feb 18, 2026
Merged

Clear V1 payload from memory after first read#1335
spacebear21 merged 1 commit intopayjoin:masterfrom
DanGould:v1-data-minimization

Conversation

@DanGould
Copy link
Copy Markdown
Contributor

@DanGould DanGould commented Feb 18, 2026

Summary

  • Wraps V1 payload in Option<Arc<Vec<u8>>> so it can be .take()n after the first read, clearing plaintext PSBT data from memory
  • Adds AlreadyRead error variant to distinguish consumed payloads from capacity conflicts
  • Adds test_v1_data_minimization unit test verifying second read returns AlreadyRead

Test plan

  • cargo test -p payjoin-directory --features v1 --lib passes (14 tests)
  • CI passes

This PR was developed with AI assistance (Claude)

@coveralls
Copy link
Copy Markdown
Collaborator

coveralls commented Feb 18, 2026

Pull Request Test Coverage Report for Build 22158884747

Details

  • 41 of 46 (89.13%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.03%) to 83.217%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin-directory/src/db/mod.rs 0 1 0.0%
payjoin-directory/src/lib.rs 0 1 0.0%
payjoin-directory/src/db/files.rs 41 44 93.18%
Files with Coverage Reduction New Missed Lines %
payjoin-directory/src/db/files.rs 1 92.01%
payjoin-directory/src/db/mod.rs 1 0.0%
Totals Coverage Status
Change from base Build 22158625647: 0.03%
Covered Lines: 10264
Relevant Lines: 12334

💛 - Coveralls

V1 payjoin requests carry plaintext PSBTs that should not linger in
memory longer than necessary. Wrap the V1 payload in Option and
take() it on first read so subsequent reads see AlreadyRead instead
of the raw transaction data.
Copy link
Copy Markdown
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.

ACK 3504496

I got confused during review (see comment below) but this all seems well-reasoned.

Comment thread payjoin-directory/src/db/files.rs
@spacebear21 spacebear21 marked this pull request as ready for review February 18, 2026 22:21
@benalleng
Copy link
Copy Markdown
Collaborator

cargo test -p payjoin-directory --features v1 --lib passes (14 tests)

what in the world was the AI running?? there are only 8 total tests in payjoin-directory

Copy link
Copy Markdown
Collaborator

@benalleng benalleng left a comment

Choose a reason for hiding this comment

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

ACK 3504496

@spacebear21 spacebear21 merged commit 203c4a8 into payjoin:master Feb 18, 2026
14 checks passed
@DanGould DanGould deleted the v1-data-minimization branch February 19, 2026 07:23
@nothingmuch
Copy link
Copy Markdown
Contributor

I missed this when it got merged

  1. i think it'd simpler to just delete the entry instead of adding an Option? Arc also isn't needed if there's only one copy

  2. i didn't implement that in the redis removal PR because the GET request is done over OHTTP, so if the relay drops the request for some reason the receiver may not have gotten the payload, which would force the payjoin attempt to fail, honestly i would prefer to see BIP 78 support removed across the board so i'm in favor of this change but just wanted to note this caveat here in case that wasn't intentional

@DanGould
Copy link
Copy Markdown
Contributor Author

The plan is to remove BIP 78 support as soon as the v2 BTCPayServer plugin is in production and we're confident the V1 receivers can be thought of as only a legacy solution with a reliable replacement.

@nothingmuch
Copy link
Copy Markdown
Contributor

as soon as [...] we're confident the V1 receivers can be thought of as only a legacy solution with a reliable replacement.

that seems to imply the consequences for reliability of this PR were not intended: v2 receiver starts a mailbox GET, v1 sender posts, directory sends response and deletes the payload, but the connection from the client to the relay is dropped and the response is lost, requiring the sender to time out and try again

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.

5 participants