afea2af1391314be0e5caaa0125c884da2405316 net: reduce log level for PCP/NAT-PMP NOT_AUTHORIZED failures (ANAVHEOBA)
Pull request description:
Cherry-picks (and tweaks) a commit from #34117 which the ANAVHEOBA
did not follow up with when changes were requested.
The tweak here is to log once at `LogWarning`, so that users have a chance
to spot misconfiguration.
----
Users running on home networks with routers that don't support PCP (Port
Control Protocol) or NAT-PMP port mapping receive frequent warning-level
log messages every few minutes:
"pcp: Mapping failed with result NOT_AUTHORIZED (code 2)"
This is expected behavior for many consumer routers that have PCP
disabled by default, not an actionable error.
Add explicit constants for the NOT_AUTHORIZED result code (value 2)
for both NAT-PMP and PCP protocols. Log the first NOT_AUTHORIZED
failure at warning level for visibility, then downgrade subsequent
occurrences to LogDebug to avoid log noise. Other failure types
continue to warn unconditionally.
Fixes#34114
ACKs for top commit:
achow101:
ACK afea2af1391314be0e5caaa0125c884da2405316
sedited:
ACK afea2af1391314be0e5caaa0125c884da2405316
Tree-SHA512: 43dad9f3cca0ef9b82446045a3ccd90418cd5595c9965e938d9d539bbba863dde6b4a358adbee56f8d97d6efbf947eb5ddbbaf426faefcf3b1e36e4c8edb0d94
fa5672dcafa154dff7409eaaf762febe1d76aad7 refactor: [gui] Use SettingTo<int64_t> over deprecated SettingToInt (MarcoFalke)
fac3ecaf69d6f2d655e71644c98364206f7e2ddc rpc: Properly parse -rpcworkqueue/-rpcthreads (MarcoFalke)
faee36f63b5fde886458d0415778719ea2233d14 util: Add SettingTo<Int>() and GetArg<Int>() (MarcoFalke)
Pull request description:
The integral arg parsing has many issues:
* There is no way to parse an unsigned integral type at all
* There is no way to parse an integral type of less width than int64_t
* As a result, calling code splatters confusing c-style casts just to let the code compile. However, usually there are no range checks and proper range handling.
For example, when someone (maybe for testing) wants to set the rpc work queue to the maximum possible number, there is no easy way to do so without reading the source code and manually crafting the exact integer value. Using the "9999 hack" will silently set it to `-1` (!)
To test:
`/bld-cmake/bin/bitcoin-qt -datadir=/tmp -regtest -rpcworkqueue=99999999999999999999999999 -printtoconsole=1 -server=1 -debug=http | grep 'set work queue of depth'`
Before:
```
[http] set work queue of depth -1
```
After:
```
[http] set work queue of depth 2147483647
ACKs for top commit:
stickies-v:
ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
pinheadmz:
ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
sedited:
ACK fa5672dcafa154dff7409eaaf762febe1d76aad7
Tree-SHA512: e5060453a0aa1c4e27080e928b0ae2d1015fe487246e4059866eef415f301bc7712ce306d95076ce5b66a5e57c620715b33998192c0ff06b0384085a0390c714
24f93c9af7f6627cd7d09a1a5f10667846b048eb release note (Pol Espinasa)
331a5279d2775fb701a0bf4607436ec05e476df3 wallet, rpc:remove settxfee and paytxfee (Pol Espinasa)
Pull request description:
**Summary**
This PR removes the settxfee RPC and paytxfee setting (Bitcoin Core 31.0).
These two features were deprecated in https://github.com/bitcoin/bitcoin/pull/31278.
ACKs for top commit:
achow101:
ACK 24f93c9af7f6627cd7d09a1a5f10667846b048eb
w0xlt:
reACK 24f93c9af7f6627cd7d09a1a5f10667846b048eb
Tree-SHA512: e090f1a72ba2cbeba7c982dd51dfdcf6baf0a164827337cf56fd85f733e143b8d6116b8cd53c59c812cacef193dfa0b101a830fc455e32bf225e8505e7b2a554
fa48d421636c256069010bc03c121c36ed9c0a0c test: Stricter unit test (MarcoFalke)
fa626bd143419a7141311e84490aacd8a6691c33 util: Remove brittle and confusing sp::Popen(std::string) (MarcoFalke)
Pull request description:
The subprocess Popen call that accepts a full `std::string` has many issues:
* It promotes brittle and broken code, where spaces are not properly quoted. Example: https://github.com/bitcoin/bitcoin/pull/33929#discussion_r2590523065
* The internally used `util::split` function does incorrectly split on spaces, instead of using `shlex.split`.
* It is redundant and not needed, because a vector interface already exists.
Fix all issues by removing it and just using the vector interface.
This pull request should not change any behavior: Note that the command taken from `gArgs.GetArg("-signer", "")` is still passed through the `sp::util::split` helper, just like before. Fixing that is left for a follow-up, so that this change here is basically just a refactor.
This also fixes a unit test bug as a side-effect: Fixes https://github.com/bitcoin/bitcoin/issues/32574.
ACKs for top commit:
janb84:
cr ACK fa48d421636c256069010bc03c121c36ed9c0a0c
fjahr:
Code review ACK fa48d421636c256069010bc03c121c36ed9c0a0c
hebasto:
re-ACK fa48d421636c256069010bc03c121c36ed9c0a0c.
Tree-SHA512: 3d29226977c9392502f9361e2bd42b471ad03761bbf6a94ef6e545cbe4492ad5858da1ac9cc64b2791aacb9b6e6f3c3f63dbcc3a2bf45f6a13b5bc33eddf8c2b
Users running on home networks with routers that don't support PCP (Port
Control Protocol) or NAT-PMP port mapping receive frequent warning-level
log messages every few minutes:
"pcp: Mapping failed with result NOT_AUTHORIZED (code 2)"
This is expected behavior for many consumer routers that have PCP
disabled by default, not an actionable error.
Add explicit constants for the NOT_AUTHORIZED result code (value 2)
for both NAT-PMP and PCP protocols. Log the first NOT_AUTHORIZED
failure at warning level for visibility, then downgrade subsequent
occurrences to LogDebug to avoid log noise. Other failure types
continue to warn unconditionally.
Fixes#34114
Co-authored-by: willcl-ark <will@256k1.dev>
- Introduce a `FeeRateFormat` enum and change `CFeeRate::ToString()`
to use it for `BTC/kvB` vs `sat/vB` output formatting.
- Handle all enum values, hence remove default case in `CFeeRate::ToString()`
and `assert(False)` when a `FeeRateFormat` value is not handled.
- Keep `FeeEstimateMode` focused on fee estimation behavior by removing fee rate format
values from `FeeEstimateMode`.
- Update all formatting call sites and tests to pass `FeeRateFormat` explicitly, separating fee rate format
from fee-estimation mode selection.
The monotonic uptime fix (#34328) used a function-local static for `g_startup_time`, which was initialized on first `GetUptime()` call instead of app startup time.
This caused the first `uptime()` call to always return 0.
Move `g_startup_time` to namespace scope so it initializes at program start, ensuring the first call returns actual elapsed time. Note that we don't need to make it `static` anymore because it is just used in this single translation unit.
Test was updated to simulate some work before the first call.
Co-authored-by: Carlo Antinarella <carloantinarella@users.noreply.github.com>
14f99cfe53f07280b6f047844fc4fba0da8cd328 rpc: make `uptime` monotonic across NTP jumps (Lőrinc)
a9440b1595be7053b17895f7ee36652bac24be6e util: add `TicksSeconds` (Lőrinc)
Pull request description:
### Problem
`bitcoin-cli uptime` was derived from wall-clock time, so it could jump by large amounts when the system clock is corrected after `bitcoind` starts (e.g. on RTC-less systems syncing NTP).
This breaks the expectation that uptime reflects process runtime.
### Fix
Compute uptime from a [monotonic clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) so it is immune to wall-clock jumps, and use that monotonic uptime for the RPC.
GUI startup time is derived from wall clock time minus monotonic uptime so it remains sensible after clock corrections.
### Reproducer
Revert the fix commit and run the `rpc_uptime` functional test (it should fail with `AssertionError: uptime should not jump with wall clock`):
Or alternatively:
```bash
cmake -B build && cmake --build build --target bitcoind bitcoin-cli -j$(nproc)
DATA_DIR=$(mktemp -d)
./build/bin/bitcoind -regtest -datadir="$DATA_DIR" -connect=0 -daemon
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" -rpcwait uptime
sleep 1
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" setmocktime $(( $(date +%s) + 20000000 ))
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" uptime
./build/bin/bitcoin-cli -regtest -datadir="$DATA_DIR" stop
```
<details>
<summary>Before (uptime jumps with wall clock)</summary>
```bash
Bitcoin Core starting
0
20000001
Bitcoin Core stopping
```
</details>
<details>
<summary>After (uptime stays monotonic)</summary>
```bash
Bitcoin Core starting
0
1
Bitcoin Core stopping
```
</details>
----------
Issue: https://github.com/bitcoin/bitcoin/issues/34326
ACKs for top commit:
maflcko:
review ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328 🎦
willcl-ark:
tACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
w0xlt:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
sedited:
ACK 14f99cfe53f07280b6f047844fc4fba0da8cd328
Tree-SHA512: 3909973f58666ffa0b784a6df087031b9e34d2022d354900a4dbb6cbe1d36285cd92770ee71350ebf64d6e8ab212d8ff0cd851f7dca1ec46ee2f19b417f53984
Compute `uptime` from `SteadyClock` so it is unaffected by system time changes after startup.
Derive GUI startup time by subtracting the monotonic uptime from the wall clock time.
Add a functional test covering a large `setmocktime` jump.
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
fa4cb13b52030c2e55c6bea170649ab69d75f758 test: [doc] Manually unify stale headers (MarcoFalke)
fa5f29774872d18febc0df38831a6e45f3de69cc scripted-diff: [doc] Unify stale copyright headers (MarcoFalke)
Pull request description:
Historically, the upper year range in file headers was bumped manually
or with a script.
This has many issues:
* The script is causing churn. See for example commit 306ccd4, or
drive-by first-time contributions bumping them one-by-one. (A few from
this year: https://github.com/bitcoin/bitcoin/pull/32008,
https://github.com/bitcoin/bitcoin/pull/31642,
https://github.com/bitcoin/bitcoin/pull/32963, ...)
* Some, or likely most, upper year values were wrong. Reasons for
incorrect dates could be code moves, cherry-picks, or simply bugs in
the script.
* The upper range is not needed for anything.
* Anyone who wants to find the initial file creation date, or file
history, can use `git log` or `git blame` to get more accurate
results.
* Many places are already using the `-present` suffix, with the meaning
that the upper range is omitted.
To fix all issues, this bumps the upper range of the copyright headers
to `-present`.
Further notes:
* Obviously, the yearly 4-line bump commit for the build system (c.f.
b537a2c02a9921235d1ecf8c3c7dc1836ec68131) is fine and will remain.
* For new code, the date range can be fully omitted, as it is done
already by some developers. Obviously, developers are free to pick
whatever style they want. One can list the commits for each style.
* For example, to list all commits that use `-present`:
`git log --format='%an (%ae) [%h: %s]' -S 'present The Bitcoin'`.
* Alternatively, to list all commits that use no range at all:
`git log --format='%an (%ae) [%h: %s]' -S '(c) The Bitcoin'`.
<!--
* The lower range can be wrong as well, so it could be omitted as well,
but this is left for a follow-up. A previous attempt was in
https://github.com/bitcoin/bitcoin/pull/26817.
ACKs for top commit:
l0rinc:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
rkrux:
re-ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
janb84:
ACK fa4cb13b52030c2e55c6bea170649ab69d75f758
Tree-SHA512: e5132781bdc4417d1e2922809b27ef4cf0abb37ffb68c65aab8a5391d3c917b61a18928ec2ec2c75ef5184cb79a5b8c8290d63e949220dbeab3bd2c0dfbdc4c5
d9319b06cf82664d55f255387a348135fd7f91c7 refactor: unify container presence checks - non-trivial counts (Lőrinc)
039307554eb311ce41648d1f9a12b543f480f871 refactor: unify container presence checks - trivial counts (Lőrinc)
8bb9219b6301215f53e43967d17445aaf1b81090 refactor: unify container presence checks - find (Lőrinc)
Pull request description:
### Summary
Instead of counting occurrences in sets and maps, the C++20 `::contains` method expresses the intent unambiguously and can return early on first encounter.
### Context
Applied clang‑tidy's [readability‑container‑contains](https://clang.llvm.org/extra/clang-tidy/checks/readability/container-contains.html) check, though many cases required manual changes since tidy couldn't fix them automatically.
### Changes
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 1` | `!m.contains(k)` |
| `m.count(k) == 1` | `m.contains(k)` |
| `m.count(k) < 1` | `!m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
> Note that `== 1`/`!= 1`/`< 1` only apply to simple [maps](https://en.cppreference.com/w/cpp/container/map/contains)/[sets](https://en.cppreference.com/w/cpp/container/set/contains) and had to be changed manually.
There are many other cases that could have been changed, but we've reverted most of those to reduce conflict with other open PRs.
-----
<details>
<summary>clang-tidy command on Mac</summary>
```bash
rm -rfd build && \
cmake -B build \
-DCMAKE_C_COMPILER="$(brew --prefix llvm)/bin/clang" \
-DCMAKE_CXX_COMPILER="$(brew --prefix llvm)/bin/clang++" \
-DCMAKE_OSX_SYSROOT="$(xcrun --show-sdk-path)" \
-DCMAKE_C_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_CXX_FLAGS="-target arm64-apple-macos11" \
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_FOR_FUZZING=ON
"$(brew --prefix llvm)/bin/run-clang-tidy" -quiet -p build -j$(nproc) -checks='-*,readability-container-contains' | grep -v 'clang-tidy'
```
</details>
Note: this is a take 2 of https://github.com/bitcoin/bitcoin/pull/33094 with fewer contentious changes.
ACKs for top commit:
optout21:
reACK d9319b06cf82664d55f255387a348135fd7f91c7
sedited:
ACK d9319b06cf82664d55f255387a348135fd7f91c7
janb84:
re ACK d9319b06cf82664d55f255387a348135fd7f91c7
pablomartin4btc:
re-ACK d9319b06cf82664d55f255387a348135fd7f91c7
ryanofsky:
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.
Tree-SHA512: e4415221676cfb88413ccc446e5f4369df7a55b6642347277667b973f515c3c8ee5bfa9ee0022479c8de945c89fbc9ff61bd8ba086e70f30298cbc1762610fe1
This is a minimal behavior change and changes log output from:
[net:error] Something bad happened
[net:warning] Something problematic happened
to either
[error] Something bad happened
[warning] Something problematic happened
or, when -loglevelalways=1 is enabled:
[all:error] Something bad happened
[all:warning] Something problematic happened
Such a behavior change is desired, because all warning and error logs
are written in the same style in the source code and they are logged in
the same format for log consumers.
-BEGIN VERIFY SCRIPT-
sed --regexp-extended --in-place \
's/LogPrintLevel\((BCLog::[^,]*), BCLog::Level::(Error|Warning), */Log\2(/g' \
$( git grep -l LogPrintLevel ':(exclude)src/test/logging_tests.cpp' )
-END VERIFY SCRIPT-
The changes made here were:
| From | To |
|-------------------|------------------|
| `m.count(k)` | `m.contains(k)` |
| `!m.count(k)` | `!m.contains(k)` |
| `m.count(k) == 0` | `!m.contains(k)` |
| `m.count(k) != 0` | `m.contains(k)` |
| `m.count(k) > 0` | `m.contains(k)` |
The commit contains the trivial, mechanical refactors where it doesn't matter if the container can have multiple elements or not
Co-authored-by: Jan B <608446+janb84@users.noreply.github.com>
The changes made here were:
| From | To |
|------------------------|------------------|
| `m.find(k) == m.end()` | `!m.contains(k)` |
| `m.find(k) != m.end()` | `m.contains(k)` |
53e4951a5b5b9d166d278db4240513d09b447f58 Switch to ANSI Windows API in `fsbridge::fopen()` function (Hennadii Stepanov)
dbe770d9210666a366f055d52b9f34fa8a3d7305 Switch to ANSI Windows API in `Win32ErrorString()` function (Hennadii Stepanov)
06d0be4e22cef08fd7517f42ee82a44475c6363b Remove no longer necessary `WinCmdLineArgs` class (Hennadii Stepanov)
f366408492f6205ee20fe23e5104813de45dd4b1 cmake: Set process code page to UTF-8 on Windows (Hennadii Stepanov)
dccbb178065f05810a0fad57a86bca2f10995ecf Set minimum supported Windows version to 1903 (May 2019 Update) (Hennadii Stepanov)
Pull request description:
The main goal is to remove [deprecated](https://github.com/bitcoin/bitcoin/issues/32361) code (removed in C++26).
This PR employs Microsoft's modern [approach](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page) to handling UTF-8:
> Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.
TODO:
- [x] Handle application manifests properly when building with MSVC.
- [x] Bump the minimum supported Windows version to 1903 (May 2019 Update).
- [x] Remove all remaining use cases of the deprecated `std:wstring_convert`.
- The instance in `subprocess.h` will be addressed in a follow-up PR, as additional tests are likely needed.
- The usage in `common/system.cpp` is handled in https://github.com/bitcoin/bitcoin/pull/32566.
Resolves partially https://github.com/bitcoin/bitcoin/issues/32361.
ACKs for top commit:
laanwj:
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
hodlinator:
re-ACK 53e4951a5b5b9d166d278db4240513d09b447f58
davidgumberg:
untested crACK 53e4951a5b
Tree-SHA512: 0dbe9badca8b979ac2b4814fea6e4a7e53c423a1c96cb76ce894253137d3640a87631a5b22b9645e8f0c2a36a107122eb19ed8e92978c17384ffa8b9ab9993b5
b63428ac9ce2c903670409b3e47b9f6730917ae8 rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0dbb6ede9f9d72691c756f4bae6c97e2 refactor: increase string_view usage (stickies-v)
b3bf18f0bac0ffe18206ee20642e11264ba0c99d rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](d127b25199 (diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32))) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9ce2c903670409b3e47b9f6730917ae8 🎉
achow101:
ACK b63428ac9ce2c903670409b3e47b9f6730917ae8
pablomartin4btc:
re-ACK [b63428a](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
These overloads were needed to allow passing `fs::path` objects directly to
libstdc++'s `fstream` constructors, but after the previous commit, there is no
longer any remaining code that does pass `fs::path` objects to `fstream`
constructors. Writing new code which does this is also discouraged because the
standard has been updated in https://wg21.link/lwg3430 to disallow it.
Dropping these also means its no longer possible to pass `fs::path` arguments
directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not
a big loss because it is already not possible to pass them to the constructor.
So this commit updates `fstream::open` calls.
Additionally, this change required updates to src/bitcoin.cpp since it was
relying on the overloaded filename() method.
As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545,
newer libc++ versions implementing https://wg21.link/lwg3430 will no longer
implicitly convert `fs::path` objects to `std::filesystem::path` objects when
constructing `std::ifstream` and `std::ofstream` types.
This is not a problem in Unix systems since `fs::path` objects use
`std::string` as their native string type, but it causes compile errors on
Windows which use `std::wstring` as their string type, since `fstream`s can't
be constructed from `wstring`s.
Fix the windows libc++ compile errors by adding a new `fs::path::std_path()`
method and using it construct `fstream`s more portably.
Additionally, delete `fs::path`'s implicit `native_string` conversion so these
errors will not go undetected in the future, even though there is not currently
a CI job testing Windows libc++ builds.
Update select functions that take a const std::string& to take a
std::string_view instead. In a next commit, this allows us to use
the {Arg,MaybeArg}<std::string_view> helper.
453b0fa286e5dce0af682b7b73684dd6415a50de bitcoin: Make wrapper not require -m (Ryan Ofsky)
29e836fae660d9a89c54a094ae1a032e6a88c334 test: add tool_bitcoin to test bitcoin wrapper behavior (Ryan Ofsky)
0972f5504021b482b27523fd3bcb8036cf6b439c init: add exe name to bitcoind, bitcoin-node -version output to be able to distinguish these in tests (Ryan Ofsky)
Pull request description:
This change makes the `bitcoin` command respect IPC command line options and _bitcoin.conf_ settings, so IPC listening can be enabled by just running `bitcoin node -ipcbind=unix` or `bitcoin node` with `ipcbind=unix` in the configuration file, and there is no longer a need to specify a multiprocess `-m` option like `bitcoin -m node [...]`
sipa and theuni in #31802 pointed out that users shouldn't be exposed to multiprocess implementation details just to use IPC features, so current need to specify the `bitcoin -m` option in conjunction with `-ipcbind` could be seen as a design mistake and not just a usage inconvenience.
This PR also adds a dedicated functional test for the `bitcoin` wrapper command and to make sure it calls the right binaries and test the new functionality.
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
Sjors:
re-ACK 453b0fa286e5dce0af682b7b73684dd6415a50de
achow101:
ACK 453b0fa286e5dce0af682b7b73684dd6415a50de
TheCharlatan:
Re-ACK 453b0fa286e5dce0af682b7b73684dd6415a50de
Tree-SHA512: 9e49cb7e183fd220fa7a4e8ac68cef55f3cb2ccec40ad2a9d3e3f31db64c4953db8337f8caf7fce877bc97002ae97568dcf47ee269a06ca1f503f119bfe392c1
This patch achieves two things:
1. Fix unused variable warning (https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362493046)
2. Enable GetTotalRAM() on other platforms where it was tested to work.
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
Added a minimal system helper to query total physical RAM on [Linux/macOS/Windows](https://stackoverflow.com/a/2513561) (on other platforms we just return an empty optional).
The added test checks if the value is roughly correct by checking if the CI platforms are returning any value and if the value is at least 1 GiB and not more than 10 TiB.
The max value is only validated on 64 bits, since it's not unreasonable for 32 bits to have max memory, but on 64 bits it's likely an error.
https://learn.microsoft.com/en-us/windows/win32/api/sysinfoapi/ns-sysinfoapi-memorystatusex
> ullTotalPhys The amount of actual physical memory, in bytes.
https://man7.org/linux/man-pages/man3/sysconf.3.html:
> _SC_PHYS_PAGES The number of pages of physical memory. Note that it is possible for the product of this value and the value of _SC_PAGESIZE to overflow.
> _SC_PAGESIZE Size of a page in bytes. Must not be less than 1.
See https://godbolt.org/z/ec81Tjvrj for further details
188de70c86414b8b2ad5143f5c607b67686526ea net: Add interrupt to pcp retry loop (TheCharlatan)
Pull request description:
Without this interrupt bitcoind takes a long time to exit if requested to do so after a failed pcp lookup on startup.
ACKs for top commit:
achow101:
ACK 188de70c86414b8b2ad5143f5c607b67686526ea
fjahr:
utACK 188de70c86414b8b2ad5143f5c607b67686526ea
hodlinator:
utACK 188de70c86414b8b2ad5143f5c607b67686526ea
Tree-SHA512: 426dabd10ac0ef5de246c83d281ba70957e4032d251054aa6028b4d7ce4e35cd35ac70e67dc07bd418673bcdd2f4457b76f174ac5e7d0dd3caa05de5da952dac
When the router doesn't support natpmp and PCP, one'd normally expect
the UDP packet to be ignored, and hit a time out. This logs a warning
that is already in the debug category. However, there's also the case in
which sending an UDP packet causes a ICMP response. This is returned to
user space as "connection refused" (despite UDP having no concept of
connections).
Move the warnings from `Send` and `Recv` to debug level too, to reduce
log spam in that case.
Closes#33301.
88db09bafe9ec363525e5e526c5f6cdd13691447 net: handle multi-part netlink responses (willcl-ark)
42e99ad77396e4e9b02d67daf46349e215e99a0f net: skip non-route netlink responses (willcl-ark)
57ce645f05d18d8ad10711c347a5989076f1f788 net: filter for default routes in netlink responses (willcl-ark)
Pull request description:
...for default route in pcp pinholing.
Currently we only make a single recv call, which trucates results from large routing tables, or in the case the kernel may split the message into multiple responses (which may happen with `NLM_F_DUMP`).
We also do not filter on the default route. For IPv6, this led to selecting the first route with an `RTA_GATEWAY` attribute, often a non-default route instead of the actual default. This caused PCP port mapping failures because the wrong gateway was used.
Fix both issues by adding multi-part handling of responses and filter for the default route.
Limit responses to ~ 1MB to prevent any router-based DoS.
ACKs for top commit:
achow101:
ACK 88db09bafe9ec363525e5e526c5f6cdd13691447
davidgumberg:
Code Review re-ACK 88db09b
Sjors:
re-utACK 88db09bafe9ec363525e5e526c5f6cdd13691447
Tree-SHA512: ea5948edebfad5896a487a61737aa5af99f529fad3cf3da68dced456266948238a7143383847e79a7bb90134e023eb173c25116d8eb80ff57fa4c4a0377ca1ed
Handle multi-part netlink responses to prevent truncated results from
large routing tables.
Previously, we only made a single recv call, which led to incomplete
results when the kernel split the message into multiple responses (which
happens frequently with NLM_F_DUMP).
Also guard against a potential hanging issue where the code would
indefinitely wait for NLMSG_DONE for non-multi-part responses by
detecting the NLM_F_MULTI flag and only continue waiting when necessary.
Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.
fac90e5261b811739ada56e06ea793a12f9c2c3d test: Check that the GUI interactive reindex works (MarcoFalke)
faaaddaaf8e5a63f19c4fc66aa79134987775f96 init: [gui] Avoid UB/crash in InitAndLoadChainstate (MarcoFalke)
Pull request description:
`InitAndLoadChainstate` is problematic, when called twice in the GUI. This can happen when it returns a failure and the user selects an interactive reindex.
There are several bugs that have been introduced since the last time this was working correctly:
* The first one is a crash (assertion failure), which happens due to a cached tip block in the notifiications from the previous run. See https://github.com/bitcoin/bitcoin/pull/31346#discussion_r2207914726
* The second one is UB (use-after-free), which happens because the block index db in the blockmanager is not reset. See https://github.com/bitcoin/bitcoin/pull/30965#discussion_r2207822121
Fix both bugs by resetting any dirty state in `InitAndLoadChainstate`.
Also, add a test, because I don't really want to keep testing this manually every time. (A failing test run can be seen in https://github.com/bitcoin/bitcoin/pull/32979/checks)
ACKs for top commit:
achow101:
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
TheCharlatan:
ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
mzumsande:
Tested ACK fac90e5261b811739ada56e06ea793a12f9c2c3d
Tree-SHA512: 9f744d36e7cdd3f5871764386ec5a5cca1ae144f1bacc26c07e60313c2bdacdc5fca351aa185cb51359540eea4534dda17e4fb6073ad90f91ba0a6936faeead8
faa1c3e80d95552bdc2c0e717065ebf8d510138f Revert "Merge bitcoin/bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke)
Pull request description:
After a fork() in a multithreaded program, the child can safely
call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
until such time as it calls execv.
The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.
There was an alternative implementation using `readdir` (https://github.com/bitcoin/bitcoin/pull/32529), but that isn't async-signal-safe either, and that implementation was still using `throw`.
So temporarily revert this feature.
A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach.
Fixes https://github.com/bitcoin/bitcoin/issues/32524
Fixes https://github.com/bitcoin/bitcoin/issues/33015
Fixes https://github.com/bitcoin/bitcoin/issues/32855
For reference, a failure can manifest in the GCC debug mode:
* While `fork`ing, a debug mode mutex is held (by any other thread).
* The `fork`ed child tries to use the stdard libary before `execv` and deadlocks.
This may look like the following:
```
(gdb) thread apply all bt
Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
#0 0xf7f4f589 in __kernel_vsyscall ()
#1 0xf79e467e in ?? () from /lib32/libc.so.6
#2 0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
#3 0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
#4 0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
#5 0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91
#6 0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162
#7 0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539
#8 0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687
#9 0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300
#10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
#11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
#12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964
#13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
#14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
#15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
...
(truncated, only one thread exists)
```
ACKs for top commit:
fanquake:
ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
darosior:
ACK faa1c3e80d95552bdc2c0e717065ebf8d510138f
Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a
fa9ca13f35be0a023aeed78775ad66f95717b28b refactor: Sort includes of touched source files (MarcoFalke)
facb152697b8d7b75a9e6108f8896f774b06b35f scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7227594e2f59499cf7f7f9420284e04 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
achow101:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
janb84:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
stickies-v:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
