e6994efe08 fix: increase rpcbind check robustness (tdb3)
d38e3aed89 fix: handle invalid rpcbind port earlier (tdb3)
83b67f2e6d refactor: move host/port checking (tdb3)
73c243965a test: add tests for invalid rpcbind ports (tdb3)
Pull request description:
Previously, when an invalid port was specified in `-rpcbind`, the `SplitHostPort()` return value in `HTTPBindAddresses()` was ignored and attempt would be made to bind to the default rpcbind port (with the host/port treated as a host).
This rearranges port checking code in `AppInitMain()` to handle the invalid port before reaching `HTTPBindAddresses()`. Also adds a check in `HTTPBindAddresses()` as a defensive measure for future changes.
Adds then updates associated functional tests as well.
ACKs for top commit:
achow101:
ACK e6994efe08
ryanofsky:
Code review ACK e6994efe08
zaidmstrr:
Code review ACK [e6994ef](e6994efe08)
Tree-SHA512: bcc3e5ceef21963821cd16ce6ecb83d5c5657755882c05872a7cfe661a1492b1d631f54de22f41fdd173512d62dd15dc37e394fe1a7abe4de484b82cd2438b92
8466329127 chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2 chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c8 test: remove wallet context from `write_wallet_settings_concurrently` (ismaelsadeeq)
Pull request description:
This PR addresses the remaining review comments from #30697
1. Disallowed overwriting settings values with a `null` value.
2. Uniformly used the `SettingsAction` enum in all settings methods instead of a boolean parameter.
3. Updated `overwriteRwSetting` to receive the `common::SettingsValue` parameter by value, enabling it to be moved safely.
4. Removed wallet context from the `write_wallet_settings_concurrently` unit test, as it is not needed.
ACKs for top commit:
achow101:
ACK 8466329127
ryanofsky:
Code review ACK 8466329127. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.
furszy:
Code review ACK 8466329127
Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052
51f7668d31 addrman: change nid_type from int to int64_t (Martin Zumsande)
051ba3290e addrman, refactor: introduce user-defined type for internal nId (Martin Zumsande)
Pull request description:
With `nIdCount` being incremented for each addr received, an attacker could cause an overflow in the past, see https://bitcoincore.org/en/2024/07/31/disclose-addrman-int-overflow/
Even though that attack was made infeasible indirectly by addr rate-limiting (PR #22387), to be on the safe side and prevent any regressions change the `nId`s used internally to `int64_t`.
This is being done by first introducing a user-defined type for `nId`s in the first commit, and then updating it to `int64_t` (thanks sipa for help with this!).
Note that `nId` is only used internally, it is not part of the serialization, so `peers.dat` should not be affected by this.
I assume that the only reason this was not done in the past is to not draw attention to this previously undisclosed issue.
ACKs for top commit:
naumenkogs:
ACK 51f7668d31
stratospher:
ACK 51f7668d31. I think it's a good change to make the nId space large(64 bits) so that the nId values are distinct.
achow101:
ACK 51f7668d31
Tree-SHA512: 68d4b8b0269a01a9544bedfa7c1348ffde00a288537e4c8bf2b88372ac7d96c4566a44dd6b06285f2fcf31b4f9336761e3bca7253fbc20db5e0d04e887156224
54227e681a rpc, cli: improve error message on multiwallet mode (pablomartin4btc)
Pull request description:
Running a CLI command when multiple wallets are loaded and `-rpcwallet` is not specified, should return a clearer error.
Currently in `master`:
```
$ bitcoin-cli -regtest -generate 1
error code: -19
error message:
Wallet file not specified (must request wallet RPC through /wallet/<filename> uri-path).
Try adding "-rpcwallet=<filename>" option to bitcoin-cli command line.
```
With this change:
```
$ bitcoin-cli -regtest -generate 1
error code: -19
error message:
Multiple wallets are loaded. Please select which wallet to use by requesting the RPC through the /wallet/<walletname> URI path. Or for the CLI, specify the "-rpcwallet=<walletname>" option before the command (run "bitcoin-cli -h" for help or "bitcoin-cli listwallets" to see which wallets are currently loaded).
```
ACKs for top commit:
maflcko:
review ACK 54227e681a
achow101:
ACK 54227e681a
furszy:
utACK 54227e681a
mzumsande:
Code Review ACK 54227e681a
jonatack:
ACK 54227e681a
Tree-SHA512: 51ff24f64858aa6be6adf6f20105c9f076ebea743780bf2a4399f7fe8b5239cbb1ea06d32b2ef5e850da2369abb0ef7a52c50c2b8f31f4ca90d3a486abc9b77e
58499b00d0 refactor: move `SignSignature` helpers to test utils (Sebastian Falbesoner)
Pull request description:
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. As suggested by instagibbs, see https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1697515508.
ACKs for top commit:
instagibbs:
ACK 58499b00d0
pablomartin4btc:
ACK 58499b00d0
Tree-SHA512: a52d3b92b477246f2ceb57c3690d0229a492b65a15dae331faeae9d96e5907f7fe1176edc1530243e0f088586984fd7ba435a0a2d2f2531c04d076fdf3f4095f
7025942687 build: drop superfluous `HAVE_BUILD_INFO` define (Sebastian Falbesoner)
0dd662510c build: drop obj/ subdir for generated build.h, rename to bitcoin-build-info.h (Sebastian Falbesoner)
Pull request description:
As indicated by the TODO, the obj subdirectory is not needed anymore now for the generated build.h header, since autotools are gone and we don't have in-source builds anymore (see #30454, #30664). In the second commit the superflous `HAVE_BUILD_INFO` macro is dropped, as suggested in https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2292424496.
ACKs for top commit:
theuni:
utACK 7025942687
Tree-SHA512: 0a3b2cbbcf638344ceb74e5ba5a0fe2b1718427b23a18c8890258db36ce7177006a146178ed88d9c5ae956a5426f3844e86c1f4cca7c40946359742bffda983b
bitcoin-build-info.h should always be generated before clientversion.cpp
is compiled due to the following explicit dependency in src/CMakeLists.txt:
add_dependencies(bitcoin_clientversion generate_build_info)
Hence there is no need to gate the inclusion of that header with an
extra define.
Now that this file is not in a subfolder anymore, prefix it with
"bitcoin-" to avoid potential collisions. Also add "info" for a more
descriptive name.
caac06f784 streams: reorder/document functions (Pieter Wuille)
67a3d59076 streams: remove unused code (Pieter Wuille)
Pull request description:
This is a follow-up to #30884.
Remove a number of dead code paths, and improve the code organization and documentation, in `AutoFile`.
ACKs for top commit:
maflcko:
re-ACK caac06f784
theStack:
Code-review ACK caac06f784
l0rinc:
ACK caac06f784
tdb3:
CR ACK caac06f784
Tree-SHA512: 297791f093e0142730f815c11dd3466b98f7e7edea86094a815dae989ef40d8056db10e0fed6e575d530903c18e80c08d36d3f1e6b828f2d955528f365b22008
facbcd4cef log: Use ConstevalFormatString (MarcoFalke)
fae9b60c4f test: Use LogPrintStr to test m_log_sourcelocations (MarcoFalke)
fa39b1ca63 doc: move-only logging warning (MarcoFalke)
Pull request description:
This changes all logging (including the wallet logging) to produce a
`ConstevalFormatString` at compile time, so that the format string can be
validated at compile-time.
I tested with `clang` and found that the compiler will use less than 1% more of time and memory.
When an error is found, the compile-time error depends on the compiler, but it may look similar to:
```
src/util/string.h: In function ‘int main(int, char**)’:
src/bitcoind.cpp:265:5: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>(((const char*)"Hi %s %s"))’
src/util/string.h:38:98: in ‘constexpr’ expansion of ‘util::ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(std::basic_string_view<char>(((const char*)((util::ConstevalFormatString<1>*)this)->util::ConstevalFormatString<1>::fmt)))’
src/util/string.h:78:34: error: expression ‘<throw-expression>’ is not a constant expression
78 | if (num_params != count) throw "Format specifier count must match the argument count!";
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
```
This refactor does not change behavior of the compiled executables.
ACKs for top commit:
hodlinator:
re-ACK facbcd4cef
l0rinc:
ACK facbcd4cef
ryanofsky:
Code review ACK facbcd4cef
pablomartin4btc:
re-ACK facbcd4cef
stickies-v:
Approach ACK and code LGTM facbcd4cef modulo a `tinyformat::format_error` concern.
Tree-SHA512: 852f74d360897020f0d0f6e5064edc5e7f7dacc2bec1d5feff22c634a2fcd2eb535aa75be0b7191d9053728be6108484c737154b02d68ad3186a2e5544ba0db8
fa99e4521b ci: Allow CCACHE_DIR bind mount (MarcoFalke)
fa252da0b9 ci: Remove hardcoded CCACHE_DIR in cirrus (MarcoFalke)
fa146904e1 ci: Bump default CCACHE_MAXSIZE to 500M (MarcoFalke)
aaaa7cf8ba cirrus: Drop CCACHE_NOHASHDIR (MarcoFalke)
fa7ca182a9 ci: Print inner env (MarcoFalke)
Pull request description:
The outer env is printed when the `/tmp/env-$USER-$CONTAINER_NAME` is created. There is also a separate env printed when building the container image (usually with default values).
To confirm that the inner container env is correctly derived from the outer env, and not from the default build env, print it a third time.
ACKs for top commit:
l0rinc:
utACK fa99e4521b
willcl-ark:
ACK fa99e4521b
Tree-SHA512: 59fd3262d551e09224866e31c14ca865461e81abbe00b83391fe3a9c7ada30fd2fd0272e4aa812df2712433ac7594d1a55cf674248b341359cec09c8d3f0c58b
a9964c0444 doc: Updating docs from autotools to cmake (kevkevinpal)
Pull request description:
A bit of a followup from https://github.com/bitcoin/bitcoin/pull/30840
- In this change the documentation where we refer to the `./configure` script which is now gone and have converted the configure params to use the `cmake` equivalent.
ACKs for top commit:
maflcko:
ACK a9964c0444
jonatack:
utACK a9964c0444
jarolrod:
ACK a9964c0444
tdb3:
ACK a9964c0444
pablomartin4btc:
re-ACK a9964c0444
Tree-SHA512: f7ed20b8ad61f028c0d242b9cc70650d8da63057d4a8f7da88f0117a8d3241c5fe8fcf19d56ec82088160b9fee9b175fe9f64e5a845260d3696dc7e94bfdd0bd
ccccb67851 ci: Use clang-19 in msan tasks (MarcoFalke)
Pull request description:
A new clang version generally comes with bugfixes, new sanitizer features, deprecations, as well as new features.
Upgrade the memory sanitizer tasks to use the new version.
(Ref https://github.com/bitcoin/bitcoin/pull/30634)
ACKs for top commit:
fanquake:
ACK ccccb67851 Tested both jobs on aarch64, and one on x86_64 with `mmap_rnd_bits`.
Tree-SHA512: a42bf2da7c08aa54c0c5ab3811ff51b98b80b276be135eed32395a55ae93a42d41d7cd32c307062dcca711a892958ea141168c2a06025560074f8c5d20190946
replaced --enable-debug with -DCMAKE_BUILD_TYPE=Debug in developer-notes
replaced --enable-multiprocess with -DWITH_MULTIPROCESS=ON
replaced --disable-zmq with -DWITH_ZMQ=OFF
735436df8c Remove outdated Eclipser fuzzing documentation (Jon Atack)
Pull request description:
Remove the Eclipser fuzzing documentation from `doc/fuzzing.md`, as that repository (https://github.com/SoftSec-KAIST/Eclipser) hasn't been updated in several years, appears possibly unmaintained, and likely isn't being actively used for fuzzing Bitcoin Core.
These docs were originally added in https://github.com/bitcoin/bitcoin/pull/22585.
ACKs for top commit:
maflcko:
review ACK 735436df8c
brunoerg:
ACK 735436df8c
Tree-SHA512: 7ccbf93c10add53e92edf67a622722935029add63f8fbb6e733b96e9d155faeb8d5d3678adb0e7f2ce8ccbdffd2a34c3dc93adbcf4e3ce0cdd03e20ad3e6bbd6
d01b85bfec ci: Use `ninja` to build in macOS native CI job (Hennadii Stepanov)
Pull request description:
This PR addresses [this](https://github.com/bitcoin/bitcoin/pull/30911#issuecomment-2354922939) comment:
> I wonder if one CI task should be using Ninja (and cmake >= 3.27), if it isn't too hard to implement. Otherwise this config will remain untested and errors may sneak in to the master branch, only being detected after merge.
ACKs for top commit:
maflcko:
review ACK d01b85bfec
theuni:
ACK d01b85bfec.
jonatack:
ACK d01b85bfec
jarolrod:
ACK d01b85bfec
Tree-SHA512: 5cbbc87f0e48512441a4f0cf10af2f6d73f24d3e8667b338b176fd1667fd5d7739349bcede3aeef973497ff67d33cb8f7d7f3681c3ede8e8b2f673b853d5bc63
Adds invalid rpcbind port checking to
`HTTPBindAddresses()`. While movement of
`CheckHostPortOptions()` in the previous
commit handles rcpbind port errors, updating
`HTTPBindAddresses()` port checking adds
a defensive measure for potential future
changes.
Previously, when an invalid port was specified
in `-rpcbind`, the `SplitHostPort()` return value
in `HTTPBindAddresses()` was ignored and attempt
would be made to bind to the default rpcbind port
(with the host/port treated as a host).
This rearranges port checking code in
`AppInitMain()` to handle the invalid
port before reaching `HTTPBindAddresses()`.
Also adjusts associated functional tests.
The primary objective is to provide users with clearer
and more informative error messages when encountering
the RPC_WALLET_NOT_SPECIFIED error, which occurs when
multiple wallets are loadad.
This commit also rectifies the error message consistency
by bringing the error message in line with the definition
established in protocol.h ("error when there are multiple
wallets loaded").
This changes all logging (including the wallet logging) to produce a
ConstevalFormatString at compile time, so that the format string can be
validated at compile-time.
Also, while touching the wallet logging, avoid a copy of the template
Params by using const Params&.
af9f987893 doc: update NeedsRedownload() comment (Sjors Provoost)
Pull request description:
Noticed two outdated comments while reviewing #29370.
Since #21009 we no longer roll back the chain, when a user updates a pre-segwit node to a modern node. In this unlikely scenario we tell the user to `-reindex`.
This PR updates a comment in `PopulateAndValidateSnapshot` to reflect that change. Ditto for the description of `nStatus` in `chain.h`.
ACKs for top commit:
maflcko:
re-ACK af9f987893
fjahr:
ACK af9f987893
Tree-SHA512: d590f1cff6823297764c863753ed5478b8933d503c43933902d50b449dfd852a02aeb318c072ad25d02e4c2583d7026cd176a10b0584292d6bbe381a063f5c45
2a581144f2 build: Minimize I/O operations in GenerateHeaderFromJson.cmake (Lőrinc)
aa003d1568 build: Minimize I/O operations in GenerateHeaderFromRaw.cmake (Lőrinc)
Pull request description:
Follow up of the https://github.com/bitcoin/bitcoin/pull/30883 revert.
Replaced multiple file writes with a single string template write.
The raw content is first grouped into 8 byte chunks, followed by another regex replace which wraps them in `std::byte` or just the raw bytes, prefixed with `0x`.
Tested the output with `diff -w` and they're the same - only whitespace differences because slightly different source formatting.
----
Tested the `Raw` performance with:
```bash
time cmake -DRAW_SOURCE_PATH=src/bench/data/block413567.raw -DHEADER_PATH=build/after/block413567.raw.h -DRAW_NAMESPACE=benchmark::data -P cmake/script/GenerateHeaderFromRaw.cmake
```
Before:
> 15.41s user 23.06s system 97% cpu 39.593 total
After:
> 0.77s user 0.06s system 97% cpu 0.849 total
----
Tested the `Json` performance with:
```bash
time cmake -DJSON_SOURCE_PATH=src/secp256k1/src/wycheproof/ecdsa_secp256k1_sha256_bitcoin_test.json -DHEADER_PATH=build/after/ecdsa_secp256k1_sha256_bitcoin_test.json -P cmake/script/GenerateHeaderFromJson.cmake
````
Before:
> 3.57s user 6.01s system 94% cpu 10.136 total
After:
> 0.17s user 0.01s system 98% cpu 0.187 total
ACKs for top commit:
maflcko:
review ACK 2a581144f2👒
hebasto:
ACK 2a581144f2.
willcl-ark:
tACK 2a581144f2
Tree-SHA512: 5e44f79d1c0dbb61d8b64f28d4c3c87a176981f72104b28800eef2037b0728076cbcf14ff07b05ff94d4e8800605586cfd5df00519db9027933c5943348c01d2
fab932b421 ci: Remove incorrectly hardcoded HOST in mac_native task (MarcoFalke)
fa8f35d786 ci: Use macos-14 GHA image (MarcoFalke)
Pull request description:
There shouldn't be any downside, because XCode remains pinned to the same version.
However, builds are expected to be a bit faster with M1, which seems nice.
ACKs for top commit:
hebasto:
ACK fab932b421.
willcl-ark:
ACK fab932b421
Tree-SHA512: 9719e05c67b8b5f3d59bd1a38eef00407b1ae5e123b18151c494b6d2dbf55bd2b0b5bb6c1a0469635c7b3bb5f23990d3bb2f339f56ce3955e8a1b97ac9f295d4
89bf11b807 guix: build Linux GCC with --enable-cet (fanquake)
Pull request description:
Similar to #29695, and in the same vein of explicitly configuring hardening options in our release toolchain.
See https://gcc.gnu.org/install/configure.html:
>` --enable-cet`
> Enable building target run-time libraries with control-flow instrumentation, see `-fcf-protection option`. When --enable-cet is specified target libraries are configured to add `-fcf-protection` and, if needed, other target specific options to a set of building options.
> `--enable-cet=auto` is default. CET is enabled on Linux/x86 if target binutils supports Intel CET instructions and disabled otherwise. In this case, the target libraries are configured to get additional `-fcf-protection` option.
ACKs for top commit:
TheCharlatan:
ACK 89bf11b807
Tree-SHA512: 772d8529713a31e5db42be4e053582bb9ba6f26079ae136c6bf8303c4992a90d61159dbb0fde7a4b4cb7b4bf5024d5397a78004e6188b36e1c36dd5e5cdc49ad
