17cf9ff7ef Use cluster size limit for -maxmempool bound, and allow -maxmempool=0 in general (Suhas Daftuar)
315e43e5d8 Sanity check `GetFeerateDiagram()` in CTxMemPool::check() (Suhas Daftuar)
de2e9a24c4 test: extend package rbf functional test to larger clusters (Suhas Daftuar)
4ef4ddb504 doc: update policy/packages.md for new package acceptance logic (Suhas Daftuar)
79f73ad713 Add check that GetSortedScoreWithTopology() agrees with CompareMiningScoreWithTopology() (Suhas Daftuar)
a86ac11768 Update comments for CTxMemPool class (Suhas Daftuar)
9567eaa66d Invoke TxGraph::DoWork() at appropriate times (Suhas Daftuar)
6c5c44f774 test: add functional test for new cluster mempool RPCs (Suhas Daftuar)
72f60c877e doc: Update mempool_replacements.md to reflect feerate diagram checks (Suhas Daftuar)
21693f031a Expose cluster information via rpc (Suhas Daftuar)
72e74e0d42 fuzz: try to add more code coverage for mempool fuzzing (Suhas Daftuar)
f107417490 bench: add more mempool benchmarks (Suhas Daftuar)
7976eb1ae7 Avoid violating mempool policy limits in tests (Suhas Daftuar)
84de685cf7 Stop tracking parents/children outside of txgraph (Suhas Daftuar)
88672e205b Rewrite GatherClusters to use the txgraph implementation (Suhas Daftuar)
1ca4f01090 Fix miniminer_tests to work with cluster limits (Suhas Daftuar)
1902111e0f Eliminate CheckPackageLimits, which no longer does anything (Suhas Daftuar)
3a646ec462 Rework RBF and TRUC validation (Suhas Daftuar)
19b8479868 Make getting parents/children a function of the mempool, not a mempool entry (Suhas Daftuar)
5560913e51 Rework truc_policy to use descendants, not children (Suhas Daftuar)
a4458d6c40 Use txgraph to calculate descendants (Suhas Daftuar)
c8b6f70d64 Use txgraph to calculate ancestors (Suhas Daftuar)
241a3e666b Simplify ancestor calculation functions (Suhas Daftuar)
b9cec7f0a1 Make removeConflicts private (Suhas Daftuar)
0402e6c780 Remove unused limits from CalculateMemPoolAncestors (Suhas Daftuar)
08be765ac2 Remove mempool logic designed to maintain ancestor/descendant state (Suhas Daftuar)
fc4e3e6bc1 Remove unused members from CTxMemPoolEntry (Suhas Daftuar)
ff3b398d12 mempool: eliminate accessors to mempool entry ancestor/descendant cached state (Suhas Daftuar)
b9a2039f51 Eliminate use of cached ancestor data in miniminer_tests and truc_policy (Suhas Daftuar)
ba09fc9774 mempool: Remove unused function CalculateDescendantMaximum (Suhas Daftuar)
8e49477e86 wallet: Replace max descendant count with cluster_count (Suhas Daftuar)
e031085fd4 Eliminate Single-Conflict RBF Carve Out (Suhas Daftuar)
cf3ab8e1d0 Stop enforcing descendant size/count limits (Suhas Daftuar)
89ae38f489 test: remove rbf carveout test from mempool_limit.py (Suhas Daftuar)
c0bd04d18f Calculate descendant information for mempool RPC output on-the-fly (Suhas Daftuar)
bdcefb8a8b Use mempool/txgraph to determine if a tx has descendants (Suhas Daftuar)
69e1eaa6ed Add test case for cluster size limits to TRUC logic (Suhas Daftuar)
9cda64b86c Stop enforcing ancestor size/count limits (Suhas Daftuar)
1f93227a84 Remove dependency on cached ancestor data in mini-miner (Suhas Daftuar)
9fbe0a4ac2 rpc: Calculate ancestor data from scratch for mempool rpc calls (Suhas Daftuar)
7961496dda Reimplement GetTransactionAncestry() to not rely on cached data (Suhas Daftuar)
feceaa42e8 Remove CTxMemPool::GetSortedDepthAndScore (Suhas Daftuar)
21b5cea588 Use cluster linearization for transaction relay sort order (Suhas Daftuar)
6445aa7d97 Remove the ancestor and descendant indices from the mempool (Suhas Daftuar)
216e693729 Implement new RBF logic for cluster mempool (Suhas Daftuar)
ff8f115dec policy: Remove CPFP carveout rule (Suhas Daftuar)
c3f1afc934 test: rewrite PopulateMempool to not violate mempool policy (cluster size) limits (Suhas Daftuar)
47ab32fdb1 Select transactions for blocks based on chunk feerate (Suhas Daftuar)
dec138d1dd fuzz: remove comparison between mini_miner block construction and miner (Suhas Daftuar)
6c2bceb200 bench: rewrite ComplexMemPool to not create oversized clusters (Suhas Daftuar)
1ad4590f63 Limit mempool size based on chunk feerate (Suhas Daftuar)
b11c89cab2 Rework miner_tests to not require large cluster limit (Suhas Daftuar)
95a8297d48 Check cluster limits when using -walletrejectlongchains (Suhas Daftuar)
95762e6759 Do not allow mempool clusters to exceed configured limits (Suhas Daftuar)
edb3e7cdf6 [test] rework/delete feature_rbf tests requiring large clusters (glozow)
435fd56711 test: update feature_rbf.py replacement test (Suhas Daftuar)
34e32985e8 Add new (unused) limits for cluster size/count (Suhas Daftuar)
838d7e3553 Add transactions to txgraph, but without cluster dependencies (Suhas Daftuar)
d5ed9cb3eb Add accessor for sigops-adjusted weight (Suhas Daftuar)
1bf3b51396 Add sigops adjusted weight calculator (Suhas Daftuar)
c18c68a950 Create a txgraph inside CTxMemPool (Suhas Daftuar)
29a94d5b2f Make CTxMemPoolEntry derive from TxGraph::Ref (Suhas Daftuar)
92b0079fe3 Allow moving CTxMemPoolEntry objects, disallow copying (Suhas Daftuar)
6c73e47448 mempool: Store iterators into mapTx in mapNextTx (Suhas Daftuar)
51430680ec Allow moving an Epoch::Marker (Suhas Daftuar)
Pull request description:
[Reopening #28676 here as a new PR, because GitHub is slow to load the page making it hard to scroll through and see comments. Also, that PR was originally opened with a prototype implementation which has changed significantly with the introduction of `TxGraph`.]
This is an implementation of the [cluster mempool proposal](https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393).
This branch implements the following observable behavior changes:
- Maintains a partitioning of the mempool into connected clusters (via the `txgraph` class), which are limited in vsize to 101 kvB by default, and limited in count to 64 by default.
- Each cluster is sorted ("linearized") to try to optimize for selecting highest-feerate-subsets of a cluster first
- Transaction selection for mining is updated to use the cluster linearizations, selecting highest feerate "chunks" first for inclusion in a block template.
- Mempool eviction is updated to use the cluster linearizations, selecting lowest feerate "chunks" first for removal.
- The RBF rules are updated to: (a) drop the requirement that no new inputs are introduced; (b) change the feerate requirement to instead check that the feerate diagram of the mempool will strictly improve; (c) replace the direct conflicts limit with a directly-conflicting-clusters limit.
- The CPFP carveout rule is eliminated (it doesn't make sense in a cluster-limited mempool)
- The ancestor and descendant limits are no longer enforced.
- New cluster count/cluster vsize limits are now enforced instead.
- Transaction relay now uses chunk feerate comparisons to determine the order that newly received transactions are announced to peers.
Additionally, the cached ancestor and descendant data are dropped from the mempool, along with the multi_index indices that were maintained to sort the mempool by ancestor and descendant feerates. For compatibility (eg with wallet behavior or RPCs exposing this), this information is now calculated dynamically instead.
ACKs for top commit:
instagibbs:
reACK 17cf9ff7ef
glozow:
reACK 17cf9ff7ef
sipa:
ACK 17cf9ff7ef
Tree-SHA512: bbde46d913d56f8d9c0426cb0a6c4fa80b01b0a4c2299500769921f886082fb4f51f1694e0ee1bc318c52e1976d7ebed8134a64eda0b8044f3a708c04938eee7
7f318e1dd0 test: Add better coverage for Autofile size() (Fabian Jahr)
b7af960eb8 refactor: Add AutoFile::size (Fabian Jahr)
ec0f75862e refactor: Modernize logging in util/asmap.cpp (Fabian Jahr)
606a251e0a tests: add unit test vectors for asmap interpreter (Pieter Wuille)
Pull request description:
This contains some commits from #28792 that can be easily reviewed and merged independently. I hope splitting this change off can make this part move a bit faster and reduce frequency of needed rebases for #28792.
The commits in order:
- Add additional unit test vectors to the asmap interpreter (written by sipa). This helps to ensure that the further refactors in #28792 don't change behavior.
- Modernizes the logging in `util/asmap.cpp`, I added this while touching the rest of the file all over anyway.
- Adds an `AutoFile::size` helper function with some additional test coverage in a separate commit
ACKs for top commit:
maflcko:
review ACK 7f318e1dd0🏀
hodlinator:
tACK 7f318e1dd0
laanwj:
Code review ACK 7f318e1dd0
Tree-SHA512: 45156b74e4bd9278a7ec24521dfdafe4dab1ba3384243c7d589ef17e16ca374ee2af7178c86b7229e80ca262dbe78c4d456d80a6ee742ec31d2ab5243dac8b57
40dcbf580d build: add -Wtrailing-whitespace=any (fanquake)
d7659cd7e6 build: add -Wleading-whitespace=spaces (fanquake)
d86650220a cmake: Disable `-Wtrailing-whitespace` warnings for RCC-generated files (Hennadii Stepanov)
aabc5ca6ed cmake: Switch from AUTORCC to `qt6_add_resources` (Hennadii Stepanov)
25ae14c339 subprocess: replace tab with space (fanquake)
0c2b9dadd5 scripted-diff: remove whitespace in sha256_sse4.cpp (fanquake)
4da084fbc9 scripted-diff: change whitespace to spaces in univalue (fanquake)
e6caf150b3 ci: add moreutils to lint job (fanquake)
Pull request description:
GCC 15 now has options to turn leading & trailing whitespace into compile failures: https://gcc.gnu.org/gcc-15/changes.html#c-family. Fix the few cases of leading tabs, and trailing whitespace, and then enable `-Wleading-whitespace` and `-Wtrailing-whitespace`.
We currently get PRs that are opened with various whitespace, i.e #33822, so turning that into compile-time failure where possible, seems useful, to avoid a CI roundtrip.
ACKs for top commit:
ajtowns:
utACK 40dcbf580d
hebasto:
re-ACK 40dcbf580d.
Tree-SHA512: a128001ab2abb41cd6d249dcf46be4167ebd608d6b0f1452212a3ec9a383747bea623ab0382ec7bc0ac7a232a47cca5174e1cd73d4eda6751aa3cb2365ad2ede
Now that the __func__ is no longer used, the
NOLINTBEGIN(bugprone-lambda-function-name) can be removed.
Also, re-format the NONFATAL_UNREACHABLE macro, while touching the
adjacent line.
Performance likely does not matter here, but from a perspective of
code-readablilty, a const reference should be preferred for read-only
access.
So use it here.
This requires to set -Wno-error=dangling-reference for GCC 13.1
compilations, but this false-positive is fixed in later GCC versions.
See also https://godbolt.org/z/fjc6be65M
Without this, compile warnings could be hit about __func__ being only
valid inside functions.
warning: predefined identifier is only valid inside function [-Wpredefined-identifier-outside-function]
note: expanded from macro Assert
115 | #define Assert(val) inline_assertion_check<true>(val, __FILE__, __LINE__, __func__, #val)
| ^
Ref https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2486258473
fa0fa0f700 refactor: Revert "disable self-assign warning for tests" (MarcoFalke)
faed118fb3 build: Bump clang minimum supported version to 17 (MarcoFalke)
Pull request description:
Most supported operating systems ship with clang-17 (or later), so bump the minimum to that and allow new code to drop workarounds for previous clang bugs.
(Apart from dropping the small workaround, this bump allows the `ci_native_nowallet_libbitcoinkernel` CI to run on riscv64 without running into an ICE with clang-16.)
This patch will only be released in version 31.x, next year (2026).
For reference:
* https://packages.debian.org/bookworm/clang-19
* https://packages.ubuntu.com/noble/clang (clang-18)
* CentOS-like 8/9/10 ship clang-17 (and later) via Stream
* FreeBSD 12/13 ship clang-17 (and later) via packages
* OpenSuse Tumbleweed ships with https://software.opensuse.org/package/clang (clang21); No idea about OpenSuse Leap
On operating systems where the clang version is not shipped by default, the user would have to use GCC, or install clang in a different way. For example:
* https://packages.debian.org/bookworm/g++ (g++-12)
* https://packages.ubuntu.com/jammy/g++ (g++-11)
* https://apt.llvm.org/, or nix, or guix, or compile clang from source, ...
*Ubuntu 22.04 LTS does not ship with clang-16 (the previous minimum required), nor with clang-17, so one of the above workarounds is needed there.*
macOS 14 is unaffected, and the previous minimum requirement of Xcode15.0 remains, see also 919e6d01e9/depends/hosts/darwin.mk (L3-L4). (Modulo compiling the fuzz tests, which requires 919e6d01e9/.github/workflows/ci.yml (L149))
ACKs for top commit:
janb84:
Concept ACK fa0fa0f700
l0rinc:
Code review ACK fa0fa0f700
hebasto:
ACK fa0fa0f700.
Tree-SHA512: 5973cec39982f80b8b43e493cde012d9d1ab75a0362300b007d155db9f871c6341e7e209e5e63f0c3ca490136b684683de270136d62cb56f6b00b0ac0331dc36
53e4951a5b Switch to ANSI Windows API in `fsbridge::fopen()` function (Hennadii Stepanov)
dbe770d921 Switch to ANSI Windows API in `Win32ErrorString()` function (Hennadii Stepanov)
06d0be4e22 Remove no longer necessary `WinCmdLineArgs` class (Hennadii Stepanov)
f366408492 cmake: Set process code page to UTF-8 on Windows (Hennadii Stepanov)
dccbb17806 Set minimum supported Windows version to 1903 (May 2019 Update) (Hennadii Stepanov)
Pull request description:
The main goal is to remove [deprecated](https://github.com/bitcoin/bitcoin/issues/32361) code (removed in C++26).
This PR employs Microsoft's modern [approach](https://learn.microsoft.com/en-us/windows/apps/design/globalizing/use-utf8-code-page) to handling UTF-8:
> Until recently, Windows has emphasized "Unicode" -W variants over -A APIs. However, recent releases have used the ANSI code page and -A APIs as a means to introduce UTF-8 support to apps. If the ANSI code page is configured for UTF-8, then -A APIs typically operate in UTF-8. This model has the benefit of supporting existing code built with -A APIs without any code changes.
TODO:
- [x] Handle application manifests properly when building with MSVC.
- [x] Bump the minimum supported Windows version to 1903 (May 2019 Update).
- [x] Remove all remaining use cases of the deprecated `std:wstring_convert`.
- The instance in `subprocess.h` will be addressed in a follow-up PR, as additional tests are likely needed.
- The usage in `common/system.cpp` is handled in https://github.com/bitcoin/bitcoin/pull/32566.
Resolves partially https://github.com/bitcoin/bitcoin/issues/32361.
ACKs for top commit:
laanwj:
re-ACK 53e4951a5b
hodlinator:
re-ACK 53e4951a5b
davidgumberg:
untested crACK 53e4951a5b
Tree-SHA512: 0dbe9badca8b979ac2b4814fea6e4a7e53c423a1c96cb76ce894253137d3640a87631a5b22b9645e8f0c2a36a107122eb19ed8e92978c17384ffa8b9ab9993b5
b63428ac9c rpc: refactor: use more (Maybe)Arg<std::string_view> (stickies-v)
037830ca0d refactor: increase string_view usage (stickies-v)
b3bf18f0ba rpc: refactor: use string_view in Arg/MaybeArg (stickies-v)
Pull request description:
The `RPCHelpMan::{Arg,MaybeArg}` helpers avoid copying (potentially) large strings by returning them as `const std::string*` (`MaybeArg`) or `const std::string&` (`Arg`). For `MaybeArg`, this has the not-so-nice effect that users need to deal with raw pointers, potentially also requiring new functions (e.g. [`EnsureUniqueWalletName` ](d127b25199 (diff-d8bfcfbdd5fa7d5c52d38c1fe5eeac9ce5c5a794cdfaf683585140fa70a32374R32))) with raw pointers being implemented.
This PR aims to improve on this by returning a trivially copyable `std::string_view` (`Arg`) or `std::optional<std::string_view>` (`MaybeArg`), modernizing the interface without introducing any additional copying overhead. In doing so, it also generalizes whether we return by value or by pointer/reference using `std::is_trivially_copyable_v` instead of defining the types manually.
In cases where functions currently take a `const std::string&` and it would be too much work / touching consensus logic to update them (`signmessage.cpp`), a `std::string` copy is made (which was already happening anyway).
The last 2 commits increase usage of the `{Arg,MaybeArg}<std::string_view>` helpers, and could be dropped/pruned if anything turns out to be controversial - I just think it's a nice little cleanup.
ACKs for top commit:
maflcko:
re-ACK b63428ac9c🎉
achow101:
ACK b63428ac9c
pablomartin4btc:
re-ACK [b63428a](b63428ac9c)
w0xlt:
reACK b63428ac9c
Tree-SHA512: b4942c353a1658c22a88d8c9b402c288ad35265a3b88aa2072b1f9b6d921cd073194ed4b00b807cb48ca440f47c87ef3d8e0dd1a5d814be58fc7743f26288277
fa37153288 util: Abort on failing CHECK_NONFATAL in debug builds (MarcoFalke)
fa0dc4bdff test: Allow testing of check failures (MarcoFalke)
faeb58fe66 refactor: Set G_ABORT_ON_FAILED_ASSUME when G_FUZZING_BUILD (MarcoFalke)
Pull request description:
A failing `CHECK_NONFATAL` will throw an exception. This is fine and even desired in production builds, because the program may catch the exception and give the user a way to easily report the bug upstream.
However, in debug development builds, exceptions for internal bugs are problematic:
* The exception could accidentally be caught and silently ignored
* The exception does not include a full stacktrace, possibly making debugging harder
Fix all issues by turning the exception into an abort in debug builds.
This can be tested by reverting the hunks to `src/rpc/node.cpp` and `test/functional/rpc_misc.py` and then running the functional or fuzz tests.
ACKs for top commit:
achow101:
ACK fa37153288
ryanofsky:
Code review ACK fa37153288, just catching subprocess.CalledProcessError in test fixing up a comment since last review
stickies-v:
ACK fa37153288
Tree-SHA512: 2d892b838ccef6f9b25a066e7c2f6cd6f5acc94aad1d91fce62308983bd3f5c5d724897a76de4e3cc5c3678ddadc87e2ee8c87362965373526038e598dfb0101
These overloads were needed to allow passing `fs::path` objects directly to
libstdc++'s `fstream` constructors, but after the previous commit, there is no
longer any remaining code that does pass `fs::path` objects to `fstream`
constructors. Writing new code which does this is also discouraged because the
standard has been updated in https://wg21.link/lwg3430 to disallow it.
Dropping these also means its no longer possible to pass `fs::path` arguments
directly to `fstream::open` in libstdc++, which is somewhat unfortunate but not
a big loss because it is already not possible to pass them to the constructor.
So this commit updates `fstream::open` calls.
Additionally, this change required updates to src/bitcoin.cpp since it was
relying on the overloaded filename() method.
As reported by hebasto in https://github.com/bitcoin/bitcoin/issues/33545,
newer libc++ versions implementing https://wg21.link/lwg3430 will no longer
implicitly convert `fs::path` objects to `std::filesystem::path` objects when
constructing `std::ifstream` and `std::ofstream` types.
This is not a problem in Unix systems since `fs::path` objects use
`std::string` as their native string type, but it causes compile errors on
Windows which use `std::wstring` as their string type, since `fstream`s can't
be constructed from `wstring`s.
Fix the windows libc++ compile errors by adding a new `fs::path::std_path()`
method and using it construct `fstream`s more portably.
Additionally, delete `fs::path`'s implicit `native_string` conversion so these
errors will not go undetected in the future, even though there is not currently
a CI job testing Windows libc++ builds.
Modernizes interface by not forcing users to deal with raw pointers,
without adding copying overhead. Generalizes the logic of whether
we return by value or by optional/pointer.
In cases where functions take a `const std::string&` and it would
be too much work to update them, a string copy is made (which was
already happening anyway).
0802398e74 fuzz: make it possible to mock (fuzz) CThreadInterrupt (Vasil Dimov)
6d9e5d130d fuzz: add CConnman::SocketHandler() to the tests (Vasil Dimov)
3265df63a4 fuzz: add CConnman::InitBinds() to the tests (Vasil Dimov)
91cbf4dbd8 fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests (Vasil Dimov)
50da7432ec fuzz: add CConnman::OpenNetworkConnection() to the tests (Vasil Dimov)
e6a917c8f8 fuzz: add Fuzzed NetEventsInterface and use it in connman tests (Vasil Dimov)
e883b37768 fuzz: set the output argument of FuzzedSock::Accept() (Vasil Dimov)
Pull request description:
Extend `CConnman` fuzz tests to also exercise the methods `OpenNetworkConnection()`, `CreateNodeFromAcceptedSocket()`, `InitBinds()` and `SocketHandler()`.
Previously fuzzing those methods would have resulted in real socket functions being called in the operating system which is undesirable during fuzzing. Now that https://github.com/bitcoin/bitcoin/pull/21878 is complete all those are mocked to a fuzzed socket and a fuzzed DNS resolver (see how `CreateSock` and `g_dns_lookup` are replaced in the first commit).
ACKs for top commit:
achow101:
ACK 0802398e74
jonatack:
Review re-ACK 0802398e74
dergoegge:
Code review ACK 0802398e74
Tree-SHA512: a717d4e79f42bacf2b029c821fdc265e10e4e5c41af77cd4cb452cc5720ec83c62789d5b3dfafd39a22cc8c0500b18169aa7864d497dded729a32ab863dd6c4d
93a29ff283 trace: Workaround GCC bug compiling with old systemtap (Luke Dashjr)
Pull request description:
ACKs for top commit:
0xB10C:
lgtm ACK 93a29ff283 - I did not test this.
Tree-SHA512: 9ce9ed8b7733af721134462073a3417e52d67e9e9853eebbddfa795842b381de98e28756ebfa6652536cbfdd08181142eccd198f4dc00a57d8748801b362b4b7
de0675f9de refactor: Move `transaction_identifier.h` to primitives (marcofleon)
6f068f65de Remove implicit uint256 conversion and comparison (marcofleon)
9c24cda72e refactor: Convert remaining instances from uint256 to Txid (marcofleon)
d2ecd6815d policy, refactor: Convert uint256 to Txid (marcofleon)
f6c0d1d231 mempool, refactor: Convert uint256 to Txid (marcofleon)
aeb0f78330 refactor: Convert `mini_miner` from uint256 to Txid (marcofleon)
326f244724 refactor: Convert RPCs and `merkleblock` from uint256 to Txid (marcofleon)
49b3d3a92a Clean up `FindTxForGetData` (marcofleon)
Pull request description:
This is the final leg of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
All of these changes are straightforward `uint256` --> `Txid` along with any necessary explicit conversions. Also, `transaction_identifier.h` is moved to primitives in the last commit, as `Txid` and `Wtxid` become fundamental types after this PR.
ACKs for top commit:
stickies-v:
re-ACK de0675f9de, no changes since a20724d926d5844168c6a13fa8293df8c8927efe except address review nits.
janb84:
re ACK de0675f9de
dergoegge:
re-ACK de0675f9de
theStack:
Code-review ACK de0675f9de
Tree-SHA512: 2413160fca7ab146a8d79d18ce3afcf7384cacc73c513d41928904aa453b4dd7a350064cee71e9c5d015da5904c7c81ac17603e50a47441ebc5b0c653235dd08
Moves the file from `src/util` to `src/primitives`. Now that the
refactor is complete, Txid and Wtxid are fundamental types, so it
makes sense for them to reside in `src/primitives`.
These remaining miscellaneous changes were identified by commenting out
the `operator const uint256&` conversion and the `Compare(const uint256&)`
method from `transaction_identifier.h`.
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>
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
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
faa1c3e80d Revert "Merge bitcoin/bitcoin#32343: common: Close non-std fds before exec in RunCommandJSON" (MarcoFalke)
Pull request description:
After a fork() in a multithreaded program, the child can safely
call only async-signal-safe functions (see [signal-safety(7)](https://www.man7.org/linux/man-pages/man7/signal-safety.7.html))
until such time as it calls execv.
The standard library (`std` namespace) is not async-signal-safe. Also, `throw`, isn't.
There was an alternative implementation using `readdir` (https://github.com/bitcoin/bitcoin/pull/32529), but that isn't async-signal-safe either, and that implementation was still using `throw`.
So temporarily revert this feature.
A follow-up in the future can add it back, using only async-signal-safe functions, or by using a different approach.
Fixes https://github.com/bitcoin/bitcoin/issues/32524
Fixes https://github.com/bitcoin/bitcoin/issues/33015
Fixes https://github.com/bitcoin/bitcoin/issues/32855
For reference, a failure can manifest in the GCC debug mode:
* While `fork`ing, a debug mode mutex is held (by any other thread).
* The `fork`ed child tries to use the stdard libary before `execv` and deadlocks.
This may look like the following:
```
(gdb) thread apply all bt
Thread 1 (Thread 0xf58f4b40 (LWP 774911) "b-httpworker.2"):
#0 0xf7f4f589 in __kernel_vsyscall ()
#1 0xf79e467e in ?? () from /lib32/libc.so.6
#2 0xf79eb582 in pthread_mutex_lock () from /lib32/libc.so.6
#3 0xf7d93bf2 in ?? () from /lib32/libstdc++.so.6
#4 0xf7d93f36 in __gnu_debug::_Safe_iterator_base::_M_attach(__gnu_debug::_Safe_sequence_base*, bool) () from /lib32/libstdc++.so.6
#5 0x5668810a in __gnu_debug::_Safe_iterator_base::_Safe_iterator_base (this=0xf58f13ac, __seq=0xf58f13f8, __constant=false) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_base.h:91
#6 0x56ddfb50 in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::forward_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:162
#7 0x56ddfacb in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::bidirectional_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:539
#8 0x56ddfa5b in __gnu_debug::_Safe_iterator<__gnu_cxx::__normal_iterator<int*, std::__cxx1998::vector<int, std::allocator<int> > >, std::__debug::vector<int, std::allocator<int> >, std::random_access_iterator_tag>::_Safe_iterator (this=0xf58f13a8, __i=3, __seq=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/safe_iterator.h:687
#9 0x56ddd3f6 in std::__debug::vector<int, std::allocator<int> >::begin (this=0xf58f13f8) at /bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/debug/vector:300
#10 0x57d83701 in subprocess::detail::Child::execute_child (this=0xf58f156c) at ./util/subprocess.h:1372
#11 0x57d80a7c in subprocess::Popen::execute_process (this=0xf58f1cd8) at ./util/subprocess.h:1231
#12 0x57d6d2b4 in subprocess::Popen::Popen<subprocess::input, subprocess::output, subprocess::error, subprocess::close_fds> (this=0xf58f1cd8, cmd_args="fake.py enumerate", args=..., args=..., args=..., args=...) at ./util/subprocess.h:964
#13 0x57d6b597 in RunCommandParseJSON (str_command="fake.py enumerate", str_std_in="") at ./common/run_command.cpp:27
#14 0x57a90547 in ExternalSigner::Enumerate (command="fake.py", signers=std::__debug::vector of length 0, capacity 0, chain="regtest") at ./external_signer.cpp:28
#15 0x56defdab in enumeratesigners()::$_0::operator()(RPCHelpMan const&, JSONRPCRequest const&) const (this=0xf58f2ba0, self=..., request=...) at ./rpc/external_signer.cpp:51
...
(truncated, only one thread exists)
```
ACKs for top commit:
fanquake:
ACK faa1c3e80d
darosior:
ACK faa1c3e80d
Tree-SHA512: 602da5f2eba08d7fe01ba19baf411e287ae27fe2d4b82f41734e05b7b1d938ce94cc0041e86ba677284fa92838e96ebee687023ff28047e2b036fd9a53567e0a
This requires adjusting some tests to force exceptions over aborts, or
accept either exceptions or aborts.
Also, remove a fuzz test in integer.cpp that is mostly redundant with
the unit test added in the prior commit.
This allows specific tests to mock the check behavior to consistently
use exceptions instead of aborts for intentionally failing checks in all
build configurations.
Benchmarks indicated that obfuscating multiple bytes already gives an order of magnitude speed-up, but:
* GCC still emitted scalar code;
* Clang’s auto-vectorized loop ran on the slow unaligned-load path.
Fix contains:
* peeling the misaligned head enabled the hot loop starting at an 8-byte address;
* `std::assume_aligned<8>` tells the optimizer the promise holds - required to keep Apple Clang happy;
* manually unrolling the body to 64 bytes enabled GCC to auto-vectorize.
Note that `target.size() > KEY_SIZE` condition is just an optimization, the aligned and unaligned loops work without it as well - it's why the alignment calculation still contains `std::min`.
> C++ compiler .......................... GNU 14.2.0
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.03 | 32,464,658,919.11 | 0.0% | 0.50 | 0.11 | 4.474 | 0.08 | 0.0% | 5.29 | `ObfuscationBench`
> C++ compiler .......................... Clang 20.1.7
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.02 | 41,231,547,045.17 | 0.0% | 0.30 | 0.09 | 3.463 | 0.02 | 0.0% | 5.47 | `ObfuscationBench`
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
All former `std::vector<std::byte>` keys were replaced with `uint64_t` (we still serialize them as vectors but convert immediately to `uint64_t` on load).
This is why some tests still generate vector keys and convert them to `uint64_t` later instead of generating them directly.
In `Obfuscation::Unserialize` we can safely throw an `std::ios_base::failure` since during mempool fuzzing `mempool_persist.cpp#L141` catches and ignored these errors.
> C++ compiler .......................... GNU 14.2.0
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.04 | 28,365,698,819.44 | 0.0% | 0.34 | 0.13 | 2.714 | 0.07 | 0.0% | 5.33 | `ObfuscationBench`
> C++ compiler .......................... Clang 20.1.7
| ns/byte | byte/s | err% | ins/byte | cyc/byte | IPC | bra/byte | miss% | total | benchmark
|--------------------:|--------------------:|--------:|----------------:|----------------:|-------:|---------------:|--------:|----------:|:----------
| 0.08 | 13,012,464,203.00 | 0.0% | 0.65 | 0.28 | 2.338 | 0.13 | 0.8% | 5.50 | `ObfuscationBench`
Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
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>
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>
a60f863d3e scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba199598 Remove old GenTxid class (marcofleon)
072a198ea4 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c7 Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b07 Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2be move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d590 refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb Implement GenTxid as a variant (marcofleon)
Pull request description:
Part of the [type safety refactor](https://github.com/bitcoin/bitcoin/pull/32189).
This PR changes the GenTxid class to a variant, which holds both Txids and Wtxids. This provides compile-time type safety and eliminates the manual type check (bool m_is_wtxid). Variables that can be either a Txid or a Wtxid are now using the new GenTxid variant, instead of uint256.
ACKs for top commit:
w0xlt:
ACK a60f863d3e
dergoegge:
Code review ACK a60f863d3e
maflcko:
review ACK a60f863d3e🎽
theStack:
Code-review ACK a60f863d3e
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
