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.
96da68a38f qa: functional test a transaction running into the legacy sigop limit (Antoine Poinsot)
367147954d qa: unit test standardness of inputs packed with legacy sigops (Antoine Poinsot)
5863315e33 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 96da68a38f🚋
achow101:
ACK 96da68a38f
glozow:
light code review ACK 96da68a38f, looks correct to me
Tree-SHA512: 106ffe62e48952affa31c5894a404a17a3b4ea8971815828166fba89069f757366129f7807205e8c6558beb75c6f67d8f9a41000be2f8cf95be3b1a02d87bfe9
50024620b9 [bench] worst case LimitOrphans and EraseForBlock (glozow)
45c7a4b56d [functional test] orphan resolution works in the presence of DoSy peers (glozow)
835f5c77cd [prep/test] restart instead of bumpmocktime between p2p_orphan_handling subtests (glozow)
b113877545 [fuzz] Add simulation fuzz test for TxOrphanage (Pieter Wuille)
03aaaedc6d [prep] Return the made-reconsiderable announcements in AddChildrenToWorkSet (Pieter Wuille)
ea29c4371e [p2p] bump DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE to 3,000 (glozow)
24afee8d8f [fuzz] TxOrphanage protects peers that don't go over limit (glozow)
a2878cfb4a [unit test] strengthen GetChildrenFromSamePeer tests: results are in recency order (glozow)
7ce3b7ee57 [unit test] basic TxOrphanage eviction and protection (glozow)
4d23d1d7e7 [cleanup] remove unused rng param from LimitOrphans (glozow)
067365d2a8 [p2p] overhaul TxOrphanage with smarter limits (glozow)
1a41e7962d [refactor] create aliases for TxOrphanage Count and Usage (glozow)
b50bd72c42 [prep] change return type of EraseTx to bool (glozow)
3da6d7f8f6 [prep/refactor] make TxOrphanage a virtual class implemented by TxOrphanageImpl (glozow)
77ebe8f280 [prep/test] have TxOrphanage remember its own limits in LimitOrphans (glozow)
d0af4239b7 [prep/refactor] move DEFAULT_MAX_ORPHAN_TRANSACTIONS to txorphanage.h (glozow)
51365225b8 [prep/config] remove -maxorphantx (glozow)
8dd24c29ae [prep/test] modify test to not access TxOrphanage internals (glozow)
44f5327824 [fuzz] add SeedRandomStateForTest(SeedRand::ZEROS) to txorphan (glozow)
15a4ec9069 [prep/rpc] remove entry and expiry time from getorphantxs (glozow)
08e58fa911 [prep/refactor] move txorphanage to node namespace and directory (glozow)
bb91d23fa9 [txorphanage] change type of usage to int64_t (glozow)
Pull request description:
This PR is part of the orphan resolution project, see #27463.
This design came from collaboration with sipa - thanks.
We want to limit the CPU work and memory used by `TxOrphanage` to avoid denial of service attacks. On master, this is achieved by limiting the number of transactions in this data structure to 100, and the weight of each transaction to 400KWu (the largest standard tx) [0]. We always allow new orphans, but if the addition causes us to exceed 100, we evict one randomly. This is dead simple, but has problems:
- It makes the orphanage trivially churnable: any one peer can render it useless by spamming us with lots of orphans. It's possible this is happening: "Looking at data from node alice on 2024-09-14 shows that we’re sometimes removing more than 100k orphans per minute. This feels like someone flooding us with orphans." [1]
- Effectively, opportunistic 1p1c is useless in the presence of adversaries: it is *opportunistic* and pairs a low feerate tx with a child that happens to be in the orphanage. So if nothing is able to stay in orphanages, we can't expect 1p1cs to propagate.
- This number is also often insufficient for the volume of orphans we handle: historical data show that overflows are pretty common, and there are times where "it seems like [the node] forgot about the orphans and re-requested them multiple times." [1]
Just jacking up the `-maxorphantxs` number is not a good enough solution, because it doesn't solve the churnability problem, and the effective resource bounds scale poorly.
This PR introduces numbers for {global, per-peer} {memory usage, announcements + number of inputs}, representing resource limits:
- The (constant) **global latency score limit** is the number of unique (wtxid, peer) pairs in the orphanage + the number of inputs spent by those (deduplicated) transactions floor-divided by 10 [2]. This represents a cap on CPU or latency for any given operation, and does not change with the number of peers we have. Evictions must happen whenever this limit is reached. The primary goal of this limit is to ensure we do not spend more than a few ms on any call to `LimitOrphans` or `EraseForBlock`.
- The (variable) **per-peer latency score limit** is the global latency score limit divided by the number of peers. Peers are allowed to exceed this limit provided the global announcement limit has not been reached. The per-peer announcement limit decreases with more peers.
- The (constant) **per-peer memory usage reservation** is the amount of orphan weight [3] reserved per peer [4]. Reservation means that peers are effectively guaranteed this amount of space. Peers are allowed to exceed this limit provided the global usage limit is not reached. The primary goal of this limit is to ensure we don't oom.
- The (variable) **global memory usage limit** is the number of peers multiplied by the per-peer reservation [5]. As such, the global memory usage limit scales up with the number of peers we have. Evictions must happen whenever this limit is reached.
- We introduce a "Peer DoS Score" which is the maximum between its "CPU Score" and "Memory Score." The CPU score is the ratio between the number of orphans announced by this peer / peer announcement limit. The memory score is the total usage of all orphans announced by this peer / peer usage reservation.
Eviction changes in a few ways:
- It is triggered if either limit is exceeded.
- On each iteration of the loop, instead of selecting a random orphan, we select a peer and delete 1 of its announcements. Specifically, we select the peer with the highest DoS score, which is the maximum between its CPU DoS score (based on announcements) and Memory DoS score (based on tx weight). After the peer has been selected, we evict the oldest orphan (non-reconsiderable sorted before reconsiderable).
- Instead of evicting orphans, we evict announcements. An orphan is still in the orphanage as long as there is 1 peer announcer. Of course, over the course of several iteration loops, we may erase all announcers, thus erasing the orphan itself. The purpose of this change is to prevent a peer from being able to trigger eviction of another peer's orphans.
This PR also:
- Reimplements `TxOrphanage` as single multi-index container.
- Effectively bounds the number of transactions that can be in a peer's work set by ensuring it is a subset of the peer's announcements.
- Removes the `-maxorphantxs` config option, as the orphanage no longer limits by unique orphans.
This means we can receive 1p1c packages in the presence of spammy peers. It also makes the orphanage more useful and increases our download capacity without drastically increasing orphanage resource usage.
[0]: This means the effective memory limit in orphan weight is 100 * 400KWu = 40MWu
[1]: https://delvingbitcoin.org/t/stats-on-orphanage-overflows/1421
[2]: Limit is 3000, which is equivalent to one max size ancestor package (24 transactions can be missing inputs) for each peer (default max connections is 125).
[3]: Orphan weight is used in place of actual memory usage because something like "one maximally sized standard tx" is easier to reason about than "considering the bytes allocated for vin and vout vectors, it needs to be within N bytes..." etc. We can also consider a different formula to encapsulate more the memory overhead but still have an interface that is easy to reason about.
[4]: The limit is 404KWu, which is the maximum size of an ancestor package.
[5]: With 125 peers, this is 50.5MWu, which is a small increase from the existing limit of 40MWu. While the actual memory usage limit is higher (this number does not include the other memory used by `TxOrphanage` to store the outpoints map, etc.), this is within the same ballpark as the old limit.
ACKs for top commit:
marcofleon:
ReACK 50024620b9
achow101:
light ACK 50024620b9
instagibbs:
ACK 50024620b9
theStack:
Code-review ACK 50024620b9
Tree-SHA512: 270c11a2d116a1bf222358a1b4e25ffd1f01e24da958284fa8c4678bee5547f9e0554e87da7b7d5d5d172ca11da147f54a69b3436cc8f382debb6a45a90647fd
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 prevent the ability for someone to
broadcast a transaction through the p2p network that is not valid according to the new rules. This
is because if it was possible it would be a trivial DoS to potentially unupgraded miners after the
soft fork activates.
We do not know for sure whether users will activate the Consensus Cleanup. However if they do such
transactions must have been made non-standard long in advance, due to the time it takes for most
nodes on the network to upgrade. In addition this limit may only be run into by pathological
transactions which pad the Script with sigops but do not use actual signatures when spending, as
otherwise they would run into the standard transaction size limit.
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 `FastRandomContext` delegates to `GetRandBytes` anyway, we can simplify new key generation to a Write/Read combo, unifying the flow of enabling obfuscation via `Read`.
The comments were also adjusted to clarify that the `m_obfuscation` field affects the behavior of `Read` and `Write` methods.
These changes are meant to simplify the diffs for the riskier optimization commits later.
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>
This adds a large simulation fuzz test for all TxOrphanage public interface
functions, using a mix of comparison with expected behavior (in case it is
fully specified), and testing of properties exhibited otherwise.
This is preparation for the simulation fuzz test added in a later commit. Since
AddChildrenToWorkSet consumes randomness, there is no way for the simulator to
exactly predict its behavior. By returning the set of made-reconsiderable announcements
instead, the simulator can instead test that it is *a* valid choice, and then
apply it to its own data structures.
For the default number of peers (125), allows each to relay a default
descendant package (up to 25-1=24 can be missing inputs) of small (9
inputs or fewer) transactions out of order.
This limit also gives acceptable bounds for worst case LimitOrphans iterations.
Functional tests aren't changed to check for larger cap because it would
make the runtime too long.
Also deletes the now-unused DEFAULT_MAX_ORPHAN_TRANSACTIONS.
This is largely a reimplementation using boost::multi_index_container.
All the same public methods are available. It has an index by outpoint,
per-peer tracking, peer worksets, etc.
A few differences:
- Limits have changed: instead of a global limit of 100 unique orphans,
we have a maximum number of announcements (which can include duplicate
orphans) and a global memory limit which scales with the number of
peers.
- The maximum announcements limit is 100 to match the original limit,
but this is actually a stricter limit because the announcement count
is not de-duplicated.
- Eviction strategy: when global limits are reached, a per-peer limit
comes into play. While limits are exceeded, we choose the peer whose
“DoS score” (max usage / limit ratio for announcements and memory
limits) is highest and evict announcements by entry time, sorting
non-reconsiderable ones before reconsiderable ones. Since announcements
are unique by (wtxid, peer), as long as 1 announcement remains for a
transaction, it remains in the orphanage.
- This eviction strategy means no peer can influence the eviction of
another peer’s orphans.
- Also, since global limits are a multiple of per-peer limits, as long
as a peer does not exceed its limits, its orphans are protected from
eviction.
- Orphans no longer expire, since older announcements are generally
removed before newer ones.
- GetChildrenFromSamePeer returns the transactions from newest to
oldest.
Co-authored-by: Pieter Wuille <pieter@wuille.net>
Move towards a model where TxOrphanage is initialized with limits that
it remembers throughout its lifetime.
Remove the param. Limiting by number of unique orphans will be removed
in a later commit.
Now that -maxorphantx is gone, this does not change the node behavior.
The parameter is only used in tests.
c18bf0bd9b refactor: cleanup index logging (Sjors Provoost)
Pull request description:
This PR removes the use of `__func__` from index logging, since we have `-logsourcelocations`.
It also improves readability by putting `GetName()` in a more logical place.
Before
> coinstatsindex: best block of the index not found. Please rebuild the index.
After:
> best block of coinstatsindex not found. Please rebuild the index.
I found myself maintaining this commit as part of https://github.com/Sjors/bitcoin/pull/86, but since that might never land here, it seemed better to split it into its own PR (or get rid of it).
ACKs for top commit:
l0rinc:
Lightweight code review ACK c18bf0bd9b
maflcko:
review ACK c18bf0bd9b🚣
Tree-SHA512: 755948371e3ff7a5515b63ce48075631ec7868d69c3c1469176d5be0e8b28e1c071e206ae3f7320f87d8c441f815894acfef61621f05795b5ff6b8a5a3031e3b
Expiry is going away in a later commit.
This is only an RPC change. Behavior of the orphanage does not change.
Note that getorphantxs is marked experimental.
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
fa894b0f3e log: Properly log warnings with warn loglevel in addrdb (MarcoFalke)
Pull request description:
The logging in addrdb is confusing, because it uses `LogPrintf` (info level) to log warnings.
Fix this by properly using the `warn` level, where needed. Also, drop unused trailing `\n` while touching the lines.
ACKs for top commit:
stickies-v:
ACK fa894b0f3e
dergoegge:
utACK fa894b0f3e
Tree-SHA512: 96d3823623ea8e1698e8cb541ca97cbab7b2a9934b2f894884171045abbca7be796f07965082e997001c97d06d1e0c4d13b29354eb4fe71c3a2ee680eada5516
- don't log function name
- take into account that GetName() always ends with " index"
- replace deprecated LogPrintf with LogInfo
- remove trailing \n
- adjusted log level where needed
fa8862723c fuzz: CheckGlobals in init (MarcoFalke)
fa26bfde98 test: Avoid resetting mocktime in testing setup (MarcoFalke)
fa6b45fa8e Add SetMockTime for time_point types (MarcoFalke)
Pull request description:
(Tracking issue https://github.com/bitcoin/bitcoin/issues/29018)
During fuzzing, `AppInitParameterInteraction` may actually disable a previously set mocktime. This is confusing and can also cause non-determinism.
Fix this issue, by
* fixing the erroneous `-mocktime` parsing in `AppInitParameterInteraction`.
* adding the missing `SetMockTime` calls to the affected fuzz init functions.
* adding a `CheckGlobals` to the fuzz init, to prevent this issue in the future.
This can be tested by
* Cherry-picking the `CheckGlobals`-commit onto current master and observing a fuzz failure in the touched fuzz targets.
* Reverting the touched fuzz fixups and observing a fuzz failure for each target.
ACKs for top commit:
w0xlt:
ACK fa8862723c
dergoegge:
utACK fa8862723c
Tree-SHA512: 5a9400f0467c82fa224713af4cc2b525afbefefc7c3f419077110925ad7af6c7fda3dcd2b50f7facf0ee7df2547c6ac20336906d707adcdfd1d652a9d9a735fe
f43571010e Resolve guix non-determinism with emplace_back instead of push_back (Ava Chow)
Pull request description:
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64 results in a single instruction difference which can be traced down to prevector.h:174. The ultimate caller of this is the copy constructor for a prevector that ends up being called by std::vector::push_back in walletmodel.cpp:183. By replacing the push_back with an emplace_back, somehow this non-determinism goes away.
Closes#32923
ACKs for top commit:
l0rinc:
code review ACK f43571010e
Sjors:
utACK f43571010e
maflcko:
lgtm ACK f43571010e
Tree-SHA512: 5bf0571f32cb72efc0c533e16d2704cfc3a79bcef2943f0892743572808610fb00ca8ab41223897536f8e5090bf4030735be910942de8116652d02bc3f231e2e
150b5c99ca wallet: replace `reload_wallet` with inline functionality (rkrux)
0f86da382d wallet: remove dead code in legacy wallet migration (rkrux)
Pull request description:
A discussion on a previous [PR 32481](https://github.com/bitcoin/bitcoin/pull/32481#discussion_r2145152084) related to legacy wallet dead
code removal made me realize that checking if the legacy
wallet was loaded prior to the start of the migration is not
required ever since legacy wallets can't be loaded in the first
place. I also verified that the `load_on_start` persistent
setting can also not cause the legacy wallets to be loaded, which
further makes the case for removal of the above mentioned checks
during migration.
The current test coverage also shows these lines uncovered.
ACKs for top commit:
achow101:
ACK 150b5c99ca
furszy:
ACK 150b5c99ca
Tree-SHA512: 9bc7043cac1f4051228557208895e43648de3c7ffae6860c0676d1aa2db3a8ed3a09d1f9defacd96ca50bbb9699ba86652ccb0c5e55cc88be248a1fe727c13d9
d7fca5c171 clusterlin: add big comment explaning the relation between tests (Pieter Wuille)
b64e61d2de clusterlin: abstract try-permutations into ExhaustiveLinearize function (Pieter Wuille)
1fa55a64ed clusterlin tests: verify that chunks are minimal (Pieter Wuille)
da23ecef29 clusterlin tests: support non-empty ReadTopologicalSubset() (Pieter Wuille)
94f3e17c33 clusterlin tests: compare with fuzz-provided linearizations (Pieter Wuille)
5f92ebee0d clusterlin tests: compare with fuzz-provided topological sets (Pieter Wuille)
6e37824ac3 clusterlin tests: optimize clusterlin_simple_linearize (Pieter Wuille)
98c1c88b6f clusterlin tests: separate testing of SimpleLinearize and Linearize (Pieter Wuille)
10e90f7aef clusterlin tests: make SimpleCandidateFinder always find connected (Pieter Wuille)
a38c38951e clusterlin tests: separate testing of Search- and SimpleCandidateFinder (Pieter Wuille)
77a432ee70 clusterlin tests: count SimpleCandidateFinder iterations better (Pieter Wuille)
Pull request description:
Part of the cluster mempool project: #30289
The current cluster linearization fuzz tests contain two tests which combine testing of production code with testing of the test code itself:
* `clusterlin_search_finder`: establishes the correctness of `SearchCandidateFinder` by comparing against both `SimpleCandidateFinder` and `ExhaustiveCandidateFinder` (which is even more simple than `SimpleCandidateFinder`). If `SimpleCandidateFinder` works correctly, then this comparison with `ExhaustiveCandidateFinder` is redundant. If it isn't, we ought to find that in a test specific to `SimpleCandidateFinder` rather than as a side-effect of testing `SearchCandidateFinder`. Split this functionality out into a new `clusterlin_simple_finder`.
* `clusterlin_linearize`: establishes the correctness of `Linearize` by comparing against both `SimpleLinearize` and literally every valid linearization for the cluster. Again, if `SimpleLinearize` works correctly, then this comparison with all valid linearizations is redundant, and if it isn't we should find it in a test for `SimpleLinearize`. Do so by splitting off that functionality into `clusterlin_simple_linearize`.
After that, a few general improvements to the affected tests are made (comparing with linearizations and subsets read from the fuzz input, plus a performance improvement).
ACKs for top commit:
marcofleon:
Re ACK d7fca5c171
ismaelsadeeq:
re-ACK d7fca5c171
monlovesmango:
ACK d7fca5c171
Tree-SHA512: 33cb76bd9b9547a5f3ee231fa452e928f064ad03af98e3d9e64246eb972f2b026c13e7367257ccdac1ae57982ee8ef98c907684588ecbb4bc4c82cbec160b3e8
For some reason, building x86_64-w64-mingw32 on x86_64 and aarch64
results in a single instruction difference which can be traced down to
prevector.h:174. The ultimate caller of this is the copy constructor for
a prevector that ends up being called by std::vector::push_back in
walletmodel.cpp:183. By replacing the push_back with an emplace_back,
somehow this non-determinism goes away.
