Fix try_preserving_privacy returning Empty when candidates exist#1629
Fix try_preserving_privacy returning Empty when candidates exist#1629DanGould wants to merge 1 commit into
try_preserving_privacy returning Empty when candidates exist#1629Conversation
Collect candidate inputs before coin selection so the fallback still sees the full list when avoid_uih scans every candidate and returns NotFound. Without this the fallback sees an empty list.
Coverage Report for CI Build 27207089758Coverage increased (+0.04%) to 85.429%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
while the baseline behavior seems far from optimal, this change potentially increases the rate at which cluster information is leaked in a way that is possibly compounded by utxo probing relative when the sender is adversarial assuming the input order is stable, picking consistently can also reduce the overall privacy leaks and is in line with BIP 78 and 79 anti probing so this seems to make fingerprinting potentially better but not clearly so and with unclear impact, but probing worse privacy impacting changes like this should have a rationale and analysis to back it up |
Yes please seperate these. One is a bug fix the other is a privacy improvment that should be scrutinized more. |
bc1cindy
left a comment
bc1cindy
left a comment
There was a problem hiding this comment.
the bug fix is correct and worth keeping, my only concern is the random fallback
| /// Privacy preservation is only supported for 2-output transactions. If the PSBT has more than | ||
| /// 2 outputs or if none of the candidates are suitable for avoiding UIH2, this function | ||
| /// defaults to the first candidate in `candidate_inputs` list. | ||
| /// randomly selects one of the provided candidates. |
There was a problem hiding this comment.
randomizing the contributed-input selection conflicts with BIP 79's anti-probing requirement
"it is essential that the receiver always returns the same contributed inputs when it's seen the same inputs."
to prevent UTXO-set enumeration.
the deterministic collection in 5/6 integrations (#1597 (comment)) approximates this, a random fallback breaks it.
Pin the non-empty-candidates invariant the payjoin#1629 fix restores.
Pin the non-empty-candidates invariant the payjoin#1629 fix restores.
bc1cindy
left a comment
bc1cindy
left a comment
There was a problem hiding this comment.
I wrote a proptest that catches this bug, which fails without this fix and passes with it: bc1cindy/rust-payjoin@362a3fb4
makes sense to add the dep?
DanGould
force-pushed
the
fix-uih-avoidance
branch
from
6af29d3 to
efc5b92
Compare
5 participants
@Talej tipped me off that
try_preserving_privacywas returningErr(Empty)whenavoid_uihfailed. I took a closer look. Becausetry_preserving_privacytakes an ownedcandidate_inputs: impl IntoIterator<Item = InputPair>theIntoIterator is there for ergonomics but thecandidate_inputsgets exhausted by the iteration that happens withavoid_uih. Thenselect_fallback_candidate` selects from an empty iterator. Doh!This also randomly selects a coin in the case
avoid_uihfrom the candidates fails in an attempt to reduce fingerprints that may be caused by a wallet ordering outputs a certain way, as the second commit. End users can always override this with manual selection. If reviewers think I'm conflating two independent decisions I am happy to separate the commits into their own PRs.random_fallback_candidate(or its ancestorselect_fallback_candidate) both consumed the iterator, so we really gotta collect no matter what. This is a convenience method. It's still possible for more complex / large utxo wallets to implement more performance-conscioius custom coin selection if they need to.I think this change does need to get into 1.0 (but since the api doesn't change I suppose it could be a patch).
Disclosure: Cursor wrote the code, I reviewed.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.