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
3ae35b427f ci: run check-deps.sh as part of clang-tidy job (Ryan Ofsky)
0aaa1298a0 contrib: fix check-deps.sh when libraries do not import symbols (Ryan Ofsky)
3c99f5a38a contrib: fix check-deps.sh to check for weak symbols (Ryan Ofsky)
86c80e9cf2 contrib: make check-deps.sh script work with cmake (Ryan Ofsky)
Pull request description:
Fix check-deps.sh to check for weak symbols so it can detect when an exported template function like is used from another library.
Also update the script to work with cmake and configure it to run as part of CI.
Problem was reported by hebasto in https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843
ACKs for top commit:
TheCharlatan:
Re-ACK 3ae35b427f
hebasto:
ACK 3ae35b427f, I have reviewed the code and it looks OK. Also I've tested it locally.
Tree-SHA512: c3b58175450b675e6e848549b81bcfe42930ea9bcd693063867ce3f0ac3999c98cd2c3e961f163ff06641e8288f3a4e81530936a296a83d45d33364f27489521
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
a7a4e11db8 cmake: scope Boost Test check to vcpkg (fanquake)
Pull request description:
This check was added for `vcpkg`, given how it packages Boost. However, we don't need to run the check for other platforms, and it's quite slow. So, scope it to just `vcpkg`.
On my machine, this reduces the time to run `time cmake -B build` from ~12 seconds, to ~6 seconds.
Fixes: #30787.
ACKs for top commit:
kevkevinpal:
lgtm ACK [a7a4e11](a7a4e11db8)
maflcko:
review ACK a7a4e11db8
davidgumberg:
Tested ACK a7a4e11db8
hebasto:
re-ACK a7a4e11db8.
Tree-SHA512: 67cf3908a5381e21aeaa168a6f76b6e066d64a8ad2127d5ae9fe71a0f04bccf58a400726d9d4e228b3bdb6fca799034fd05a38388278fea30a1a841f6adac017
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
`FORTIFY_SOURCE` should be used if `ENABLE_HARDENING=ON` and optimisations
are being used. This should not be coupled to any particular build type,
because even if the build type is `Debug`, optimisations might still
be in use.
Fixes: #30800.
This check was added for vcpkg, given how it packages Boost. However, we
don't need to run the check for other platforms, and it's quite slow.
So, scope it to VCPKG. On my machine, this reduces the time to run
`cmake -B build` from ~12 seconds, to ~6 seconds.
Fixes: #30787
fa3a7ebe5b lint: Check for release note snippets in the wrong folder (MarcoFalke)
Pull request description:
It is a common mistake to place the snippets in the wrong folder, where they could be missed. For example https://github.com/bitcoin/bitcoin/pull/30719#pullrequestreview-2262535007 or commit 84900ac34f.
Fix all issues by adding a simple lint check.
Can be tested by reverting a prior commit that violated the rule and then running the new check:
```
git revert 35ef34eab7
( cd ./test/lint/test_runner/ && RUST_BACKTRACE=1 cargo run -- --lint=doc_release_note_snippets )
ACKs for top commit:
l0rinc:
ACK fa3a7ebe5b
TheCharlatan:
Re-ACK fa3a7ebe5b
Tree-SHA512: 65a13696178aa8f94daa12a767cc74861293c631c19da9ca23c0fd43cedd47e7928d0ef14ad9ad83a434c1ac0e006f5a632ba9679756e071dea65b3cbf927c2d
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
d9fcbfc372 build: Add `JOBS` variable support to `CoverageFuzz.cmake` script (Hennadii Stepanov)
e7cf4a6f27 build: Add missed `-g` for "Coverage" build configuration (Hennadii Stepanov)
fe2003ab12 build: Add `COMMAND_ERROR_IS_FATAL` to every process in coverage scrips (Hennadii Stepanov)
Pull request description:
The first commit ensures early error catching.
The second commit adds the `-g` flag that was missed during the migration from Autotools.
This PR is intended to be tested with GCC compiler (as clang support is still under [scrutiny](https://github.com/hebasto/bitcoin/issues/341)). Depending on the `lcov` version, additional flags `-DCMAKE_C_FLAGS="-fprofile-update=atomic" -DCMAKE_CXX_FLAGS="-fprofile-update=atomic"` may be required.
ACKs for top commit:
maflcko:
review ACK d9fcbfc372
tdb3:
cr re ACK d9fcbfc372
Tree-SHA512: 0998411dc1ccd60d7bd6b36f4e2881f699202c65dcc8c177b46380d0f255d291d9537f1dc6fb35478b632f3515d3484d8e7d2877126c57e3f02b21f90160f1eb
5567754087 depends: build libevent with -D_GNU_SOURCE (fanquake)
Pull request description:
Currently, builds of libevent in depends, using CMake, fail on some systems, like Alpine, with the following:
```bash
/bitcoin/depends/work/build/aarch64-unknown-linux-musl/libevent/2.1.12-stable-1516ed47ea8/evmap.c: In function 'evmap_signal_add_':
/bitcoin/depends/work/build/aarch64-unknown-linux-musl/libevent/2.1.12-stable-1516ed47ea8/evmap.c:456:31: error: 'NSIG' undeclared (first use in this function)
456 | if (sig < 0 || sig >= NSIG)
```
From what I can tell the `GNU_SOURCE` "detection" in libevents CMake build system, never? really worked, primarily relies on looking for a deprecated define, and it's not clear what a nice fix is. For now, always build with `_GNU_SOURCE`, to match the autotools behaviour.
ACKs for top commit:
TheCharlatan:
ACK 5567754087
Tree-SHA512: 4552b4a92867e8fa2af0ffa39b2be6c994bf739de7ce6a7c581590be486da81f7d93fca816854548c1e912347d33a35218c441b5058c3cbd3e82c74a9b7c78d9
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
In case of a failure to create the dump, the node should not be left in an inconsistent state like deactivated network activity or an invalidated blockchain.
e5f7272ad3 doc: fix assumeutxo design doc link (marcofleon)
Pull request description:
A correction to a link as I was exploring Assumeutxo stuff.
ACKs for top commit:
fjahr:
ACK e5f7272ad3
tdb3:
ACK e5f7272ad3
MarnixCroes:
ACK e5f7272ad3
Tree-SHA512: b7380d884a196b89eed32bc14af8ca11191c9f2bcb1c5c163bb627fd87a6231dbd86da6e659baddb8c652961b5e44f80509606fee9ae38a53e90ebb05d082404
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.
8131bf7483 ci: parse TEST_RUNNER_EXTRA into an array (Max Edwards)
c4762b0aa0 test: allow excluding func test by name and arg (Max Edwards)
Pull request description:
While working on CI I wanted to disable some functional tests so I used the `TEST_RUNNER_EXTRA` var. The problem I had was tests that have flags such as `rpc_bind.py --ipv6` must be passed in quotes otherwise the `--ipv6` portion will be considered an argument to `test_runner.py` rather than a test name.
This change allows proper parsing of quotes and complex values such as:
```shell
TEST_RUNNER_EXTRA='--exclude "rpc_bind.py --ipv6,feature_proxy.py"'
```
Update:
While testing this it was noticed that `test_runner.py` when given `--exclude "rpc_bind.py --ipv6"` will exclude all `rpc_bind.py` tests so this PR has been updated to include a change to the test runner to only exclude the specific test if you pass an arg or exclude all tests of that name if you do not pass an arg. `--exclude rpc_bind.py` will exclude all three variants and `--exclude rpc_bind --ipv6` will only exclude the IPV6 variant.
ACKs for top commit:
maflcko:
ACK 8131bf7483
achow101:
ACK 8131bf7483
hebasto:
ACK 8131bf7483, tested on Ubuntu 23.10 and Windows 11.
Tree-SHA512: 82b73f12d627f533d8e5be4a518d455ef4427a755bbe03ccd11d0bb70c7ff3cee76220b0264fcfb236661c4cf5deba034cbfc2372b96d5861f3436c21eae8264
fafdb7df34 lint: Speed up flake8 checks (MarcoFalke)
faf17df7fb lint: Document missing py_lint dependency (MarcoFalke)
faebeb828f lint: Remove python whitespace and shadowing lint rules (MarcoFalke)
7777047835 lint: Remove python lint rules that are SyntaxError (MarcoFalke)
faaf3e53f0 test: [refactor] Fix F841 flake8 (MarcoFalke)
444421db69 test: [refactor] Fix E714 pycodestyle (MarcoFalke)
Pull request description:
The checks have many issues:
* Some checks that could in theory hide bugs are not applied -> Fix them and apply them going forward
* Some checks are redundant Python 2 checks, or of low value -> Remove them
* The checks are slow -> Speed them up from ~10 seconds to about ~20 milliseconds
ACKs for top commit:
davidgumberg:
review and tested reACK fafdb7df34
kevkevinpal:
ACK [fafdb7d](fafdb7df34)
achow101:
ACK fafdb7df34
Tree-SHA512: a0488b722cfaf7071bd6848cd3be002e0b6c38af80d8b5cbb08613c0b174ef63277289f960db8ac31adb09fe563a4973203b8fb10b83cbcfdc6f0ef39bd04410
Script was failing when called on libraries that do not import symbols, because
bash pipefail option was specified, and grep was used in some pipelines to
filter symbols, and grep returns status 1 when it doesn't match any lines. This
could cause the script to fail on some systems and configurations, such as the
clang-tidy CI configuration
https://cirrus-ci.com/task/4801670352207872?logs=ci#L6191 where the
libbitcoin_crypto_x86_shani.a library does not import symbols.
Fix check-deps.sh to check for weak symbols so it can detect when an exported
template function is used from another library.
In a previous version of this commit, this change caused an invalid dependency
in the consensus library on the TryParseHex template function from the util
library to be detected, and a suppression was added here. But #30377 removed
the invalid dependency so the suppression is no longer needed.
The invalid dependency and problem detecting weak symbol usage was originally
reported by Hennadii Stepanov in
https://github.com/bitcoin/bitcoin/pull/29015#issuecomment-2209258843
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
These helpers haven't been used in production code since segwit was
merged more than eight years ago (see commit 605e8473, PR #8149),
so it seems appropriate to move them to the test utils module.
Can be reviewed via `--color-moved=dimmed-zebra`.
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
ec317bc44b test: update satoshi_round function (naiyoma)
Pull request description:
This PR refactors `satoshi_round` to accept different rounding modes and make rounding a required argument.
Continuation of https://github.com/bitcoin/bitcoin/pull/23225
ACKs for top commit:
maflcko:
review ACK ec317bc44b
achow101:
ACK ec317bc44b
AngusP:
ACK ec317bc44b
Tree-SHA512: 070f0aa6f66e58bff7412cae6b71f5f4ab8c718c7b5eeba4bb604fe22c6416a1ced0474294f504e1c28943ddc073104466b5944b43bae27e42bee5ca85afa468
fb6d51eb25 signet/miner: Use argparse exclusive groups (Anthony Towns)
338a266a9a signet/miner: add support for a poolnum/poolid tag in mined blocks (Anthony Towns)
409ab7d35b signet/miner: add Generate.mine function (Anthony Towns)
7b31332370 signet/miner: add Generate.gbt function (Anthony Towns)
85c5c0bea9 signet/miner: add Generate.next_block_time function (Anthony Towns)
5540e6ca49 signet/miner: move next_block_* functions into new Generator class (Anthony Towns)
35f4631196 signet/miner: rename do_decode_psbt to decode_psbt (Anthony Towns)
aac040b439 signet/miner: drop create_coinbase function (Anthony Towns)
16951f549e signet/miner: drop do_createpsbt function (Anthony Towns)
3aed0a4284 signet/miner: drop get_reward_address function (Anthony Towns)
Pull request description:
Refactors the code a bunch, and adds `--poolnum` / `--poolid` options so that signers can tag their coinbases in a way that explorers can recognise (see also https://github.com/bitcoin-data/mining-pools/pull/82 and https://github.com/mempool/mempool/issues/2903).
The refactoring in particular helps enable the "try using inquisition's getblocktemplate, and if that doesn't work fall back to core's getblocktemplate" logic, as described/implemented in https://github.com/bitcoin-inquisition/bitcoin/pull/7
ACKs for top commit:
achow101:
ACK fb6d51eb25
danielabrozzoni:
Code review ACK fb6d51eb25
Tree-SHA512: d84095c4045ab196685b847e04ce2cdaedf387bc2527430ede918318dc5b70bf3d87b754264016f895f506fac70d4fdea5ef3cd8c3c375fd586afeae01e045e5
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
