In Base58 fuzz the two roundtrips are merged now, the new `decode_input` switches between a completely random input and a valid encoded one, to make sure the decoding passes more often.
The `max_ret_len` can also exceed the original length now and is being validated more thoroughly.
Github-Pull: #31917
Rebased-From: d5537c18a9
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
They seem to cause timeouts:
> Issue 397734700: bitcoin-core:base58check_encode_decode: Timeout in base58check_encode_decode
The `encoded_string.empty()` check was corrected here to `decoded.empty()` to make sure the `(0, decoded.size() - 1)` range is always valid.
Github-Pull: #31917
Rebased-From: bad1433ef2
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Co-authored-by: marcofleon <marleo23@proton.me>
Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
e637dc2c01 refactor: Replace uint256 type with Wtxid in PackageMempoolAcceptResult struct (marcofleon)
a3baead7cb validation: use wtxid instead of txid in CheckEphemeralSpends (marcofleon)
Pull request description:
This PR addresses a small bug in [`AcceptMultipleTransactions`](45719390a1/src/validation.cpp (L1598)) where a txid was being inserted into a map that should only hold wtxids. `CheckEphemeralSpends` has an out parameter on failure that records that the child transaction did not spend the parent's dust. Instead of using the txid of this child, use its wtxid.
The second commit in this PR is a refactor of the `PackageMempoolAcceptResult` struct to use the `Wtxid` type instead of `uint256`. This helps to prevent errors like this in the future.
ACKs for top commit:
instagibbs:
ACK e637dc2c01
glozow:
ACK e637dc2c01, hooray for type safety
dergoegge:
Code review ACK e637dc2c01
Tree-SHA512: 17039efbb241b7741e2610be5a6d6f88f4c1cbe22d476931ec99e43f993d259a1a5e9334e1042651aff49edbdf7b9e1c1cd070a28dcba5724be6db842e4ad1e0
568fcdddae scripted-diff: Adjust documentation per top-level target output location (Hennadii Stepanov)
026bb226e9 cmake: Set top-level target output locations (Hennadii Stepanov)
Pull request description:
This PR sets the target output locations to the `bin` and `lib` subdirectories within the build tree, creating a directory structure that mirrors that of the installed targets.
This approach is widely adopted by the large projects, such as [LLVM](e146c1867e/lldb/cmake/modules/LLDBStandalone.cmake (L128-L130)):
```cmake
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_LIBRARY_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/lib${LLVM_LIBDIR_SUFFIX})
```
The `libsecp256k1` project has also recently [adopted](https://github.com/bitcoin-core/secp256k1/pull/1553) this approach.
With this PR, all binaries are conveniently located. For example, run:
```
$ ./build/bin/fuzz
```
instead of:
```
$ ./build/src/test/fuzz/fuzz
```
On Windows, all required DLLs are now located in the same directory as the executables, allowing to run `bitcoin-chainstate.exe` (which loads `bitcoinkernel.dll`) without the need to copy DLLs or modify the `PATH` variable.
The idea was briefly discussed among the build team during the recent CoreDev meeting.
---
**Warning**: This PR changes build locations of newly built executables like `bitcoind` and `test_bitcoin` from `src/` to `bin/` without deleting previously built executables. A clean build is recommended to avoid accidentally running old binaries.
ACKs for top commit:
theStack:
Light re-ACK 568fcdddae
ryanofsky:
Code review ACK 568fcdddae. Only change since last review was rebasing. I'm ok with this PR in its current form if other developers are happy with it. I just personally think it is inappropriate to \*silently\* break an everyday developer workflow like `git pull; make bitcoind`. I wouldn't have a problem with this PR if it triggered an explicit error, or if the problem was limited to less common workflows like changing cmake options in an existing build.
TheCharlatan:
Re-ACK 568fcdddae
theuni:
ACK 568fcdddae
Tree-SHA512: 1aa5ecd3cd49bd82f1dcc96c8e171d2d19c58aec8dade4bc329df89311f9e50cbf6cf021d004c58a0e1016c375b0fa348ccd52761bcdd179c2d1e61c105e3b9f
The translations for the following languages, which appear to be the
result of a mistake or an act of vandalism, have been discarded:
- Czech (cs)
- Danish (da)
- Dutch (nl)
Changes to the Thai (th) translation have been discarded due to multiple
unsolicited pronunciation notes.
f5d8b66a8c Squashed 'src/minisketch/' changes from eb37a9b8e7..d1e6bb8bbf (fanquake)
Pull request description:
Includes:
* https://github.com/bitcoin-core/minisketch/pull/92
ACKs for top commit:
hebasto:
ACK 4fde88bc46, I've updated the subtree locally and got zero diff with this PR.
Tree-SHA512: 0ddaa6b64ca14da244d455594bc122a059fd1d199d28a7a78f266e352811568bd0f30d3b1e5e5d859f92753d3979831c095e3f6078f0ba2c909b1566a0e74a0c
14f1674855 chainparams: add mainnet assumeutxo param at height 880_000 (Sjors Provoost)
Pull request description:
#31940 suggests adding a snapshot at every major release.
This snapshot should be suitable for v29. I picked the most recent multiple of 10K blocks.
You can either download this torrent:
```
magnet:?xt=urn:btih:559bd78170502971e15e97d7572e4c824f033492&dn=utxo-880000.dat&tr=udp%3A%2F%2Ftracker.bitcoin.sprovoost.nl%3A6969
```
Or generate the snapshot yourself:
```sh
bitcoin-cli -rpcclienttimeout=0 -named dumptxoutset utxo-880000.dat rollback=880000
```
The SHA-256 hash should be:
```
shasum -a 256 utxo-880000.dat
43b3b1ad6e1005ffc0ff49514d0ffcc3e3ce671cc8d02da7fa7bac5405f89de4
```
And then load it on a fresh node during IBD with:
```
bitcoin-cli -rpcclienttimeout=0 loadtxoutset utxo-880000.dat
```
Note that it's more performant to turn off networking while the snapshot is loading, see #29993:
```sh
bitcoin-cli setnetworkactive false
```
Once the snapshot is loaded:
```sh
bitcoin-cli setnetworkactive true
```
And enjoy a speedy ride to the tip.
ACKs for top commit:
achow101:
ACK 14f1674855
fjahr:
tACK 14f1674855
hodlinator:
ACK 14f1674855
rkrux:
Concept ACK 14f1674855
polespinasa:
ACK 14f1674855
Tree-SHA512: e7ed3e8ce3a247614545ecd3254a91814d7f87b3ca1be46df3b9a4c1e6353b46c82ab97d9fc9c5bed8938f28a6a61e6b70baa7c9649fe5da0f2f390b7932f15e
44041ae0ec init: Handle dropped UPnP support more gracefully (laanwj)
Pull request description:
Closesbitcoin-core/gui#843.
In that issue it was brought up that users likely don't care what kind of port forwarding is used, and that the setting is opportunistic anyway, so instead of showing an extensive warning, we can simply "upgrade" from UPNP to NAT-PMP+PCP.
- Change the logic for removed runtime setting `-upnp` to set `-natpmp` instead, and log a message.
- Also remove any lingering `upnp` from `settings.json` and replace it with `natpmp`, when it makes sense (this is important so that the UI shows the right values in the settings):
```json
{
"upnp": true
}
```
becomes
```json
{
"natpmp": true
}
```
and
```json
{
"upnp": false
}
```
becomes
```json
{
"natpmp": false
}
```
ACKs for top commit:
darosior:
tACK 44041ae0ec
davidgumberg:
lightly reviewed code, tested ACK 44041ae0ec
achow101:
ACK 44041ae0ec
ryanofsky:
Code review ACK 44041ae0ec. Code changes look good. Could potentially add test coverage for this, though I don't think it is too important.
hodlinator:
cr-ACK 44041ae0ec
Tree-SHA512: ca822f7160529e59973bab6a7cc31753ffa3caaa806887b5073b42c4ae5c918a5ea2cf93c666e5125ea70d10c6954709a535a264b04c2fd4cf916b3c59ab9964
ecf54a32ed cmake: Add support for builtin `codegen` target (Hennadii Stepanov)
a8c78a0574 cmake: Revamp handling of data files (Hennadii Stepanov)
Pull request description:
This PR leverages the approach from the https://github.com/chaincodelabs/libmultiprocess project and introduces a new functions `target_json_data_sources()` and `target_raw_data_sources()`, which minimize the amount of code required to assign to assign a `*.json` or `*.raw` data file to the `test_bitcoin`, `bench_bitcoin` or `unitester` targets.
As requested in https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2654622689, the `codegen` build target is now supported, if available:
```
$ cmake --version
cmake version 3.31.5
CMake suite maintained and supported by Kitware (kitware.com/cmake).
$ cmake -G "Ninja" -B build
$ cmake --build build --target codegen
```
ACKs for top commit:
fjahr:
re-ACK ecf54a32ed
Sjors:
re-tACK ecf54a32ed
theuni:
ACK ecf54a32ed
Tree-SHA512: bab92df6b81c47d9d97ba8db37470a6d7aa435d5578afe40df7154885eda55afc59f0bf20dc9db3b2fd88ceb9a0319b9678f9e9af01e7afd4851ec3a79f3f402
Closesbitcoin-core/gui#843.
In that issue it was brought up that users likely don't care what kind
of port forwarding is used, and the setting is opportunistic anyway, so
instead of showing an extensive warning, we can simply migrate from
UPNP to NAT-PMP+PCP. This prevents nodes dropping from the public
network.
- Change the logic for removed runtime setting `-upnp` to set `-natpmp`
instead, and only log a message.
- Also replace any lingering `upnp` in `settings.json` with `natpmp`.
c73b59d47f fuzz: implement targets for PCP and NAT-PMP port mapping requests (Antoine Poinsot)
1695c8ab5b fuzz: in FuzzedSock::GetSockName(), return a random-length name (Antoine Poinsot)
0d472c1953 fuzz: never return an uninitialized sockaddr in FuzzedSock::GetSockName (Antoine Poinsot)
39b7e2b590 fuzz: add steady clock mocking to FuzzedSock (Antoine Poinsot)
6fe1c35c05 pcp: make NAT-PMP error codes uint16_t (Antoine Poinsot)
01906ce912 pcp: make the ToString method const (Antoine Poinsot)
Pull request description:
Based on https://github.com/bitcoin/bitcoin/pull/31022, this introduces a fuzz target for `PCPRequestPortMap` and `NATPMPRequestPortMap`.
Like in #31022 we set `CreateSock` to return a `Sock` which mocks the responses from the server and uses a mocked steady clock for the `Wait`s. Except here we simply respond with fuzzer-provided data until the client stop sending requests. We also sometimes inject errors and connection failures based on fuzzer-provided data.
We reuse the existing `FuzzedSock`, so a preparatory commit is included that adds steady clock mocking to it. This may be useful for other harnesses as well.
ACKs for top commit:
laanwj:
re-ACK c73b59d47f
marcofleon:
ACK c73b59d47f
dergoegge:
utACK c73b59d47f
Tree-SHA512: 24cd4d958a0999946a0c3d164a242fc3f0a0b66770630252b881423ad0065d29fdaab765014d193b705d3eff397f201d51a88a3ca80c63fd3867745e6f21bb2b
7267ed0518 qt: Update `src/qt/locale/bitcoin_en.xlf` after string freeze (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](864386a744/doc/release-process.md) and implements the ["Translation string freeze"](https://github.com/bitcoin/bitcoin/issues/31029) step.
Steps to reproduce the diff on Ubuntu:
```
$ cmake --preset dev-mode -DWITH_USDT=OFF -DWITH_MULTIPROCESS=OFF
$ cmake --build build_dev_mode --target translate
```
At the moment, the multiprocess-specific code does not introduce any new translatable strings. Therefore, there is no need to build depends with `MULTIPROCESS=1` to review this PR.
ACKs for top commit:
stickies-v:
ACK 7267ed0518 - I get the same results when building the `translate` target.
pablomartin4btc:
tACK 7267ed0518
Tree-SHA512: dc3641d3288c00cb7802714680508de517e56c615716e52181555634ad489fbed676229063995170edb8efeaa4e900ef2d3d5a0f1b8ce7cec143ef364c96e1c0
This change:
1. Collects build artifacts in dedicated locations.
2. Allows running bitcoin-chainstate.exe with bitcoinkernel.dll directly
from the build tree on Windows.
d871d77825 test: Remove non-portable IPv6 test (Hennadii Stepanov)
Pull request description:
On Illumos-based systems, such as OpenIndiana and OmniOS, the assumption that "the default zone ID of 0 can be omitted for the default scope" is incorrect. As a result, `getaddrinfo("fe80::1%0", ...)` returns the `EAI_NONAME` error instead of resolving to "fe80::1".
See: https://www.illumos.org/man/3SOCKET/getaddrinfo.
This PR removes the problematic code introduced in https://github.com/bitcoin/bitcoin/pull/19951.
ACKs for top commit:
fanquake:
ACK d871d77825
Tree-SHA512: 2ef5c22f826d16661deb1d6d005cbda64179e8b83be43d3d6ac51caff02187cf224355c9da144ff110a6ae2cb68f0338ea9b62af8e0f9f1014a518cf9dad6ab5
ca6aa0b9be 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 ca6aa0b9be
maflcko:
lgtm ACK ca6aa0b9be
rkrux:
ACK ca6aa0b9be
jonatack:
ACK ca6aa0b9be
Tree-SHA512: 123ae118c79ee1843ed65861e7a008658a53e47d4d14f2b7612561bba1b1dbdb6744f9aaac1587aac231c62d0c1804de848a6d732f1382788b437d9fe6474012
c4c5cf1748 cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree (Hennadii Stepanov)
eb540a2629 cmake: Remove `core_sanitizer_{cxx,linker}_flags` helper variables (Hennadii Stepanov)
Pull request description:
On the master branch @ 70e20ea024, the `APPEND_CPPFLAGS`, `APPEND_CFLAGS` and `APPEND_LDFLAGS` are not correctly applied when building C code in the `secp256k1` subtree, as intended.
This behaviour occurs due to two issues:
1. The command here: 70e20ea024/src/CMakeLists.txt (L77)
does not affect the code in `add_subdirectory(secp256k1)` above it.
2. `APPEND_LDFLAGS` is not passed to the subtree's build system at all.
This PR fixes both issues.
Additionally, the helper variables `core_sanitizer_cxx_flags` and `core_sanitizer_linker_flags` have been removed.
ACKs for top commit:
theuni:
utACK c4c5cf1748.
TheCharlatan:
ACK c4c5cf1748
Tree-SHA512: 707acfa623f0436e34e9e6ba8ce2979e0fde5e196e2242fd1cde4c50f433938549781193d8a06419a0866bbe6d69d76f8383d973afbd87d944407963b318c5c9
ff4ddd3d2e Revert "cmake: Ensure generated sources are up to date for `translate` target" (Hennadii Stepanov)
03b3166aac cmake: Exclude generated sources from translation (Hennadii Stepanov)
Pull request description:
This PR fixes an error encountered when building the `translate` target:
```
$ gmake -j $(nproc) -C depends MULTIPROCESS=1
$ cmake -G "Unix Makefiles" --preset dev-mode --toolchain depends/x86_64-pc-linux-gnu/toolchain.cmake -DWITH_USDT=OFF
$ cmake --build build_dev_mode -t translate
gmake[3]: *** No rule to make target 'src/test/ipc_test.capnp.c++', needed by 'src/qt/CMakeFiles/translate'. Stop.
gmake[2]: *** [CMakeFiles/Makefile2:1646: src/qt/CMakeFiles/translate.dir/all] Error 2
gmake[1]: *** [CMakeFiles/Makefile2:1653: src/qt/CMakeFiles/translate.dir/rule] Error 2
gmake: *** [Makefile:699: translate] Error 2
```
The previous [attempt](864386a744) to address this issue worked only with Ninja generators and has been reverted.
Essentially, this PR modifies the `translate` target so that it ignores generated sources rather than attempting to update them.
At present, multiprocess-specific sources do not contain any translatable strings. Nonetheless, it is prudent to maintain a general approach.
ACKs for top commit:
TheCharlatan:
ACK ff4ddd3d2e
pablomartin4btc:
tACK ff4ddd3d2e
Tree-SHA512: 6471498a33b145e073f76bd007591b0449e5d520f141c3e3afeca02a09c160fd0f572ec7172dd84703cdc2a1175ad8f3c91e8b0bf705d671338d760786765f56
09b150bb8a In `InitHardwareRand`, do trail test for `RNDRRS` by `VerifyRNDRRS` (Eval EXEC)
Pull request description:
This PR want to fix#31817 by added a maximum retry limit (`max_retries`) to the `GetRNDRRS` function to prevent it from entering an infinite loop if the hardware random number generator fails to return a valid random number. This change improves stability and ensures that the function terminates after a predefined number of retries.
ACKs for top commit:
achow101:
ACK 09b150bb8a
sipa:
utACK 09b150bb8a
Tree-SHA512: 5626b6b182a55d344a3aba11b782259ecc6bbec513771d50077874c5f70934750e68add8f63aa0bf69c6b7b313112940a85508af5447622c703cc5e92439ab4a
e4dd5a351b test: wallet, abandon coinbase txs and their descendants during startup (furszy)
474139aa9b 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 e4dd5a351b
rkrux:
tACK e4dd5a351b
mzumsande:
Code Review ACK e4dd5a351b
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.
`OBJECT` libraries have historically exhibited poor support for various
features, both in the past and now. For example, see one of the latest
issues:
- https://gitlab.kitware.com/cmake/cmake/-/issues/24058
Furthermore, CMake maintainers have acknowledged:
> In general, however, where there is a choice, static libraries will
> typically be the more convenient choice in CMake projects.
This change:
1. Converts the `bitcoin_clientversion` library from an `OBJECT` library
to a `STATIC` library.
2. Removes an obsolete workaround.
9cf746d663 cmake: add optional source files to crc32c directly (Daniel Pfeifer)
9c7823c5b5 cmake: add optional source files to bitcoin_crypto directly (Daniel Pfeifer)
Pull request description:
Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly.
Set the necessary compile options at the source file level, rather than the target level.
fixes: #31697
ACKs for top commit:
s373nZ:
ACK 9cf746d663
hebasto:
re-ACK 9cf746d663.
TheCharlatan:
ACK 9cf746d663
Tree-SHA512: 04b468ccbd284d63fc83b382177bb8183b325369835c3b92e555e159955c73d71712a63a2e556f8da68a1232ac07d3845e11f1057c50666843db91db98fca979
a85e8c0e61 doc: Add some general documentation about negated options (Ryan Ofsky)
490c8fa178 doc: Add release notes summarizing negated option behavior changes. (Ryan Ofsky)
458ef0a11b refactor: Avoid using IsArgSet() on -connect list option (Ryan Ofsky)
752ab9c3c6 test: Add test to make sure -noconnect disables -dnsseed and -listen by default (Ryan Ofsky)
3c2920ec98 refactor: Avoid using IsArgSet() on -signetseednode and -signetchallenge list options (Ryan Ofsky)
d05668922a refactor: Avoid using IsArgSet() on -debug, -loglevel, and -vbparams list options (Ryan Ofsky)
3d1e8ca53a Normalize inconsistent -noexternalip behavior (Ryan Ofsky)
ecd590d4c1 Normalize inconsistent -noonlynet behavior (Ryan Ofsky)
5544a19f86 Fix nonsensical bitcoin-cli -norpcwallet behavior (Ryan Ofsky)
6e8e7f433f Fix nonsensical -noasmap behavior (Ryan Ofsky)
b6ab350806 Fix nonsensical -notest behavior (Ryan Ofsky)
6768389917 Fix nonsensical -norpcwhitelist behavior (Ryan Ofsky)
e03409c70f Fix nonsensical -norpcbind and -norpcallowip behavior (Ryan Ofsky)
40c4899bc2 Fix nonsensical -nobind and -nowhitebind behavior (Ryan Ofsky)
5453e66fd9 Fix nonsensical -noseednode behavior (Ryan Ofsky)
Pull request description:
The PR changes behavior of negated `-noseednode`, `-nobind`, `-nowhitebind`, `-norpcbind`, `-norpcallowip`, `-norpcwhitelist`, `-notest`, `-noasmap`, `-norpcwallet`, `-noonlynet`, and `-noexternalip` options, so negating these options just clears previously specified values doesn't have other side effects.
Negating options on the command line can be a useful way of resetting options that may have been set earlier in the command line or config file. But before this change, negating these options wouldn't fully reset them, and would have confusing and undocumented side effects (see commit descriptions for details). Now, negating these options just resets them and behaves the same as not specifying them.
Motivation for this PR is to fix confusing behaviors and also to remove incorrect usages of the `IsArgSet()` function. Using `IsArgSet()` tends to lead to negated option bugs in general, but it especially causes bugs when used with list settings returned by `GetArgs()`, because when these settings are negated, `IsArgSet()` will return true but `GetArgs()` will return an empty list. This PR eliminates all uses of `IsArgSet()` and `GetArgs()` together, and followup PR #17783 makes it an error to use `IsArgSet()` on list settings, since calling `IsArgSet()` is never actually necessary. Most of the changes here were originally made in #17783 and then moved here to be easier to review and avoid a dependency on #16545.
ACKs for top commit:
achow101:
ACK a85e8c0e61
danielabrozzoni:
re-ACK a85e8c0e61
hodlinator:
re-ACK a85e8c0e61
Tree-SHA512: dd4b19faac923aeaa647b1c241d929609ce8242b43e3b7bc32523cc48ec92a83ac0dc5aee79f1eba8794535e0314b96cb151fd04ac973671a1ebb9b52dd16697
