[24.x] Backports for 24.0.1 #26616
[24.x] Backports for 24.0.1 #26616
Conversation
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: bitcoin#26594 Rebased-From: 86ef7b3
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: bitcoin#26594 Rebased-From: 88afc73
Github-Pull: bitcoin#26594 Rebased-From: 5e65a21
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
In theory this can be merged cleanly without the need to attach metadata: 24.x...achow101:fix-migratewallet-cleanup-segfault |
… fully connected peers Github-Pull: bitcoin#26569 Rebased-From: 845e3a3
… 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: bitcoin#26569 Rebased-From: ce63fca
Github-Pull: bitcoin#26569 Rebased-From: 8f2dac5
…e set The loop break shouldn't have being there. Github-Pull: bitcoin#26560 Rebased-From: f930aef
Github-Pull: bitcoin#26560 Rebased-From: 341ba7f
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: bitcoin#26560 Rebased-From: cf79384
|
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? |
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? |
We aren't.
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.
Everything I've seen, and been told, is that the wallet issue is significant enough to warrant a 24.0.1. |
|
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). |
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'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 |
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. |
|
ACK 8b726bf more familiar with commits from #26560 , but did a quick code review of the other commits and they look good. cherry picked d464b2a, e5d097b, 9d73176, and 8b726bf onto the |
There was a problem hiding this comment.
ACK 8b726bf, 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.
- e15b306 is missing the
m_next_inv_send_timelock annotation from 845e3a3. That's fine since it has no impact on the binary. - 195f0df does not match f930aef,
coins_to_removeis changed from astd::unordered_set<COutPoint, SaltedOutpointHasher>to astd::set<COutPoint>since that is the type ofpreset_coinson 24.x.
Appartently, the Bitcoin Core version 24.0 was released widely and similarily to the previous versions - in the Releases section in the Github repository: 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). |
Backports remaining changes on the 24.0.1 milestone.
Currently backports: