The variable is conditionally assigned toward the end of the loop and
not used after. It's then set back to its default value at the beginning
of the loop.
1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4 Add release note for #31223 (Martin Zumsande)
997757dd2b4d7b20b17299fbd21970b2efb8bbc8 test: add functional test for -port behavior (Martin Zumsande)
0e2b12b92a28a2949e75bf50f31563f52e647d6e net, init: derive default onion port if a user specified a -port (Martin Zumsande)
Pull request description:
This resolves#31133 (setups with multiple local nodes each using a different `-port` no longer working with v28.0, see the issue description for more details) by deriving the default onion listening port to be the value specified by `-port` incremented by 1 (idea by vasild / laanwj).
Note that with this fix, the chosen `-port` values of two local nodes cannot be adjacent, otherwise there will be port collisions again.
From the discussion in the linked issue, this was the most popular option, followed by doing nothing and telling affected users to change their setups to use `-bind` instead of `-port`. But more opinions are certainly welcome!
I think that if we decide to do something about the problem described in the issue, we should do so soon (in 28.1.), so I opened this PR.
Fixes#31133
ACKs for top commit:
achow101:
ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
laanwj:
Tested ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
tdb3:
Code review ACK 1dd3af8fbc350c6f1efa8ae6449e67e1b42ccff4
Tree-SHA512: 37fda2b23bbedcab5df3a401cf5afce66ae5318fb78f9660f83e3fd075b528e8156d7a0903f9a12ffe97ab5d83860587116b74af28670a1f4c2f0d1be4999f40
37946c0aafeebc1585f1316fb05f252f7fb51e91 Set notifications m_tip_block in LoadChainTip() (Sjors Provoost)
Pull request description:
Ensure KernelNotifications `m_tip_block` is set even if no new block arrives.
Suggested in https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486457573
ACKs for top commit:
ryanofsky:
Code review ACK 37946c0aafeebc1585f1316fb05f252f7fb51e91, fixing comment bug caught by @mzumsande in https://github.com/bitcoin/bitcoin/pull/31346#discussion_r1870315593 in another really helpful clarification
mzumsande:
Code Review ACK 37946c0aafeebc1585f1316fb05f252f7fb51e91
TheCharlatan:
ACK 37946c0aafeebc1585f1316fb05f252f7fb51e91
Tree-SHA512: 931bf820440a0cdda276f6dbd63f03fdbcdc90b18e7d5e160a74bdd9d0290acc706c35aab15bbdcd6e5e0b77565b3d07ff49b0dcf6551cb83961bae67be5d1bb
fa47baa03bcfcf44fb2ed05f009a32d32f860c45 ci: Bump centos gcc (MarcoFalke)
Pull request description:
Currently the centos stream9 CI task is using gcc-11. This is fine, because this is also the minimum supported.
However:
* There is already a CI task that is checking the minimum supported version: 62bd61de11/ci/test/00_setup_env_native_previous_releases.sh (L11-L12)
* The CI log is a bit useless, because it is mostly just `#warning _FORTIFY_SOURCE > 2 is treated like 2 on this platform [-Werror=cpp]`. This makes it harder to spot real warnings, such as https://github.com/bitcoin/bitcoin/issues/31476
Fix both issues by using gcc-12.
ACKs for top commit:
hebasto:
ACK fa47baa03bcfcf44fb2ed05f009a32d32f860c45.
Tree-SHA512: 573618efc949437d33365a24f77a26a9b68457f7fb9bd603ee92bc5f17fec73ccba114cafb900eddee3531af47508ce5c246def93268787cdfa2b99e6f45a13d
5cd9e95eea12fddd2a1a668d509f0a5b6e267516 depends: update capnproto to 1.0.2 (fanquake)
Pull request description:
This fixes compilation on FreeBSD:
```bash
-- Build files have been written to: /tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4
Building native_capnp...
gmake[1]: Entering directory '/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4'
[ 1%] Building CXX object src/kj/CMakeFiles/kj.dir/array.c++.o
[ 2%] Building CXX object src/kj/CMakeFiles/kj.dir/cidr.c++.o
/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4/src/kj/cidr.c++:112:71: error: member access into incomplete type 'const struct sockaddr_in6'
otherBits = reinterpret_cast<const struct sockaddr_in6*>(addr)->sin6_addr.s6_addr;
^
/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4/src/kj/cidr.c++:112:51: note: forward declaration of 'sockaddr_in6'
otherBits = reinterpret_cast<const struct sockaddr_in6*>(addr)->sin6_addr.s6_addr;
^
/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4/src/kj/cidr.c++:123:63: error: member access into incomplete type 'const struct sockaddr_in'
&reinterpret_cast<const struct sockaddr_in*>(addr)->sin_addr.s_addr);
^
/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4/src/kj/cidr.c++:123:44: note: forward declaration of 'sockaddr_in'
&reinterpret_cast<const struct sockaddr_in*>(addr)->sin_addr.s_addr);
^
/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4/src/kj/cidr.c++:133:69: error: member access into incomplete type 'const struct sockaddr_in6'
otherBits = reinterpret_cast<const struct sockaddr_in6*>(addr)->sin6_addr.s6_addr;
^
/tmp/cirrus-ci-build/bitcoin-core/depends/work/build/x86_64-unknown-freebsd14.0/native_capnp/1.0.1-867405dd2c4/src/kj/cidr.c++:133:49: note: forward declaration of 'sockaddr_in6'
otherBits = reinterpret_cast<const struct sockaddr_in6*>(addr)->sin6_addr.s6_addr;
^
3 errors generated.
```
See: 1c19c362b4.
ACKs for top commit:
kevkevinpal:
Concept ACK [5cd9e95](5cd9e95eea)
theuni:
utACK 5cd9e95eea12fddd2a1a668d509f0a5b6e267516
ryanofsky:
Code review ACK 5cd9e95eea12fddd2a1a668d509f0a5b6e267516. Downloaded the file and checked the hash. Also followed theuni's lead and looked at the source changes which were very minor. It did look like thousands of lines changed in the autotools build, but this should not affect us as we are using the cmake build.
Tree-SHA512: 5d78887a9e950c8532c427b17969128de0c6d466ec5ffba85241457e8e19673c22ddb3493cdfce5086f57ba760eac5e91f703992b2f70f2a7c82ba885255279c
62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8 wallet: Migrate non-HD keys to combo() descriptor (Ava Chow)
Pull request description:
Non-HD keys do not have an HD seed ID associated with them, so if this value is the null value (all 0s), then we should not perform any seed ID comparison that would result in excluding the keys from combo() migration.
This changes the migration of non-HD wallets (or blank wallets with imported private keys) to make a single combo() descriptors for the non-HD/imported keys, rather than pk(), pkh(), sh(wpkh()), and wpkh() descriptors for the keys.
Implements https://github.com/bitcoin/bitcoin/pull/31374#discussion_r1876650074
ACKs for top commit:
laanwj:
Concept and code review ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
brunoerg:
code review ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
furszy:
Nice catch. ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
theStack:
ACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
rkrux:
tACK 62b2d23edbad76c1d68bb8c0fc8af54e7c0d7dc8
Tree-SHA512: 86a80b7dcc1598ab18068a2572ff4b4920b233178b760f7b76c5b21a9e6608005ac872f90e082a8f99b51daab0b049e73e4bee5b8e0b537d56ed0d34122a1f49
df27ee9f024f69a8bdac8b0ee3bd7882f6a54294 refactor: Fix "modernize-use-starts-ends-with" clang-tidy warning (Hennadii Stepanov)
Pull request description:
This PR is a follow-up to #31306 and fixes the "modernize-use-starts-ends-with" warning in the multiprocess code (see https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2527008761).
Fixes https://github.com/chaincodelabs/libmultiprocess/issues/124.
ACKs for top commit:
l0rinc:
ACK df27ee9f024f69a8bdac8b0ee3bd7882f6a54294
theuni:
utACK df27ee9f024f69a8bdac8b0ee3bd7882f6a54294
ryanofsky:
Code review ACK df27ee9f024f69a8bdac8b0ee3bd7882f6a54294
Tree-SHA512: b5685818e9a3f161949b79586138e4341c5fbcc77296d5f4b13ff0183b6efaac1baea8a6d9304566f25517018d4f67b6d407105a36971a03f4077697d53f17ff
e2d3372e558d227aa6dd604b67724685d62e547e lint: Disable signature output in git log (Hodlinator)
Pull request description:
Necessary for users that have signature output enabled by default, since the script would stumble on them and error out.
---
### Testing setup
Set local repo config to show signatures in log by default, simulating a user having that setting turned on globally.
```
₿ git config set log.showSignature true
```
### Command under test
```
₿ ( cd ./test/lint/test_runner/ && COMMIT_RANGE='HEAD^..HEAD' cargo run )
```
#### Before
```
...
fatal: invalid object name 'gpg'.
Traceback (most recent call last):
File "/home/hodlinator/bitcoin/test/lint/lint-git-commit-check.py", line 52, in <module>
main()
File "/home/hodlinator/bitcoin/test/lint/lint-git-commit-check.py", line 42, in main
commit_info = check_output(["git", "log", "--format=%B", "-n", "1", hash], text=True, encoding="utf8").splitlines()
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 466, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/nix/store/wfbjq35kxs6x83c3ncpfxdyl5gbhdx4h-python3-3.12.6/lib/python3.12/subprocess.py", line 571, in run
raise CalledProcessError(retcode, process.args,
subprocess.CalledProcessError: Command '['git', 'log', '--format=%B', '-n', '1', 'gpg: Signature made ons 11 dec 2024 10:46:34 CET']' returned non-zero exit status 128.
^---- ⚠️ Failure generated from lint-git-commit-check.py
...
```
#### After
(No failure generated by *lint-git-commit-check.py*).
ACKs for top commit:
maflcko:
lgtm ACK e2d3372e558d227aa6dd604b67724685d62e547e
willcl-ark:
ACK e2d3372e558d227aa6dd604b67724685d62e547e
Tree-SHA512: 584ccece1e6e0f4691683a2b1816eff33b88f48e9ead9272e2dc73ea9c637b182632108fbeddea1ffc8ed6ba5a5838d7eac7a9f33dfda5bdf325dd7a41e43365
b7ec69c25cf316c21bafa075dd895bf90dc4e59b depends: add -g to *BSD_debug flags (fanquake)
Pull request description:
To match the other HOST_debug_flags. Pulled out of #29796.
ACKs for top commit:
theuni:
utACK b7ec69c25cf316c21bafa075dd895bf90dc4e59b
Tree-SHA512: 654a6dc2c1e295021380f18565379ccde5c5bebcbb5e48ab0364aa79c6f15d301b4acf058d75629a4b217483c6788a0ecb60560e8701882e09490b92c4c346d0
f6496a838828aa6d5a2a40380c2338fda2e0d812 guix: disable gcov in base-linux-gcc (fanquake)
Pull request description:
In a `x86_64-linux-gnu` build, this drops:
```bash
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov-dump
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov-tool
x86_64-linux-gnu/lib/gcc/x86_64-linux-gnu/12.4.0: libgcov.a
```
For mingw-w64-gcc, `--disable-gcov` is currently passed for this target in Guix, due to issues with mingw-w64, see
8bed031e58/gnu/packages/gcc.scm (L99-L102). However we'll add it in any case, in case it's re-enabled in future, when the underlying issues are fixed.
ACKs for top commit:
TheCharlatan:
ACK f6496a838828
Tree-SHA512: ad6de53f63e7bb658cac05fb023fb1f8e76103073c7dffb4267412d3046148e1389df8848010128c1bd3d428f05e1587b656ef2cad8c7d9078ebec83a68bad49
c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 test: Add missing %c character test (Hodlinator)
76cca4aa6fcd363dee821c837b1b627ea137b7a4 test: Document non-parity between tinyformat and ConstevalFormatstring (Hodlinator)
533013cba20664e3581a8e4633cc188d5be3175a test: Prove+document ConstevalFormatString/tinyformat parity (Hodlinator)
b81a4659950a6c4e22316f66b55cae8afc4f4d9a refactor test: Profit from using namespace + using detail function (Hodlinator)
Pull request description:
Clarifies and puts the extent of parity under test.
Broken out from #30546 based on https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263 and https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304.
ACKs for top commit:
maflcko:
re-ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1 🗜
l0rinc:
ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1
ryanofsky:
Code review ACK c93bf0e6e2cd0e9dc979ed996e41cc1034bc1ad1. Just a few cleanups tweaking function declarations and commit comments and consolidating some test cases since last review.
Tree-SHA512: 5ecc893b26cf2761c0009861be392ec4c4fceb0ef95052a2f6f9df76b2e459cfb3f9e257f61be07c3bb2ecc6e525e72c5ca853be1f63b70b52785323d3db6b42
988721d37a3c65e4ef86945133207fda243f2fba test: avoid internet traffic in rpc_net.py (Sebastian Falbesoner)
Pull request description:
In order to avoid connecting to the internet in the functional test `rpc_net.py`, specify a non-working proxy (parameter `-proxy=127.0.0.1:1`, same approach as in #31142) for the nodes. There is at least one known instance where this is currently happening on master where a connection attempt to a public IP is made (see also the discussion in #31339):
17834bd197/test/functional/rpc_net.py (L253)
Can be tested by running
```
$ sudo tcpdump -i eth0 host 11.22.33.44
```
both on master and the PR branch and verifying that no packets appear in the tcpdump in the latter anymore.
ACKs for top commit:
achow101:
ACK 988721d37a3c65e4ef86945133207fda243f2fba
tdb3:
ACK 988721d37a3c65e4ef86945133207fda243f2fba
vasild:
ACK 988721d37a3c65e4ef86945133207fda243f2fba
Tree-SHA512: 0f51fedccbfac0f80a7e6f9c5ba9193d0c20b5a788553c7cd7e583225df7b1151b86cd848d6ccf61f7b2de848f0ac98d73d7b5db100aa54fe8cbeeb4c0549106
ec777917d6eba0b417dbc90b9b891240a44b7ec4 test: Fix intermittent issue in wallet_backwards_compatibility.py (Randall Naar)
Pull request description:
When creating and replacing a transaction using `bumpfee`, an async update is sent in the form of the `TransactionAddedToMempool` and `TransactionRemovedFromMempool` signals. When `wallet_backwards_compatibility.py` creates `tx3_id` this way and replaces it with `tx4_id`, the `abandontransaction` rpc is called right after. In some cases the `TransactionAddedToMempool` and `TransactionRemovedFromMempool` is handled after the transaction is abandoned in the wallet, and overwrites the transaction's `abandoned` flag. This PR forces the signals to get handled before `abandontransaction` is called by invoking `self.sync_mempools` which calls `syncwithvalidationinterfacequeue` on every node's rpc connection.
This will mitigate the immediate inconsistency observed with the abandontransaction call, but the potential race conditions between the signals and wallet operations may also be useful to note in a separate issue (if it's okay to not address it in this one).
Fixes#29806
ACKs for top commit:
achow101:
ACK ec777917d6eba0b417dbc90b9b891240a44b7ec4
tdb3:
ACK ec777917d6eba0b417dbc90b9b891240a44b7ec4
Tree-SHA512: e75bc2c1f7fefc4f4910bb353654848fed5661c1436416798a5f4e0c5a76bde15617a5af04c2384464005953326317b8f273039e47508d5124677908cf36d31e
bb7e686341e437b2e7aae887827710918c00ae0f fuzz: add cstdlib to FuzzedDataProvider (fanquake)
Pull request description:
Same as https://github.com/llvm/llvm-project/pull/113951.
Avoids compile failures under clang-20 & `D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`:
```bash
In file included from /bitcoin/src/test/fuzz/addition_overflow.cpp:5:
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:209:5: error: use of undeclared identifier 'abort'
209 | abort();
| ^
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:250:5: error: use of undeclared identifier 'abort'
250 | abort();
```
ACKs for top commit:
dergoegge:
utACK bb7e686341e437b2e7aae887827710918c00ae0f
brunoerg:
ACK bb7e686341e437b2e7aae887827710918c00ae0f
Tree-SHA512: 22efd5505273ec7254e8dccbb275e648fe02107397c45eff6752e4a6ea787d9d2e45eb0f2ee309df431e9b92ffd14cbcba4b0f4b11a127664466e20be43c383e
Non-HD keys in legacy wallets without a HD seed ID were being migrated
to separate pk(), pkh(), sh(wpkh()), and wpkh() descriptors for each key.
These could be more compactly represented as combo() descriptors, so
migration should make combo() for them.
It is possible that existing non-HD wallets that were migrated, or
wallets that started blank and had private keys imported into them have
run into this issue. However, as the 4 descriptors produce the same output
scripts as the single combo(), so any previously migrated wallets are
not missing any output scripts. The only observable difference should be
performance related, and the wallet size on disk.
cdd207c0e48081ab13e2c8c9886f3e2d5da1857e test: add coverage for migrating standalone imported keys (furszy)
297a876c9809e267e37481fc776cbec90056b078 test: add coverage for migrating watch-only script (furszy)
932cd1e92b6d39c6879f546867698bc8441d09cd wallet: fix crash during watch-only wallet migration (furszy)
Pull request description:
The crash occurs because we assume the cached scripts structure will not be empty,
but it can be empty for watch-only wallets that start blank.
This also adds test coverage for standalone imported keys, which were also crashing
because pubkey imports are treated the same way as hex script imports through
`importaddress()`.
Testing Notes:
This can be verified by cherry-picking and running any of the test commits on master.
It will crash there but pass on this branch.
ACKs for top commit:
theStack:
re-ACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
brunoerg:
reACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
achow101:
ACK cdd207c0e48081ab13e2c8c9886f3e2d5da1857e
Tree-SHA512: e05c77cf3e9f35f10f122a73680b3f131f683c56685c1e26b5ffc857f95195b64c8c9d4535960ed3d6f931935aa79b0b1242537462006126bdb68251f0452954
Same as https://github.com/llvm/llvm-project/pull/113951.
Avoids compile failures under clang-20 &
`D_LIBCPP_REMOVE_TRANSITIVE_INCLUDES`:
```bash
In file included from /bitcoin/src/test/fuzz/addition_overflow.cpp:5:
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:209:5: error: use of undeclared identifier 'abort'
209 | abort();
| ^
/bitcoin/src/test/fuzz/FuzzedDataProvider.h:250:5: error: use of undeclared identifier 'abort'
250 | abort();
```
In a `x86_64-linux-gnu` build, this drops:
```bash
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov-dump
x86_64-linux-gnu/bin/x86_64-linux-gnu-gcov-tool
x86_64-linux-gnu/lib/gcc/x86_64-linux-gnu/12.4.0: libgcov.a
```
For mingw-w64-gcc, `--disable-gcov` is currently passed for this
target in Guix, due to issues with mingw-w64, see
8bed031e58/gnu/packages/gcc.scm (L99-L102).
However we'll add it in any case, in case it's re-enabled in future,
when the underlying issues are fixed.
31e59d94c67b58f24dd701fc7ccfee7d79f311cb iwyu: Drop backported mapping (Hennadii Stepanov)
fe9bc5abef3dcbe0f6395c3233f9d122579cd1f0 ci: Update Clang in "tidy" job (Hennadii Stepanov)
Pull request description:
This PR switches to the latest [IWYU 0.23](https://github.com/include-what-you-use/include-what-you-use/releases/tag/0.23), which is compatible with Clang 19.
New "bugprone-use-after-move" and "modernize-use-starts-ends-with" warnings that emerged have been addressed.
ACKs for top commit:
maflcko:
lgtm ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb
l0rinc:
ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb
theuni:
ACK 31e59d94c67b58f24dd701fc7ccfee7d79f311cb
Tree-SHA512: ae0ca150673e1bfa78664f2ef35dbc965094b32374cafeeae390c6d368c28169a7f7790debe9a6eeb5efc39c9a468f5032d92f30cc4032b09d8265f6a75de882
- For "%n", which is supposed to write to the argument for printf.
- For string/integer mismatches of width/precision specifiers.
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
Also adds BOOST_CHECK_NO_THROW() while touching that line, clarifying part of what we are checking for.
Also removed redundant inline from template functions in .cpp file.
The crash occurs because we assume the cached scripts
structure will not be empty, but it can be empty when
the legacy wallet contained only watch-only and
solvable but not spendable scripts
0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 scripted-diff: Replace strprintf(Untranslated) with Untranslated(strprintf) (Ryan Ofsky)
006e4d1d5984d841c9ac0a6f3c40cfd51e774eda refactor: Use + instead of strformat to concatenate translated & untranslated strings (Ryan Ofsky)
831d2bfcf94117957a90f60fa5bc84a53bb61f7c refactor: Don't embed translated string in untranslated string. (Ryan Ofsky)
058021969b542fc865d17d22fa21e48c9abe4a6e refactor: Avoid concatenation of format strings (Ryan Ofsky)
Pull request description:
This PR cleans up string formatting in the codebase so other PRs adding compile time checking can be simpler and easier to review (specifically #30928, #31061, #31074, and #31149).
Currently these PRs are hard to review because in addition to changing formatting APIs, they have to update callers that are using the API's in unusual ways. Clean up these callers now so later PRs can be simpler. Specifically:
- Use string literals instead of `std::string` format strings to enable more compile-time checking.
- Avoid using untranslated bilingual strings as format strings. Use originals so they can by checked at compile time.
- Favor `Untranslated(strprintf(...))` over `strprintf(Untranslated(...), ...)` for consistency and to prevent translated and untranslated strings from being unintentionally combined.
ACKs for top commit:
maflcko:
lgtm ACK 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 🔹
l0rinc:
ACK 0184d33b3d28fe78a2ee228417ebfd6f46fcaee5 - no overall difference because of the rebase
Tree-SHA512: 37eb771187d30977f5f054eddb82af6dd69878ace89cede72032bb389e57200898119f9fe486ce6903ebb00cb24648be215ab3e44842b3d206b35e26038da750
7239ddb7cec38ab5a8aca93c18fb9efa6376421d test: make sure node has all transactions (brunoerg)
ee1b9bef000b7fb42a452faf06c6748a59ac02c7 test: replace `is not` to `!=` when comparing block hash (brunoerg)
Pull request description:
`test_invalid_tx_in_compactblock` tests that we don't get disconnected if we relay a compact block with valid header, but invalid transactions.
In this test, after sending the block with invalid transactions, this test checks two things: the tip in the receiver node did not advance and the sender did not get disconnected. However, even if the block contains only valid transactions, the tip would not advance because the receiver does not have all transactions to reconstruct the valid and would request them back. This PR fixes it by sending all the transactions.
Also, comparing block hash (int) using `is not` can lead to subtle bugs, this PR fixes it by replacing it to `!=`.
--------------
Can be tested by applying:
```diff
diff --git a/test/functional/p2p_compactblocks.py b/test/functional/p2p_compactblocks.py
index 274ef9532c..419153a32f 100755
--- a/test/functional/p2p_compactblocks.py
+++ b/test/functional/p2p_compactblocks.py
@@ -723,11 +723,8 @@ class CompactBlocksTest(BitcoinTestFramework):
utxo = self.utxos[0]
block = self.build_block_with_transactions(node, utxo, 5)
- del block.vtx[3]
block.hashMerkleRoot = block.calc_merkle_root()
# Drop the coinbase witness but include the witness commitment.
- add_witness_commitment(block)
- block.vtx[0].wit.vtxinwit = []
block.solve()
# Make sure node has the transactions to reconstruct the block
```
ACKs for top commit:
instagibbs:
ACK 7239ddb7cec38ab5a8aca93c18fb9efa6376421d
glozow:
ACK 7239ddb7cec38ab5a8aca93c18fb9efa6376421d
lucasbalieiro:
Tested ACK for commit [7239ddb](7239ddb7ce)
Tree-SHA512: 6d04fb7c50b5e635c83ede75c12130cbd8e1b229887a86a2e1bfe747e4208731faecc7265cae063c1ace187b20c5f37080d5116760766fa2948f38971e5f6fbf
fae76393bdbf867176e65447799d6ee3d3567b18 test: Avoid F541 (f-string without any placeholders) (MarcoFalke)
Pull request description:
An extra `f` string-prefix is mostly harmless, but could be confusing or hint to a mistake where a format argument was forgotten.
Try to avoid the confusion and mistakes by applying the `F541` linter rule.
ACKs for top commit:
lucasbalieiro:
**Tested ACK** [fae7639](fae76393bd)
danielabrozzoni:
ACK fae76393bdbf867176e65447799d6ee3d3567b18
tdb3:
Code review ACK fae76393bdbf867176e65447799d6ee3d3567b18
Tree-SHA512: 4992a74fcf0c19b32e4d95f7333e087b4269b5c5259c556789fb86721617db81c7a4fe210ae136c92824976f07f71ad0f374655e7008b1967c02c73324862d9a
97a18c85458b898fe5e3abda9528a2de442766ad cmake: Fix `IF_CHECK_PASSED` option handling (Hennadii Stepanov)
Pull request description:
`IF_CHECK_PASSED` is a multi-value keyword, resulting in a list value. Convert it to a string before applying any `string()` command.
Split from https://github.com/bitcoin/bitcoin/pull/30861.
No current CMake code is affected by this bug.
ACKs for top commit:
theuni:
utACK 97a18c85458b898fe5e3abda9528a2de442766ad
Tree-SHA512: d2556ca38c35a8992175e9f948c2028a789e71c2b2d5fdf365b31710c8ed3d5edf5d0363853c5d750d29abb58cfda3c78cdc2971a627e5b4c61aca4ec2a33356
edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf util: use explicit cast in MultiIntBitSet::Fill() (Vasil Dimov)
Pull request description:
The current code does not have a bug, but is implicitly casting -1 to 65535 and the sanitizer has no way to know whether we intend that or not.
```
FUZZ=bitset src/test/fuzz/fuzz /tmp/fuz
error: implicit conversion from type 'int' of value -1 (32-bit, signed)
to type 'value_type' (aka 'unsigned short') changed the value to 65535
(16-bit, unsigned)
Base64: Qv7bX/8=
```
https://api.cirrus-ci.com/v1/task/5685829642747904/logs/ci.log
ACKs for top commit:
sipa:
ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
maflcko:
lgtm ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
Empact:
ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
tdb3:
code review ACK edb41e4814ccc2c06a5694b2d2632dbbd22bc0cf
Tree-SHA512: a53835d654d9a7246ec0dab30fa5fbc08155dadb40d9bee3297060aa90816e0ce3d3e92dbdcd7af9474446d842d03f2781b7645a68ffef7fb5fc32ee02545112
55347a5018b2c252c56548f0a6dc1e88e42f66b6 test: Rework migratewallet to use previous release (v28.0) (Ava Chow)
f42ec0f3bfbea8c2cd0c02f4b4d64b71ded6d081 wallet: Check specified wallet exists before migration (Ava Chow)
Pull request description:
This PR reworks wallet_migration.py to use previous releases to produce legacy wallets for testing so that the test will continue to work once legacy wallets are removed.
Split from #28710
ACKs for top commit:
maflcko:
re-ACK 55347a5018b2c252c56548f0a6dc1e88e42f66b6 🥊
rkrux:
re-ACK 55347a5
Tree-SHA512: f90a2f475febc73d29e8ad3cb20d134c368a40a3b5934c3e4aaa77ae704af6314d4dd2e85c261142bd60a201902ac4ba00b8e2443d3cef7c8cc45d23281fa831
cccca8a77f3c1b22fb0ea825ca92aebd63b16d77 test: Avoid logging error when logging error (MarcoFalke)
Pull request description:
Currently a logging error in the form of `--- Logging error ---` happens when an error is logged in the `_on_data` helper.
Fix it by properly logging the error.
Also, treat pylint errors as errors, to avoid this problem in the future.
Can be tested by running `p2p_addrv2_relay.py` with the following example diff:
```diff
diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py
index 523e1bd068..0f1eb29d13 100755
--- a/test/functional/test_framework/p2p.py
+++ b/test/functional/test_framework/p2p.py
@@ -137,7 +137,7 @@ MESSAGEMAP = {
b"notfound": msg_notfound,
b"ping": msg_ping,
b"pong": msg_pong,
- b"sendaddrv2": msg_sendaddrv2,
+ #b"sendaddrv2": msg_sendaddrv2,
b"sendcmpct": msg_sendcmpct,
b"sendheaders": msg_sendheaders,
b"sendtxrcncl": msg_sendtxrcncl,
ACKs for top commit:
fanquake:
ACK cccca8a77f3c1b22fb0ea825ca92aebd63b16d77
Tree-SHA512: dd19f3feed0093246cb205903529fb9ebd5ad9a6c9330cfc5987c0154253c9dcec8d0e25ff99e4ac806a464ff58c3787a205378b8dfb7a1a521da25eac429136
f6afca46a1d7936d8d63439c64050c27cadab624 lint: use clearer wording on error message (willcl-ark)
811a65d3c6b35a91ccd437fa81ddd36bdd28a07b lint: bump MLC to v0.19.0 (willcl-ark)
Pull request description:
Fixes: #31044
This MLC update includes a change which will ignore files being ignored by git, and help avoid false-positives when linting in this repo.
Top commit has no ACKs.
Tree-SHA512: d3edd0125f719c7a4456f7089e298dc851352a082b8119bbd8d642de518bb193827af9994ba416dd18a6a6f1359ee96122d95a31232da1623c679db39b370370
The current code does not have a bug, but is implicitly casting -1 to
65535 and the sanitizer has no way to know whether we intend that or
not.
```
FUZZ=bitset src/test/fuzz/fuzz /tmp/fuz
error: implicit conversion from type 'int' of value -1 (32-bit, signed)
to type 'value_type' (aka 'unsigned short') changed the value to 65535
(16-bit, unsigned)
Base64: Qv7bX/8=
```
This change switches to the latest IWYU 0.23, which is compatible with
Clang 19.
Fixed new "modernize-use-starts-ends-with" warnings.
The new "bugprone-use-after-move" warning in `result_tests.cpp` is a
false positive caused by a bug in Boost.Test versions < 1.87. This has
been addressed by introducing a local variable.
See upstream references:
- Issue: https://github.com/boostorg/test/issues/343
- Fix: https://github.com/boostorg/test/pull/348
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
0f84cdd26614a229d9aee6cd74a1aa01b204a1f5 func: test orphan parent is re-requested from 2nd peer (Greg Sanders)
Pull request description:
Small test which I couldn't find coverage for.
ACKs for top commit:
glozow:
lgtm ACK 0f84cdd26614a229d9aee6cd74a1aa01b204a1f5
tdb3:
code review ACK 0f84cdd26614a229d9aee6cd74a1aa01b204a1f5
theStack:
ACK 0f84cdd26614a229d9aee6cd74a1aa01b204a1f5
marcofleon:
tACK 0f84cdd26614a229d9aee6cd74a1aa01b204a1f5. Removing `node.bumpmocktime(GETDATA_TX_INTERVAL)` results in failure.
Tree-SHA512: fe8cb9d56aabc8f2ef1f49b6cd4e87e28a51ada8070c698f60c5fd945a28d849f0c5793f2e3e29f013e610168b860e0bf1c0aa010eec5b339688269d2b9e69af
