86e3a0a8cb refactor: standardize obfuscation memory alignment (Lőrinc)
13f00345c0 refactor: write `Obfuscation` object when new key is generated in dbwrapper (Lőrinc)
e5b1b7c557 refactor: rename `OBFUSCATION_KEY_KEY` (Lőrinc)
298bf95105 refactor: simplify `Obfuscation::HexKey` (Lőrinc)
2dea045425 test: make `obfuscation_serialize` more thorough (Lőrinc)
a17d8202c3 test: merge xor_roundtrip_random_chunks and xor_bytes_reference (Lőrinc)
Pull request description:
Follow up for https://github.com/bitcoin/bitcoin/pull/31144
Applied the remaining comments in separate commits - except for the last one where I could group them.
Please see the commit messages for more context.
ACKs for top commit:
achow101:
ACK 86e3a0a8cb
ryanofsky:
Code review ACK 86e3a0a8cb, just tweaking key write assert as suggested
hodlinator:
ACK 86e3a0a8cb
Tree-SHA512: 967510a141fbb57bf9d088d92b554cf2fffc2f6aa0eab756cbae3230f53e9b04ceebcc6fea5f3383c01ad41985ecde5b5686c64a771ca9deae3497b9b88c1c8b
18d1071dd1 init: replace deprecated PermissionsStartOnly systemd directive (Florian Schmaus)
1caaf65043 init: remove Group= as it will default to the user's default group (Florian Schmaus)
Pull request description:
> This removes the redundant 'Group=' directive and replaces the deprecated 'PermissionsStartOnly' directive.
Picks up #16994 / #19513. The concern in both of these PRs was changing this too early, while systemd v240 was still prelevant on supported systems. That was ~5 years ago, and from what I can see, no modern/supported OS is still using an older systemd.
Separately , I am wondering if we should move these files out to https://github.com/bitcoin-core/packaging/.
ACKs for top commit:
willcl-ark:
reACK 18d1071dd1
Tree-SHA512: a994e38099e68e8377ac820d3cd2047cbfca065ba617eff0d621e3c3b99b05bbd2329631aa8c885a83cf5d0066d97ff5be75bf5834e9f759d8f0d2c6c9b64851
fdbade6f8d kernel: create monolithic kernel static library (Cory Fields)
Pull request description:
Currently, consuming `libbitcoinkernel.a` requires all its dependency static libraries to be available. A switch to a monolithic variant, which contains object files from its dependencies, was discussed in the Kernel WG. The necessary preparations in the libsecp256k1 build scripts were completed in https://github.com/bitcoin-core/secp256k1/pull/1678, which are now available in this repository since https://github.com/bitcoin/bitcoin/pull/33036.
The changes in this PR were picked from https://github.com/theuni/bitcoin/commits/static_kernel/, with an additional adjustment in `libbitcoinkernel.pc.in`.
This PR can be tested as described in https://github.com/bitcoin/bitcoin/pull/30814#issue-2505698234.
ACKs for top commit:
TheCharlatan:
ACK fdbade6f8d
stickies-v:
tACK fdbade6f8d
Tree-SHA512: bd9e9dbb0b765bdcb162fb3f4ad3c4e01fe5fa0b7061f97d0bad64442b21db036cbe0e4341fd45d43a8862df76d62c9532ca8945f76423aca753c6b528f70873
b093a19ae2 cmake: Proactively avoid use of `SECP256K1_DISABLE_SHARED` (Hennadii Stepanov)
eb59a192d9 cmake, refactor: Encapsulate adding secp256k1 subtree in function (Hennadii Stepanov)
Pull request description:
The `SECP256K1_DISABLE_SHARED` CMake variable has been [removed](https://github.com/bitcoin-core/secp256k1/pull/1688) upstream.
This PR removes its usage ahead of the next `secp256k1` subtree update to prevent breakage and simplify integration.
ACKs for top commit:
fanquake:
ACK b093a19ae2
Tree-SHA512: a87cee71cf356f458f68d3163253ca5c4f86e56d268006b6b8e1d4b2c009ba436148a07a6b67b89ddbb2d0e3c1113ab4b4906c5fc5624cb3082b20e916e0e82b
3543bfdfec test: Fix 'getdescriptoractivity' RPCHelpMan, add test to verify 'spend_vin' is the correct field (Chris Stewart)
Pull request description:
Fixes bug in `getdescriptoractivity` RPC help manual.
Here is the line that pushes `spend_vin` field, there is no `spend_vout` json field.
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/blockchain.cpp#L2757
ACKs for top commit:
nervana21:
tACK 3543bfd
luke-jr:
utACK 3543bfdfec
jonatack:
ACK 3543bfdfec
Tree-SHA512: 2cd543569a87261d8d804d9afe36f8e8ead55839c01da9c4831aea3ced7d1251e6885621e628898105700aae4d76cbb8a682f518f33c1c52163e66f75ec87a61
a26fbee38f qt: Translations update (Hennadii Stepanov)
ca04eebd72 cmake: Switch to generated `ts_files.cmake` file (Hennadii Stepanov)
95341de6ca cmake, refactor: Move handling of Qt TS files into `locale` directory (Hennadii Stepanov)
Pull request description:
This PR:
1. Moves handling of Qt TS files into the `locale` directory.
2. Switches from inferior globbing to the explicit file list generated by the [`bitcoin-maintainer-tools/update-translations.py`](https://github.com/bitcoin-core/bitcoin-maintainer-tools/blob/main/update-translations.py) script.
Closes#32653.
ACKs for top commit:
fanquake:
ACK a26fbee38f
Tree-SHA512: 10596768c120d9da21a2340b693c5f39e9e1e02976805e14284cf9785780756f953d73d3d2b4a7246ada37acafe3e5d0e8927a8f51bf9fce3bc93a3544231489
fa1d2f6380 ci: Pass CI_FAILFAST_TEST_LEAVE_DANGLING into container (MarcoFalke)
Pull request description:
After commit fd813bf863, the env var `CI_FAILFAST_TEST_LEAVE_DANGLING` is no longer passed into the container.
This is harmless, because it isn't needed for the Linux containers and macos doesn't use containers at all.
However, it would be nice to document it as an allowed setting and consistently pass it on, when set. So do that here.
ACKs for top commit:
fanquake:
ACK fa1d2f6380
Tree-SHA512: b61780a27f4c2e11359827b1360a34a132e15bff94f358cbf4d453805afcafb873e2c395908b17610fae026ead93226c19a8f6be40fbb87b623ee992f3e2b43e
3333d3f75f ci: Only pass documented env vars (MarcoFalke)
Pull request description:
The CI currently inherits almost all env vars from the host. This was problematic in the past and causing non-determinism, e.g. the fix in commit fa12558d21. It is still problematic today, see e.g. https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2586828644, or https://github.com/bitcoin/bitcoin/issues/32935
This fixes https://github.com/bitcoin/bitcoin/issues/32935 by only passing env vars documented in `./ci/test/00_setup_env.sh`.
Implementation-wise, instead of cramming the python code into the `python -c ""` statement, just start a fresh py file, which is easier to handle.
ACKs for top commit:
willcl-ark:
ACK 3333d3f75f
Tree-SHA512: f922e481a844128d7fbf773563278a3992c178ead60a3050eceb9ded2aad979afc815a5cbdb9f68494493c5d8d942cdd1111c21e32a5746d19505b87745cb84a
ad132761fc [allocators] Apply manual ASan poisoning to PoolResource (dergoegge)
Pull request description:
Currently ASan will not detect use-after-free issues for memory allocated by a `PoolResource`. This is because ASan is only aware of the memory chunks allocated by `PoolResource` but not the individual "sub-chunks" within.
E.g. this test will not produce an ASan error even though the referenced coin has been deallocated:
```c++
diff --git a/src/test/coins_tests.cpp b/src/test/coins_tests.cpp
index c46144b34b..aa6ca15ce1 100644
--- a/src/test/coins_tests.cpp
+++ b/src/test/coins_tests.cpp
@@ -508,6 +508,17 @@ BOOST_FIXTURE_TEST_CASE(updatecoins_simulation_test, UpdateTest)
BOOST_CHECK(spent_a_duplicate_coinbase);
}
+BOOST_AUTO_TEST_CASE(asan_uaf)
+{
+ CCoinsMapMemoryResource cache_coins_memory_resource{};
+ CCoinsMap map(0, SaltedOutpointHasher(/*deterministic=*/true), CCoinsMap::key_equal{}, &cache_coins_memory_resource);
+ COutPoint outpoint{};
+ map.emplace(outpoint, Coin{});
+ auto& coin = map.at(outpoint);
+ map.erase(outpoint);
+ coin.coin.nHeight = 1;
+}
+
BOOST_AUTO_TEST_CASE(ccoins_serialization)
{
// Good example
```
Fix this by applying [manual ASan poisoning](https://github.com/google/sanitizers/wiki/AddressSanitizerManualPoisoning) for memory allocated by `PoolResource`:
* Newly allocated chunks are poisoned as a whole
* "Sub-chunks" are unpoisoned/re-poisoned during allocation/deallocation
With the poisoning applied, ASan catches the issue in the test above:
```
$ ./build_unit/bin/test_bitcoin --run_test="coins_tests/asan_uaf"
Running 1 test case...
=================================================================
==366064==ERROR: AddressSanitizer: use-after-poison on address 0x7f99c3204870 at pc 0x55569dab6f8a bp 0x7ffe0210e4d0 sp 0x7ffe0210e4c8
READ of size 4 at 0x7f99c3204870 thread T0 (b-test)
```
ACKs for top commit:
achow101:
ACK ad132761fc
marcofleon:
code review ACK ad132761fc
Tree-SHA512: eb5e80bfa9509225e784151807bd8aa21fb0826ca1781dfe81b1d60bd3766019384ea3f9cb8e53398fde2f4e994a9c201b5a9962b4d279d7e52bb60e8961be11
1252eeb997 rpc: fix getpeerinfo ping duration unit docs (0xb10c)
Pull request description:
The docs have been incorrect since a3789c700b (released in v25; master since Sept. 2022). Noticed while setting up monitoring using getpeerinfo.
0cb1ed2b7c/src/rpc/net.cpp (L249-L257)
ACKs for top commit:
luke-jr:
utACK 1252eeb997
maflcko:
lgtm ACK 1252eeb997
jonatack:
ACK 1252eeb997
theStack:
ACK 1252eeb997
janb84:
ACK 1252eeb997
Tree-SHA512: 33f576336b2a4d9533f51f4641d564ee59ef692c5fa9a3cad239fc31465883d5da534bfd0e069be1e1d688e5f0dea3fe6850be19bf35335041b8f414d08f7f09
When the detailed peers list is requested, return the shortened services in the
-netinfo header in the same format as the "serv" column, instead of the full names
list in the report.
The getpeerinfo docs incorrectly specified the ping durations as
milliseconds. This was incorrectly changed in a3789c700b
(released in v25; master since Sept. 2022). The correct duration unit
is seconds.
Also, remove the documentation of the getpeerinfo RPC response from the
ping RPC since it's incomplete. Better to just reference the getpeerinfo
RPC and it's documenation for this.
A stripped witness is detected as a special case in mempool acceptance to make sure we do not add
the wtxid (which is =txid since witness is stripped) to the reject filter. This is because it may
interfere with 1p1c parent relay which currently uses orphan reconciliation (and originally it was
until wtxid-relay was widely adopted on the network.
This commit adds a test for this special case in the p2p_segwit function test, both when spending
a native segwit output and when spending a p2sh-wrapped segwit output.
Thanks to Eugene Siegel for pointing out the p2sh-wrapped detection did not have test coverage by
finding a bug in a related patch of mine.
e017ef3c7e init: make `-blockmaxweight` startup option debug-only (ismaelsadeeq)
Pull request description:
This PR updates `-blockmaxweight` startup option to be debug-only so that it will be hidden from help text.
The option is currently unlikely to be used on mainnet, after the addition of the new `blockreservedweight` option. however it can be useful for test and signet network see https://github.com/bitcoin/bitcoin/pull/32654#issuecomment-2925674473
ACKs for top commit:
Sjors:
tACK e017ef3c7e
fjahr:
ACK e017ef3c7e
polespinasa:
tACK e017ef3c7e
Tree-SHA512: 6c18781826b2f96b13b70b7f1624481f5971746a613079d0d9528366f274ba657a02611f134d7a64f35ecb7e5faf2e3cd025458b04574ac68f804372f6eb715f
a3cf623364 test: Test max_selection_weight edge cases (Murch)
57fe8acc8a test: Check max_weight_exceeded error (Murch)
Pull request description:
I tested all of the reported surviving mutants that @brunoerg reported in https://gist.github.com/brunoerg/834063398d5002f738506d741513e310.
I found that all Mutants except for 12, 14, 17, 37, and 39 were now being caught by one of the existing tests. This fixes Mutants 14, 37, and 39.
Mutant 17 is not fixed, because I consider it acceptable that running BnB for 100,001 instead of 100,000 comparisons doesn’t cause an issue, and Mutant 12 is not yet fixed, because at `fee` = `long_term_fee`, the waste of inputs is 0 and only excess matters, and I haven’t evaluated yet, whether it needs to be fixed.
ACKs for top commit:
achow101:
ACK a3cf623364
jlest01:
ACK a3cf623364
brunoerg:
code review ACK a3cf623364
Tree-SHA512: db67c52127ed98f809f64a903c6b3a012e56cf665a0cd851457af7c85c37ec3af8bb72035d7ad370dd883f99cf3014464e3576559899e37c1d6ee01230511754
This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if Ctrl-C is pressed when `bitcoin-node` is started, it is
handled by both `bitcoin-node` and `bitcoin-wallet` processes, causing the
wallet to shutdown abruptly instead of waiting for the node and shutting down
cleanly.
This change fixes the problem by having the wallet process print to stdout when
it receives a Ctrl-C signal but not otherwise react, letting the node shut
everything down cleanly.
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.
Use EventLoopRef to avoid reference counting bugs and be more exception safe
and deal with removal of addClient/removeClient methods in
https://github.com/bitcoin-core/libmultiprocess/pull/160
A test update is also required due to
https://github.com/bitcoin-core/libmultiprocess/pull/160 to deal with changed
reference count semantics. In IpcPipeTest(), it is now necessary to destroy
the client Proxy object instead of just the client Connection object to
decrease the event loop reference count and allow the loop to exit so the test
does not hang on shutdown.
cc33e45789 test: improve assertion for SRD max weight test (yancy)
Pull request description:
Replace generic assertion with a result specific assertion showing the correctness of the solution found. If the max weight parameter is exceeded, the least valuable `UTXOs` are removed from the result. Therefore, only the most valued _encountered_ `UTXO's` are selected. While the smallest set would include all the most valued `UTXO's`, in the case of the test there is one high value `UTXO` that is never found before the target value is reached.
Correct the test comment to be more specific about why the assertion is a good result.
ACKs for top commit:
murchandamus:
ACK cc33e45789
furszy:
ACK cc33e45789
Tree-SHA512: bad224063ba830c27fba1b7b80e411ac7cd6c3edcb60bade4e6e3010f3b5d360a921de742c7c20efea8fa839d7939f338270658f66bbcebedebe5c5c8a3e8f9b
c0642e558a [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92 scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b92448923 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c6704 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f1 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e01696 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3 [doc] 31829 release note (glozow)
Pull request description:
Followup to #31829:
- Release notes
- Have the orphanage auto-trim itself whenever necessary (and test changes) https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2169508690
- Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3001627814
- Rename `OrphanTxBase` to `OrphanInfo`
- Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
- Rename outpoints index since they point to wtxids, not iterators https://github.com/bitcoin/bitcoin/pull/31829#discussion_r2205557613
- Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed https://github.com/bitcoin/bitcoin/pull/31829#issuecomment-3074385460
ACKs for top commit:
sipa:
utACK c0642e558a
marcofleon:
Nice, ACK c0642e558a
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
