Skip to content

wallet: unify max signature logic #25481

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 8, 2022
Merged

wallet: unify max signature logic #25481

merged 2 commits into from
Jul 8, 2022

Conversation

S3RK
Copy link
Contributor

@S3RK S3RK commented Jun 27, 2022 •
edited
Loading

Currently DummySignTx and DummySignInput use different ways to determine signature size.
This PR unifies the way wallet estimates signature size for various inputs.
Instead of passing boolean flags from calling code the use_max_sig is now calculated at the place of signature creation using information available in CCoinControl

furszy
furszy reviewed Jun 27, 2022
View reviewed changes
Copy link
Member

@furszy furszy left a comment

Choose a reason for hiding this comment

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

Aside from the quick code review that made, want to point you in #24699 direction.

The first two commits there are implementing part of what you did here. Would be nice to get that one reviewed first.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 27, 2022 •
edited
Loading

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #24699 (wallet: Improve AvailableCoins performance by reducing duplicated operations by achow101)
  • #24584 (wallet: avoid mixing different OutputTypes during coin selection by josibake)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

This was referenced Jun 27, 2022
Merged
Merged
@S3RK S3RK force-pushed the wallet_unify_max_sig branch from ae07464 to 11b46bd Compare June 28, 2022 06:42
@S3RK S3RK force-pushed the wallet_unify_max_sig branch from 11b46bd to d54c5c8 Compare June 28, 2022 06:55
@S3RK
Copy link
Contributor Author

S3RK commented Jun 28, 2022

@furszy thanks for the review and the pointer. I don't want to add hints for the flag, because it's removed in the next commit.
I made this PR compatible with #24699 and now they nicely compliment each other.
008d117 Is fully covered by the first commit here and it does even more.
c685bf5 Is taking a slightly different direction, but it's compatible with the code here
It seems either PR can go first.

@achow101
Copy link
Member

ACK d54c5c8

theStack
theStack approved these changes Jul 6, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK d54c5c8

@fanquake fanquake requested a review from murchandamus July 6, 2022 14:00
@achow101 achow101 merged commit 194710d into bitcoin:master Jul 8, 2022
murchandamus
murchandamus reviewed Jul 12, 2022
View reviewed changes
Copy link
Contributor

@murchandamus murchandamus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Post-merge ACK d54c5c8

@bitcoin bitcoin locked and limited conversation to collaborators Jul 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@achow101 achow101 achow101 left review comments

+3 more reviewers

@murchandamus murchandamus murchandamus left review comments

@furszy furszy furszy left review comments

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@S3RK @DrahtBot @achow101 @theStack @murchandamus @furszy @fanquake