Support multi-output UIH selection - #1837
Conversation
Coverage Report for CI Build 33716840747Coverage increased (+0.05%) to 86.688%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
xstoicunicornx
left a comment
xstoicunicornx
left a comment
There was a problem hiding this comment.
When considering supporting UIH selection for 2+ outputs there is more to consider than just un-gating the number of outputs allowed, which is largely what looks like was done here.
Unnecessary Input Heuristic (UIH) is fairly straight forward to avoid with a 2 output transaction, which is currently the only scenario that avoid_uih accounts for. It chooses an input where the minimum of all inputs in the tx are larger than the minimum of all outputs in the tx, which creates a transaction where both inputs look like they are required to create the larger output value and avoids UIH. This calculation explicitly relies on the assumption that there are 2 outputs, as finding the minimum of the outputs implies the value of the other (maximum) output and consequently that both inputs were required to create the larger output (hence avoiding UIH). This assumption breaks at 3 outputs.
Admittedly I have not given much thought on how to expand this to >2 outputs myself, however I do expect any PR trying to address this to have robust rationales on the design decisions made.
| .enumerate() | ||
| .filter(|(vout, _)| *vout != self.proposal.change_vout) | ||
| .map(|(_, output)| output.value) |
There was a problem hiding this comment.
Why are we filtering out the change output? Doesn't this mess up our existing 2 output avoid_uih calculation? How does this help with >2 outputs?
There was a problem hiding this comment.
change_vout is the receiver's own output, not the sender's change — contribute_inputs bumps it
with output[change_vout].value += change_amount. The name misleads.
The filter is a separate bug fix, not part of un-gating the output count. avoid_uih documents a
post-contribution minimum but took the min over all outputs at pre-contribution values, so when
the receiver's output is the smallest, min(min_out, prior + candidate) discards the bumped term.
On 2 outputs: receiver 1 sat, sender 4,000,000, candidate 3,000,000 -> the real min_out is
3,000,001 against min_in 3,000,000, yet the check passes on master.
It doesn't break the 2-output path: the test vector pays the receiver its largest output
(95,983,068 vs 2,000,000), so nothing existing moves — reverting only the filter flips exactly one
test, the new one. And on its own it doesn't help >2 outputs either; the 3-output test passes either
way. Hence two commits now.
484eca0 to
d4919d9
Compare
|
Fair — the PR didn't make the argument. Two commits now, with the reasoning recorded on Your derivation is right for 2-in/2-out, but the code never computes it; it only evaluates What does change at 3+ outputs is the number of change hypotheses available, and this check only Main argument for landing it: it can't make anything worse. A 3-output proposal currently falls |
d4919d9 to
5d31dc8
Compare
avoid_uih documents its minimum output as a post-contribution value, but computed the minimum over every output at its pre-contribution value and only then folded the receiver's output back in increased by the candidate. Whenever the receiver's output is the strictly smallest, its pre-contribution value survives the outer minimum, so the check runs against an amount lower than the transaction will actually have and accepts candidates that do not avoid UIH2. With a receiver output of 1 sat, a sender output of 4,000,000 sat and a 3,000,000 sat candidate, the post-contribution minimum output is 3,000,001 sat against a 3,000,000 sat minimum input, yet the check passes. Exclude the receiver's output from the minimum so the only value it contributes is the post-contribution one. The two-output test vector pays the receiver its largest output, so this leaves every existing expectation unchanged and the buggy branch had no coverage at all.
avoid_uih rejected any transaction without exactly two outputs, so batched senders, and receivers who split their own output, fell back to the first candidate with no privacy check at all. The check does not depend on the output count. An analyst who assumes output o is the spender's change treats input i as unnecessary when i <= o, since dropping i would still fund every other output. The optimal change heuristic points that assumption at the smallest output, so a transaction avoids UIH2 exactly when its smallest input exceeds its smallest output, for any number of outputs. Only the single-output case is genuinely unsupported: with no change output there is nothing for the heuristic to identify. More outputs do give an analyst more change hypotheses to try, and this check only defeats the one the optimal change heuristic picks. That is weaker than the two-output case, but never weaker than returning the first candidate unexamined, which is what these transactions get today. Relax the guard to fewer than two outputs and record the reasoning on avoid_uih. Cover both the three-output case and the n-input, m-output shape that motivates the change, where the minimum input comes from an existing input rather than the candidate.
5d31dc8 to
4c80781
Compare
|
Closing as unresearched slop |
4 participants
Closes #551.
Two commits, split after review so the correctness fix and the design decision can be judged
separately. Each passes CI on its own.
1.
Compare UIH against post-contribution outputsavoid_uihdocuments its minimum output as post-contribution, but computed the minimum over everyoutput at its pre-contribution value and only then folded the receiver's output back in. When
the receiver's output is the strictly smallest, its pre-contribution value survives the outer
minand the check runs against an amount the transaction never has, accepting candidates that do not
avoid UIH2.
Reachable in the existing 2-output path: receiver output 1 sat, sender output 4,000,000 sat,
candidate 3,000,000 sat -> post-contribution
min_out = 3,000,001vsmin_in = 3,000,000, yet thecheck passes on master. No existing expectation changes, since the 2-output test vector pays the
receiver its largest output (95,983,068 vs 2,000,000) — which is why the buggy branch had no
coverage.
2.
Support multi-output UIH selectionRelax the
!= 2guard to< 2.The condition
min_in > min_outdoes not depend on the output count. An analyst assuming outputois the spender's change treats input
ias unnecessary wheni <= o, because droppingiwouldstill fund every other output; the optimal change heuristic points that at the smallest output. Only
the single-output case is genuinely unsupported — with no change output there is nothing for the
heuristic to identify.
More outputs give an analyst more change hypotheses, and this check only defeats the UIH1 one, so it
is weaker at n>2. It is never weaker than the status quo: today those transactions fall through to
select_first_candidatewith no privacy check at all. Full reasoning is recorded onavoid_uih.Against #551's two motivations
Anonymity set (n-input, m-output). Covered.
avoid_uih_supports_three_outputsplusavoid_uih_supports_multiple_inputs_and_outputs, the latter with two existing inputs and threeoutputs, where the binding minimum input comes from an existing input rather than the candidate.
Multiparty / NS1R. Not exercisable here, and I would rather say so than imply otherwise. NS1R
was removed from master in 6b87adc (backed up to
ns1r-master-backup, see #922), so there is nomultiparty receiver path to route through
try_preserving_privacyor to test against. Relaxing theguard is the enabling change the issue asks for — when NS1R returns it inherits UIH-aware selection
instead of
select_first_candidate— but the multiparty half cannot be demonstrated until then.I did not take the issue's "relax only in the multi-party context" alternative: relaxing generally is
a superset, and a multiparty-only gate would currently have no call site.
Output splitting, candidate scoring, and ranking against non-UIH1 change hypotheses stay out of
scope, as proposed when claiming the issue.
Tests
cargo test -p payjoin --all-features(on each commit)cargo clippy -p payjoin --all-targets --all-features -- -D warnings(on each commit)cargo fmt --all -- --checkcodespellco-authored by Codex and Claude