Skip to content

bitcoin#8365: Treat high-sigop transactions as larger rather than rejecting them #4562

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
UdjinM6 merged 1 commit into from
Nov 8, 2021
Merged

bitcoin#8365: Treat high-sigop transactions as larger rather than rejecting them #4562

UdjinM6 merged 1 commit into from
Nov 8, 2021

Conversation

@kwvg
Copy link
Collaborator

@kwvg kwvg commented Nov 2, 2021 •
edited
Loading

Split as #4562 (this PR) and #4560 for ease of review

@kwvg kwvg mentioned this pull request Nov 2, 2021
Merged
@kwvg kwvg requested review from PastaPastaPasta and UdjinM6 and removed request for UdjinM6 November 2, 2021 06:03
PastaPastaPasta
PastaPastaPasta previously approved these changes Nov 2, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm not positive that this is correct / ideal. Waiting for Udjin
utACK for squash merge

UdjinM6
UdjinM6 requested changes Nov 3, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

I don't think we can just drop nSigOps > nSize / nBytesPerSigOp and tweak help text without implementing actual changes it describes, that feels wrong. Moreover, going through 8365 discussion it looks like it's a bugfix and it makes sense to backport it imo. I guess we missed it due to a very segwit-only looking code but it's not segwit-only actually. Also, I found a couple of related bugs (in test and bench) while trying to make it work. Anyway, pls see https://github.com/UdjinM6/dash/commits/pr4562

PastaPastaPasta
PastaPastaPasta approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

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

UtACK for squash merge

UdjinM6
UdjinM6 approved these changes Nov 8, 2021
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 added this to the 18 milestone Nov 8, 2021
@UdjinM6 UdjinM6 changed the title partial bitcoin#8365: Treat high-sigop transactions as larger rather than rejecting them bitcoin#8365: Treat high-sigop transactions as larger rather than rejecting them Nov 8, 2021
@UdjinM6 UdjinM6 merged commit 0a760bb into dashpay:develop Nov 8, 2021
@UdjinM6 UdjinM6 mentioned this pull request Nov 15, 2021
Merged
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Nov 18, 2021
@kwvg @pravblockc
merge bitcoin#8365: Treat high-sigop transactions as larger rather th…
8f5e66a 
…an rejecting them (dashpay#4562)
gades pushed a commit to cosanta/cosanta-core that referenced this pull request May 24, 2022
@kwvg @gades
merge bitcoin#8365: Treat high-sigop transactions as larger rather th…
223eb30 
…an rejecting them (dashpay#4562)
@kwvg kwvg deleted the hsigopstx branch July 18, 2023 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

@PastaPastaPasta PastaPastaPasta PastaPastaPasta approved these changes

Assignees

No one assigned

Labels

Bitcoin Backport

Projects

None yet

Milestone

18

Development

Successfully merging this pull request may close these issues.

3 participants

@kwvg @UdjinM6 @PastaPastaPasta