Skip to content

[28.x] 28.1 backports and final changes #31594

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 6 commits into from
Jan 7, 2025
Merged

[28.x] 28.1 backports and final changes #31594

achow101 merged 6 commits into from
Jan 7, 2025

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Jan 2, 2025

MarcoFalke and others added 3 commits January 2, 2025 14:16
rpc: Extend scope of validation mutex in generateblock
621c634 
The mutex (required by TestBlockValidity) must be held after creating
the block, until TestBlockValidity is called. Otherwise, it is possible
that the chain advances in the meantime and leads to a crash in
TestBlockValidity:

 Assertion failed: pindexPrev && pindexPrev == chainstate.m_chain.Tip() (validation.cpp: TestBlockValidity: 4338)

The diff can be reviewed with the git options
--ignore-all-space --function-context

Github-Pull: 31563
Rebased-From: fa62c8b
test: generateblocks called by multiple threads
05cd448 
Co-Authored-By: David Gumberg <davidzgumberg@gmail.com>

Github-Pull: 31563
Rebased-From: fa63b82
@hebasto
depends: Fix CXXFLAGS on NetBSD
5b368f8 
This change corrects an issue where CXXFLAGS were mistakenly overridden
by CFLAGS.

Github-Pull: 31502
Rebased-From: a10bb40
@DrahtBot
Copy link
Contributor

DrahtBot commented Jan 2, 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/31594.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK glozow, achow101

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

This was referenced Jan 2, 2025
Closed
Merged
@maflcko maflcko changed the title 28.1rc3 backports [28.x] 28.1rc3 backports Jan 2, 2025
@achow101
Copy link
Member

achow101 commented Jan 2, 2025

Are these significant enough to warrant a rc3 or can we go straight to final?

@maflcko maflcko changed the title [28.x] 28.1rc3 backports [28.x] 28.1 backports Jan 2, 2025
@maflcko maflcko changed the title [28.x] 28.1 backports [28.x] 28.1 backports (final?) Jan 2, 2025
@maflcko
Copy link
Member Author

maflcko commented Jan 2, 2025

Are these significant enough to warrant a rc3 or can we go straight to final?

Yes, seems fine in this special case, as the depends patch shouldn't affect the release bin and the other change is test-only. I went ahead and picked your commits from #31582

@achow101
Copy link
Member

achow101 commented Jan 3, 2025

ACK 1451c56cce3e3df784e1dac969ffb0165df313c7

@mzumsande mzumsande mentioned this pull request Jan 3, 2025
Closed
1 task
glozow
glozow reviewed Jan 3, 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 1451c56cce3e3df784e1dac969ffb0165df313c7 except for release note

@glozow glozow changed the title [28.x] 28.1 backports (final?) [28.x] 28.1 backports and final changes Jan 3, 2025
@laanwj
Copy link
Member

laanwj commented Jan 3, 2025 •
edited
Loading

Are these significant enough to warrant a rc3 or can we go straight to final?

The only change here that could trigger a new RC is the change in src/rpc/mining.cpp, which affects the release binary, but as that's contained to a test-only RPC i don't think it's worth rolling another rc for. The rest are test changes (and build system for an OS that's only tangentially supported).

@Sjors
Copy link
Member

Sjors commented Jan 4, 2025

In light of #31600 (comment) it may be worth back-porting that PR as well. On the other hand it only impacts testnet4.

@maflcko
Copy link
Member Author

maflcko commented Jan 6, 2025

@Sjors I don't think it is useful to backport PRs which have neither been reviewed, nor merged into a final tag without a rc even. Especially, given that the fix is for test-only network with brittle mining anyway.

@Sjors
Copy link
Member

Sjors commented Jan 6, 2025

@maflcko obviously I'm not advocating backporting the PR before it's merged, but rather to delay the release until it is.

Especially, given that the fix is for test-only network with brittle mining anyway.

That's probably a good reason to not wait, but it seemed good to bring it up.

@glozow
Copy link
Member

glozow commented Jan 6, 2025

reACK 36314b8

@DrahtBot DrahtBot requested a review from achow101 January 6, 2025 12:31
@achow101
Copy link
Member

achow101 commented Jan 7, 2025

ACK 36314b8

@achow101 achow101 merged commit 32efe85 into bitcoin:28.x Jan 7, 2025
16 checks passed
@maflcko maflcko deleted the 2501-28-back branch January 7, 2025 19:57
Naomielenag

This comment was marked as off-topic.

Sign in to view

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Mar 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@glozow glozow glozow left review comments

@achow101 achow101 Awaiting requested review from achow101

+1 more reviewer

@Naomielenag Naomielenag Naomielenag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Backport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@maflcko @DrahtBot @achow101 @laanwj @Sjors @glozow @Naomielenag @hebasto