db3228042b util: detect and warn when using exFAT on macOS (willcl-ark)
Pull request description:
exFAT is known to cause intermittent corruption on MacOS.
Therefore we should warn when using this fs format for either the blocks or data directories.
See #28552 for more context.
ACKs for top commit:
l0rinc:
ACK db3228042b
marcofleon:
reACK db3228042b
ismaelsadeeq:
reACK db3228042b
Tree-SHA512: e4453a8e24b35c135e4eb0b4e47fe0c80f8b54700f458909c403aa37a0d2979ee165347bcd76e48e4d1ae5d3bae13f50e6afe714e33226a52f907b95df9d3b46
ca64b71ed5 test: fix scripts in `blockfilter_basic_test` (UdjinM6)
Pull request description:
`std::vector` fill ctor is like this:
```
// Constructs a vector with `count` copies of elements with value `value`.
explicit vector( size_type count, const T& value = T(), const Allocator& alloc = Allocator() ); // (until C++11)
vector( size_type count, const T& value, const Allocator& alloc = Allocator() ); // (since C++11)(constexpr since C++20)
```
https://en.cppreference.com/w/cpp/container/vector/vector.html
i.e. `std::vector<unsigned char>(0, 65)` means a vector with `0` copies of `65` which feels wrong. I believe `count` and `value` were swapped in `blockfilter_basic_test` scripts.
ACKs for top commit:
furszy:
ACK ca64b71ed5
pablomartin4btc:
ACK ca64b71ed5
janb84:
ACK ca64b71ed5
Tree-SHA512: 2cfc7f09788b0a1afdffc9cd6663204c7f1775dabdbe1046cdcd42936c479658c348cb46e0d8835645e6c508e8b40a598cbe6534084b6780a6b60378bcbd0f96
83950275ed qa: unit test sighash caching (Antoine Poinsot)
b221aa80a0 qa: simple differential fuzzing for sighash with/without caching (Antoine Poinsot)
92af9f74d7 script: (optimization) introduce sighash midstate caching (Pieter Wuille)
8f3ddb0bcc script: (refactor) prepare for introducing sighash midstate cache (Pieter Wuille)
9014d4016a tests: add sighash caching tests to feature_taproot (Pieter Wuille)
Pull request description:
This introduces a per-txin cache for sighash midstate computation to the script interpreter for legacy (bare), P2SH, P2WSH, and (as collateral effect, but not actually useful) P2WPKH. This reduces the impact of certain types of quadratic hashing attacks that use standard transactions. It is not known to improve the situation for attacks involving non-standard transaction attacks.
The cache works by remembering for each of the 6 sighash modes a `(scriptCode, midstate)` tuple, which gives a midstate `CSHA256` object right before the appending of the sighash type itself (to permit all 256, rather than just the 6 ones that match the modes). The midstate is only reused if the `scriptCode` matches. This works because - within a single input - only the sighash type and the `scriptCode` affect the actual sighash used.
The PR implements two different approaches:
* The initial commits introduce the caching effect always, for both consensus and relay relation validation. Despite being primarily intended for improving the situation for standard transactions only, I chose this approach as the code paths are already largely common between the two, and this approach I believe involves fewer code changes than a more targetted approach, and furthermore, it should not hurt (it may even help common multisig cases slightly).
* The final commit changes the behavior to only using the cache for non-consensus script validation. I'm open to feedback about whether adding this commit is worth it.
Functional tests are included that construct contrived cases with many sighash types (standard and non-standard ones) and `OP_CODESEPARATOR`s in all script types (including P2TR, which isn't modified by this PR).
ACKs for top commit:
achow101:
ACK 83950275ed
dergoegge:
Code review ACK 83950275ed
darosior:
re-ACK 83950275ed
Tree-SHA512: 65ae8635429a4d563b19969bac8128038ac2cbe01d9c9946abd4cac3c0780974d1e8b9aae9bb83f414e5d247a59f4a18fef5b37d93ad59ed41b6f11c3fe05af4
d3b8a54a81 Refactor CFeeRate to use FeeFrac internally (Pol Espinasa)
Pull request description:
The `FeeFrac` type represents a fraction, intended to be used for `sats/vbyte` or `sats/WU`. It was added to improve accuracy when evaluating fee rates in cluster mempool. [1]
But it can also be used to fix the precision issues that the current `CFeeRate` class has now.
At the moment, `CFeeRate` handles the fee rate as satoshis per kilovirtualbyte: `CAmount / kvB` using an integer.
This PR fix `CFeeRate` precision issues by encapsulating `FeeFrac` internally keeping backwards compatibility.
This PR can also be used as a based to use multiple units on RPC calls as detailed in this issue [2].
Some previous discussions:
[1] https://github.com/bitcoin/bitcoin/pull/30535
[2] https://github.com/bitcoin/bitcoin/issues/32093
ACKs for top commit:
achow101:
ACK d3b8a54a81
murchandamus:
code review, lightly tested ACK d3b8a54a81
ismaelsadeeq:
re-ACK d3b8a54a81📦
theStack:
Code-review ACK d3b8a54a81
Tree-SHA512: 5a8149d81e82ad4e60a0e76ff6a82a5b1c4e212cf5156c1cdd16bf9acbb351e7be458eac3f0a2ae89107f331062b299c1d9ca649d3b820ad0b68e6d1a14292e5
exFAT is known to cause corruption on macOS. See #28552.
Therefore we should warn when using this fs format for either the blocks
or data directories on macOS.
Co-authored-by: l0rinc <pap.lorinc@gmail.com>
27aefac425 validation: detect witness stripping without re-running Script checks (Antoine Poinsot)
2907b58834 policy: introduce a helper to detect whether a transaction spends Segwit outputs (Antoine Poinsot)
eb073209db qa: test witness stripping in p2p_segwit (Antoine Poinsot)
Pull request description:
Since it was introduced in 4eb515574e (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation for every single input 3 times.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot, P2WPKH and P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus). For undefined program types, Script validation is always going to fail regardless of the witness (by standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation. However this might lead to more "false positives" (cases where we return witness stripping for an otherwise invalid transaction) than the existing implementation. For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as consensus invalid while the implementation in this PR would always consider it witness stripped.
h/t AJ: this essentially implements a variant of https://github.com/bitcoin/bitcoin/pull/33066#issuecomment-3135258539.
ACKs for top commit:
sipa:
re-ACK 27aefac425
Crypt-iQ:
re-ACK 27aefac425
glozow:
reACK 27aefac425
Tree-SHA512: 70cf76b655b52bc8fa2759133315a3f11140844b6b80d9de3c95f592050978cc01a87bd2446e3a9c25cc872efea7659d6da3337b1a709511771fece206e9f149
656e16aa5e qt: Update the `src/qt/locale/bitcoin_en.xlf` translation source file (Hennadii Stepanov)
a0eaa44925 Fix typos (Hennadii Stepanov)
8d4aaaec49 Update Transifex slug for 30.x (Hennadii Stepanov)
Pull request description:
This PR follows our [Release Process](53a996f122/doc/release-process.md).
It is required to open Transifex translations for v30.0, as scheduled in https://github.com/bitcoin/bitcoin/issues/32275.
For reference, see the previous similar PR: https://github.com/bitcoin/bitcoin/pull/31809.
**Note for reviewers:**
To reproduce the diff in the last commit, run:
```
cmake --preset dev-mode
cmake --build build_dev_mode --target translate
```
ACKs for top commit:
laanwj:
Code review ACK 656e16aa5e
stickies-v:
ACK 656e16aa5e , was able to reproduce
Tree-SHA512: 403b534329755079584fcdf98b696e3e75952dfc8d069f305843dbfa85de95f6816ee1d5dfc9b553c7c7f52cc296cb8d3cb03207051d26e0e76ff30d377f49e4
Since it was introduced in 4eb515574e (#18044), the detection of a
stripped witness relies on running the Script checks 3 times. In the worst case, this consists in
running Script validation 3 times for every single input.
Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's
wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with
txid-based orphan resolution as used in 1p1c package relay.
However it is not necessary to run Script validation to detect a stripped witness (much less so
doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot,
P2WPKH, P2WSH), undefined types, and the Pay-to-anchor carve-out.
For defined program types, Script validation with an empty witness will always fail (by consensus).
For undefined program types, Script validation is always going to fail regardless of the witness (by
standardness). For P2A, an empty witness is never going to lead to a failure.
Therefore it holds that we can always detect a stripped witness without re-running Script validation.
However this might lead to more "false positives" (cases where we return witness stripping for an
otherwise invalid transaction) than the existing implementation. For instance a transaction with one
P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing
implementation would treat it as consensus invalid while the implementation in this commit would
always consider it witness stripped.
f49840dd90 doc: Fix typo in files.md (Ryan Ofsky)
f5cf0b1ccc bitcoin wrapper: improve help output (Ryan Ofsky)
c810b168b8 doc: Add description of installed files to files.md (Ryan Ofsky)
94ffd01a02 doc: Add release notes describing libexec/ binaries (Ryan Ofsky)
cd97905ebc 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 f49840dd90
Sjors:
re-ACK f49840dd90
achow101:
ACK f49840dd90
janb84:
re ACK f49840dd90
BrandonOdiwuor:
Tested ACK f49840dd90
hodlinator:
re-ACK f49840dd90
willcl-ark:
utACK f49840dd90
Tree-SHA512: 858a2e1a53db11ee3c5c759bfdeea566f242b9ce5e8a898fa435222e41662b8184577c0dc2c4c058294b4de41d8cb3ba3e5d24c748c280efa4a3f84e3ec4344d
9a5d29711a Squashed 'src/crc32c/' changes from b60d2b7334..efb8ea04e4 (fanquake)
Pull request description:
Sync the subtree with latest upstream. The changes here are a no-op, but pull them to fix the drive-by-typo-fixing: #33057.
Includes https://github.com/bitcoin-core/crc32c-subtree/pull/8.
ACKs for top commit:
maflcko:
lgtm ACK 8ef8dd6871
janb84:
ACK 8ef8dd6871
Tree-SHA512: b20a47514218206b934c4aa27ec667fb9b3ec7f7388a78725c52fc6e916358d2b9a2075a37808dbc2430b4c7816511ecf20e36bfe2fbd2d8a26bc8882a46d5e7
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.
