97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3 util: Remove `fsbridge::get_filesystem_error_message()` (Hennadii Stepanov)
Pull request description:
The `fsbridge::get_filesystem_error_message()` function exhibits several drawbacks:
1. It was introduced in https://github.com/bitcoin/bitcoin/pull/14192 to account for platform-specific variations in
`boost::filesystem::filesystem_error::what()`. Since [migrating](https://github.com/bitcoin/bitcoin/pull/20744) to `std::filesystem`, those discrepancies no longer exist.
2. It fails to display UTF-8 paths correctly on Windows:
```
> build\bin\Release\bitcoind.exe -datadir="C:\Users\hebasto\dd_₿_🏃" -regtest
...
2025-04-30T00:17:48Z DeleteAuthCookie: Unable to remove random auth cookie file: remove: Access is denied.: "C:\Users\hebasto\dd_?_??\regtest\.cookie"
...
```
3. It relies on `std::wstring_convert`, which was deprecated in C++17 and removed in C++26 (also see https://github.com/bitcoin/bitcoin/issues/32361).
This PR removes the obsolete `fsbridge::get_filesystem_error_message()` function, thereby resolving all of the above issues.
ACKs for top commit:
maflcko:
lgtm re-ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
davidgumberg:
untested crACK 97eaadc3bf
achow101:
ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
laanwj:
Code review ACK 97eaadc3bf9f621ba397e29bb1c0cd99af55f2e3
Tree-SHA512: 3c7378a9b143ac2a71add967318a13c346ae3bccbec6e9879d7873083f3fa469b3eef529b2c9c142b2489ba9563e4e12f685745c09a8a219d58b384f7ecf1be1
The `fsbridge::get_filesystem_error_message()` function exhibits several
drawbacks:
1. It was introduced in https://github.com/bitcoin/bitcoin/pull/14192 to
account for platform-specific variations in
`boost::filesystem::filesystem_error::what()`. Since migrating to
`std::filesystem`, those discrepancies no longer exist.
2. It fails to display UTF-8 paths correctly on Windows.
3. It relies on `std::wstring_convert`, which was deprecated in C++17
and removed in C++26.
This change removes the `fsbridge::get_filesystem_error_message()`
function, thereby resolving all of the above issues.
Additionally, filesystem error messages now use the "Warning" log level.
a8333fc9ff9adaa97a1f9024f5783cc071777150 scripted-diff: wallet: rename plain and encrypted master key variables (Sebastian Falbesoner)
5a92077fd5317f936da2fa0aa45e0173248f765b wallet: refactor: dedup master key decryption (Sebastian Falbesoner)
846545947cd3b993c40362b9d0afcd7b4f5f05bd wallet: refactor: dedup master key encryption / derivation rounds setting (Sebastian Falbesoner)
a6d9b415aa3afcfe463887d0fde00c3d2d32672a wallet: refactor: introduce `CMasterKey::DEFAULT_DERIVE_ITERATIONS` constant (Sebastian Falbesoner)
62c209f50d9c33fde5062ebca317b9a4233aff62 wallet: doc: remove mentions of unavailable scrypt derivation method (Sebastian Falbesoner)
Pull request description:
This PR contains various cleanups around the wallet's master key encryption logic. The default/minimum key derivation rounds magic number of 25000 is hoisted into a constant (member of `CMasterKey`) and two new functions `EncryptMasterKey`/`DecryptMasterKey` are introduced in order to deduplicate code for the derivation round determination and master key en/decryption. Also, mentions of the never-implemented derivation method `scrypt` are removed from the wallet crypter header and both plain and encrypted master key instances are renamed to adapt to moderning coding style (hopefully improving readability).
ACKs for top commit:
davidgumberg:
ACK a8333fc9ff
achow101:
ACK a8333fc9ff9adaa97a1f9024f5783cc071777150
Tree-SHA512: 5a66d3b26f481347d0b5b4f742dd237803a35aad6e3480ed15fd38b7fa3700650bd5f67f4c30ed88f5fad45d6cd4c893fe4f1657e36e563b4294fd3596187724
55b931934a34bab11446e8eed7bdaef92bb056de removed duplicate calling of GetDescriptorScriptPubKeyMan (Saikiran)
Pull request description:
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs https://github.com/bitcoin/bitcoin/issues/32013.
**Steps to reproduce in testnet environment**
**Input size:** 2 million address in the wallet
**Step1:** call importaddresdescriptor rpc method
observe the time it has taken.
**With the provided fix:**
Do the same steps again
observe the time it has taken.
There is a huge improvement in the performance. (previously it may take 5 to 6 seconds now it will take 1 seconds or less)
main changes i've made during this pr:
1. remove duplicate call to GetDescriptorScriptPubKeyMan method
2. And inside GetDescriptorScriptPubKeyMan method previously we checking **each address linearly** so each time it is calling HasWallet method which has aquired lock.
3. Now i've modified this logic call **find method on the map (O(logn)**) time it is taking, so only once we calling HasWallet method.
**Note:** Smaller inputs in the wallet you may not see the issue but huge wallet size it will definitely impact the performance.
ACKs for top commit:
achow101:
ACK 55b931934a34bab11446e8eed7bdaef92bb056de
w0xlt:
ACK 55b931934a
Tree-SHA512: 4a7fdbcbb4e55bd034e9cf28ab4e7ee3fb1745fc8847adb388c98a19c952a1fb66d7b54f0f28b4c2a75a42473923742b4a99fb26771577183a98e0bcbf87a8ca
Legacy wallets do not have the descriptors flag set. Don't load wallets
without the descriptors flag.
At the same time, we will no longer load BDB databases since they are
only used for legacy wallets.
fa86190e6ed2aeda7bcceaf96f52403816bcd751 rpc: Allow fullrbf fee bump (MarcoFalke)
Pull request description:
The RPCs (psbt)bumpfee, and the GUI, reject fee bumps when BIP 125 signalling is absent in the transaction even when the mempool and other RPCs allow them. Fix the confusion by allowing the fee bump.
This is done after fullrbf is always on (https://github.com/bitcoin/bitcoin/pull/30592)
ACKs for top commit:
1440000bytes:
reACK fa86190e6e
achow101:
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
w0xlt:
ACK fa86190e6e
rkrux:
reACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
glozow:
ACK fa86190e6ed2aeda7bcceaf96f52403816bcd751
Tree-SHA512: b2ffe8dcadbe71e9be767a16cf8aa0bf383c2de7aa1aee9438d125f444e24f3f7e4f02ddb28981bd3b8b645b6a24a407b4ad6bb0b21946ae637e78f6386e05bf
acee5c59e68f31acba111bc4d1892b08243ea5e0 descriptors: Have GetPrivKey fill keys directly (Ava Chow)
4b0303197e40a556bea2d9df76d1407981c361e6 descriptors: Move FlatSigningProvider pubkey filling to GetPubKey (Ava Chow)
25a3b9b0f52e61e0189d6e7e727a0ffd2b1e39fa descriptors: Have GetPubKey fill origins directly (Ava Chow)
6268bde0af0aa9cfb3df08d6b9b67fdffa2a1a12 descriptor: Remove unused parent_info from BIP32PUbKeyProvider::GetPubKey (Ava Chow)
0ff072caa14e4d32f6f4118f15f4f3718cef1d6a wallet, rpc: Only allow keypool import from single key descriptors (Ava Chow)
Pull request description:
Instead of having `MakeScripts` infer what pubkeys need to go into the output `FlatSigningProvider`, have each of the `PubkeyProviders` that have `GetPubKey` and `GetPrivKey` called fill it directly with relevant keys and origins.
This allows for keys and origins to be added that won't directly appear in the output, which is necessary for `musig()` descriptors.
Split from #29675
ACKs for top commit:
fjahr:
Code review ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
theStack:
re-ACK acee5c59e68f31acba111bc4d1892b08243ea5e0
rkrux:
ACK acee5c5
Tree-SHA512: c1841359bcb08cdd433122deef96579236928660785f3357a3eb584e47d290cd1c60ebe8f7fba50f178ba45c9a90773124e0f509e36c5a0df97c1a4890e03e5c
Also, fix the incorrect documention of the 'replaceable' RPC argument
with respect to sequence number handling. The docs were incorrect
before, so the fix could be extracted, but it seems fine to include here
as well.
32dcec269bf33f7be28245d88a1d8f2889cc39ae rpc: update RPC help of `createpsbt` (rkrux)
931117a46f5854d487e13b2b1446b621409c8371 rpc: update the doc for `data` field in `outputs` argument (rkrux)
8134a6b5d40568dcf32fdb21163cb1792efddc27 rpc: add cli example for `walletcreatefundedpsbt` RPC (rkrux)
Pull request description:
### add cli example for `walletcreatefundedpsbt` and `createpsbt` RPCs
The only example present earlier was one that creates an OP_RETURN output. This
lack of examples has discouraged me earlier to use this RPC. Adding an example
that creates PSBT sending bitcoin to address, a scenario that is much more common.
### rpc: update the doc for `data` field in `outputs` argument
It was not evident to me that this field creates an `OP_RETURN` output until
I read the code and tried it out. Thus, making the doc explicitly mention it.
This affects docs of the following RPCs:
`bumpfee`, `psbtbumpfee`, `send`, `walletcreatefundedpsbt`, `createpsbt`,
and `createrawtransaction`
ACKs for top commit:
sipa:
utACK 32dcec269bf33f7be28245d88a1d8f2889cc39ae
1440000bytes:
utACK 32dcec269b
achow101:
ACK 32dcec269bf33f7be28245d88a1d8f2889cc39ae
ryanofsky:
Concept ACK 32dcec269bf33f7be28245d88a1d8f2889cc39ae. These seem like helpful clarifications, but I did not look into the details
Tree-SHA512: f994488ba7d52d00960fc52064bb419cf548e29822fe23d6ee0452fdf514dd93f089145eddb32b8086a7918cf8cf33a4c3f16bfcb7948f3c9d5afd95e8d3a1cb
Legacy wallets should only import keys to the keypool if they came in a
single key descriptor. Instead of relying on assumptions about the
descriptor based on how many pubkeys show up after expanding the
descriptor, explicitly mark descriptors as being single key type and use
that for the check.
The new helper will be used to fix a crash in the
wallet migration process (watch-only, non-blank,
private keys disabled, empty wallet - no scripts
or addresses imported).
Co-authored-by: Matias Furszyfer <mfurszy@protonmail.com>
28dc118001b061aed42880ff418fde7cc5f6255d fuzz: wallet: fix crypter target (brunoerg)
Pull request description:
The crypter target has an issue, it's calling `DecryptKey` with a random secret and a random public key that will unlikely be related to the key used to encrypt, so it won't have any effect. This PR changes fixes it and also removes the `DecryptSecret` call since this function is already (and only) called within `DecryptKey`.
ACKs for top commit:
maflcko:
lgtm ACK 28dc118001b061aed42880ff418fde7cc5f6255d 🥊
Tree-SHA512: e96b7d33879bf06eeec0726e74e8e0d7020997659bf97dfca5d7c1a7ba65c4d93c78e666b97eebde110564cef2eefc7209d3e3586e4658145827b14d1b01dfc9
0ff66b1c4ab0e5cba00a178b24f2c5de75df360f fuzz: coinselection: cover `SetBumpFeeDiscount` (brunoerg)
Pull request description:
`SetBumpFeeDiscount` sets the bump fee discount which is used to calculate the waste. We currently have no fuzz coverage for this function, so this PR adds it by calling `SetBumpFeeDiscount` before `RecalculateWaste`.
ACKs for top commit:
marcofleon:
ACK 0ff66b1c4ab0e5cba00a178b24f2c5de75df360f
Tree-SHA512: d5c1d97daaeb7f9b096bf9bdf6374b8a674a75f464e2b9bb3e1e1774a5805b22840ca1f31bae63f106640d9ce27a99432c3034524340be91c235f6ec3b185cff
It was highlighted in a PR discussion previously that the recently
moved `Assert` macro call inside the block disconnected loop had
been redundant for quite a while because of the presence of the
`assert` macro call at the start of the function. Therefore, it
is removed now.
refs #https://github.com/bitcoin/bitcoin/pull/31757#discussion_r1995416821
This affects docs of the following RPCs:
`bumpfee`, `psbtbumpfee`, `send`, `walletcreatefundedpsbt`, `createpsbt`,
and `createrawtransaction`
It was not evident to me that this field creates an `OP_RETURN` output until
I read the code and tried it out. Thus, making the doc explicitly mention it.
Checking for IsArgSet before calling GetArg while providing an arbitrary
default value as fallback is both confusing and fragile.
It is confusing, because the provided fallback is dead code. So it would
be better to just call GetArg without a fallback.
Even better would be to provide the true fallback value and sanitize it
as if it were user-input, but this can be done in a follow-up.
Removing the redundant call to IsArgSet will have to be done either way,
so do it now.
a015b7e13daacdfb6db0eada50563dec70c5afb2 test: Add expected result assertions (yancy)
Pull request description:
~This is a trivial addition to the test suit, however it shouldn't be required to add debug statements and manually run the tests if someone needs to know the results of this test.~
Add an assertion for the values returned. The goal of the test is to show that a minimal weight selection of UTXOs is returned by coin-grinder. Since there are multiple possible solutions, the added assertion shows that coin-grinder finds the solution with the lowest weight. Without this assertion, it's ambiguous whether or not coin-grinder is returning the solution with the lowest weight.
Remove the check that a result is returned since the expected result assertion implies a result.
ACKs for top commit:
janb84:
re ACK [a015b7e](a015b7e13d)
murchandamus:
ACK a015b7e13daacdfb6db0eada50563dec70c5afb2
Tree-SHA512: ee3c2688b4a4a07ab209f7655c3956e62a1084419df5e87c27d751a38ff64d4c3457df2317f8077149a6947cdb05b249975de2b8f0e18ca8b17b41f4735fb1c6
2f2ab47bf74f4da37aad75a186cb0bb16e8af579 Release notes (Pol Espinasa)
bf194c920cf768d1339d41aef1441a78e2f5fcbe wallet, rpc: deprecate settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR deprecates the settxfee RPC and paytxfee setting, marking it for removal in Bitcoin Core 31.0.
**Motivation**
The PR was initially motivated by https://github.com/bitcoin/bitcoin/issues/31088. The intention was to create a new function `settxfeerate` to allow users to set a static fee rate in `sat/vB` instead of `btc/kvB`.
The `settxfee` RPC allows users to set a static fee rate for all transactions created by the wallet. However, in a dynamic fee environment, this can lead to poor fee choices, either overpaying when the mempool is empty or underpaying when congestion is high. The preferred approach is to rely on fee estimation, which is designed to adapt to network conditions, and is the one by default. Same argument apply for `paytxfee` setting.
During discussion the consensus was that static fee settings are a footgun and that users should instead specify the fee rate per transaction if they don't want to rely on the fee estimation. Given this, rather than introducing a `settxfeerate` alternative, this PR goes towards removing `settxfee` and `paytxfee` entirely.
**Key Changes**
`settxfee` and `paytxfee` is now deprecated and will be removed in Bitcoin Core 31.0.
Users should rely on fee estimation or explicitly specify a fee rate when constructing transactions.
**Impact on Users**
If users currently use settxfee or paytxfee, they should transition to specifying fees per transaction.
No immediate breakage in 30.0 (must use `-deprecatedrpc=settxfee`), but `settxfee` and `paytxfee` will be removed in 31.0.
**Alternative Approaches Considered**
A settxfeerate alternative (using sat/vB) was initially proposed but ultimately rejected in favor of deprecating static fee setting entirely.
**Notes for removal**
- When removing paytxfee we should also update txconfirmtarget startup option help text.
- Get back the comment from `rpc_deprecated.py` test. [+info](https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1998876768)
ACKs for top commit:
fjahr:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
ismaelsadeeq:
re-ACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
rkrux:
Concept and utACK 2f2ab47bf74f4da37aad75a186cb0bb16e8af579
Tree-SHA512: 0272812cbe5a519737c5d0683acc2072e67559792b4a6472bca8b23426e5ce9e88a3a1eba987feda70a082b8b474b3126893848628d7bf11e1520357b18c8d3e
Removed duplicate call to GetDescriptorScriptPubKeyMan and
Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan
after this fix improved performance of importdescriptor part refs #32013.
ba82240553ddd534287845e10bc76b46b45329fe fuzz: split `coinselection` harness (brunoerg)
Pull request description:
This PR splits the `coinselection` fuzz harness into 3 targets (`coinselection_bnb`, `coinselection_knapsack`, `coinselection_srd`). The goal is to be able to fuzz each algorithm separately (to avoid performance issues) and also all of them together.
ACKs for top commit:
janb84:
Tested ACK [ba82240](ba82240553)
maflcko:
review ACK ba82240553ddd534287845e10bc76b46b45329fe 👐
marcofleon:
reACK ba82240553ddd534287845e10bc76b46b45329fe
zaidmstrr:
reACK [ba82240](ba82240553)
Tree-SHA512: 277cffd524e57d286dbbbcb2aa0a9f1d720b4c56331dfb0f4425e1666246330616508e47977da23f28a72705aa142bbaf536e2cf7fe4703a2cd2e4b2fd441d9d
ffff4a293ad878494e12f8f00108cc99ee2b713e bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97bcb1790a7cd4363a13fda7c80c3dd90 refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b40c97375af0722f32f7575bca3af819 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179c062b7ca92d120455ce02a9f4e9e19 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e6e80e3da1ab6448b6212244bafa5d3 scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c03db00a2be3f703aa7e5eec4312bd2e refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717ec18d64b7ff7bba71b8f0c202ba31d test: Fix broken span_tests (MarcoFalke)
fadf02ef8bf96ad5b3b8e34fd425b31b555f4371 refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be17fa9e7c91188710e6a04939ceab11 refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293ad878494e12f8f00108cc99ee2b713e
theuni:
ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
21e9d39a3725cd6107b742f0cb97f65b3640201b docs: add release notes for 31603 (brunoerg)
a8b548d75d9a376c9bb66e06bb918c876416d615 test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62cf5d3ea9b98d5a76e4e54cac07bc3c test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea16a04844c83e56fd6deaa1f0dc0a7e descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef6c02ecbaa0cf448567355b6b86b510 test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
darosior:
re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
sipa:
utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
4cd95a2921805f447a8bcecc6b448a365171eb93 refactor: modernize remaining outdated trait patterns (Lőrinc)
ab2b67fce20fd7d8017f8a26425cab99e91f420d scripted-diff: modernize outdated trait patterns - values (Lőrinc)
8327889f358289f918d04ddb9469fb5562720bf4 scripted-diff: modernize outdated trait patterns - types (Lőrinc)
Pull request description:
The use of [`std::underlying_type_t<T>`](https://en.cppreference.com/w/cpp/types/underlying_type) or [`std::is_enum_v<T>`](https://en.cppreference.com/w/cpp/types/is_enum) (and similar ones, introduced in C++14) replace the `typename std::underlying_type<T>::type` and `std::is_enum<T>::value` constructs (available in C++11).
The `_t` and `_v` helper alias templates offer a more concise way to extract the type and value directly.
I've modified the instances I found in the codebase one-by-one (noticed them while investigating https://github.com/bitcoin/bitcoin/pull/31868), and afterwards extracted scripted diff commits to do the trivial ones automatically.
The last commit contains the values that were easier done manually.
I've excluded changes from `src/bench/nanobench.h`, `src/leveldb`, `src/minisketch`, `src/span.h` and `src/sync.h` - let me know if you think they should be included instead.
A few of the code changes can also be reproduced by clang-tidy (but not all of them):
```bash
cmake -B build -DCMAKE_C_COMPILER=clang -DCMAKE_CXX_COMPILER=clang++ -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON && cmake --build build -j$(nproc)
run-clang-tidy -quiet -p build -j $(nproc) -checks='-*,modernize-type-traits' -fix $(git grep -lE '::(value|type)' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
```
ACKs for top commit:
laanwj:
Concept and code review ACK 4cd95a2921805f447a8bcecc6b448a365171eb93
Tree-SHA512: a4bcf0f267c0f4e02983b4d548ed6f58d464ec379ac5cd1f998b9ec0cf698b53a9f2557a05a342b661f1d94adefc9a0ce2dc8f764d49453aaea95451e2c4c581
36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8 build: require sqlite when building the wallet (Sjors Provoost)
Pull request description:
Require that sqlite is available in order to compile the wallet. Removes instances of `USE_SQLITE` since it is no longer possible to not have sqlite available.
The `NO_SQLITE` option is dropped from depends.
This is another step towards dropping the legacy wallet, extracted from #31250.
ACKs for top commit:
m3dwards:
ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8
davidgumberg:
crACK 36b6f36ac4
hebasto:
re-ACK 36b6f36ac4724cb2c9ed0e25314c3bbf55e4ebb8.
Tree-SHA512: 870a0135671c80c4f28602119eb8637a1ed43b51b1673bfe88425782fb62ec6ef0f3d6baf0d5984d6a243779b0f63423fd4c4dc324ef87bffba13d63e05ad793
11f8ab140fe63857f6a93b81021efda8f90ceeda test: wallet, coverage for crash on dup block disconnection during unclean shutdown (Martin Zumsande)
9ef429b6ae65f6ad3e9ac11c2d9c0a6c52beb865 wallet: fix crash on double block disconnection (furszy)
Pull request description:
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because `disconnectBlock` provides `TxStateInactive` without the "abandoned"
flag for coinbase transactions to `SyncTransaction`, while `AddToWallet()` internally modifies
it to retain the abandoned state.
The crash flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
`AddToWallet` internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to `SyncTransaction()`, the state equality assertion fails and crashes the wallet.
Reviewers Note:
The crash can easily be replicated by cherry-picking the test commit in master.
ACKs for top commit:
mzumsande:
Code Review ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
rkrux:
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
pinheadmz:
ACK 11f8ab140fe63857f6a93b81021efda8f90ceeda
Tree-SHA512: 971069bca562f0afb06c34a2516842d01b5cbc2b18ed851392aa3caa3bb7488f4a84a5d017ea334e6361261d3c44aa597cc67a1d4fa16781f85e081f3d1f8771
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
Require that sqlite is available in order to compile the wallet. Removes
instances of USE_SQLITE since it is no longer possible to not have
sqlite available.
The NO_SQLITE option is dropped from depends.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Transifex interprets the "-0" substring as a number in translatable
strings. Since not all translations preserve "-0," this triggers a
corresponding warning. While this warning could be disabled globally, it
is more reasonable to adjust the original string instead.
The only example present earlier was one that creates an OP_RETURN output. This
lack of examples has discouraged me earlier to use this RPC. Adding an example
that creates PSBT sending bitcoin to address, a scenario that is much more common.
This test returns the lowest weight solution. Other possibilities
either exceed allowed weight or result in a higher weight. Add an
assertion which shows that the lowest weight solution is found and
update the test description accordingly.
Remove the check that a result is returned since the expected result
assertion implies a result.
The use of e.g. `std::underlying_type_t<T>` replaces the older `typename std::underlying_type<T>::type`.
The `_t` helper alias template (such as `std::underlying_type_t<T>`) introduced in C++14 offers a cleaner and more concise way to extract the type directly.
See https://en.cppreference.com/w/cpp/types/underlying_type for details.
-BEGIN VERIFY SCRIPT-
sed -i -E 's/(typename )?(std::[a-z_]+)(<[^<>]+>)::type\b/\2_t\3/g' $(git grep -l '::type' ./src ':(exclude)src/bench/nanobench.h' ':(exclude)src/leveldb' ':(exclude)src/minisketch' ':(exclude)src/span.h' ':(exclude)src/sync.h')
-END VERIFY SCRIPT-
ca6aa0b9bee3fdf355b7154a9a686a80977f2a02 doc: loadwallet loads from relative walletdir (am-sq)
Pull request description:
## Why this change?
https://github.com/bitcoin/bitcoin/issues/30269 describes a need for documentation improvement with the `loadwallet` RPC. Namely, [some users have found](https://bitcoin.stackexchange.com/questions/123331/how-do-you-load-a-regtest-wallet) the usage description confusing when it comes to loading wallets that are not in the normal case of being in the default wallet directory.
The default wallet directory, depending on the machine OS, has the base directory defined here: 9c5cdf07f3/src/common/args.cpp (L699) which is then appended with `/wallets`. So for example, for MacOS, it would be `~/Library/Application Support/Bitcoin/wallets`.
## The changes implemented
1. Change the help text to indicate that the filename (or directory) passed in to `loadwallet` is relative to the base wallet directory
2. Adds additional examples to the help page showing how to fetch a wallet within a subdirectory of the base data directory for wallets, or from an absolute path
ACKs for top commit:
achow101:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
maflcko:
lgtm ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
rkrux:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
jonatack:
ACK ca6aa0b9bee3fdf355b7154a9a686a80977f2a02
Tree-SHA512: 123ae118c79ee1843ed65861e7a008658a53e47d4d14f2b7612561bba1b1dbdb6744f9aaac1587aac231c62d0c1804de848a6d732f1382788b437d9fe6474012
The wallet crashes if it processes the same block disconnection event twice in a row due
to an incompatible coinbase transaction state.
This happens because 'disconnectBlock' provides 'TxStateInactive' without the "abandoned"
flag for coinbase transactions to 'SyncTransaction', while 'AddToWallet()' internally
modifies it to retain the abandoned state.
The flow is as follows:
1) On the first disconnection, the transaction state transitions from "confirmed" to
"inactive," bypassing the state equality check since the provided state differs. Then,
'AddToWallet' internally updates the state to "inactive + abandoned"
2) On the second disconnection, as we provide only the "inactive" state
to 'SyncTransaction()', the state equality assertion fails and crashes the wallet.
e4dd5a351bde88a94326824945f4c8b1e4c15df2 test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139aa9bf7109df78e46936e5a649c70703386 wallet: abandon inactive coinbase tx and their descendants during startup (furszy)
Pull request description:
Since #26499, we mark coinbase transactions and their descendants as abandoned when a reorg arises through the "block disconnection" signal handler. However, this does not cover all scenarios; external wallets could contain coinbase transactions from blocks the node has not seen yet, or the user could have replaced the chain with an earlier or different version (one without the coinbase chain).
This affects balance calculation as well as mempool rebroadcast (descendants shouldn't be relayed).
Fix this by marking orphaned coinbase transactions and their descendants as abandoned during wallet startup.
ACKs for top commit:
achow101:
ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
rkrux:
tACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
mzumsande:
Code Review ACK e4dd5a351bde88a94326824945f4c8b1e4c15df2
Tree-SHA512: 461a43de7a6f5a580f2e6e3b56ec9bc92239cd45e850a2ff594ab5488dcd4a507f68fbbf550a33d7173b2add0de80de1e1b3841e1dfab0c95b284212d8ced08a
Improves the documentation of help output for loadwallet
to clarify that filename is relative to the default
wallet directory. Adds examples that get a wallet from
sub-directories.
4818da809f0e300016390dd41e02c6067ffa008f wallet: fix rescanning inconsistency (Martin Zumsande)
Pull request description:
If the chain advances during a rescan, ScanForWalletTransactions would previously process the new blocks without adjusting `m_last_processed_block`, which would leave the wallet in an inconsistent state temporarily, and could lead to crashes in the GUI reported in #31474.
Fix this by not rescanning blocks beyond `m_last_processed_block` - for all blocks beyond that height, there will be pending BlockConnected notifications that will process them after the rescan is finished.
This means that if rescanning was triggered with `cs_wallet` permanently held (`AttachChain`), additional blocks that were connected during the rescan will only be processed with the pending `blockConnected` notifications after the lock is released.
If rescanning without a permanent `cs_wallet` lock (`RescanFromTime`), additional blocks that were connected during the rescan can be re-processed here because `m_last_processed_block` was already updated by `blockConnected`.
Fixes#31474
ACKs for top commit:
psgreco:
Not that it matters much, but UTACK 4818da809f0e300016390dd41e02c6067ffa008f
achow101:
ACK 4818da809f0e300016390dd41e02c6067ffa008f
furszy:
utACK 4818da809f0e300016390dd41e02c6067ffa008f
Tree-SHA512: 8e7dbc9e00019aef4f80a11776f3089cd671e0eadd3c548cc6267b5c722433f80339a9b2b338ff9b611863de75ed0a817a845e1668e729b71af70c9038b075af
