3bbdbc0a5e qt, docs: Unify term "clipboard" (Hennadii Stepanov)
Pull request description:
A translator on Transifex noticed:
> The term "system clipboard" appears twice. The term "clipboard" appears 10 times. Perhaps we could standardize on just saying "clipboard"?
This PR addresses this issue.
ACKs for top commit:
davidgumberg:
ACK 3bbdbc0a5e
pablomartin4btc:
ACK 3bbdbc0a5e
Tree-SHA512: 61a100f60890d81122a4b8ce3e2cb7d355c7fb643de3196573f7f9107c6f52fa0b3e7a4f743ce2833e8c67b9cdad3568b761d730fef5c9781f5e1c45252888c4
002b792b9a gui: decouple WalletModel from RPCExecutor (furszy)
Pull request description:
A more comprehensive fix for the issue described in #837.
Since the `WalletModel` class is unavailable when compiling without wallet support
`(-DENABLE_WALLET=0)`, the RPC executor class should not be coupled to it.
This decoupling ensures GUI compatibility with builds that omit wallet support.
This also drops an extra `#ifdef ENABLE_WALLET` block which is always good.
ACKs for top commit:
w0xlt:
Code Review ACK 002b792b9a
pablomartin4btc:
tACK 002b792b9a
BrandonOdiwuor:
tACK 002b792b9a
hebasto:
ACK 002b792b9a, I have reviewed the code and it looks OK.
Tree-SHA512: a8e6b7e9d88dd8e0ff5e2d0de91be2f85fd0559265267d3bf6cae5a37606cf1ab6bc7415d5817a11006008de362f2ca3557ba772b4e1bd9fbef5f564be3b53bb
ab878a7e74 build: simplify *ifaddr handling (fanquake)
Pull request description:
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
`iffaddrs.h` that implements one, but not the other.
ACKs for top commit:
hebasto:
ACK ab878a7e74. Only addressed my [comment](https://github.com/bitcoin/bitcoin/pull/32446#discussion_r2079994126) and rebased since my recent [review](https://github.com/bitcoin/bitcoin/pull/32446#pullrequestreview-2825606189).
TheCharlatan:
ACK ab878a7e74
Tree-SHA512: 7667305df9fef4728526c7217f85b51e739ec63b38e808da51d6ae65cb6f2696afa5ba82e5a72ed4a7a9b79ffa2402640448af4392587253027122eab7618e30
a58cb3b1c1 qa: sanity check mined block have their coinbase timelocked to height (Antoine Poinsot)
8f2078af6a miner: timelock coinbase transactions (Antoine Poinsot)
788aeebf34 qa: use prev height as nLockTime for coinbase txs created in unit tests (Antoine Poinsot)
c76dbe9b8b qa: timelock coinbase transactions created in fuzz targets (Antoine Poinsot)
9c94069d8b contrib: timelock coinbase transactions in signet miner (Antoine Poinsot)
a5f52cfcc4 qa: timelock coinbase transactions created in functional tests (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes enforcing that coinbase transactions set their
nLockTime field to the block height minus 1, as well as their nSequence such as to not disable the
timelock. If such a fork were to be activated by Bitcoin users, miners need to be ready to produce
compliant blocks at the risk of losing substantial amounts mining would-be invalid blocks. As miners
are unfamously slow to upgrade, it's good to make this change as early as possible.
Although Bitcoin Core's GBT implementation does not provide the `coinbasetxn` field, and mining
pool software crafts the coinbase on its own, updating the Bitcoin Core mining code is a first step
toward convincing pools to update their (often closed source) code. A possible followup is also to
introduce new fields to GBT. In addition, this first step also makes it possible to test future
Consensus Cleanup changes.
The commit making the change also updates a bunch of seemingly-unrelated tests. This is because those tests were asserting error messages based on the txid of transactions involved, and changing the coinbase transaction structure necessarily changes the txid of all tests' transactions.
ACKs for top commit:
Sjors:
Code review ACK a58cb3b1c1
achow101:
ACK a58cb3b1c1
TheCharlatan:
Re-ACK a58cb3b1c1
Tree-SHA512: a2aae009a187eb760d34435f518a895ee76c6b02a667eb030ddf6bd584da6e8eae2737d974dbf81a928d60c07bcb4820f055adc067e18d8819640db0240bb513
1ee698fde2 test: refactor: negate signature-s using libsecp256k1 (Sebastian Falbesoner)
Pull request description:
This small PR gets rid of manual mod-n inversion of the ECDSA signature-s part in unit tests (introduced a long time ago in #5256, triggered by https://github.com/bitcoin-core/secp256k1/pull/69) by using secp256k1 instead. The function wasn't available at that time, but was introduced about three years later, see https://github.com/bitcoin-core/secp256k1/pull/408. Note that as the name suggests, `secp256k1_ec_seckey_negate` is meant to be used for secret keys, but it obviously works in general for scalars modulo the group order.
ACKs for top commit:
achow101:
ACK 1ee698fde2
laanwj:
Code review ACK 1ee698fde2
w0xlt:
ACK 1ee698fde2
rkrux:
tACK 1ee698fde2
Tree-SHA512: dc36ea1572b538d11ae34e1871f310a1cda8083ffb753e93e7ee9d56e91ebd8ec78d35758dfb700254720914b734ef7a071eeef71b6239f19e1e2fb289fb5435
It is confusing that the chain client flush happens between
StopHTTPServer and StopMapPort. Also, it is unused code. Seems best to
just add it back properly when it is needed again.
31c5ebc400 tracing: fix invalid argument in mempool_monitor (William Casarin)
Pull request description:
The mempool_monitor tracing tool is incorrectly reading the reason as the first argument. Fix this!
Noticed this during the bitcoin++ mempool hackathon 😅
cc 0xB10C
ACKs for top commit:
0xB10C:
Code Review ACK 31c5ebc400
Tree-SHA512: 6f3d64f0f75a44e1fdcad71af8e737ce948833498cd3879ef74cbabf53e3649145b83febceca19b1662de55346c199bf4259e17f5b28cf0352aefa730e07ea63
10845cd7cc qa: Add feature_framework_startup_failures.py (Hodlinator)
28e282ef9a qa: assert_raises_message() - Stop assuming certain structure for exceptions (Hodlinator)
1f639efca5 qa: Work around Python socket timeout issue (Hodlinator)
9b24a403fa qa: Only allow calling TestNode.stop() after connecting (Hodlinator)
6ad21b4c01 qa: Include ignored errors in RPC connection timeout (Hodlinator)
879243e81f qa refactor: wait_for_rpc_connection - Treat OSErrors the same (Hodlinator)
Pull request description:
Improves handling of startup errors in functional tests and puts tests in place to ensure knock-on errors don't creep in.
- `wait_for_rpc_connection()` now appends specific failures leading up to the `Unable to connect to bitcoind` error to that error message:
`[node 0] Unable to connect to bitcoind after 60s (ignored errors: {'missing_credentials': 1, 'OSError.ECONNREFUSED': 239}, latest error: ConnectionRefusedError(111, 'Connection refused'))`
- Fixes Windows Python issue where `socket.timeout` exceptions end up with unset `errno`-fields.
- Also adds comments, refactors code, improves logging.
The underlying purpose is to ensure developer efficiency in finding root causes of test failures.
Prior iterations of the PR partially focused on fixing the same issue as #31620.
Originally inspired by #30390.
### Testing
Can be tested by reverting either faf2f2c654 or fae3bf6b87 from #31620, or the "qa: Avoid calling stop-RPC if not connected" from this PR, and running *feature_framework_startup_failures.py*.
ACKs for top commit:
l0rinc:
ACK 10845cd7cc
ryanofsky:
Code review ACK 10845cd7cc. Only changes since last review were adding a new commit tweaking assert_raises_message(), extending the new test to have a self-check, and to pass through all options to child tests instead of a hardcoded list of options. I left some cleanup suggestions below but they are not important.
Tree-SHA512: f0235c5cbb6d1bb85d8dc5de492a08a34f6edc83499cbf0a5f9a3824809ff84635888c62c9c01101e3cc9ef9f1cdee2c9ab6537fea6feeb005b29f428caf8b22
f9dfe8d5e0 contrib: remove bdb exception from FORTIFY check (fanquake)
Pull request description:
BDB has been removed (#28710), so we no-longer need to ignore functions from BDB in this check.
Guix building this branch, and looking for `*_chk` functions across all binaries produces:
```
# nm -C * | grep -i _chk | sort | uniq
U __fdelt_chk@GLIBC_2.15
U __fprintf_chk@GLIBC_2.3.4
U __fread_chk@GLIBC_2.7
U __longjmp_chk@GLIBC_2.11
U __memcpy_chk@GLIBC_2.3.4
U __printf_chk@GLIBC_2.3.4
U __snprintf_chk@GLIBC_2.3.4
U __sprintf_chk@GLIBC_2.3.4
U __stack_chk_fail@GLIBC_2.4
U __vsnprintf_chk@GLIBC_2.3.4
```
ACKs for top commit:
achow101:
ACK f9dfe8d5e0
theuni:
utACK f9dfe8d5e0
laanwj:
Code review ACK f9dfe8d5e0
Tree-SHA512: e9491c8b348a0d777c3f7186cab48b478548654712f8b85e7bde2f8b94f3a8b52bc7be8fb1b4a486954359d3109cfb74e3485ccfff67c6546f0efcabf2eda0e0
We really just want to skip this when building for Windows. So do that,
and remove the two header checks (we also already use both of these
headers, unguarded, in the !windows part of the codebase).
Squash the two *iffaddrs defines into one, as I haven't seen an
iffaddrs.h that implements one, but not the other.
edde96376a cmake: Respect user-provided configuration-specific flags (Hennadii Stepanov)
Pull request description:
This PR addresses [this](https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2542140874) comment:
> I suppose that should only happen if the `-O3` isn't coming from an explicitly set `CMAKE_CXX_FLAGS_RELEASE`.
With this PR:
```
$ cmake -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_CXX_FLAGS_RELEASE="-O3"
<snip>
C++ compiler flags .................... -O3 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/dev/bitcoin/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
Linker flags .......................... -O3 -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
```
and
```
$ cmake -B build -DCMAKE_BUILD_TYPE=Release
<snip>
C++ compiler flags .................... -O2 -std=c++20 -fPIC -fno-extended-identifiers -fdebug-prefix-map=/home/hebasto/dev/bitcoin/src=. -fmacro-prefix-map=/home/hebasto/dev/bitcoin/src=. -fstack-reuse=none -Wall -Wextra -Wformat -Wformat-security -Wvla -Wredundant-decls -Wdate-time -Wduplicated-branches -Wduplicated-cond -Wlogical-op -Woverloaded-virtual -Wsuggest-override -Wimplicit-fallthrough -Wunreachable-code -Wbidi-chars=any -Wundef -Wno-unused-parameter -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection
Linker flags .......................... -O2 -fstack-reuse=none -fstack-protector-all -fcf-protection=full -fstack-clash-protection -Wl,-z,relro -Wl,-z,now -Wl,-z,separate-code -fPIE -pie
```
When calling `cmake` repeatedly using the same build directory, each newly provided `CMAKE_CXX_FLAGS_RELEASE` value will be accommodated. In such a scenario, if the user wishes to revert to the build system defaults, they should unset the `CMAKE_CXX_FLAGS_RELEASE` variable by passing `-UCMAKE_CXX_FLAGS_RELEASE` to `cmake`.
---
This PR does not aim to resolve _all_ issues mentioned in https://github.com/bitcoin/bitcoin/issues/31491.
ACKs for top commit:
purpleKarrot:
ACK edde96376a
janb84:
ACK [edde963](edde96376a)
ryanofsky:
Code review ACK edde96376a
Tree-SHA512: 1fbc879bd02cf0be726ced490f65985e728f0686ccb3a32cd38787b56377aa666e1965448e5069515abc814df49a0083c8000bc3f6f322f5f395695638168fb6
1372eb09c5 doc: swap "Docker image" for "container image" (fanquake)
Pull request description:
I haven't used Docker for some time (now Podman), and the images are generic, so just use "container image". I'll be pushing some changes to https://github.com/fanquake/core-review/tree/master/guix, to reflect this.
ACKs for top commit:
janb84:
ACK 1372eb09c5
laanwj:
ACK 1372eb09c5
hebasto:
ACK 1372eb09c5.
Tree-SHA512: 45bb74d25a0faf7e5c3666d6897fb6b999144308c43cdf8a290d3a4210285b1e95286d27bb3d90bc50be4784c2242ad3f93794086f4634439a46a48ff68c7343
fa24fdcb7f lint: Remove string exclusion from locale check (MarcoFalke)
Pull request description:
The exclusion isn't needed. In fact, it prevents detection of `"bla" + wrong()`.
For example, the following is not detected:
```diff
diff --git a/src/wallet/rpc/addresses.cpp b/src/wallet/rpc/addresses.cpp
index 1c2951deee..c1209013e5 100644
--- a/src/wallet/rpc/addresses.cpp
+++ b/src/wallet/rpc/addresses.cpp
@@ -336,7 +336,8 @@ RPCHelpMan addmultisigaddress()
RPCHelpMan keypoolrefill()
{
return RPCHelpMan{"keypoolrefill",
- "\nFills the keypool."+
+ "\nRefills each descriptor keypool in the wallet up to the specified number of new keys.\n"
+ "By default, descriptor wallets have 4 active ranged descriptors (\"legacy\", \"p2sh-segwit\", \"bech32\", and \"bech32m\"), each with " + std::to_string(DEFAULT_KEYPOOL_SIZE) + " entries.\n" +
HELP_REQUIRING_PASSPHRASE,
{
{"newsize", RPCArg::Type::NUM, RPCArg::DefaultHint{strprintf("%u, or as set by -keypool", DEFAULT_KEYPOOL_SIZE)}, "The new keypool size"},
```
Fix the script by detecting it.
ACKs for top commit:
laanwj:
Code review ACK fa24fdcb7f.
rkrux:
ACK fa24fdcb7f
w0xlt:
ACK fa24fdcb7f
Tree-SHA512: cb7e6ed9fec5d2089e94031329ebf26b83a1814ffbbbca94f7527c127bc759d13c0f4ea79b71ff7f5f009d071dcf01958c8921163d6dc5e1ae6256cc40b57eea
4e8ab5e00f crypto: disable ASan for sha256_sse4 with Clang (fanquake)
Pull request description:
This also fails to compile when optimisations are being used, see: https://github.com/bitcoin/bitcoin/issues/31913.
So just disable ASan under any optimisation level.
Closes#31913.
ACKs for top commit:
maflcko:
lgtm ACK 4e8ab5e00f
davidgumberg:
Tested ACK 4e8ab5e00f
laanwj:
Code review ACK 4e8ab5e00f
Tree-SHA512: 680fb424f43b35730e03e0c7443c80445a2cf423d4f9161414ea22fea0b955f49197f8a96d1241896d981c6c13814d3eb7b5e4d8c9138813fb69e437ac4768ea
ff35a4b021 docs: Improve `keypoolrefill` RPC docs (w0xlt)
Pull request description:
Update `keypoolrefill` RPC docs to make it clear that descriptor wallets have 4 ScriptPubKeyManagers by default and each of them is updated in this command, as suggested https://github.com/bitcoin/bitcoin/issues/29924#issuecomment-2849321859
Closes https://github.com/bitcoin/bitcoin/issues/29924
ACKs for top commit:
achow101:
ACK ff35a4b021
brunoerg:
code review ACK ff35a4b021
Tree-SHA512: b6b9abe3fecebf9551b4ce9280794292c6ac44ccaeb2b9d60eeb4b2c177fe8372d0fe103f99c9cc0baeb2559ec019d1c495c233f24a600531153a38eeacb9670
Using helper variables has two issues:
1. They contaminate the global namespace of the main build script.
2. They can be used as `set(var)`, effectively exposing a cache variable
`var`, which makes the toolchain file susceptible to the build
environment.
de054df6dc contrib: Remove legacy wallet RPCs from bash completions (Ava Chow)
5dff04a1bb legacy spkm: Make IsMine() and CanProvide() private and migration only (Ava Chow)
c0f3f3264f wallet: Remove unused db functions (Ava Chow)
83af1a3cca wallet: Delete LegacySPKM (Ava Chow)
8ede6dea0c wallet, rpc: Remove legacy wallet only RPCs (Ava Chow)
4de3cec28d test: rpcs disabled for descriptor wallets will be removed (Ava Chow)
84f671b01d test: Run multisig script limit test (Ava Chow)
810476f31e test: Remove unused options and variables, correct comments (Ava Chow)
04a7a7a28c build, wallet, doc: Remove BDB (Ava Chow)
Pull request description:
The final step of #20160.
A bare minimum of legacy wallet code is kept in order to perform wallet migration. Migration of legacy wallets uses the independent BDB parser and a minimal `LegacyDataSPKM` that allows the legacy data to be loaded so that the migration can be completed.
BDB has been removed as a dependency and documentation have been updated to reflect that.
ACKs for top commit:
Sjors:
re-ACK de054df6dc
maflcko:
re-ACK de054df6dc🔗
w0xlt:
reACK de054df6dc
rkrux:
Concept ACK de054df6dc
Tree-SHA512: 16a6c265bc1ada5e7a5ef9b95f0ff65015672ca46d9a43b7e10d60e9e085052e9bbfe01ac3e494cc606afb652a1b476b10e434d13e9877b67d2cb0196a9bd190
The string exclusion would fail to detect `"bla" + wrong()`.
Also, remove /* */ comment exclusion, which would fail to detect stuff
like `/* bla */ wrong()`.
Instead, require the function to be called by adding \\( to the regex.
Finally, also remove the section in the dev notes, because:
* It was outdated and missing some functions such as std::to_string in
the list.
* The maintenance overhead of having to update two places is fragile and
questionable.
* Many other linters are also not mentioned in the dev notes, even
though they are important.
* A dev (and CI) is more likely to run the linters than to read the dev
notes.
* The dev notes are more than 1000 lines of dense information. It would
be easier to digest if they focused on the important stuff that is not
checked by automated tools.
fa4804009c fuzz: Remove unused TimeoutExpired catch in fuzz runner (MarcoFalke)
Pull request description:
Currently, the way to check for libFuzzer is to search the stderr of the fuzz executable when passed `-help=1` for the string `libFuzzer`. See also 14b8dfb2bd/contrib/devtools/deterministic-fuzz-coverage/src/main.rs (L90-L101)
The python test runner additionally includes a timeout catch, which was needed before the plain `read_file` fallback was implemented, see 14b8dfb2bd/src/test/fuzz/fuzz.cpp (L251).
However, it is no longer needed and the printed error message would be wrong, so remove it.
(side-note: On Windows the fuzz executable seems to time out when an assert is hit in a debug build, see https://github.com/bitcoin/bitcoin/issues/32341#issuecomment-2842716175. However, no one is running fuzz debug on Windows. Also, the newly added debug logging is a preferable replacement in this case anyway.)
ACKs for top commit:
kevkevinpal:
crACK [fa48040](fa4804009c)
Crypt-iQ:
crACK fa4804009c
marcofleon:
crACK fa4804009c
brunoerg:
code review ACK fa4804009c
Tree-SHA512: 64f5e3862fece9ab2b6592615b72b81e9c087dcd394b1d062a96df0d88d8b5999674f0faa1165a5998c05289c1874e29311d7b24d84fee9bc6c46d1662d29e4d
b5f580c580 scripted-diff: adapt script error constant names in feature_taproot.py (Sebastian Falbesoner)
Pull request description:
While reviewing #31622 I noticed that the constant name `(SCRIPT_)ERR_SIG_HASHTYPE` is used for two different script verification error codes, namely one for legacy and one for Schnorr signatures:
eba5f9c4b6/src/script/script_error.cpp (L56-L57)eba5f9c4b6/test/functional/feature_taproot.py (L600)
In order to resolve this confusion, this PR adapts all script error constant names in the functional tests (currently only in feature_taproot.py) to the ones used in our C++ codebase (see [script_error.cpp](https://github.com/bitcoin/bitcoin/blob/master/src/script/script_error.cpp)) with a scripted diff. This also makes checking whether we have test coverage for a certain script error easier.
ACKs for top commit:
jamesob:
crACK b5f580c580
achow101:
ACK b5f580c580
rkrux:
tACK b5f580c580
stratospher:
ACK b5f580c. liked the consistency in script error names.
Tree-SHA512: bc0ccec70bc3cb6ce51ce8e27a5e54770d1bb93c1db5a9c815caa25f3d96ebb382104bd9b51626f501d4f5b95148db8d20c806a27153e9bb9cf823a20d3046c0
85368aafa0 test: Run simple tests at various feerates (Murch)
d610951c15 test: Recreate BnB iteration exhaustion test (Murch)
2a1b2754f1 test: Remove redundant repeated test (Murch)
4781f5c8be test: Recreate simple BnB failure tests (Murch)
a94030ae98 test: Recreate BnB clone skipping test (Murch)
7db6f012c0 test: Move BnB feerate sensitivity tests (Murch)
2bafc46261 test: Recreate simple BnB success tests (Murch)
Pull request description:
This PR is splitting off some of the improvements made in #28985 and starts addressing the issues raised in #27754.
I aim to completely replace `coinselector_tests` with `coinselection_tests`. The goal is to generally use coins created per a nominal _effective value_ so we can get away from testing with `CoinSelectionParams` that are non-representative and effectuate counterintuitive behavior such as `feerate = 0` or `cost_of_change = 0`
ACKs for top commit:
achow101:
ACK 85368aafa0
monlovesmango:
ACK 85368aafa0
w0xlt:
ACK 85368aafa0
Tree-SHA512: 1a984837b4efddc0d8abe11668898fb207fb539e784bf911d4038211274b82e0fe1f8fffe7e5a19e0e013ccb7dc40e3f62d853a2a729980d0d935e66f12b9156
