Skip to content

[29.x] Backport #32521 #33013

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 4 commits into from
Jul 24, 2025
Merged

[29.x] Backport #32521 #33013

merged 4 commits into from
Jul 24, 2025

Conversation

darosior
Copy link
Member

This backports PR #32521 to make the change available to miners who can't (or don't want to) upgrade past version 29.

darosior added 3 commits July 18, 2025 16:51
@darosior
policy: make pathological transactions packed with legacy sigops non-…
204b965 
…standard.

The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should prevent the ability for someone to
broadcast a transaction through the p2p network that is not valid according to the new rules. This
is because if it was possible it would be a trivial DoS to potentially unupgraded miners after the
soft fork activates.

We do not know for sure whether users will activate the Consensus Cleanup. However if they do such
transactions must have been made non-standard long in advance, due to the time it takes for most
nodes on the network to upgrade. In addition this limit may only be run into by pathological
transactions which pad the Script with sigops but do not use actual signatures when spending, as
otherwise they would run into the standard transaction size limit.

Github-Pull: bitcoin#32521
Rebased-From: 5863315
@darosior
qa: unit test standardness of inputs packed with legacy sigops
0a4671d 
Check bounds and different output types.

Github-Pull: bitcoin#32521
Rebased-From: 3671479
@darosior
qa: functional test a transaction running into the legacy sigop limit
3130233 
It's useful to have an end-to-end test in addition to the unit test to sanity check the RPC error as
well as making sure the transaction is otherwise fully standard.

Github-Pull: bitcoin#32521
Rebased-From: 96da68a
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 18, 2025 •
edited
Loading

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33013.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK marcofleon, glozow

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@fanquake fanquake changed the title Backport #32521 to 29 [29.x] Backport #32521 Jul 21, 2025
@glozow
Copy link
Member

glozow commented Jul 21, 2025 •
edited
Loading

@darosior could you make the release notes updates as well please? Thanks

For example

diff --git a/doc/release-notes.md b/doc/release-notes.md
index 3a39efadbce..63327ea13a5 100644
--- a/doc/release-notes.md
+++ b/doc/release-notes.md
@@ -49,9 +49,10 @@ Notable changes
 - #31757 wallet: fix crash on double block disconnection
 - #32553 wallet: Fix logging of wallet version
 
-### P2P
+### P2P and Mempool Policy
 
 - #32826 p2p: add more bad ports
+- #32521 policy: make pathological transactions packed with legacy sigops non-standard
 
 ### Test

@darosior
Copy link
Member Author

Sure, will do.

@darosior
Copy link
Member Author

Added a commit with the suggested release note. While i was there i also changed my nym for my name in the credits.

marcofleon
marcofleon reviewed Jul 22, 2025
View reviewed changes
Copy link
Contributor

@marcofleon marcofleon left a comment

Choose a reason for hiding this comment

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

ACK 716b436

Made sure the changes here match #32521. All unit and functional tests pass on 29.x.

@fanquake
Copy link
Member

Can you squash the last two commits into doc: update release notes for 29.x? Although, I think this change should have it's full release note displayed (#33037). i.e something like this: fanquake@fc2e2ef.

@fanquake fanquake added this to the 29.1 milestone Jul 23, 2025
@darosior darosior force-pushed the 2507_backport_32521_29 branch from 716b436 to f25dc84 Compare July 23, 2025 16:48
@darosior
Copy link
Member Author

Cherry-picked your commit

@maflcko
Copy link
Member

maflcko commented Jul 24, 2025

the ci failure is unrelated. The fix was cd8089c, but that never made it into 29.x

@marcofleon
Copy link
Contributor

reACK f25dc84

@darosior
Copy link
Member Author

Opened a separate backport PR for the flakiness fix: #33052

@darosior
Copy link
Member Author

Should i rebase now that #33052 is merged?

glozow
glozow reviewed Jul 24, 2025
View reviewed changes
Copy link
Member

@glozow glozow left a comment

Choose a reason for hiding this comment

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

ACK f25dc84

No you don't need to rebase

@glozow glozow merged commit 4bf7795 into bitcoin:29.x Jul 24, 2025
17 of 18 checks passed
@darosior darosior deleted the 2507_backport_32521_29 branch July 24, 2025 16:39
@fanquake fanquake mentioned this pull request Jul 24, 2025
Merged
@darosior darosior mentioned this pull request Sep 3, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glozow glozow glozow left review comments

+1 more reviewer

@marcofleon marcofleon marcofleon left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
Backport CI failed
Projects
None yet
Milestone
29.1
Development

Successfully merging this pull request may close these issues.
6 participants
@darosior @DrahtBot @glozow @fanquake @maflcko @marcofleon