Skip to content

[28.x] More backports #33415

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
achow101 merged 4 commits into from
Sep 25, 2025
Merged

[28.x] More backports #33415

achow101 merged 4 commits into from
Sep 25, 2025

Conversation

@fanquake
Copy link
Member

@fanquake fanquake commented Sep 17, 2025 •
edited
Loading

@fanquake fanquake added this to the 28.3 milestone Sep 17, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 17, 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/33415.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK willcl-ark, achow101
Stale ACK 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 marked this pull request as ready for review September 17, 2025 13:54
This was referenced Sep 17, 2025
Merged
Merged
willcl-ark
willcl-ark approved these changes Sep 17, 2025
View reviewed changes
Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

ACK 7575828dd2ea539e103067cd35e31333797d22e3

Backports look good. Release notes contain all commits and authors.

@achow101
Copy link
Member

achow101 commented Sep 17, 2025

Should we also backport #33106 if we're going to be doing a 28.3 anyways?

@glozow
Copy link
Member

glozow commented Sep 17, 2025 •
edited
Loading

Should we also backport #33106 if we're going to be doing a 28.3 anyways?

Here is a branch to do this: https://github.com/glozow/bitcoin/tree/2025-09-28.3-backport

Similar to #33226, needed to do some test massaging. I also needed #30948 and #30784 for the test utils/refactors. Feel free to pull; I updated the final changes as well.

@fanquake
Copy link
Member Author

fanquake commented Sep 18, 2025

Here is a branch to do this: https://github.com/glozow/bitcoin/tree/2025-09-28.3-backport

Thanks. Looks like there are some issues with this branch. i.e:

test/mempool_tests.cpp:434: Entering test case "MempoolSizeLimitTest"
<snip>
test/mempool_tests.cpp:562: error: in "mempool_tests/MempoolSizeLimitTest": check pool.GetMinFee(1).GetFeePerK() == maxFeeRateRemoved.GetFeePerK() + 1000 has failed [19941 != 20841]
2025-09-18T09:27:52.459923Z (mocktime: 1970-01-01T12:00:42Z) [test] [validationinterface.cpp:228] [MempoolTransactionsRemovedForBlock] [validation] Enqueuing MempoolTransactionsRemovedForBlock: block height=1 txs removed=0
test/mempool_tests.cpp:566: error: in "mempool_tests/MempoolSizeLimitTest": check pool.GetMinFee(1).GetFeePerK() == llround((maxFeeRateRemoved.GetFeePerK() + 1000)/2.0) has failed [9971 != 10421]
test/mempool_tests.cpp:570: error: in "mempool_tests/MempoolSizeLimitTest": check pool.GetMinFee(pool.DynamicMemoryUsage() * 5 / 2).GetFeePerK() == llround((maxFeeRateRemoved.GetFeePerK() + 1000)/4.0) has failed [4985 != 5210]
test/mempool_tests.cpp:574: error: in "mempool_tests/MempoolSizeLimitTest": check pool.GetMinFee(pool.DynamicMemoryUsage() * 9 / 2).GetFeePerK() == llround((maxFeeRateRemoved.GetFeePerK() + 1000)/8.0) has failed [2493 != 2605]
test/mempool_tests.cpp:578: error: in "mempool_tests/MempoolSizeLimitTest": check pool.GetMinFee(1).GetFeePerK() == 1000 has failed [100 != 1000]

Want to PR / debug that separately?

@glozow
Copy link
Member

glozow commented Sep 23, 2025

Thanks. Looks like there are some issues with this branch. i.e:

Ah sorry about that! It's fixed on the branch now.

Want to PR / debug that separately?

Happy to review this and do a separate PR if that's easier. The default changes also require manpage modifications, so it might be better to defer the rc final changes if you'd like a separate PR.

@fanquake
Copy link
Member Author

fanquake commented Sep 23, 2025

Ok, I'll just pull the branch in here (shortly).

glozow
glozow reviewed Sep 23, 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 1b1f359fc43385cb4d465b15754dac84eef06873

Could probably have someone double check the min feerate backport. The CI failure is related to #31210 / #30125. IIUC we skipped backporting #30125 as it's quite involved (also see #31422 (review))

@DrahtBot DrahtBot requested a review from willcl-ark September 23, 2025 18:26
achow101
achow101 reviewed Sep 23, 2025
View reviewed changes
Copy link
Member

@achow101 achow101 left a comment

