f49840dd902cd9b14b6aadb431b16a4aeb719c3f doc: Fix typo in files.md (Ryan Ofsky)
f5cf0b1ccc8fd426135809a8a4becdae2d797bb5 bitcoin wrapper: improve help output (Ryan Ofsky)
c810b168b89dc07017e9feaec1a8746a449a60b1 doc: Add description of installed files to files.md (Ryan Ofsky)
94ffd01a0294afbe045f1b17a77e4a3caf21e674 doc: Add release notes describing libexec/ binaries (Ryan Ofsky)
cd97905ebc564b8b095099a28d1d5437951927c4 cmake: Move internal binaries from bin/ to libexec/ (Ryan Ofsky)
Pull request description:
This change moves binaries that are not typically invoked directly by users from the `bin/` directory to the `libexec/` directory in CMake installs and binary releases. The goal of the PR is to introduce a distinction between internal and external binaries so starting with #31802, we can use IPC to implement features in new binaries without adding those binaries to the CLI. The change also helps reduce clutter in `bin/`, making it easier for users to identify useful tools to run. Summary of changes:
- For **source builds** (i.e. developer builds) — There are no changes.
- For **source installs** (i.e. `cmake --install` result) — `test_bitcoin`, `test_bitcoin-qt`, and `bench_bitcoin` are installed in `${CMAKE_PREFIX_PATH}/libexec` instead of `${CMAKE_PREFIX_PATH}/bin`, so they are no longer on the system `PATH`. However, they can still be invoked from the `libexec/` directory, or from the CLI as `bitcoin test`, `bitcoin test-gui`, and `bitcoin bench`, respectively.
- For **binary releases** — Since `test_bitcoin` is the only test binary enabled in releases, the only change is moving `test_bitcoin` from `bin/` to `libexec/`.
<details><summary>Details</summary>
<p>
The table below shows the install location of each binary after this change, and the availability of each binary.
| Binary | Location | Availability | Change |
|----------------------|--------------|----------------------|-------------------------------|
| `bitcoin` | `bin/` | 📦 Binary release (since #31375) | Unchanged |
| `bitcoin-cli` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoind` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-qt` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-tx` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-util` | `bin/` | 📦 Binary release | Unchanged |
| `bitcoin-wallet` | `bin/` | 📦 Binary release | Unchanged |
| `bench_bitcoin` | `libexec/` | 🛠 Source build only | Moved from `bin/` |
| `bitcoin-chainstate` | `libexec/` | 🛠 Source build only | Newly installed (was built) |
| `bitcoin-gui` | `libexec/` | 🛠 Source build only (until #31802) | Moved from `bin/` |
| `bitcoin-node` | `libexec/` | 🛠 Source build only (until #31802) | Moved from `bin/` |
| `test_bitcoin` | `libexec/` | 📦 Binary release | Moved from `bin/` |
| `test_bitcoin-qt` | `libexec/` | 🛠 Source build only | Moved from `bin/` |
</p>
</details>
---
This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/issues/28722).
ACKs for top commit:
l0rinc:
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
Sjors:
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
achow101:
ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
janb84:
re ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
BrandonOdiwuor:
Tested ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
hodlinator:
re-ACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
willcl-ark:
utACK f49840dd902cd9b14b6aadb431b16a4aeb719c3f
Tree-SHA512: 858a2e1a53db11ee3c5c759bfdeea566f242b9ce5e8a898fa435222e41662b8184577c0dc2c4c058294b4de41d8cb3ba3e5d24c748c280efa4a3f84e3ec4344d
86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0 refactor: standardize obfuscation memory alignment (Lőrinc)
13f00345c061a8df2fe41ff9d0a6aadfb6137fd8 refactor: write `Obfuscation` object when new key is generated in dbwrapper (Lőrinc)
e5b1b7c5577ee36b5bcfb6c02b92da88455411e9 refactor: rename `OBFUSCATION_KEY_KEY` (Lőrinc)
298bf9510578263a1439513729e5ff955a453437 refactor: simplify `Obfuscation::HexKey` (Lőrinc)
2dea0454254180d79464dc6afd3312b1caf369a7 test: make `obfuscation_serialize` more thorough (Lőrinc)
a17d8202c36abf8a17fb8736e05f318422a3c7fb 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 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0
ryanofsky:
Code review ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0, just tweaking key write assert as suggested
hodlinator:
ACK 86e3a0a8cbd30cfee98f5b4acf4ce6d0a75a3ef0
Tree-SHA512: 967510a141fbb57bf9d088d92b554cf2fffc2f6aa0eab756cbae3230f53e9b04ceebcc6fea5f3383c01ad41985ecde5b5686c64a771ca9deae3497b9b88c1c8b
ad132761fc49c38769c09653a265fdbc3b93eda5 [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 ad132761fc49c38769c09653a265fdbc3b93eda5
marcofleon:
code review ACK ad132761fc49c38769c09653a265fdbc3b93eda5
Tree-SHA512: eb5e80bfa9509225e784151807bd8aa21fb0826ca1781dfe81b1d60bd3766019384ea3f9cb8e53398fde2f4e994a9c201b5a9962b4d279d7e52bb60e8961be11
c0642e558a02319ade33dc1014e7ae981663ea46 [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0d92d42809e74377e4380abdc70f74de5d scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b924489238220710326e9031c7aaa0d606c9064 [doc] comment fixups for orphanage changes (glozow)
1384dbaf6d0bfcdb05f97e1e3cb3d5e498bee505 [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b298d4d2b7dddfecdbeb0edc624b8e6eb2 fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c67043a2a46950fd3f055afbe4a93922f75 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb3f151600f00c94a2732d2595446011295 [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e87d70580ae47da228e2f88cd53c40c675 scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f0df6e6e2cc9b9aeb3c3c8e1c78fa5be1d [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e016969098c486f413f4f57dcffe35241785 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402ccfa7f19177b5f422f596a3ab2bd1e9633 [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25ff3c3ac090b68e370efc6dd9374b6ad3b [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 c0642e558a02319ade33dc1014e7ae981663ea46
marcofleon:
Nice, ACK c0642e558a02319ade33dc1014e7ae981663ea46
Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a
In the `txgraph` fuzz test, the `CommitStaging` step updates the
`SimTxGraph` levels simply by erasing the front (=main) one in the
`sims` vector, i.e. the staging level instance takes the place of the
main level instance. This also includes the `real_is_optimal` flag
(reflecting whether the corresponding real graph is known to be
optimally linearized), without taking into account that this flag
should only be set if _both_ levels before the commiting are optimal.
E.g. in case of #33097, the main level is not optimally linearized,
while the staging level is, and due to the incorrect propagation of the
latter to the simulation incorrectly assumes that the main level is
optimal, leading to the assertion fail. Fix this by setting the flag
in the resulting main level explicitly.
Resolves the fuzzing assertion fail in issue #33097.
-BEGIN VERIFY SCRIPT-
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/node/txorphanage.h
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/orphanage_tests.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/test/fuzz/txorphan.cpp
sed -i 's/max_global_ann/max_global_latency_score/g' src/bench/txorphanage.cpp
sed -i 's/max_ann/max_lat/g' src/node/txorphanage.cpp
-END VERIFY SCRIPT-
This introduces an invariant that TxOrphanageImpl never holds more than one
announcement with m_reconsider=true for a given wtxid. This avoids duplicate
work, both in the caller might otherwise reconsider the same transaction multiple
times before it is ready, and internally in AddChildrenToWorkSet, which might
otherwise iterate over all announcements multiple times.
ea17a9423fb431a86d36927b02d3624f654fd867 [doc] release note for relaxing requirement of all unconfirmed parents present (glozow)
12f48d5ed302e92a334dbe971c66df467d402655 test: add chained 1p1c propagation test (Greg Sanders)
525be56741cff5f89d596b8a0c44df00f2209bcb [unit test] package submission 2p1c with 1 parent missing (glozow)
f24771af0581e8117bd638227469e37cc69c5103 relax child-with-unconfirmed-parents rule (glozow)
Pull request description:
Broadens the package validation interface, see #27463 for wider context.
On master, package rules include that (1) the package topology must be child-wth-parents (2) all of the child's unconfirmed parents must be present. This PR relaxes the second rule, leaving the first rule untouched (there are plans to change that as well, but not here).
Original motivation for this rule was based on the idea that we would have a child-with-unconfirmed-parents package relay protocol, and this would verify that the peer provided the "correct" package. For various reasons, we're not planning on doing this. We could potentially do this for ancestor packages (with a similar definition that all UTXOs to make the tx valid are available in this package), but it's also questionable whether it's useful to enforce this.
This rule gets in the way of certain usage of 1p1c package relay currently. If a transaction has multiple parents, of which only 1 requires a package CPFP, this rule blocks the package from relaying. Even if all the non-low-feerate parents are already in mempool, when the p2p logic submits the 1p1c package, it gets rejected for not meeting this rule.
ACKs for top commit:
ishaanam:
re-utACK ea17a9423fb431a86d36927b02d3624f654fd867
instagibbs:
ACK ea17a9423fb431a86d36927b02d3624f654fd867
Tree-SHA512: c2231761ae7b2acea10a96735e7a36c646f517964d0acb59bacbae1c5a1950e0223458b84c6d5ce008f0c1d53c1605df0fb3cd0064ee535ead006eb7c0fa625b
62ed1f92efff42bc79c50935e6dbd9da4e072020 txgraph: check that DoWork finds optimal if given high budget (tests) (Pieter Wuille)
f3c2fc867fc4332dfed0a3766997433e1676dbe3 txgraph: add work limit to DoWork(), try optimal (feature) (Pieter Wuille)
e96b00d99ebe27eeadba88841db32b2b8e741433 txgraph: make number of acceptable iterations configurable (feature) (Pieter Wuille)
cfe9958852be0e0763c924bdbadc37e784f5aee5 txgraph: track amount of work done in linearization (preparation) (Pieter Wuille)
6ba316eaa0321faf94eb31769deb18781ff9667c txgraph: 1-or-2-tx split-off clusters are optimal (optimization) (Pieter Wuille)
fad0eb091e58f345b922a49335c60bbeae6d5c6f txgraph: reset quality when merging clusters (bugfix) (Pieter Wuille)
Pull request description:
Part of #30289. Builds on top of #31553.
So far, the `TxGraph::DoWork()` function took no parameters, and just made all clusters reach the "acceptable" internal quality level by performing a minimum number of improvement iterations on it, but:
* Did not attempt to go beyond that.
* Was broken, as the QualityLevel of optimal clusters that merge together was not being reset.
Fix this by adding an argument to `DoWork()` to control how much work it is allowed to do right now, which will first be used to get all clusters to the acceptable level, and if more budget remains, use it to try to get some or all clusters optimal. The function will now return `true` if all clusters are known to be optimal (and thus no further work remains). This is verified in the tests, by remembering whether the graph is optimal, and if it is at the end of the simulation run, verify that the overall linearization cannot be improved further.
ACKs for top commit:
instagibbs:
ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
ismaelsadeeq:
Code review ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
glozow:
ACK 62ed1f92efff42bc79c50935e6dbd9da4e072020
Tree-SHA512: 5f57d4052e369f3444e72e724f04c02004e0f66e365faa59c9f145323e606508380fc97bb038b68783a62ae9c10757f1b628b3b00b2ce9a46161fca2d4336d73
The current `prevector` size of 28 bytes (chosen to fill the `sizeof(CScript)` aligned size) was introduced in 2015 (https://github.com/bitcoin/bitcoin/pull/6914) before SegWit and TapRoot.
However, the increasingly common `P2WSH` and `P2TR` scripts are both 34 bytes, and are forced to use heap (re)allocation rather than efficient inline storage.
The core trade-off of this change is to eliminate heap allocations for common 34-36 byte scripts at the cost of increasing the base memory footprint of all `CScript` objects by 8 bytes (while still respecting peak memory usage defined by `-dbcache`).
Increasing the `prevector` size allows these scripts to be stored inline, avoiding extra heap allocations, reducing potential memory fragmentation, and improving performance during cache flushes. Massif analysis confirms a lower stable memory usage after flushing, suggesting the elimination of heap allocations outweighs the larger base size for common workloads.
Due to memory alignment, increasing the `prevector` size to 36 bytes doesn't change the overall `sizeof(CScript)` compared to an increase to 34 bytes, allowing us to include `P2PK` scripts as well at no additional memory cost.
Performance benchmarks for AssumeUTXO load and flush show:
* Small dbcache (450MB): ~1-3% performance improvement (despite more frequent flushes)
* Large dbcache (4500MB): ~6-8% performance improvement due to fewer heap allocations (and basically the number of flushes)
* Very large dbcache (4500MB): ~5-6% performance improvement due to fewer heap allocations (and memory limit not being reached, so there's no memory penalty)
Full IBD and reindex-chainstate with larger `dbcache` values also show an overall ~3-4% speedup.
Co-authored-by: Ava Chow <github@achow101.com>
Co-authored-by: Andrew Toth <andrewstoth@gmail.com>
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Verifies that script types are correctly allocated using prevector's direct or indirect storage based on their size:
Direct allocated script types (size ≤ 28 bytes):
* OP_RETURN (small)
* P2WPKH
* P2SH
* P2PKH
Indirect allocated script types (size > 28 bytes):
* P2WSH
* P2TR
* P2PK
* MULTISIG (small)
This test provides a baseline for verifying changes to prevector's inline capacity.
The `CHECK_SCRIPT_STATIC_SIZE` and `CHECK_SCRIPT_DYNAMIC_SIZE` macros were added to differentiate the two cases - while preserving the correct source code line in case of failure.
1cb23997033c395d9ecd7bf2f54787b134485f41 doc: clarify the GetAddresses/GetAddressesUnsafe documentation (Daniela Brozzoni)
e5a7dfd79f618b04e0140ec2c50c6e95c2a2e2e4 p2p: rename GetAddresses -> GetAddressesUnsafe (Daniela Brozzoni)
Pull request description:
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this function should only be used in trusted contexts. This helps avoid accidental privacy leaks by preventing the uncached version from being used in non-trusted scenarios, like P2P.
Additionally, better reflect in the documentation that the two methods should be used in different contexts.
Also update the outdated "call the function without a parameter" phrasing in the cached version. This wording was accurate when the cache was introduced in #18991, but became outdated after later commits (f26502e9fc8a669b30717525597e3f468eaecf79, 81b00f87800f40cb14f2131ff27668bd2bb9e551) added parameters to each
function, and the previous commit changed the function naming completely.
ACKs for top commit:
stickies-v:
re-ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
l0rinc:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
luisschwab:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
brunoerg:
ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
theStack:
Code-review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
mzumsande:
Code review ACK 1cb23997033c395d9ecd7bf2f54787b134485f41
Tree-SHA512: 02c05d88436abcdfabad994f47ec5144e9ba47668667a2c1818f57bf8710727505faf8426fd0672c63de14fcf20b96f17cea2acc39fe3c1f56abbc2b1a9e9c23
face8123fdc10549676c6679ee3225c178a7f30c log: [refactor] Use info level for init logs (MarcoFalke)
fa183761cb09d916ed2a3bbab71b80c5c7942a30 log: Remove function name from init logs (MarcoFalke)
Pull request description:
Many of the normal, and expected init logs, which are run once after startup use the deprecated alias of `LogInfo`.
Fix that by using `LogInfo` directly, which is a refactor, except for a few log lines that also have `__func__` removed.
(Also remove the unused trailing `\n` char while touching those logs)
ACKs for top commit:
stickies-v:
re-ACK face8123fdc10549676c6679ee3225c178a7f30c
fanquake:
ACK face8123fdc10549676c6679ee3225c178a7f30c
Tree-SHA512: 28c296129c9a31dff04f529c53db75057eae8a73fc7419e2f3068963dbb7b7fb9a457b2653f9120361fdb06ac97d1ee2be815c09ac659780dff01d7cd29f8480
fa1a14a13a15ecfb7587a94ee86b4ace7c819519 fuzz: Reset chainman state in process_message(s) targets (MarcoFalke)
fa9a3de09b4c6eef40f1073f09c9a0bd1858adf2 fuzz: DisableNextWrite (MarcoFalke)
aeeeeec9f749dddeaf8eaa357b69cd45ed3dd76c fuzz: Reset dirty connman state in process_message(s) targets (MarcoFalke)
fa11eea4059a608f591db4469c07a341fd33a031 fuzz: Avoid non-determinism in process_message(s) target (PeerMan) (MarcoFalke)
Pull request description:
`process_message(s)` are the least stable fuzz targets, according to OSS-Fuzz.
Tracking issue: https://github.com/bitcoin/bitcoin/issues/29018.
### Testing
Needs coverage compilation, as explained in `./contrib/devtools/README.md`. And then, using 32 threads:
```
cargo run --manifest-path ./contrib/devtools/deterministic-fuzz-coverage/Cargo.toml -- $PWD/bld-cmake/ $PWD/../b-c-qa-assets/fuzz_corpora/ process_messages 32
```
Each commit can be reverted to see more non-determinism re-appear.
ACKs for top commit:
marcofleon:
ReACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519
dergoegge:
reACK fa1a14a13a15ecfb7587a94ee86b4ace7c819519
Tree-SHA512: 37b5b6dbdde6a39b4f83dc31e92cffb4a62a4b8f5befbf17029d943d0b2fd506f4a0833570dcdbf79a90b42af9caca44e98e838b03213d6bc1c3ecb70a6bb135
This rule was originally introduced along with a very early proposal for
package relay as a way to verify that the "correct"
child-with-unconfirmed-parents package was provided for a transaction,
where correctness was defined as all of the transactions unconfirmed
parents. However, we are not planning to introduce a protocol where
peers would be asked to send these packages.
This rule has downsides: if a transaction has multiple parents but only
1 that requires package CPFP to be accepted, the current rule prevents
us from accepting that package. Even if the other parents are already in
mempool, the p2p logic will only submit the 1p1c package, which fails
this check. See the test in p2p_1p1c_network.py
06ab3a394ade1e0d4fdb1fef636ff0d6bad71948 tests: speed up coins_tests by parallelizing (Anthony Towns)
Pull request description:
Updates the cmake logic to generate a separate test for each BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp into three separate suites so that they can be run in parallel. Also updates the convention enforced by test/lint/lint-tests.py.
ACKs for top commit:
l0rinc:
reACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
maflcko:
lgtm ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
achow101:
ACK 06ab3a394ade1e0d4fdb1fef636ff0d6bad71948
Tree-SHA512: 940d9aa31dab60d1000b5f57d8dc4b2c5b4045c7e5c979ac407aba39f2285d53bc00c5e4d7bf2247551fd7e1c8681144e11fc8c005a874282c4c59bd362fb467
Rename GetAddresses to GetAddressesUnsafe to make it clearer that this
function should only be used in trusted contexts. This helps avoid
accidental privacy leaks by preventing the uncached version from being
used in non-trusted scenarios, like P2P.
Updates the cmake logic to generate a separate test for each
BOOST_FIXTURE_TEST_SUITE declaration in a file, and splits coins_tests.cpp
into three separate suites so that they can be run in parallel. Also
updates the convention enforced by test/lint/lint-tests.py.
31c4e77a256c15c221dcc278883f6d3be55660b4 test: fix ReadTopologicalSet unsigned integer overflow (ismaelsadeeq)
Pull request description:
This PR is a simple fix for a potential unsigned integer overflow in ReadTopologicalSet.
We obtain the value of `mask` from fuzz input, which can be the maximum representable value.
Adding 1 to it would then cause an overflow.
The fix skips the addition when the read value is already the maximum.
See https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569 for more context
ACKs for top commit:
maflcko:
lgtm ACK 31c4e77a256c15c221dcc278883f6d3be55660b4
Tree-SHA512: f58d7907f66a0de0ed8d4b1cad6a4971f65925a99f3c030537c21c4d84126b643257c65865242caf7d445b9cbb7a71a1816a9f870ab7520625c4c16cd41979cb
96da68a38fa295d2414685739c41b8626e198d27 qa: functional test a transaction running into the legacy sigop limit (Antoine Poinsot)
367147954d16c961bbd28c361abf27b4cb665f10 qa: unit test standardness of inputs packed with legacy sigops (Antoine Poinsot)
5863315e33ba9b75a1e5189ee3da3d7311bbf193 policy: make pathological transactions packed with legacy sigops non-standard. (Antoine Poinsot)
Pull request description:
The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should make transactions that are not valid
according to the new rules non-standard first because it would otherwise be a trivial DoS to
potentially unupgraded miners after the soft fork activates.
ML post: https://gnusha.org/pi/bitcoindev/49dyqqkf5NqGlGdinp6SELIoxzE_ONh3UIj6-EB8S804Id5yROq-b1uGK8DUru66eIlWuhb5R3nhRRutwuYjemiuOOBS2FQ4KWDnEh0wLuA=@protonmail.com/T/#u
ACKs for top commit:
instagibbs:
reACK 96da68a38f
maflcko:
review ACK 96da68a38fa295d2414685739c41b8626e198d27 🚋
achow101:
ACK 96da68a38fa295d2414685739c41b8626e198d27
glozow:
light code review ACK 96da68a38fa, looks correct to me
Tree-SHA512: 106ffe62e48952affa31c5894a404a17a3b4ea8971815828166fba89069f757366129f7807205e8c6558beb75c6f67d8f9a41000be2f8cf95be3b1a02d87bfe9
This is required in the process_message(s) fuzz targets to avoid leaking
the next write time from one run to the next. Also, disable it
completely because it is not needed and due to leveldb-internal
non-determinism.
The PeerManager has several members, such as the FastRandomContext,
which need to be reset before every run to avoid leaking state from one
run into the next.
Also, style fixups in p2p_handshake.cpp, where this code is copied from.
This is meant to focus the usages to narrow the scope of the obfuscation optimization.
`Obfuscation::Xor` is mostly a move.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
Mechanical refactor of the low-level "xor" wording to signal the intent instead of the implementation used.
The renames are ordered by heaviest-hitting substitutions first, and were constructed such that after each replacement the code is still compilable.
-BEGIN VERIFY SCRIPT-
sed -i \
-e 's/\bGetObfuscateKey\b/GetObfuscation/g' \
-e 's/\bxor_key\b/obfuscation/g' \
-e 's/\bxor_pat\b/obfuscation/g' \
-e 's/\bm_xor_key\b/m_obfuscation/g' \
-e 's/\bm_xor\b/m_obfuscation/g' \
-e 's/\bobfuscate_key\b/m_obfuscation/g' \
-e 's/\bOBFUSCATE_KEY_KEY\b/OBFUSCATION_KEY_KEY/g' \
-e 's/\bSetXor(/SetObfuscation(/g' \
-e 's/\bdata_xor\b/obfuscation/g' \
-e 's/\bCreateObfuscateKey\b/CreateObfuscation/g' \
-e 's/\bobfuscate key\b/obfuscation key/g' \
$(git ls-files '*.cpp' '*.h')
-END VERIFY SCRIPT-
The two tests are doing different things - `xor_roundtrip_random_chunks` does black-box style property-based testing to validate that certain invariants hold - that deobfuscating an obfuscation results in the original message (higher level, it doesn't have to know about the implementation details).
The `xor_bytes_reference` test makes sure the optimized xor implementation behaves in every imaginable scenario exactly as the simplest possible obfuscation - with random chunks, random alignment, random data, random key.
Since we're touching the file, other related small refactors were also applied:
* `nullpt` typo fixed;
* manual byte-by-byte xor key creations were replaced with `_hex` factories;
* since we're only using 64 bit keys in production, smaller keys were changed to reflect real-world usage;
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Since 31 byte xor-keys are not used in the codebase, using the common size (8 bytes) makes the benchmarks more realistic.
Co-authored-by: maflcko <6399679+maflcko@users.noreply.github.com>
