[24.x] Backports for 24.0.1

fanquake commented at 10:27 am on December 1, 2022: member

Backports remaining changes on the 24.0.1 milestone.

Currently backports:

wallet: Avoid null pointer deref when cleaning up migratewallet

If migratewallet fails, we do a cleanup which removes the watchonly and
solvables wallets if they were created. However, if they were not, their
pointers are nullptr and we don't check for that, which causes a
segfault during the cleanup. So check that they aren't nullptr before
cleaning them up.

Github-Pull: #26594
Rebased-From: 86ef7b3c7be84e4183098f448c77ecc9ea7367ab

7a97a56ffb

tests: Test for migrating encrypted wallets

Due to an oversight, we cannot currently migrate encrypted wallets,
regardless of whether they are unlocked. Migrating such wallets will
trigger an error, and result in the cleanup being run. This conveniently
allows us to check some parts of the cleanup code.

Github-Pull: #26594
Rebased-From: 88afc73ae0c67a4482ecd3d77eb2a8fd2673f82d

d464b2af30

wallet: Explicitly say migratewallet on encrypted wallets is unsupported

Github-Pull: #26594
Rebased-From: 5e65a216d1fd00c447757736d4f2899d235e731a

95fded1069

fanquake added the label Backport on Dec 1, 2022

fanquake added this to the milestone 24.0.1 on Dec 1, 2022

DrahtBot commented at 10:27 am on December 1, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	josibake

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

maflcko commented at 4:46 pm on December 1, 2022: member

In theory this can be merged cleanly without the need to attach metadata: https://github.com/bitcoin/bitcoin/compare/24.x...achow101:fix-migratewallet-cleanup-segfault

[net processing] Ensure transaction announcements are only queued for fully connected peers

Github-Pull: #26569
Rebased-From: 845e3a34c49abcc634b5a10ccdd6b10fb4fcf449

e15b306017