Choose a reason for hiding this comment

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

It looks like 3eab8b7 was skipped in the backport? It doesn't seem like there are significant conflicts and it makes 2e756e8b02d30e9baacd36d4e519ab64421778ba easier to review.

IIUC we skipped backporting #30125 as it's quite involved (also see #31422 (review))

It backports without issue to 28.x. For 27.x, the second commit is complicated, but I think it could've been skipped.

willcl-ark
willcl-ark approved these changes Sep 23, 2025
View reviewed changes
Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

ACK 1b1f359fc43385cb4d465b15754dac84eef06873

#33106 backport is naturally a bit tricky to review, especially as I didn't review the original PR.

That said, git-range-diff helped out a fair bit e.g. git range-diff e5f896bb1f052fb8c7811c6024cb49143b427512..ba84a25deec0b3b9b94ee51b373e715fec995791 6090af0d350..2e756e8b02d uses the range-diff algo on the two commit ranges, which is pretty neat! It helped me identify the "massaging" that was needed for the backport, which looks fine to me.

I wonder if we want to also include the getmininginfo followup here too? :hide: 9169a50 (#33189)

@willcl-ark
Copy link
Member

willcl-ark commented Sep 23, 2025 •
edited
Loading

It looks like 3eab8b7 was skipped in the backport? It doesn't seem like there are significant conflicts and it makes 2e756e8 easier to review.

Ah, I was just complaining about this being tricky to review offline, but now see it in the range-diff too. That would have indeed made it easier for me to review.

 6:  3eab8b72404 <  -:  ----------- [prep/test] replace magic number 1000 with respective feerate vars

@glozow
Copy link
Member

glozow commented Sep 23, 2025

It looks like 3eab8b7 was skipped in the backport? It doesn't seem like there are significant conflicts and it makes 2e756e8 easier to review.

My bad - I missed that commit and it didn't hit me that there was a separate commit while I was resolving the conflicts.

@maflcko maflcko removed the CI failed label Sep 24, 2025
achow101
achow101 reviewed Sep 24, 2025
View reviewed changes
hebasto and others added 3 commits September 24, 2025 14:37
@hebasto @fanquake
Fix benchmark CSV output
a381de7 
The `SHA256AutoDetect` return output is used, among other use cases, to
name benchmarks. Using a comma breaks the CSV output.

This change replaces the comma with a semicolon, which fixes the issue.

Github-Pull: bitcoin#33340
Rebased-From: 790b440
@mzumsande @fanquake
net: Do not apply whitelist permission to onion inbounds
9e56d88 
Tor inbound connections do not reveal the peer's actual network address.
Therefore do not apply whitelist permissions to them.

Co-authored-by: Vasil Dimov <[email protected]>

Github-Pull: bitcoin#33395
Rebased-From: f563ce9
@fanquake
doc: Remove wrong and redundant doxygen tag
4598dfc 
Remove it in feerate.

Fix it in the other places.

Github-Pull: bitcoin#33236
Rebased-From: 966666d
@glozow glozow mentioned this pull request Sep 24, 2025
Merged
@glozow
Copy link
Member

glozow commented Sep 24, 2025

Opened #33476 to follow

willcl-ark
willcl-ark approved these changes Sep 25, 2025
View reviewed changes
Copy link
Member

@willcl-ark willcl-ark left a comment

Choose a reason for hiding this comment

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

ACK a5e4fec

This now only includes #33236, #33395 and #33340, with the other changes moving into #33476.

Backports look fine, 966666d was the only commit that appears to have needed resolution.

All backports are credited in the release notes.

@DrahtBot DrahtBot requested a review from glozow September 25, 2025 12:39
@achow101
Copy link
Member

achow101 commented Sep 25, 2025

ACK a5e4fec

@achow101 achow101 merged commit a0b5730 into bitcoin:28.x Sep 25, 2025
17 checks passed
@kevkevinpal kevkevinpal mentioned this pull request Sep 25, 2025
Closed
@fanquake fanquake deleted the 28_x__backports branch September 26, 2025 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@achow101 achow101 achow101 left review comments

@glozow glozow Awaiting requested review from glozow

+1 more reviewer

@willcl-ark willcl-ark willcl-ark approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Backport

Projects

None yet

Milestone

28.3

Development

Successfully merging this pull request may close these issues.

8 participants

@fanquake @DrahtBot @achow101 @glozow @willcl-ark @maflcko @hebasto @mzumsande