"IP" stands for "Internet Protocol".
"IP address" is sometimes shortened to just "IP" or "address".
However, Tor or I2P addresses are not "IP addresses", nor "IPs".
Thus, use "Addr" instead of "IP" for addresses that could be IP, Tor or
I2P addresses:
`CService::ToStringIPPort()` -> `CService::ToStringAddrPort()`
`CNetAddr::ToStringIP()` -> `CNetAddr::ToStringAddr()`
-BEGIN VERIFY SCRIPT-
sed -i 's/ToStringIPPort/ToStringAddrPort/g' -- $(git grep -l ToStringIPPort src)
sed -i 's/ToStringIP/ToStringAddr/g' -- $(git grep -l ToStringIP src)
-END VERIFY SCRIPT-
956c67059caf3807b3ceacdd5de1caaae538f009 refactor, doc: Improve SetupAddressRelay call in version processing (Martin Zumsande)
3c43d9db1e0f84279b08a93afd730caeece061a9 p2p: Don't self-advertise during VERSION processing (Gleb Naumenko)
Pull request description:
This picks up the last commit from #19843.
Previously, we would prepare to self-announce to a new peer while parsing a `version` message from that peer.
This is redundant, because we do something very similar in `MaybeSendAddr()`, which is called from `SendMessages()` after
the version handshake is finished.
There are a couple of differences:
1) `MaybeSendAddr()` self-advertises to all peers we do address relay with, not just outbound ones.
2) `GetLocalAddrForPeer()` called from `MaybeSendAddr()` makes a probabilistic decision to either advertise what they think we are or what we think we are, while `PushAddress()` on `version` deterministically only does the former if the address from the latter is unroutable.
3) During `version` processing, we haven't received a potential sendaddrv2 message from our peer yet, so self-advertisements with addresses from addrV2-only networks would always be dropped in `PushAddress()`.
Since it's confusing to have two slightly different mechanisms for self-advertising, and the one in `MaybeSendAddr()` is better, remove the one in `version`.
ACKs for top commit:
stratospher:
ACK 956c670
naumenkogs:
ACK 956c67059caf3807b3ceacdd5de1caaae538f009
amitiuttarwar:
reACK 956c67059c
Tree-SHA512: 933d40615289f055c022170dde7bad0ac0a1d4be377538bfe9ba64375cfeb03bcd803901591f0739ac4850c880e8475a68fd1ab0330800030ab7f19e38c00274
8f5c560e11a9f3c7c5877f444caa605bdc296e81 refactor: Refactored RequestMethodString function to follow developer notes (JoaoAJMatos)
7fd3b9491b0e553aa7d60a68bda82968a79da8f5 refactor: Deleted unreachable code in httpserver.cpp (JoaoAJMatos)
Pull request description:
Some of the code in httpserver.cpp was unreachable, and didn't follow the developer notes.
Continuation of [#26570 ](https://github.com/bitcoin/bitcoin/pull/26570)
ACKs for top commit:
stickies-v:
re-ACK [8f5c560](8f5c560e11)
Tree-SHA512: ba8cf4c6dde9e2bb0ca9d63a0de86dfa37b070803dde71ac8384c261045835697a2335652cf5894511b3af8fd99f30e1cbda4e4234815b8b39538ade90fab3f9
293849a26088a16f6187dfdc36bef1d4d83ebb9d univalue: Remove confusing getBool method (Ryan Ofsky)
Pull request description:
Drop `UniValue::getBool` method because it is easy to confuse with the `UniValue::get_bool` method, and could potentially cause bugs. Unlike `get_bool`, `getBool` doesn't ensure that the value is a boolean and returns false for all integer, string, array, and object values instead of throwing an exception.
The `getBool` method is also redundant because it is an alias for `isTrue`. There were only 5 `getBool()` calls in the codebase, so this commit replaces them with `isTrue()` or `get_bool()` calls as appropriate.
These changes were originally made by MarcoFalke in https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the scope of that PR.
ACKs for top commit:
justinpickering:
ACK 293849a260
sipa:
utACK 293849a26088a16f6187dfdc36bef1d4d83ebb9d
w0xlt:
ACK 293849a260
hebasto:
ACK 293849a26088a16f6187dfdc36bef1d4d83ebb9d, also verified that the removed `getBool` method is not mentioned in any docs:
furszy:
ACK 293849a2
Tree-SHA512: 9fbfe5e2083410f123b18703a0cc0161ecbbb4958f331c9ff808dcfcc6ad499b0e896abd16fb8ea200c53ba29878db9812ce141e59cc5e0fd174741b0bcb192d
fa7d71acccab4e5b416c9dd1e2080a9652107f84 test: Move rpc_fundrawtransaction.py to wallet_fundrawtransaction.py (MarcoFalke)
fa933d6985830382c7e2da740bc363bf6a52c4ae test: Move feature_backwards_compatibility.py to wallet_backwards_compatibility.py (MarcoFalke)
Pull request description:
The tests only tests the wallet and it doesn't make sense to extend it for other stuff, so clarify that.
ACKs for top commit:
fanquake:
ACK fa7d71acccab4e5b416c9dd1e2080a9652107f84
pablomartin4btc:
re-ACK fa7d71accc
Tree-SHA512: 9dc131ed8ff119bd6d43d04ecc5c96d02f2d6fdc3d0805492774a414c0fbe6a984da7631154122a080c54ddd25fc5a5bdd52b282c918fce4932057ba72c2bf71
Replacing `install` with `install-lib` and `install-bin` is not strictly
necessary just to update the library, but it takes advantage of recent
changes in the new version, and makes the build more minimal.
Drop UniValue::getBool method because it is easy to confuse with the
UniValue::get_bool method, and could potentially cause bugs. Unlike get_bool,
getBool doesn't ensure that the value is a boolean and returns false for all
integer, string, array, and object values instead of throwing an exceptions.
The getBool method is also redundant because it is an alias for isTrue. There
were only 5 getBool() calls in the codebase, so this commit replaces them with
isTrue() or get_bool() calls as appropriate.
These changes were originally made by MarcoFalke in
https://github.com/bitcoin/bitcoin/pull/26213 but were dropped to limit the
scope of that PR.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
fabb24cbef6ccccf5e82ac52ca2aafd47c34455a test: Use last release in compatibility tests (MarcoFalke)
Pull request description:
In compatibility tests it makes sense to always use the last release without the new feature, as it is likely more in use than any even older previous release.
ACKs for top commit:
Sjors:
utACK fabb24c
Tree-SHA512: beb854f4d28ba313282e1e0303abb0e09377828b138bde5a3e209337210b6b4c24855ab90a68f8789387001e4ca33b15cc37dbc9b7809929f4e7d1b69833a527
c29bff5b91061b565076093b7245dfb1717e97b7 test: Fix backwards compatibility intermittent failure (Aurèle Oulès)
Pull request description:
Fixes#24400.
See https://github.com/bitcoin/bitcoin/issues/24400#issuecomment-1341531696 to reproduce the failure.
As MarcoFalke suggested in https://github.com/bitcoin/bitcoin/issues/24400#issuecomment-1342282165, it can happen that the wallet is not fully flushed before being copied over to the other node. Fixed by unloading the wallet before copying the file.
ACKs for top commit:
Sjors:
utACK c29bff5
Tree-SHA512: ea93b859878873d07fa44ccb1c5b4c1b1014be8568dc2b9bbef02bb7a9230fefa7a71b149eb553870d3f4e986263342ea4f996fa0221098a2244f38952100471
affbf58a1e52a8e60c830be6a9e0347e0ff4c28e build: Move environment variables into `$(package)_config_env` (Hennadii Stepanov)
d44fcd3c976572883bbf7f386bc88e2610dc1a58 build: Make $(package)_*_env available to all $(package)_*_cmds (Hennadii Stepanov)
Pull request description:
On master (1e7564eca8a688f39c75540877ec3bdfdde766b1) the depends build system, which is based on pure GNU Make, works, but it lacks robustness, and in some corner cases it fails. For example, see bitcoin/bitcoin#22552.
Another [bug](https://github.com/bitcoin/bitcoin/issues/22719) in the depends build system has already become a problem at least two times in the past (https://github.com/bitcoin/bitcoin/pull/16883#issuecomment-683817472 and https://github.com/bitcoin/bitcoin/pull/24134). Each time the problem was solved with other means.
The initial [solution](https://github.com/bitcoin/bitcoin/pull/19882) had some discussion. Also it was discussed on the IRC meeting in #bitcoin-core-builds channel. This PR, actually, is a resurrection of it, as the bug silently struck pretty [recently](https://github.com/bitcoin/bitcoin/pull/24134).
The bug is well described in bitcoin/bitcoin#22719.
Here is another, a bit simpler description, which requires only basic shell (bash, dash etc) experience.
After creating targets by this code:1e7564eca8/depends/funcs.mk (L280) a "draft" line of recipe like `$($(1)_config_env) $(call $(1)_config_cmds, $(1))` becomes a shell command sequence `VAR1=foo VAR2=bar command1 && command2` which is supposed to be executed in a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution).
Please note that `VAR1=foo VAR2=bar` part is visible for the first `command1` only (for details see shell docs). Example:
```sh
$ VAR1="foo" VAR2="bar" echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
$ echo $?
1
```
Using the `export` command is a trivial solution:
```sh
$ export VAR1="foo" VAR2="bar"; echo "begin" && printenv VAR1 && printenv VAR2 && echo "end"
begin
foo
bar
end
$ echo $?
0
```
As a [new sub-shell](https://www.gnu.org/software/make/manual/html_node/Execution.html#Execution) is invoked for each line of the recipe, there are no side effects of using `export`. It means this solution should not be considered invasive.
Fixesbitcoin/bitcoin#22719.
---
Also this PR removes no longer needed crutch from `qt.mk`.
ACKs for top commit:
fanquake:
ACK affbf58a1e52a8e60c830be6a9e0347e0ff4c28e
Tree-SHA512: 0ce2cf82870a7774bdf1592fac50857126ae47da902e349f1092d50109223be9d6a8efd5e92ec08c2ca775b17516482aabaf232378950ade36484a883acc177b
fa579f306363ed03c1138121415b67b9b36b4d53 refactor: Pass reference to last header, not pointer (MacroFake)
Pull request description:
It is never a nullptr, otherwise an assertion would fire in UpdatePeerStateForReceivedHeaders.
Passing a reference makes the code easier to read and less brittle.
ACKs for top commit:
john-moffett:
ACK fa579f3
aureleoules:
ACK fa579f306363ed03c1138121415b67b9b36b4d53
Tree-SHA512: 9725195663a31df57ae46bb7b11211cc4963a8f3d100f60332bfd4a3f3327a73ac978b3172e3007793cfc508dfc7c3a81aab57a275a6963a5ab662ce85743fd0
07dfbb5bb8115c680621f361c65d9cde2f8c52f2 Make static nLastFlush and nLastWrite Chainstate members (Aurèle Oulès)
Pull request description:
Fixes#22189.
The `static std::multimap<uint256, FlatFilePos> mapBlocksUnknownParent; ` referenced in the issue was already fixed by #25571. I don't believe Chainstate references any other static variables.
ACKs for top commit:
jamesob:
ACK 07dfbb5bb8115c680621f361c65d9cde2f8c52f2 ([`jamesob/ackr/26513.1.aureleoules.make_static_nlastflush_a`](https://github.com/jamesob/bitcoin/tree/ackr/26513.1.aureleoules.make_static_nlastflush_a))
theStack:
Concept and code-review ACK 07dfbb5bb8115c680621f361c65d9cde2f8c52f2
Tree-SHA512: 0f26463c079bbc5e0e62707d4ca4c8c9bbb99edfa3391d48d4915d24e2a1190873ecd4f9f11da25b44527671cdc82c41fd8234d56a4592a246989448d34406b0
d7f61e7d5909da7227c9e34be06ce9eb872ba074 rpc: reduce LOCK(cs_main) scope in gettxoutproof (Andrew Toth)
4d92b5aabaf371225cdc37a7b114adc040bf4da5 rpc: reduce LOCK(cs_main) scope in GetUndoChecked and getblockstats (Andrew Toth)
efd82aec8a2dd0fca8f2597c3f84cefe057d1243 rpc: reduce LOCK(cs_main) scope in blockToJSON (Andrew Toth)
f00808e932c1f67bc76702a3f05778074b10025c rpc: reduce LOCK(cs_main) scope in GetBlockChecked and getblock (Andrew Toth)
7d253c943f44612431be894b198ffb49ff988fff zmq: remove LOCK(cs_main) from NotifyBlock (Andrew Toth)
c75e3d2772b00acc3850f72a8cb733a0345aa773 rest: reduce LOCK(cs_main) scope in rest_block (Andrew Toth)
Pull request description:
Picking up from #21006.
After commit ccd8ef65f9 it is no longer required to hold `cs_main` when calling `ReadBlockFromDisk`. This can be verified in `master` at https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L755. Same can be seen for `UndoReadFromDisk` https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L485.
The first commit moves `ReadBlockFromDisk` outside the lock scope in `rest_block`, where we can see a huge performance improvement when fetching blocks with multiple threads.
My test setup, on an Intel i7 with 8 cores (16 threads):
1. Start a fully synced bitcoind, with this `bitcoin.conf`:
```
rest=1
rpcthreads=16
rpcworkqueue=64
rpcuser=user
rpcpassword=password
```
2. Run ApacheBench: 10000 requests, 16 parallel threads, fetching block nr. 750000 in binary:
```
ab -n 10000 -c 16 "http://127.0.0.1:8332/rest/block/0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e.bin"
```
Time per request (mean)
183 ms on master
30 ms this branch
So this can process 6.1 times as many requests, and saturates all the cores instead of keeping them partly idle waiting in the lock. With 8 threads the mean times were 90 ms on master and 19 ms on this branch, a speedup of 4.7x.
Big thanks to martinus for finding this and the original PR.
The second commit is from a suggestion on the original PR by jonatack to remove the unnecessary `LOCK(cs_main)` in the zmq notifier's `NotifyBlock`.
I also found that this approach could be applied to rpcs `getblock` (including `verbosity=3`), `getblockstats`, and `gettxoutproof` with similar very good results. The above benchmarks steps need to be modified slightly for RPC. Run the following ApacheBench command with different request data in a file named `data.json`:
```
ab -p data.json -n 10000 -c 16 -A user:password "http://127.0.0.1:8332/"
```
For `getblock`, use the following in `data.json`:
```
{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e"]}
```
master - 184 ms mean request time
branch - 28 ms mean request time
For `getblock` with verbosity level 3, use the following in `data.json`:
```
{"jsonrpc": "1.0", "id": "curltest", "method": "getblock", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", 3]}
```
This verbosity level fetches an undo file from disk, so it benefits from this approach as well. However, a lot of time is spent serializing to JSON so the performance gain is not as severe.
master - 818 ms mean request time
branch - 505 ms mean request time
For `getblockstats`, use the following in `data.json`:
```
{"jsonrpc": "1.0", "id": "curltest", "method": "getblockstats", "params": ["0000000000000000000592a974b1b9f087cb77628bb4a097d5c2c11b3476a58e", ["minfeerate","avgfeerate"]]}
```
This request used a lock on reading both a block and undo file, so the results are very good.
master - 244 ms mean request time
branch - 28 ms mean request time
ACKs for top commit:
MarcoFalke:
re-ACK d7f61e7d5909da7227c9e34be06ce9eb872ba074 💫
hebasto:
ACK d7f61e7d5909da7227c9e34be06ce9eb872ba074, I have reviewed the code and it looks OK. Did not make benchmarking though.
Tree-SHA512: 305ac945b4571c5f47646d4f0e78180d7a3d40b2f70ee43e4b3e00c96a465f6d0b9c750b8e85c89ed833e557e2cdb5896743f07ef90e4e53d4ad85452b545886
4e362c2b7269ae0426010850c605e5c1d0d58234 doc: add release note for 25934 (brunoerg)
fe488b4c4b7aa07fb83d528e2942ef914fd188c0 test: add coverage for `label` in `listsinceblock` (brunoerg)
722e9a418d078ed34aedd1ca55c1ae104f29a7d3 wallet, rpc: add `label` to `listsinceblock` (brunoerg)
852891ff98cffd37a74b9cb96394f43b2e6ca30e refactor, wallet: use optional for `label` in `ListTransactions` (brunoerg)
Pull request description:
This PR adds `label` parameter to `listsinceblock` to be able to fetch all incoming transactions having the specified label since a specific block.
It's possible to use it in `listtransactions`, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. `getreceivedbylabel` only returns the total amount received, not the txs info. `listreceivedbylabel` doesn't list all the informations about the transactions and it's not possible to fetch since a block.
ACKs for top commit:
achow101:
ACK 4e362c2b7269ae0426010850c605e5c1d0d58234
w0xlt:
ACK 4e362c2b72
aureleoules:
ACK 4e362c2b7269ae0426010850c605e5c1d0d58234
Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737
ccba4fe7e3113f5d3bb2fbe54147b3a17e1bfa24 doc: Add completion subdir to contrib/README.md (willcl-ark)
7075848f9646a5b37044beb78c48e57e1f32b6d0 script: Add fish completions (willcl-ark)
a27a445b71dbee41f81c7786e667eaa3d9813028 refactor: Sub-folder bash completions (willcl-ark)
Pull request description:
The completions are dynamically generated from the respective binary
help pages.
Completions should be sourced into the shell or added to
`$XDG_CONFIG/fish/completions`. See [where to put completions](https://fishshell.com/docs/current/completions.html#where-to-put-completions) for more information.
As the completions are auto-generated they should only require as much maintenance as the bash equivalents, which is to say very little!
ACKs for top commit:
achow101:
ACK ccba4fe7e3113f5d3bb2fbe54147b3a17e1bfa24
josibake:
ACK ccba4fe7e3
Tree-SHA512: fe6ed899ea1fe90f82970bde7739db11dd0c845ccd70b65f28ad5212f75b57d9105a3a7f70ccdff552d5b21fa3fe9c697d128fb10740bae31fe1854e716b4b8b
This makes the code more robust, see previous commit.
In general replacing isTrue with get_bool is not equivalent because
get_bool can throw exceptions, but in this case, exceptions won't happen
because of RPCTypeCheck() and isNull() checks in the preceding code.
6fb102c9f361a7ba0a6aa0a9b41315f5e04559f7 test: Changed small_txpuzzle_randfee to return the virtual size instead of the transaction hex for feerate calculation. (Randall Naar)
Pull request description:
The fee rates used in feature_fee_estimation.py are calculated using the raw transaction size instead of the virtual transaction size (which is used in 'CBlockPolicyEstimator::processBlockTx' and 'CBlockPolicyEstimator::processBlock'). This leads to inconsistencies as the fee rates used in check_raw_estimates are incorrect and can cause assertions to fail.
refs #25179
ACKs for top commit:
MarcoFalke:
ACK 6fb102c9f361a7ba0a6aa0a9b41315f5e04559f7
Tree-SHA512: b2bca85fffa605aeb17574f050736d4556506d782dd7fd969e165e48e108fd95ef4c4e2abbae83cce05ca777a00f4459cabfa0932694fa8bb93ddfba09d84357
b19c4124b3c9a15fe3baf8792c55eb26eca51c0f refactor: Rename ambiguous interfaces::MakeHandler functions (Ryan Ofsky)
dd6e8bd71c6025a51d88000caf28121ec00499db build: remove BOOST_CPPFLAGS from libbitcoin_util (fanquake)
82e272a109281f750909d1feade784c778d8b592 refactor: Move src/interfaces/*.cpp files to libbitcoin_common.a (Ryan Ofsky)
Pull request description:
These belong in `libbitcoin_common.a`, not `libbitcoin_util.a`, because they aren't general-purpose utilities, they just contain some common glue code that is used by both the node and the wallet. Another reason not to include these in `libbitcoin_util.a` is to prevent them from being used by the kernel library.
Also rename ambiguous `MakeHandler` functions to `MakeCleanupHandler` and `MakeSignalHandler`. Cleanup function handler was introduced after boost signals handler, so original naming didn't make much sense.
This just contains a move-only commit, and a rename commit. There are no actual code or behavior changes.
This PR is an alternative to #26293, and solves the same issue of removing a boost dependency from the _util_ library. The advantages of this PR compared to #26293 are that it keeps the source directory structure more flat, and it avoids having to change #includes all over the codebase.
ACKs for top commit:
hebasto:
ACK b19c4124b3c9a15fe3baf8792c55eb26eca51c0f
Tree-SHA512: b3a1d33eedceda7ad852c6d6f35700159d156d96071e59acae2bc325467fef81476f860a8855ea39cf3ea706a1df2a341f34fb2dcb032c31a3b0e9cf14103b6a
fa825bd227b9d2bace896a2d29b5ce78bbd4e59c util: Include full version id in bug reports (MarcoFalke)
Pull request description:
This will show the unique id of the full source code when the bug occurred, which can help debugging
ACKs for top commit:
1440000bytes:
utACK fa825bd227
theStack:
ACK fa825bd227b9d2bace896a2d29b5ce78bbd4e59c
john-moffett:
ACK fa825bd227b9d2bace896a2d29b5ce78bbd4e59c
Tree-SHA512: a7a775718f5f9796b5cffafbb3ace8adb5c163414ec584a57143157fc9dfb86f799e3b9c8365fcb831ee1e9eafc59d699d1653d772c68392de421b3de74dcd61
5d332da2cfdc67d69a165119c6ff598e29b28afa doc: Drop no longer relevant comment (Hennadii Stepanov)
Pull request description:
The comment was introduced in 4cf3411056f6a59fc5fe07784b6b6a512d76b046, and since 7e4bd19785ff9120b7242ff9f15231868aaf7db4 it has been no longer relevant.
ACKs for top commit:
jarolrod:
ACK 5d332da2cfdc67d69a165119c6ff598e29b28afa
Tree-SHA512: 6d32561336993b1ff7d7c524d090ac52aefb40078ed706ca4c6d5026cc3f63244c49c0e00e45ff192ba0e9f1527faf63249aa18bc8aa677b9e053d387e0f4027
896fca16a3818cc9102196863627e66d4d89307a doc: move release notes to 24.0.1 and add notice (fanquake)
Pull request description:
This mirrors what was done with 0.19.0.1.
ACKs for top commit:
instagibbs:
ACK 896fca16a3
gruve-p:
ACK 896fca16a3
w0xlt:
ACK 896fca16a3
Tree-SHA512: 590462555422c0f96895152d1a2f9f9cf0ebf2c61dc342094d747f4d48b878e5d91840b9c2ac6825bb7214239f035789f1765a907fb614017205377ed89631fd
38941a703e079709bb465ad1fcde50e11350f8f1 refactor: Move `txmempool_entry.h` --> `kernel/mempool_entry.h` (Hennadii Stepanov)
Pull request description:
This PR addresses the https://github.com/bitcoin/bitcoin/pull/17786#discussion_r1027818360:
> why not move it to the right place, that is to `kernel/txmempool_entry.h`?
ACKs for top commit:
MarcoFalke:
review ACK 38941a703e079709bb465ad1fcde50e11350f8f1 📊
Tree-SHA512: 0145974b63b67ca1d9d89af2dd9d4438beca480c16a563f330da05fec49b8394d7ba20ed83cf7d50b2e19454e006978ebed42b0e07887b98d00210f3201ce9ba
203886c443c4ad76f8a1dba740a286e383e55206 Fixup clang-tidy named argument comments (fanquake)
Pull request description:
Fix comments so they are checked/consistent.
Fix incorrect comments.
ACKs for top commit:
hebasto:
ACK 203886c443c4ad76f8a1dba740a286e383e55206, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: e1257840f91fe39842e2b19299c1633604697b8584fe44b1977ada33cdde5433c877ed0b669fa334e20b04971dc89cd47d58b2783b6f7004521f01d05a1245da
3eb041f014870954db564369a4be4bd0dea48fbe wallet: Change coin selection fee assert to error (Andrew Chow)
c6e7f224c119f47af250a9e0c5b185cb98b30c4c util: Add StrFormatInternalBug and STR_INTERNAL_BUG (MarcoFalke)
Pull request description:
Returning an error instead of asserting for the low fee check will be better as it does not crash the node and instructs users to report the bug.
ACKs for top commit:
S3RK:
ACK 3eb041f014870954db564369a4be4bd0dea48fbe
aureleoules:
ACK 3eb041f014870954db564369a4be4bd0dea48fbe
furszy:
ACK 3eb041f0
Tree-SHA512: 118c13d7cdfce492080edd4cb12e6d960695377b978c7573f9c58b6d918664afd0e8e591eed0605d08ac756fa8eceed456349de5f3a025174069abf369bb5a5f
d885bb2f6ea34cd21dacfebe763a07dbb389c1bd test: Test exclusion of OP_RETURN from getblockstats (Fabian Jahr)
ba9d288b2468f047e5a8e4637dd8749e247ff547 test: Fix getblockstats test data generator (Fabian Jahr)
2ca5a496c2f3cbcc63ea15fa05c1658e7f527bbc rpc: Improve getblockstats (Fabian Jahr)
cb94db119f4643f49da63520d64efc99fb0c0795 validation, index: Add unspendable coinbase helper functions (Fabian Jahr)
Pull request description:
Fixes#19885
The genesis block does not have undo data saved to disk so the RPC errored because of that.
ACKs for top commit:
achow101:
ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd
aureleoules:
ACK d885bb2f6ea34cd21dacfebe763a07dbb389c1bd
stickies-v:
ACK d885bb2f6
Tree-SHA512: f37bda736ed605b7a41a81eeb4bfbb5d2b8518f847819e5d6a090548a61caf1455623e15165d72589ab3f4478252b00e7b624f9313ad6708cac06dd5edb62e9a
3198e4239e848bbb119e3638677aa9bcf8353ca6 test: check that loading descriptor wallet with legacy entries throws error (Sebastian Falbesoner)
349ed2a0eed3aaaf199ead93057c97730869c3a3 wallet: throw error if legacy entries are present on loading descriptor wallets (Sebastian Falbesoner)
Pull request description:
Loading a descriptor wallet currently leads to a segfault if a legacy key type entry is present that can be deserialized successfully and needs SPKman-interaction. To reproduce with a "cscript" entry (see second commit for details):
```
$ ./src/bitcoin-cli createwallet crashme
$ ./src/bitcoin-cli unloadwallet crashme
$ sqlite3 ~/.bitcoin/wallets/crashme/wallet.dat
SQLite version 3.38.2 2022-03-26 13:51:10
Enter ".help" for usage hints.
sqlite> INSERT INTO main VALUES(x'07637363726970740000000000000000000000000000000000000000', x'00');
$ ./src/bitcoin-cli loadwallet crashme
--- bitcoind output: ---
2022-11-06T13:51:01Z Using SQLite Version 3.38.2
2022-11-06T13:51:01Z Using wallet /home/honey/.bitcoin/wallets/crashme
2022-11-06T13:51:01Z init message: Loading wallet…
2022-11-06T13:51:01Z [crashme] Wallet file version = 10500, last client version = 249900
Segmentation fault (core dumped)
```
Background: In the wallet key-value-loading routine, most legacy type entries require a `LegacyScriptPubKeyMan` instance after successful deserialization. On a descriptor wallet, creating that (via method `GetOrCreateLegacyScriptPubKeyMan`) fails and then leads to a null-pointer dereference crash. E.g. for CSCRIPT: 50422b770a/src/wallet/walletdb.cpp (L589-L594)
~~This PR fixes this by simply ignoring legacy entries if the wallet flags indicate that we have a descriptor wallet. The second commits adds a regression test to the descriptor wallet's functional test (fortunately Python includes sqlite3 support in the standard library).~~
~~Probably it would be even better to throw a warning to the user if unexpected legacy entries are found in descriptor wallets, but I think as a first mitigation everything is obvisouly better than crashing. As far as I'm aware, descriptor wallets created/migrated by Bitcoin Core should never end up in a state containing legacy type entries though.~~
This PR fixes this by throwing an error if legacy entries are found in descriptor wallets on loading.
ACKs for top commit:
achow101:
ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6
aureleoules:
ACK 3198e4239e848bbb119e3638677aa9bcf8353ca6
Tree-SHA512: ee43da3f61248e0fde55d9a705869202cb83df678ebf4816f0e77263f0beac0d7bae9490465d1753159efb093ee37182931d76b2e2b6e8c6f8761285700ace1c
