27c976d11a68d66db97d9a7a30c6a6a71c6ab586 fix: increase consistency of rpcauth parsing (tdb3)
2ad3689512a36eaff957df9ac28e65b2fedbc36c test: add norpcauth test (tdb3)
67df0dec1abe547773e532aa60aff0317e018123 test: blank rpcauth CLI interaction (tdb3)
ecc98ccff25b7e758337e764e59d764076772fec 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 27c976d11a68d66db97d9a7a30c6a6a71c6ab586
ryanofsky:
Code review ACK 27c976d11a68d66db97d9a7a30c6a6a71c6ab586. Since last review commit message was just tweaked to clarify previous behavior.
Tree-SHA512: af2e9dd60d1ad030409ae2c3805ab139c7435327823d9f8bbeede815f376cb696a5929b08a6e8c8b5f7278ed49cfb231789f9041bd57f1f03ec96501b669da5b
30803a35d54acda19ded88474c205f8954fea5e1 cmake: decouple FORTIFY_SOURCE check from Debug build type (fanquake)
Pull request description:
`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.
Also somewhat of a followup to https://github.com/bitcoin/bitcoin/pull/30778#discussion_r1742257436.
ACKs for top commit:
ryanofsky:
Code review ACK 30803a35d54acda19ded88474c205f8954fea5e1
TheCharlatan:
ACK 30803a35d54acda19ded88474c205f8954fea5e1
Tree-SHA512: 298f8805a5bb2f1ff54e51ea31324d712c2070cc3eba26561c31001ace4bfa37ae6d18531cbd45e2faf610a0a1b83b420fcde6e329e17f02b021d26563583913
1f054eca4e779cfa5f9f6e9278071adf65e5eafe 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 1f054eca4e779cfa5f9f6e9278071adf65e5eafe.
TheCharlatan:
ACK 1f054eca4e779cfa5f9f6e9278071adf65e5eafe
Tree-SHA512: efed91b8aa0813100304ee58e169bbf5cfbb7db465ec4f7e6cbbae6053f09a36757bf96b4d1cb9ddf4c1cab0ceb3ab18805ebefa122535518ffb501c9b489d3d
7a669fde183b2f98c4faeff3eedaf95a1cba3b09 docs: Fix minor typo (Gutflo)
Pull request description:
Fix typo in doc/build-windows-msvc.md:
- "Micsrosoft" -> Microsoft
No test required.
ACKs for top commit:
l0rinc:
ACK 7a669fde183b2f98c4faeff3eedaf95a1cba3b09
Tree-SHA512: fd3815ebf449885e8a27d4f21e61a4482a7983ccfe40b13a4d658a304845775acd2f4f8acec2e85c24b6179223bb21baaecf1dd0a4d2921427686148ac1ed208
Since the move to cmake, the kernel static library that is installed
after a cmake --install build is unusable. It lacks symbols for the
internal libraries, besides those defined in the kernel library target.
This is because cmake, unlike the libtool archiver, does not combine
multiple static libraries into a single static library on installation.
This is likely an intentional design choice, since there were a bunch of
caveats to the way libtool calculated these libraries.
Fix this problem by installing all the required libraries. The user must
then link all of them along with the bitcoin kernel library.
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.
fa9d7d5d205ada8915cbbc29599ab8e7bf1fffe0 test: Work around boost compilation error (MarcoFalke)
fa3ecdf778bcde7713004ff8bb86d3fdfc916969 Revert "build: work around issue with Boost <= 1.80 and Clang >= 18" (MarcoFalke)
Pull request description:
There seems to be an issue compiling the `chainstatemanager_rebalance_caches` test case with some specific versions of Boost in combination with some specific versions of Clang. For example, Boost 1.74 may fail in combination with Clang 18. [1]
The error stems from a mixed-type closeness comparison. Given that the comparison is using floating point, and isn't meant to be exact, work around the compile error by ensuring both sides of the comparison are using the same type (`double`).
This also allows to drop a previous workaround.
[1] Error:
```
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.
ACKs for top commit:
hebasto:
ACK fa9d7d5d205ada8915cbbc29599ab8e7bf1fffe0.
fanquake:
ACK fa9d7d5d205ada8915cbbc29599ab8e7bf1fffe0
Tree-SHA512: 4964b23162f2351c7d3cf7e9efa7860d62f3b6717c3cc5be967d286f1ddb3539c2637247c79aa83123d36ff111ba77df22be2a25487ddd94dc1321d5e751dc70
This change enables building and installing only `libbitcoinkernel`,
without the need to disable other targets during the project build
system generation.
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
3ae35b427fe59bc9ab24d07c1adb46faa702de20 ci: run check-deps.sh as part of clang-tidy job (Ryan Ofsky)
0aaa1298a08f898318916661f2317b2e755206e6 contrib: fix check-deps.sh when libraries do not import symbols (Ryan Ofsky)
3c99f5a38a47e4e10a0daab3a114b5e476fcacfa contrib: fix check-deps.sh to check for weak symbols (Ryan Ofsky)
86c80e9cf296f6560f2f846bd4e7286f7b958b93 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 3ae35b427fe59bc9ab24d07c1adb46faa702de20
hebasto:
ACK 3ae35b427fe59bc9ab24d07c1adb46faa702de20, I have reviewed the code and it looks OK. Also I've tested it locally.
Tree-SHA512: c3b58175450b675e6e848549b81bcfe42930ea9bcd693063867ce3f0ac3999c98cd2c3e961f163ff06641e8288f3a4e81530936a296a83d45d33364f27489521
fadbcd51fc77a3f4e877851463f3c7425fb751d2 bench: Remove redundant logging benchmarks (MarcoFalke)
fa8dd952e279a87f6027ddd2e2119bf2ae2f9943 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 fadbcd51fc77a3f4e877851463f3c7425fb751d2
Tree-SHA512: 4fe137f374aa4ee1aa0e1da4a1f9839c0e52c23dbb93198ecafee98de39d311cc47304bba4191f3807aa00c51b1eae543e3f270f03d341c84910e5e341a1d475
fa84f9decd224ea1c25dc7095bad70a48fa1a534 test: Pin and document TEST_DIR_PATH_ELEMENT (MarcoFalke)
2222f7a87404078984c7189768a3422deb114302 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 fa84f9decd224ea1c25dc7095bad70a48fa1a534
ryanofsky:
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
Tree-SHA512: eb35d6598bb08f9b996e3a4762d8f26b2441c0ca00780798e473015af735dfc9997120895a922b94d4b6ada45adadba4a686e9cf9c285ddf688848e764c64840
a7a4e11db8722c85175bcc4d9f73a713e9e61513 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 a7a4e11db8722c85175bcc4d9f73a713e9e61513
davidgumberg:
Tested ACK a7a4e11db8
hebasto:
re-ACK a7a4e11db8722c85175bcc4d9f73a713e9e61513.
Tree-SHA512: 67cf3908a5381e21aeaa168a6f76b6e066d64a8ad2127d5ae9fe71a0f04bccf58a400726d9d4e228b3bdb6fca799034fd05a38388278fea30a1a841f6adac017
cd062d6684908d526be7423f8f1057b891254a3c 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 cd062d6684908d526be7423f8f1057b891254a3c
hebasto:
ACK cd062d6684908d526be7423f8f1057b891254a3c, 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
