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

Try not to use unconfirmed coins when BnB. #12306

Merged
merged 3 commits into from
Jan 26, 2024

Conversation

molnard
Copy link
Collaborator

@molnard molnard commented Jan 24, 2024

Fixes: #11951

I used the same logic as used for coins in the critical cj phase which is the following currently:

  • The pocket selector and coin-selector create an initial tx.
  • That is the tx, which is shown to the user on the tx preview dialog.
  • BnB checks if the initial tx includes critical coins, if yes then it is allowed to use them in BnB. If the initial tx is not using critical coins, then BnB won't use them either.

This PR adds the same behavior regarding unconfirmed coins.

Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

This means if the initial transaction used an unconfirmed coin, then BaB is allowed to choose any coin from Critical phase and vice versa. I think this is wrong.

I think we should separate the two exclusion.

coinsToUse = spentCoins.Any(coinsToExclude.Contains) ? coinsToUse : coinsToUse.Except(coinsToExclude).ToImmutableArray();

coinsToUse = spentCoins.Any(unconfirmedCoins.Contains) ? coinsToUse : coinsToUse.Except(unconfirmedCoins).ToImmutableArray();

or something like this.

@molnard
Copy link
Collaborator Author

molnard commented Jan 24, 2024

I think we should separate the two exclusion.

You are right. I fixed that.

adamPetho
adamPetho previously approved these changes Jan 24, 2024
Copy link
Collaborator

@adamPetho adamPetho left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@MarnixCroes MarnixCroes left a comment

Choose a reason for hiding this comment

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

utack c79a8ed

@molnard
Copy link
Collaborator Author

molnard commented Jan 24, 2024

Can someone test this? It is not trivial, maybe on RegTest with CTRL C-D

@soosr soosr self-requested a review January 25, 2024 09:17
soosr
soosr previously approved these changes Jan 25, 2024
Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

tACK
minor thing (can be ignored if we want to stick to the convention):

@molnard molnard dismissed stale reviews from soosr and adamPetho via 58c22f5 January 25, 2024 10:30
@molnard molnard requested a review from soosr January 25, 2024 10:31
@molnard
Copy link
Collaborator Author

molnard commented Jan 25, 2024

@BTCparadigm can you check if this solves your issue?

Copy link
Collaborator

@soosr soosr left a comment

Choose a reason for hiding this comment

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

tACK

@BTCparadigm
Copy link
Collaborator

cACK

@molnard unfortunately I can't before next week.

@molnard
Copy link
Collaborator Author

molnard commented Jan 25, 2024

I can't before next week.

We have time until the release so this can stay open until that. Can you test and reply back here?

@BTCparadigm
Copy link
Collaborator

@molnard I can't reasonably test this in mainnet at the moment, so anyone willing to play with regtest or with mainnet test coins can do it too.
I will try to make time for it next week but no promises.

@soosr
Copy link
Collaborator

soosr commented Jan 25, 2024

@molnard @BTCparadigm

I tested it on regtest, it worked as expected

@molnard molnard merged commit 7cdbda3 into WalletWasabi:master Jan 26, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Privacy suggestion selects unconfirmed coins unnecessarily
5 participants