[net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack

This commit documents our assumption about
TxRelay::m_tx_inventory_to_send being empty prior to version handshake
completion.

The added Assume acts as testing oracle for our fuzzing tests to
potentially detect if the assumption is violated.

Github-Pull: #26569
Rebased-From: ce63fca13e9b500e9f687d80a457175ac967a371

c8426706de

[test] Add p2p_tx_privacy.py

Github-Pull: #26569
Rebased-From: 8f2dac54096c20afd8fd12c21a2ee5465fea085e

e5d097b639

wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set

The loop break shouldn't have being there.

Github-Pull: #26560
Rebased-From: f930aefff9690a1e830d897d0a8c53f4219ae4a8

195f0dfd0e

test: wallet, coverage for CoinsResult::Erase function

Github-Pull: #26560
Rebased-From: 341ba7ffd8cdb56b4cde1f251768c3d2c2a9b4e9

9d73176d00

test: Coin Selection, duplicated preset inputs selection

This exercises the bug inside CoinsResult::Erase that
ends up on (1) a wallet crash or (2) a created and
broadcasted tx that contains a reduced recipient's amount.

This is covered by making the wallet selects the preset
inputs twice during the coin selection process.

Making the wallet think that the selection process result covers
the entire tx target when it does not. It's actually creating
a tx that sends more coins than what inputs are covering for.

Which, combined with the SFFO option, makes the wallet
incorrectly reduce the recipient's amount by the difference
between the original target and the wrongly counted inputs.
Which means, a created and relayed tx sending less coins to
the destination than what the user inputted.

Github-Pull: #26560
Rebased-From: cf793846978a8783c23b66ba6b4f3f30e83ff3eb

8b726bf556

instagibbs commented at 6:35 pm on December 5, 2022: member

LGTM https://github.com/bitcoin/bitcoin/pull/26616/commits/8b726bf556e05edf02946d4b1c3356df17fd0d57

Includes the expected fix commits from #26560 and only minor conflicts for the relay piece.

luke-jr commented at 2:13 am on December 6, 2022: member

Why 24.0.1 instead of 24.1? What was the point of changing the version scheme if we’re just going to revert back to the old one?

maflcko commented at 8:16 am on December 6, 2022: member

Why 24.0.1 instead of 24.1?

It is based on the assumption that 24.0 will be nuked from the bin folder, because of the wallet bug. Or is the wallet bug less severe than thought?

fanquake commented at 9:21 am on December 6, 2022: member

What was the point of changing the version scheme if we’re just going to revert back to the old one?

We aren’t.

Why 24.0.1 instead of 24.1?

While 24.0 has been tagged, it hasn’t been more widely released (no website post, no mailing list annoucements, a few tweets doesn’t count). Rather than tagging a 24.1, which would give the false impression of a more stable point release, containing a number of bug fixes after months of 24.0 running in production, it’s more appropritate to incorporate these additional fixes into 24.x, essentially consider 24.0 DOA, and tag a 24.0.1.

Or is the wallet bug less severe than thought?

Everything I’ve seen, and been told, is that the wallet issue is significant enough to warrant a 24.0.1.

luke-jr commented at 9:21 am on December 6, 2022: member

I don’t see why that would involve going back to X.Y.Z versions. The way things stand, this should be v24.1, while v24.0 gets deleted (or not; irrelevant to the next version number).

josibake commented at 9:24 am on December 6, 2022: member

Why 24.0.1 instead of 24.1?

It is based on the assumption that 24.0 will be nuked from the bin folder, because of the wallet bug. Or is the wallet bug less severe than thought?

it’s pretty severe in that it affects users doing manual input selection, either in the GUI via coin control or via the RPC, which I think warrants nuking v24.0.

I don’t see why that would involve going back to X.Y.Z versions. The way things stand, this should be v24.1, while v24.0 gets deleted (or not; irrelevant to the next version number).

I’m not super familiar with semantic versioning, but v24.0.1 feels more appropriate as this isn’t really a major update and more just replacing v24.0 with a bugfix

luke-jr commented at 9:39 am on December 6, 2022: member

v24.0.1 feels more appropriate as this isn’t really a major update and more just replacing v24.0 with a bugfix

With our X.Y, the Y is for minor bugfix-only release, not major ones…

fanquake commented at 9:47 am on December 6, 2022: member

With our X.Y, the Y is for minor bugfix-only release, not major ones…

That’s why Y is going to remain 0, and we’re releasing a 24.0.1.

We’ve done this this before, with 0.19.0 and 0.19.0.1, under the same circumstances. A last minute issue, where a major release was tagged, but never actually widely released, and we shortly after released a X.0.1.

josibake commented at 11:02 am on December 6, 2022: member

ACK https://github.com/bitcoin/bitcoin/pull/26616/commits/8b726bf556e05edf02946d4b1c3356df17fd0d57

more familiar with commits from #26560 , but did a quick code review of the other commits and they look good.

cherry picked https://github.com/bitcoin/bitcoin/pull/26616/commits/d464b2af30f2b02be2ce0b5e45dc6c141529dba5, https://github.com/bitcoin/bitcoin/pull/26616/commits/e5d097b639c7f75b530349b524836804cb753597, https://github.com/bitcoin/bitcoin/pull/26616/commits/9d73176d00a013e1383ae18cb5c0f8cbdd186cba, and https://github.com/bitcoin/bitcoin/pull/26616/commits/8b726bf556e05edf02946d4b1c3356df17fd0d57 onto the 24.x branch and verified the newly introduced unit and functional tests failed as expected and then verified they all passed on the backports branch :tada:

maflcko merged this on Dec 6, 2022

maflcko closed this on Dec 6, 2022

fanquake deleted the branch on Dec 6, 2022

glozow commented at 11:48 am on December 6, 2022: member

ACK 8b726bf556e05edf02946d4b1c3356df17fd0d57, diff matches my local backport. The commits look correct to me though I haven’t reviewed #26594.

Noting that this isn’t a straight cherry-pick but isn’t incorrect.

e15b3060179f94962eff82f3ed87a1d26ef65c88 is missing the m_next_inv_send_time lock annotation from 845e3a34c49abcc634b5a10ccdd6b10fb4fcf449. That’s fine since it has no impact on the binary.
195f0dfd0ec7fadfbbb3d86decb3f6d96beae159 does not match f930aefff9690a1e830d897d0a8c53f4219ae4a8, coins_to_remove is changed from a std::unordered_set<COutPoint, SaltedOutpointHasher> to a std::set<COutPoint> since that is the type of preset_coins on 24.x.

GregTonoski commented at 9:54 am on December 7, 2022: none

While 24.0 has been tagged, it hasn’t been more widely released (no website post, no mailing list annoucements, a few tweets doesn’t count). Rather than tagging a 24.1, which would give the false impression of a more stable point release, containing a number of bug fixes after months of 24.0 running in production, it’s more appropritate to incorporate these additional fixes into 24.x, essentially consider 24.0 DOA, and tag a 24.0.1.

Appartently, the Bitcoin Core version 24.0 was released widely and similarily to the previous versions - in the Releases section in the Github repository: There is also media covarage.

At least for me, the 24.1 would not give the false impression of a more stable point release (in comparison to 24.0.1).

There are already many instances of 24.0 run by people so I think that it cannot be considered dead on arrival (DOA).

The update to the previous version 24.0 would be clearer to me if its version number is 24.1 instead of 24.0.1. There wouldn’t have been many 24.0 versions to confuse them with one another. There isn’t the 3rd number expected in the next version of the Bitcoin Core if it is not used in the previous one (24.0).

bitcoin deleted a comment on Mar 17, 2023

bitcoin locked this on Mar 19, 2024

[24.x] Backports for 24.0.1 #26616

Reviews