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

Fall back to first candidate if avoid_uih fails #533

Merged
merged 2 commits into from
Feb 12, 2025

Conversation

DanGould
Copy link
Contributor

try_preserving_privacy is about a best-effort attempt, not a guaranteed success. This implementation is what was already implied by the docstring. So it's a fix.

Making avoid_uih and select_first_candidate public later can be public to allow for more granular downstream control.

@DanGould DanGould requested a review from 0xBEEFCAF3 February 11, 2025 16:38
@DanGould DanGould force-pushed the only-try-preserving-privacy branch 3 times, most recently from 649d305 to b88511c Compare February 11, 2025 16:46
@DanGould
Copy link
Contributor Author

DanGould commented Feb 11, 2025

It's not yet clear to me whether we want to expose the private coin selection methods in-place as public, or whether we want to make public pure functions that a sophisticated downstream receiver can implement themselves. I think the latter is a whole lot cleaner but is also more code to maintain. It might be a good place for us to start addressing the coin selection problem in general in this repo.

Using Psbt V2 Inputs would make the interface a whole lot cleaner instead of using our custom InputPair type.

Copy link
Collaborator

@0xBEEFCAF3 0xBEEFCAF3 left a comment

Choose a reason for hiding this comment

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

utACK b88511c

@@ -408,7 +408,7 @@ impl WantsInputs {
/// Proper coin selection allows payjoin to resemble ordinary transactions.
/// To ensure the resemblance, a number of heuristics must be avoided.
///
/// UIH "Unnecessary input heuristic" is avoided for multi-output transactions.
/// Attempt to avoid UIH (Unnecessary input heuristic) for multi-output transactions.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: specify "two-output transaction" instead of "multi-output transaction".

@DanGould DanGould force-pushed the only-try-preserving-privacy branch from b88511c to 1201492 Compare February 12, 2025 20:13
try_preserving_privacy is about a best-effort attempt, not a guaranteed
success. This implementation is what was already implied by the
docstring. So it's a fix.

Making avoid_uih and select_first_candidate public later can be public
to allow for more granular downstream control.
@DanGould DanGould requested a review from 0xBEEFCAF3 February 12, 2025 20:23
Copy link
Collaborator

@0xBEEFCAF3 0xBEEFCAF3 left a comment

Choose a reason for hiding this comment

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

utACK 1201492

@DanGould DanGould merged commit f58a457 into payjoin:master Feb 12, 2025
6 checks passed
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.

Not directly related to this change, I noticed select_first_candidate returns a InternalSelectionError::NotFound if there are no candidates - that should be InternalSelectionError::Empty instead.

@coveralls
Copy link
Collaborator

coveralls commented Feb 13, 2025

Pull Request Test Coverage Report for Build 13294028995

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 6 of 7 (85.71%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall first build on only-try-preserving-privacy at 79.331%

Changes Missing Coverage Covered Lines Changed/Added Lines %
payjoin/src/receive/error.rs 0 1 0.0%
Totals Coverage Status
Change from base Build 13251464274: 79.3%
Covered Lines: 4030
Relevant Lines: 5080

💛 - Coveralls

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