992f83bb6f test: add coverage for assumeUTXO honest peers disconnection (furszy)
6d5812e5c8 assumeUTXO: fix peers disconnection during sync (furszy)
Pull request description:
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (`NODE_NETWORK`), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness from the AssumeUTXO
node.
This lack of response occurs because nodes ignore `getdata` requests when they do
not have the block data available (further discussion can be found in #30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal `NODE_NETWORK_LIMITED` support. Then, full-node (`NODE_NETWORK`)
support will be re-enabled once the background chain sync is completed.
Thanks mzumsande for a post-#30385 convo too.
Testing notes:
Just cherry-pick the second commit (bb08c22) on master.
It will fail there, due to the IBD node requesting historical blocks to the snapshot
node - which is bad because the snapshot node will ignore the requests and
stall + disconnect after some time.
ACKs for top commit:
achow101:
ACK 992f83bb6f
naumenkogs:
ACK 992f83bb6f
mzumsande:
ACK 992f83bb6f
Tree-SHA512: fef525d1cf3200c2dd89a346be9c82d77f2e28ddaaea1f490a435e180d1a47a371cadea508349777d740ab56e94be536ad8f7d61cc81f6550c58b609b3779ed3
Because AssumeUTXO nodes prioritize tip synchronization, they relay their local
address through the network before completing the background chain sync.
This, combined with the advertising of full-node service (NODE_NETWORK), can
result in an honest peer in IBD connecting to the AssumeUTXO node (while syncing)
and requesting an historical block the node does not have. This behavior leads to
an abrupt disconnection due to perceived unresponsiveness (lack of response)
from the AssumeUTXO node.
This lack of response occurs because nodes ignore getdata requests when they do
not have the block data available (further discussion can be found in PR 30385).
Fix this by refraining from signaling full-node service support while the
background chain is being synced. During this period, the node will only
signal 'NODE_NETWORK_LIMITED' support. Then, full-node ('NODE_NETWORK')
support will be re-enabled once the background chain sync is completed.
43cd83b0c7 test: move uint256_tests/operator_with_self to arith_uint256_tests (stickies-v)
c6c994cb2b test: remove test-only uint160S (stickies-v)
62cc4656e2 test: remove test-only uint256S (stickies-v)
adc00ad728 test: remove test-only arith_uint256S (stickies-v)
f51b237723 refactor: rpc: use uint256::FromHex for ParseHashV (stickies-v)
Pull request description:
_Continuation of #30569._
Since fad2991ba0, `uint256S()` has been [deprecated](fad2991ba0 (diff-800776e2dda39116e889839f69409571a5d397de048a141da7e4003bc099e3e2R138)) because it is less robust than the `base_blob::FromHex()` introduced in https://github.com/bitcoin/bitcoin/pull/30482. Specifically, it tries to recover from length-mismatches, recover from untrimmed whitespace, 0x-prefix and garbage at the end, instead of simply requiring exactly 64 hex-only characters. (see also https://github.com/bitcoin/bitcoin/pull/30532)
This PR removes `uint256S()` (and `uint160S()`) completely, with no non-test behaviour change.
Specifically, the main changes in this PR are:
- the (minimal) last non-test usage of `uint256S()` in `ParseHashV()` is removed without behaviour change, which can partially be verified by cherry-picking and/or modifying [this test commit](1f2b0fa86d)).
- the test usage of `uint{160,256}S()` is removed, largely replacing it with `uint{160,256}::FromHex()` where applicable, potentially modifying the test by removing non-hex characters or dropping the test entirely if removing non-hex characters makes it redundant
- the now unused `uint{160,256}S()` functions are removed completely.
- unit test coverage on converting `uint256` <-> `arith_uint256` through `UintToArith256()` and `ArithToUint256()` is beefed up, and `arith_uint256` tests are moved to `arith_uint256_tests.cpp`, removing the `uint256_tests.cpp` dependency on `uint256h`, mirroring how the code is structured.
_Note: `uint256::FromUserHex()` exists to more leniently construct uint256 from user input, allowing "0x" prefixes and too-short-input, as safer alternative to `uint256S()` where necessary._
ACKs for top commit:
l0rinc:
reACK 43cd83b0c7
hodlinator:
re-ACK 43cd83b0c7
ryanofsky:
Code review ACK 43cd83b0c7. Only code change is a small refactoring which looks good. The rest of the PR is all test changes, which I only lightly reviewed, but seem to be positive and do what's described
Tree-SHA512: 48147a4c6af671597df0f72c1b477ae4631cd2cae4645ec54d0e327611ff302c9899e344518c81242cdde82930f6ad23a3a7e6e0b80671816e9f457b9de90a5c
d4c7c4009d init: error out if -maxconnections is negative (Sergi Delgado Segura)
c773649481 init: improves file descriptors accounting and docs (Sergi Delgado Segura)
29008a7ff4 init: fixes fd accounting regarding poll/select (Sergi Delgado Segura)
Pull request description:
The current logic for file descriptor accounting is pretty convoluted and hard to follow. This is partially caused by the lack of documentation plus non-intuitive variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain (as shown in the first commit of this PR).
Redefine some of the constants, plus document what are we accounting for so this can be extended more easily
Fixes #18911
ACKs for top commit:
sr-gi:
> ACK [d4c7c40](d4c7c4009d)
naumenkogs:
ACK d4c7c4009d
vasild:
ACK d4c7c4009d
TheCharlatan:
ACK d4c7c4009d
Tree-SHA512: 1524d10c8ad8f354f6ab9c244699adbcdae2dd7aba37de5b8f9e177c629e8a2cce0f6e8117e076dde3a592f5283bd30a4201db96a3c011e335c02d1fde7414bc
30073e6b3a multiprocess: Add -ipcbind option to bitcoin-node (Russell Yanofsky)
73fe7d7230 multiprocess: Add unit tests for connect, serve, and listen functions (Ryan Ofsky)
955d4077aa multiprocess: Add IPC connectAddress and listenAddress methods (Russell Yanofsky)
4da20434d4 depends: Update libmultiprocess library for CustomMessage function and ThreadContext bugfix (Ryan Ofsky)
Pull request description:
Add `-ipcbind` option to `bitcoin-node` to make it listen on a unix socket and accept connections from other processes. The default socket path is `<datadir>/node.sock`, but this can be customized.
This option lets potential wallet, gui, index, and mining processes connect to the node and control it. See examples in #19460, #19461, and #30437.
Motivation for this PR, in combination with #30510, is be able to release a bitcoin core node binary that can generate block templates for a separate Stratum v2 mining service, like the one being implemented in https://github.com/Sjors/bitcoin/pull/48, that connects over IPC.
Other things to know about this PR:
- While the `-ipcbind` option lets other processes to connect to the `bitcoin-node` process, the only thing they can actually do after connecting is call methods on the [`Init`](https://github.com/bitcoin/bitcoin/blob/master/src/ipc/capnp/init.capnp#L17-L20) interface which is currently very limited and doesn't do much. But PRs [#30510](https://github.com/bitcoin/bitcoin/pull/30510), [#29409](https://github.com/bitcoin/bitcoin/pull/29409), and [#10102](https://github.com/bitcoin/bitcoin/pull/10102) expand the `Init` interface to expose mining, wallet, and gui functionality respectively.
- This PR is not needed for [#10102](https://github.com/bitcoin/bitcoin/pull/10102), which runs GUI, node, and wallet code in different processes, because [#10102](https://github.com/bitcoin/bitcoin/pull/10102) does not use unix sockets or allow outside processes to connect to existing processes. [#10102](https://github.com/bitcoin/bitcoin/pull/10102) lets parent and child processes communicate over internal socketpairs, not externally accessible sockets.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
achow101:
ACK 30073e6b3a
TheCharlatan:
Re-ACK 30073e6b3a
itornaza:
Code review ACK 30073e6b3a
Tree-SHA512: 2b766e60535f57352e8afda9c3748a32acb5a57b2827371b48ba865fa9aa1df00f340732654f2e300c6823dbc6f3e14377fca87e4e959e613fe85a6d2312d9c8
c2b779da4e refactor: Manage dumptxoutset RAII classes with std::optional (Fabian Jahr)
4b5bf335ad test: Add coverage for failing dumptxoutset behavior (Fabian Jahr)
Pull request description:
This adds a test that checks that network activity is not suspended if dumptxoutset fails in the middle of its process which is implemented with the `NetworkDisable` RAII class. I would have liked to add coverage for the `TemporaryRollback` RAII class but that seems a lot more tricky since the failure needs to happen at some point after the rollback and on the scale of our test chain here I couldn't find a way to do it yet. This was requested by pablomartin4btc here: https://github.com/bitcoin/bitcoin/pull/30808#pullrequestreview-2280450117. To test the test you can comment out the content of the destructor of `NetworkDisable`.
It also addresses the feedback by ryanofsky to use `std::optional` instead of `std::unique_ptr` for the management of the RAII object: https://github.com/bitcoin/bitcoin/pull/30808#discussion_r1744149228
ACKs for top commit:
achow101:
ACK c2b779da4e
pablomartin4btc:
cr & tACK c2b779da4e
tdb3:
ACK c2b779da4e
BrandonOdiwuor:
Code Review ACK c2b779da4e
theStack:
ACK c2b779da4e
Tree-SHA512: 9556e75014a2599bb870b70faf887608b332f2312626333f771d4ec11c04f863a2cf17e223ec473d4e8b0c9e8008394a4e0c321561f7ef3a2eec713dcfaea58a
ee47ca29d6 init: fix fatal error on '-wallet' negated option value (furszy)
Pull request description:
Currently, if users provide a double negated value such as '-nowallet=0' or a non-boolean
convertible value to a negated option such as '-nowallet=not_a_boolean', the initialization
process results in a fatal error, causing an unclean shutdown and displaying a poorly
descriptive error message:
"JSON value of type bool is not of expected type string." (On bitcoind. The GUI
does not display any error msg - upcoming PR -).
This PR fixes the issue by ensuring that only string values are returned in the
the "wallet" settings list, failing otherwise. It also improves the clarity of the
returned error message.
Note:
This bug was introduced in https://github.com/bitcoin/bitcoin/pull/22217. Where the `GetArgs("-wallet")` call was
replaced by `GetSettingsList("-wallet")`.
ACKs for top commit:
achow101:
ACK ee47ca29d6
ryanofsky:
Code review ACK ee47ca29d6, just adding the suggested test since last review
TheCharlatan:
ACK ee47ca29d6
ismaelsadeeq:
Tested ACK ee47ca29d6
Tree-SHA512: 5f01076f74a048019bb70791160f0accc2db7a457d969cb23687bed81ccbbdec1dda68311e7c6e2dd56250e23e8d926d4066e5014b2a99a2fc202e24ed264fbd
27c976d11a fix: increase consistency of rpcauth parsing (tdb3)
2ad3689512 test: add norpcauth test (tdb3)
67df0dec1a test: blank rpcauth CLI interaction (tdb3)
ecc98ccff2 test: add cases for blank rpcauth (tdb3)
Pull request description:
The current `rpcauth` parsing behavior is inconsistent and unintuitive (see https://github.com/bitcoin/bitcoin/pull/29141#issuecomment-1972085251 and additional details below).
The current behavior inconsistently treats empty `rpcauth` as an error (or not) depending on the location within CLI/bitcoin.conf and the location of adjacent valid `rpcauth` params.
Empty `rpcauth` is now consistently treated as an error and prevents bitcoind from starting.
Continuation of the upforgrabs PR #29141.
### Additional details:
Current `rpcauth` behavior is nonsensical:
- If an empty `rpcauth` argument was specified as the last command line argument, it would cause all other `rpcauth` arguments to be ignored.
- If an empty `rpcauth` argument was specified on the command line followed by any nonempty `rpcauth` argument, it would cause an error.
- If an empty `rpcauth=` line was specified after non-empty rpcauth line in the config file it would cause an error.
- If an empty `rpcauth=` line in a config file was first it would cause other rpcauth entries in the config file to be ignored, unless there were `-rpcauth` command line arguments and the last one was nonempty, in which case it would cause an error.
New behavior is simple:
- If an empty rpcauth config line or command line argument is used it will cause an error
ACKs for top commit:
naiyoma:
Tested ACK [27c976d11a)
achow101:
ACK 27c976d11a
ryanofsky:
Code review ACK 27c976d11a. Since last review commit message was just tweaked to clarify previous behavior.
Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b
1f054eca4e cmake: add USE_SOURCE_PERMISSIONS to all configure_file usage (fanquake)
Pull request description:
`USE_SOURCE_PERMISSIONS` is the default, so this should not change behaviour. However, being explicit makes it clear what we are doing.
Related to #30815.
See https://cmake.org/cmake/help/latest/command/configure_file.html#options.
ACKs for top commit:
hebasto:
ACK 1f054eca4e.
TheCharlatan:
ACK 1f054eca4e
Tree-SHA512: efed91b8aa0813100304ee58e169bbf5cfbb7db465ec4f7e6cbbae6053f09a36757bf96b4d1cb9ddf4c1cab0ceb3ab18805ebefa122535518ffb501c9b489d3d
move/formatting-only change.
These tests do not cover uint256, so move them to the appropriate
test suite. Additionally, apply clang-format suggestions.
uint160S is a test-only function, and testing input that
is not allowed in uint160::FromHex() is superfluous.
Tests that can't use uint160::FromHex() because they use input
with non-hex digit characters are
a) modified by dropping the non-hex digit characters if that
provides useful test coverage.
b) dropped if the test without non-hex digit characters does
not provide useful test coverage, e.g. because it is now
duplicated.
uint256S was previously deprecated for being unsafe. All non-test
usage has already been removed in earlier commits.
1. Tests now use uint256::FromHex() or other constructors wherever
possible without further modification.
2. Tests that can't use uint256::FromHex() because they use input
with non-hex digit characters are
a) modified by dropping the non-hex digit characters if that
provides useful test coverage.
b) dropped if the test without non-hex digit characters does
not provide useful test coverage, e.g. because it is now
duplicated.
Additionally, use BOOST_CHECK_EQUAL where relevant on touched lines
to make error messages more readable.
Tests that are solely testing constructing from a hex string
are dropped, others are modified to use a uint256 constructor
or the arith_uint256 uint64_t constructor.
Since an arith_uint256 can not be constructed from a string
directly, we need to ensure that test coverage on
UintToArith256(uint256::FromHex()) is not reduced.
uint256::FromHex() already has good test coverage, but
the test coverage on UintToArith256() and ArithToUint256()
is increased in this commit by upgrading the `conversion`
test case.
Moreover, since `uint256.h` does not have any dependencies
on `arith_uint256.h`, the conversion tests are moved to
`arith_uint256_tests.cpp` so the dependency can be cleaned
up entirely in a future commit.
Add `-ipcbind` option to `bitcoin-node` to listen on an IPC socket and accept
connections from other processes. In the future, there will be an `-ipcconnect`
option added to `bitcoin-wallet` and `bitcoin-node` to allow wallet and gui
processes to connect to the node and access it.
Example usage:
src/bitcoin-node -regtest -debug -ipcbind=unix
src/bitcoin-wallet -regtest -ipcconnect=unix info
src/bitcoin-gui -regtest -ipcconnect=unix
src/bitcoin-mine -regtest -ipcconnect=unix
fadbcd51fc bench: Remove redundant logging benchmarks (MarcoFalke)
fa8dd952e2 bench: Use LogInfo instead of the deprecated alias LogPrintf (MarcoFalke)
Pull request description:
`LogPrint*ThreadNames` is redundant with `LogWith(out)ThreadNames`,
because they all measure toggling the thread names (and check that it
has no effect on performance).
Fix it by removing the redundant ones. This also allows to drop a deprecated logging alias.
ACKs for top commit:
stickies-v:
ACK fadbcd51fc
Tree-SHA512: 4fe137f374aa4ee1aa0e1da4a1f9839c0e52c23dbb93198ecafee98de39d311cc47304bba4191f3807aa00c51b1eae543e3f270f03d341c84910e5e341a1d475
fa84f9decd test: Pin and document TEST_DIR_PATH_ELEMENT (MarcoFalke)
2222f7a874 test: Rename SeedRand::SEED to FIXED_SEED for clarity (MarcoFalke)
Pull request description:
Two small test changes:
* A refactor to update the name and documentation around `SeedRand::FIXED_SEED`.
* A change to extract and document `TEST_DIR_PATH_ELEMENT`, and to change its value to better match the `TMPDIR_PREFIX` in functional tests. The value previously included `PACKAGE_NAME`, which is cute, but doesn't explain why it was used (to include a space). So just use `test_common bitcoin` to achieve the same with less effort.
ACKs for top commit:
hodlinator:
ACK fa84f9decd
ryanofsky:
Code review ACK fa84f9decd
Tree-SHA512: eb35d6598bb08f9b996e3a4762d8f26b2441c0ca00780798e473015af735dfc9997120895a922b94d4b6ada45adadba4a686e9cf9c285ddf688848e764c64840
cd062d6684 build: work around issue with Boost <= 1.80 and Clang >= 18 (fanquake)
Pull request description:
Our current minimum supported Boost is `1.73.0`. However, when compiling with Boost `1.74.0` (Debian Stable), using Clang `18`, compilation fails with:
```bash
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'udt_builtin_mixture_enum' [-Wenum-constexpr-conversion]
73 | typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AUX_WRAPPER_VALUE_TYPE, (value - 1)) ) prior;
| ^
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
| ^
In file included from ../../../src/test/validation_chainstatemanager_tests.cpp:8:
In file included from ../../../src/node/chainstatemanager_args.h:9:
In file included from ../../../src/validation.h:28:
In file included from ../../../src/txmempool.h:26:
In file included from /usr/include/boost/multi_index/hashed_index.hpp:38:
In file included from /usr/include/boost/multi_index/detail/node_handle.hpp:22:
In file included from /usr/include/boost/multi_index_container_fwd.hpp:18:
In file included from /usr/include/boost/multi_index/indexed_by.hpp:17:
In file included from /usr/include/boost/mpl/vector.hpp:36:
In file included from /usr/include/boost/mpl/vector/vector20.hpp:18:
In file included from /usr/include/boost/mpl/vector/vector10.hpp:18:
In file included from /usr/include/boost/mpl/vector/vector0.hpp:24:
In file included from /usr/include/boost/mpl/vector/aux_/clear.hpp:18:
In file included from /usr/include/boost/mpl/vector/aux_/vector0.hpp:22:
In file included from /usr/include/boost/mpl/vector/aux_/iterator.hpp:19:
In file included from /usr/include/boost/mpl/plus.hpp:19:
In file included from /usr/include/boost/mpl/aux_/arithmetic_op.hpp:17:
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'int_float_mixture_enum' [-Wenum-constexpr-conversion]
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
| ^
2 errors generated.
```
Work around this issue by ignoring this diagnostic for this include. I did attempt to just downgrade the error into a warning, but that did not seem to work. Not a huge fan of inline warning/issue suppression, but this seems like the cleanest thing to do here (and easy to backport to `28.x`).
Can be tested with something like:
```bash
docker pull debian:bookworm
docker run -it debian:bookworm /bin/bash
apt update && apt install ccache cmake git pkg-config libboost-dev libevent-dev python3 libsqlite3-dev lsb-release wget software-properties-common gnupg
git clone https://github.com/bitcoin/bitcoin
wget https://apt.llvm.org/llvm.sh
chmod +x llvm.sh
./llvm.sh 18
cd bitcoin
cmake -B build -DCMAKE_C_COMPILER=clang-18 -DCMAKE_CXX_COMPILER=clang++-18
cmake --build build -j17
<snip>
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'int_float_mixture_enum' [-Wenum-constexpr-conversion]
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
| ^
2 errors generated.
Apply the patch
cmake --build build -j17
ctest --test-dir build -j17
```
Fixes #30751.
ACKs for top commit:
achow101:
ACK cd062d6684
hebasto:
ACK cd062d6684, tested on Fedora 40 using the downloaded [Boost 1.74](https://archives.boost.io/release/1.74.0/source/) and commands as follows:
Tree-SHA512: 13e5b3a544496ed2a6529ad45d03a2d872ebf41caaa06d0eec23a639d678ae1c55d73f2d4b164a4cc9e2c163264e736cd85eae90fde8089ca999cd810b16ecb5
The current logic for file descriptor accounting is pretty convoluted and hard
to follow. This is partially caused by the lack of documentation plus non-intuitive
variable naming (which was more intuitive when fewer things were accounted for, but
hasn't aged well). This has led to this accounting being error-prone and hard to maintain
(as shown in he previous commit).
Redefine some of the constants, plus document what are we accounting for so this can be
extended more easily
Remove FreeBSD workaround to #2695
We are computing our file descriptors limits based on whether we use
poll or select. However, we are taking that into account only partially
(subtracting from fd_max in one case, but from nFD later on). Moreover,
nBind is also only accounted for partially.
Simplify and fix this logic
Our current minimum supported Boost is `1.73.0`. However, when compiling
with Boost `1.74.0` (Debian Stable), using Clang `18`, compilation fails
with:
```bash
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'udt_builtin_mixture_enum' [-Wenum-constexpr-conversion]
73 | typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AUX_WRAPPER_VALUE_TYPE, (value - 1)) ) prior;
| ^
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
| ^
In file included from ../../../src/test/validation_chainstatemanager_tests.cpp:8:
In file included from ../../../src/node/chainstatemanager_args.h:9:
In file included from ../../../src/validation.h:28:
In file included from ../../../src/txmempool.h:26:
In file included from /usr/include/boost/multi_index/hashed_index.hpp:38:
In file included from /usr/include/boost/multi_index/detail/node_handle.hpp:22:
In file included from /usr/include/boost/multi_index_container_fwd.hpp:18:
In file included from /usr/include/boost/multi_index/indexed_by.hpp:17:
In file included from /usr/include/boost/mpl/vector.hpp:36:
In file included from /usr/include/boost/mpl/vector/vector20.hpp:18:
In file included from /usr/include/boost/mpl/vector/vector10.hpp:18:
In file included from /usr/include/boost/mpl/vector/vector0.hpp:24:
In file included from /usr/include/boost/mpl/vector/aux_/clear.hpp:18:
In file included from /usr/include/boost/mpl/vector/aux_/vector0.hpp:22:
In file included from /usr/include/boost/mpl/vector/aux_/iterator.hpp:19:
In file included from /usr/include/boost/mpl/plus.hpp:19:
In file included from /usr/include/boost/mpl/aux_/arithmetic_op.hpp:17:
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'int_float_mixture_enum' [-Wenum-constexpr-conversion]
/usr/include/boost/mpl/aux_/static_cast.hpp:24:47: note: expanded from macro 'BOOST_MPL_AUX_STATIC_CAST'
24 | # define BOOST_MPL_AUX_STATIC_CAST(T, expr) static_cast<T>(expr)
| ^
2 errors generated.
```
Work around this issue by ignoring this diagnostic for this include.
I did attempt to just downgrade the error into a warning, but that did
not seem to work.
See https://github.com/bitcoin/bitcoin/issues/30751 for further
discussion.
7346b01092 qt, build: remove unneeded `Q_IMPORT_PLUGIN` macro calls (Sebastian Falbesoner)
Pull request description:
After the recent full removal of Autotools (PR [#30664](https://github.com/bitcoin/bitcoin/pull/30664)), these macros are not needed anymore in the .cpp files according to the TODO in qt's CMakeLists.txt. Tested building on OpenBSD 7.5, where the XCB plugin was still imported according to the debug log:
```
2024-09-02T21:13:27Z Bitcoin Core version v28.99.0-7346b0109208 (release build)
2024-09-02T21:13:27Z Qt 5.15.12 (dynamic), plugin=xcb
2024-09-02T21:13:27Z No static plugins.
2024-09-02T21:13:27Z Style: fusion / QFusionStyle
2024-09-02T21:13:27Z System: OpenBSD 7.5, x86_64-little_endian-lp64
```
ACKs for top commit:
hebasto:
ACK 7346b01092.
Tree-SHA512: ffa033fc6e0412a99d2c167044cc7af89512a731172d6911db71434f5353e811802c268d853a76d3732e9da954444cf6c39a852aeb25938c435826e117a16fca
faecca9a85 test: Use span for raw data (MarcoFalke)
fac973647d test: Use string_view for json_tests (MarcoFalke)
Pull request description:
The build system converts raw data into a C++ header file for tests.
This change modernizes the code to use the convenience wrappers `std::span` and `std::string_view`, so that redundant copies can be avoided.
ACKs for top commit:
fjahr:
re-ACK faecca9a85
TheCharlatan:
ACK faecca9a85
stickies-v:
ACK faecca9a85
hebasto:
ACK faecca9a85, I have reviewed the code and the generated headers.
Tree-SHA512: 1f4951c54aff11ba27c41fb70f2821bdb79e06ca0abae734b970bd0d64dda9d8cced824a891fd51b3e9d4e5715ee9eb49ed5d369010a45eca7c3bec9f8641235
This change allows to drop brittle sizeof calls in favor of the
std::span::size method.
Other improvements include:
* Use of a namespace to mark test and bench data
* Use of the modern std::byte
* Drop of a no longer used std::vector copy and the bench/data module
LogPrint*ThreadNames is redundant with LogWith(out)ThreadNames, because
they all measure toggling the thread names (and check that it has no
effect on performance).
This also allows to remove unused and deprecated macros.
ae48a22a3d test: fixing failing system_tests/run_command under some Locales (Jadi)
Pull request description:
the run_command test under system_tests fails if the locale is anything
other than English ones because results such as "No such file or directory"
will be different under Non-English locales.
On the old version, a `ls nonexistingfile` was used to generate the error
output which is not ideal. In the current version we are using a Python one-liner
to generate a non 0 zero return value and "err" on stderr and check the
expected value against this.
fixes https://github.com/bitcoin/bitcoin/issues/30608
ACKs for top commit:
maflcko:
review ACK ae48a22a3d
achow101:
ACK ae48a22a3d
hebasto:
ACK ae48a22a3d, tested on Ubuntu 24.04 by switching locale.
Tree-SHA512: af7522ddcd786fa4a6832c8336ca89d8ff05f49ff963cbe1a96653b0edf29e0f950a032f23d742b16b3895e90cf5117b5f6a95464268dec67039df166d7d8639
01960c53c7 fuzz: make FuzzedDataProvider usage deterministic (Martin Leitner-Ankerl)
Pull request description:
There exist many usages of `fuzzed_data_provider` where it is evaluated directly in the function call.
Unfortunately, [the order of evaluation of function arguments is unspecified](https://en.cppreference.com/w/cpp/language/eval_order), and a simple example shows that it can differ e.g. between clang++ and g++: https://godbolt.org/z/jooMezWWY
When the evaluation order is not consistent, the same fuzzing/random input will produce different output, which is bad for coverage/reproducibility. This PR fixes all these cases I have found where unspecified evaluation order could be a problem.
Finding these has been manual work; I grepped the sourcecode for these patterns, and looked at each usage individually. So there is a chance I missed some.
* `fuzzed_data_provider`
* `.Consume`
* `>Consume`
* `.rand`
I first discovered this in https://github.com/bitcoin/bitcoin/pull/29013#discussion_r1420236394. Note that there is a possibility that due to this fix the evaluation order is now different in many cases than when the fuzzing corpus has been created. If that is the case, the fuzzing corpus will have worse coverage than before.
Update: In list-initialization the order of evaluation is well defined, so e.g. usages in `initializer_list` or constructors that use `{...}` is ok.
ACKs for top commit:
achow101:
ACK 01960c53c7
vasild:
ACK 01960c53c7
ismaelsadeeq:
ACK 01960c53c7
Tree-SHA512: e56d087f6f4bf79c90b972a5f0c6908d1784b3cfbb8130b6b450d5ca7d116c5a791df506b869a23bce930b2a6977558e1fb5115bb4e061969cc40f568077a1ad
c8e6771af0 test: restrict multiple CLI arguments (naiyoma)
8838c4f171 common/args.h: automate check for multiple cli commands (naiyoma)
Pull request description:
This PR is a continuation of the validation suggested [here](https://github.com/bitcoin/bitcoin/pull/27815) to ensure that only one Request Handler can be specified at a time.
ACKs for top commit:
stratospher:
reACK c8e6771.
achow101:
ACK c8e6771af0
tdb3:
cr re ACK c8e6771af0
Tree-SHA512: f4fe036fee342a54f1a7ac702ac35c40bf3d420fde6ab16313a75327292d5ee5c8ece1be9f852a13fcf73da1148b143b37b4894e294052abdde6eefb2e8c6f3f
6eeb188d40 test: adds seednode functional tests (Sergi Delgado Segura)
3270f0adad net: Favor peers from addrman over fetching seednodes (Sergi Delgado Segura)
Pull request description:
This is a follow-up of #28016 motivated by https://github.com/bitcoin/bitcoin/pull/28016#pullrequestreview-1913140932 and https://github.com/bitcoin/bitcoin/pull/28016#issuecomment-1984448937.
The current behavior of seednode fetching is pretty eager: we do it as the first step under `ThreadOpenNetworkConnections` even if some peers may be queryable from our addrman. This poses two potential issues:
- First, if permanently set (e.g. running with seednode in a config file) we'd be signaling such seed every time we restart our node
- Second, we will be giving the seed node way too much influence over our addrman, populating the latter with data from the former even when unnecessary
This changes the behavior to only add seednodes to `m_addr_fetch` if our addrman is empty, or little by little after we've spent some time trying addresses from our addrman. Also, seednodes are added to `m_addr_fetch` in random order, to avoid signaling the same node in case more than one seed is added and we happen to try them over multiple restarts
ACKs for top commit:
achow101:
ACK 6eeb188d40
cbergqvist:
ACK 6eeb188d40
itornaza:
Tested ACK 6eeb188d40
tdb3:
ACK 6eeb188d40
Tree-SHA512: b04445412f22018852d6bef4d3f1e88425ee6ddb434f61dcffa9e0c41b8e31f8c56f83858d5c7686289c86dc4c9476c437df15ea61a47082e2bb2e073cc62f15
a3108a7c56 rpc: Manage dumptxoutset rollback with RAII class (Fabian Jahr)
c5eaae3b89 doc: Add -rpcclienttimeout=0 to loadtxoutset examples (Fabian Jahr)
598b9bba5a rpc: Don't re-enable previously disabled network after dumptxoutset (Fabian Jahr)
Pull request description:
First, this addresses two left-over comments in #29553:
- When running `dumptxoutset` the network gets disabled in the beginning and then re-enabled at the end. The network would be re-enabled even if the user had already disabled the network themself before running `dumptxoutset`. The network is now not re-enabled anymore since that might not be what the user wants.
- The `-rpcclienttimeout=0` option is added to `loadtxoutset` examples in documentation
Additionally, pablomartin4btc notified me that he found his node stuck at the invalidated height after some late testing after #29553 was merged. We could not find the actual source of the issue since his logs got lost. However, it seems likely that some kind of disruption stopped the process before the node could roll forward again. We fixed this issue for network disablement with a RAII class previously and it seems logical that this can happen the same way for the rollback part so I suggest to also fix it the same way.
An example to reproduce the issue described above as I think it happened: Remove the `!` in the following line in `PrepareUTXOSnapshot()` to simulate an issue occurring during `GetUTXOStats()`.
```
if (!maybe_stats) {
```
This leaves the node in the following state on master:
```
$ build/src/bitcoin-cli -rpcclienttimeout=0 -named dumptxoutset utxo-859750.dat rollback=859750
error code: -32603
error message:
Unable to read UTXO set
$ build/src/bitcoin-cli getchaintips
[
{
"height": 859762,
"hash": "00000000000000000002ec7a0fcca3aeca5b35545b52eb925766670aacc704ad",
"branchlen": 12,
"status": "headers-only"
},
{
"height": 859750,
"hash": "0000000000000000000010897b6b88a18f9478050200d8d048013c58bfd6229e",
"branchlen": 0,
"status": "active"
},
```
(Note that the first tip is `headers-only` and not `invalid` only because I started `dumptxoutset` before my node had fully synced to the tip. pablomartin4btc saw it as `invalid`.)
ACKs for top commit:
maflcko:
re-ACK a3108a7c56🐸
achow101:
ACK a3108a7c56
pablomartin4btc:
cr ACK a3108a7c56
Tree-SHA512: d2ab32f62de2253312e27d7d753ec0995da3fe7a22ffc3d6c7cfa3b68a4a144c59210aa82b7a704c2a29c3b2aad6ea74972e3e8bb979ee4b7082a20f4bfddc9c
66d13c8702 test: add check that large txs aren't put into orphanage (Sebastian Falbesoner)
ed7d224666 test: add `BulkTransaction` helper to unit test transaction utils (Sebastian Falbesoner)
Pull request description:
This PR adds test coverage for the following check in `TxOrphanage::AddTx`, where large orphan txs are ignored in order to avoid memory exhaustion attacks:
5abb9b1af4/src/txorphanage.cpp (L22-L34)
Note that this code-path isn't reachable under normal circumstances, as txs larger than `MAX_STANDARD_TX_WEIGHT` are already rejected earlier in the course of doing the mempool standardness checks (see `MemPoolAccept::PreChecks` -> `IsStandardTx` -> `reason = "tx-size";`), so this is only relevant if tx standardness rules are disabled via `-acceptnonstdtxns=1`. The ignore path is checked ~~by asserting the debug log, which is ugly, but as far as I know there is currently no way to access the orphanage entries from the outside~~ via unit test that checks the return value of `AddTx`. As an alternative to adding test coverage, one might consider removing this check altogether (or replacing it with an `Assume`), as it's redundant as explained above.
ACKs for top commit:
maflcko:
review ACK 66d13c8702
glozow:
ACK 66d13c8702
tdb3:
re-ACK 66d13c8702
Tree-SHA512: 88e8254ab5fca70c387a5992649ea6a704a65162999be972cc86bd74fc26c5f0f1e13e04856708d07ad5524cb77c0918e19663db92b3593e842469dfe04af6a1
8888beea8d scripted-diff: fuzz: Rename fuzz_seed_corpus to fuzz_corpora (MarcoFalke)
Pull request description:
Now that cmake was a breaking change for all fuzz scripts, it seems fine to bundle it with another breaking change to rename the fuzz corpora directory, as discussed and approved in https://github.com/bitcoin-core/qa-assets/issues/200:
* The word "seed" in the old name doesn't really apply. In reality it is a collection of fuzz input seeds, as well as fuzz inputs.
* The rename will also allow in the future (when there is a need and desire) to provide a minimal set of possibly hand-crafted or otherwise non-fuzz-generated fuzz seed inputs to some fuzz targets (and possibly store them in a separate folder and validate that their format is still accurate and matches the fuzz target code).
* Finally, "corpus" is renamed to corpora, to clarify that the folder holds the fuzz inputs for several fuzz targets.
ACKs for top commit:
brunoerg:
ACK 8888beea8d
marcofleon:
ACK 8888beea8d
Tree-SHA512: abc693ca5d946850f04b6349e2a98f8fbc2ba9991be5a025bc0f357e341cbe7510f2f5f0e47b997d07136736d818df361270f372b8fb70860995a0605ca81e4d
fa78ed83be doc: Clarify libbitcoin_consensus in design/libraries.md (MarcoFalke)
Pull request description:
Now that the shared library has been removed in commit 80f8b92f4f, update the documentation to drop the no-longer applicable prefix "Stable...".
ACKs for top commit:
hebasto:
ACK fa78ed83be.
fanquake:
ACK fa78ed83be
Tree-SHA512: d7b946d50f734c0474ff6155a655a2bb873f76e071bfeeca1dd42ea5fdd32bc1e45129826bb54e3f111265d19c2aba2d02cb77ad7663f9fc40c8c875e5fddda2
