a60f863d3e276534444571282f432b913d3967db scripted-diff: Replace GenTxidVariant with GenTxid (marcofleon)
c8ba1995986323cd9e76097acc1f15eed7c60943 Remove old GenTxid class (marcofleon)
072a198ea4bc9f1e8449cd31e55d397b75ce4ad5 Convert remaining instances of GenTxid to GenTxidVariant (marcofleon)
1b528391c79497ae19f7e481439e350533c7cd1a Convert `txrequest` to GenTxidVariant (marcofleon)
bde4579b0780aa3754af35beffbcfeb31f28045b Convert `txdownloadman_impl` to GenTxidVariant (marcofleon)
c876a892ec0b04851bea0a688d7681b6aaca4cb7 Replace GenTxid with Txid/Wtxid overloads in `txmempool` (marcofleon)
de858ce2bea83c53635dee9a49c8c273a12440dd move-only: make GetInfo a private CTxMemPool member (stickies-v)
eee473d9f3019a0ea4ebbc9c41781813ad574a86 Convert `CompareInvMempoolOrder` to GenTxidVariant (marcofleon)
243553d59071f3e43a42f3809706790495b17ffc refactor: replace get_iter_from_wtxid with GetIter(const Wtxid&) (stickies-v)
fcf92fd640eae60d1f601136a4e1c9de8ccb68b5 refactor: make CTxMemPool::GetIter strongly typed (marcofleon)
11d28f21bb8f0c3094934b3fef45871f73bb216a 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 a60f863d3e276534444571282f432b913d3967db
maflcko:
review ACK a60f863d3e276534444571282f432b913d3967db 🎽
theStack:
Code-review ACK a60f863d3e276534444571282f432b913d3967db
Tree-SHA512: da9b73b7bdffee2eb9281a409205519ac330d3336094d17681896703fbca8099608782c9c85801e388e4d90af5af8abf1f34931f57bbbe6e9674d802d6066047
fa8862723c14cdd471bbffc7a4897ece2e6d8a7f fuzz: CheckGlobals in init (MarcoFalke)
fa26bfde988b1940e6b0d21accc60fbc4e45f9b1 test: Avoid resetting mocktime in testing setup (MarcoFalke)
fa6b45fa8ec8248544d22ba8429be8f6df19024a 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 fa8862723c14cdd471bbffc7a4897ece2e6d8a7f
Tree-SHA512: 5a9400f0467c82fa224713af4cc2b525afbefefc7c3f419077110925ad7af6c7fda3dcd2b50f7facf0ee7df2547c6ac20336906d707adcdfd1d652a9d9a735fe
d7fca5c171f450621112457ea6f7c99b2e21d354 clusterlin: add big comment explaning the relation between tests (Pieter Wuille)
b64e61d2de65026c46f70cb91e3cb2213c336be0 clusterlin: abstract try-permutations into ExhaustiveLinearize function (Pieter Wuille)
1fa55a64ed18a0a3defbed33f6bd355929f67b48 clusterlin tests: verify that chunks are minimal (Pieter Wuille)
da23ecef29b7e6a1e03de9697a6bdf6fbe944ef7 clusterlin tests: support non-empty ReadTopologicalSubset() (Pieter Wuille)
94f3e17c33e6fe63c4f791de7aacfb912059e394 clusterlin tests: compare with fuzz-provided linearizations (Pieter Wuille)
5f92ebee0d2401d0d7b5c3466d66a4b09e51ccb0 clusterlin tests: compare with fuzz-provided topological sets (Pieter Wuille)
6e37824ac390835d3d9c82d197f58e992ccc584f clusterlin tests: optimize clusterlin_simple_linearize (Pieter Wuille)
98c1c88b6f8d928b3893e2910ce0c400a6fd1928 clusterlin tests: separate testing of SimpleLinearize and Linearize (Pieter Wuille)
10e90f7aef9cff6fa63dc15adbe22e68f76aac58 clusterlin tests: make SimpleCandidateFinder always find connected (Pieter Wuille)
a38c38951e10a61af80e3bca1e1ae04de978d5c0 clusterlin tests: separate testing of Search- and SimpleCandidateFinder (Pieter Wuille)
77a432ee704b4a83d56135ed10cf2adf4b3c18af 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 d7fca5c171f450621112457ea6f7c99b2e21d354
ismaelsadeeq:
re-ACK d7fca5c171f450621112457ea6f7c99b2e21d354
monlovesmango:
ACK d7fca5c171f450621112457ea6f7c99b2e21d354
Tree-SHA512: 33cb76bd9b9547a5f3ee231fa452e928f064ad03af98e3d9e64246eb972f2b026c13e7367257ccdac1ae57982ee8ef98c907684588ecbb4bc4c82cbec160b3e8
8cc3ac6c2328873e57c31f237d194c5f22b6748a validation: Don't use IsValid() to filter for invalid blocks (Martin Zumsande)
86d98b94e5469e6476dc87f54b8ac25074603f0c test: verify that ancestors of a reconsidered block can become the chain tip (stratospher)
3c39a55e64bedfc384f6a7b9de1410bc8b46a9bb validation: Add ancestors of reconsiderblock to setBlockIndexCandidates (Martin Zumsande)
Pull request description:
When we call `reconsiderblock` for some block, `Chainstate::ResetBlockFailureFlags` puts the descendants of that block into `setBlockIndexCandidates` (if they meet the criteria, i.e. have more work than the tip etc.), but never put any ancestors into the set even though we do clear their failure flags.
I think that this is wrong, because `setBlockIndexCandidates` should always contain all eligible indexes that have at least as much work as the current tip, which can include ancestors of the reconsidered block. This is being checked by `CheckBlockIndex()`, which could fail if it was invoked after `ActivateBestChain` connects a block and releases `cs_main`:
``` diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 7b04bd9a5b..ff0c3c9f58 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3551,6 +3551,7 @@ bool Chainstate::ActivateBestChain(BlockValidationState& state, std::shared_ptr<
}
}
// When we reach this point, we switched to a new tip (stored in pindexNewTip).
+ m_chainman.CheckBlockIndex();
if (exited_ibd) {
// If a background chainstate is in use, we may need to rebalance our
```
makes `rpc_invalidateblock.py` fail on master.
Even though we don't currently have a `CheckBlockIndex()` in that place, after `cs_main` is released other threads could invoke it, which is happening in the rare failures of #16444 where an invalid header received from another peer could trigger a `CheckBlockIndex()` call that would fail.
Fix this by adding eligible ancestors to `setBlockIndexCandidates` in `Chainstate::ResetBlockFailureFlags` (also simplifying that function a bit).
Fixes#16444
ACKs for top commit:
achow101:
ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
TheCharlatan:
Re-ACK 8cc3ac6c2328873e57c31f237d194c5f22b6748a
stratospher:
reACK 8cc3ac6.
Tree-SHA512: 53f27591916246be4093d64b86a0494e55094abd8c586026b1247e4a36747bc3d6dbe46dc26ee4a22f47b8eb0d9699d13e577dee0e7198145f3c9b11ab2a30b7
1632fc104be8f171f59a023800b2f9f20d0a3cff txgraph: Track multiple potential would-be clusters in Trim (improvement) (Pieter Wuille)
4608df37e02abde09da5d6668dcd4dc5a6e7c569 txgraph: add Trim benchmark (benchmark) (Pieter Wuille)
9c436ff01cffe51e34d0f29c445db11bb807c6c3 txgraph: add fuzz test scenario that avoids cycles inside Trim() (tests) (Pieter Wuille)
938e86f8fecd65ca90b97e6cf896f8c59fb590ba txgraph: add unit test for TxGraph::Trim (tests) (glozow)
a04e205ab03efa105f7886449c2c9316f67a85c1 txgraph: Add ability to trim oversized clusters (feature) (Pieter Wuille)
eabcd0eb6fca5790ea19e7c1613ae06df8d21918 txgraph: remove unnecessary m_group_oversized (simplification) (Greg Sanders)
19b14e61eae7f0f0bfc8d17b06cc0e3ebd1f994d txgraph: Permit transactions that exceed cluster size limit (feature) (Pieter Wuille)
c4287b9b71c6d5222bcd0d2af3185de93ce76078 txgraph: Add ability to configure maximum cluster size/weight (feature) (Pieter Wuille)
Pull request description:
Part of cluster mempool (#30289).
During reorganisations, it is possible that dependencies get added which would result in clusters that violate policy limits (cluster count, cluster weight), when linking the new from-block transactions to the old from-mempool transactions. Unlike RBF scenarios, we cannot simply reject the changes when they are due to received blocks. To accommodate this, add a `TxGraph::Trim()`, which removes some subset of transactions (including descendants) in order to make all resulting clusters satisfy the limits.
Conceptually, the way this is done is by defining a rudimentary linearization for the entire would-be too-large cluster, iterating it from beginning to end, and reasoning about the counts and weights of the clusters that would be reached using transactions up to that point. If a transaction is encountered whose addition would violate the limit, it is removed, together with all its descendants.
This rudimentary linearization is like a merge sort of the chunks of the clusters being combined, but respecting topology. More specifically, it is continuously picking the highest-chunk-feerate remaining transaction among those which have no unmet dependencies left. For efficiency, this rudimentary linearization is computed lazily, by putting all viable transactions in a heap, sorted by chunk feerate, and adding new transactions to it as they become viable.
The `Trim()` function is rather unusual compared to the `TxGraph` functionality added in previous PRs, in that `Trim()` makes it own decisions about what the resulting graph contents will be, without good specification of how it makes that decision - it is just a best-effort attempt (which is improved in the last commit). All other `TxGraph` mutators are simply to inform the graph about changes the calling mempool code decided on; this one lets the decision be made by txgraph.
As part of this, the "oversized" property is expanded to also encompass a configurable cluster weight limit (in addition to cluster count limit).
ACKs for top commit:
instagibbs:
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff
glozow:
reACK 1632fc104be via range-diff
ismaelsadeeq:
reACK 1632fc104be8f171f59a023800b2f9f20d0a3cff 🛰️
Tree-SHA512: ccacb54be8ad622bd2717905fc9b7e42aea4b07f824de1924da9237027a97a9a2f1b862bc6a791cbd2e1a01897ad2c7c73c398a2d5ccbce90bfbeac0bcebc9ce
c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702 flatfile: check whether the file has been closed successfully (Vasil Dimov)
4bb5dd78ea4b578922a3316b37b486f96cb0beec util: check that a file has been closed before ~AutoFile() is called (Vasil Dimov)
8bb34f07df9ad45faf25c32c99a4dd70759b25be Explicitly close all AutoFiles that have been written (Vasil Dimov)
a69c4098b273b6db5d2212ba91cfc713c1634c5d rpc: take ownership of the file by WriteUTXOSnapshot() (Hodlinator)
Pull request description:
`fclose(3)` may fail to flush the previously written data to disk, thus a failing `fclose(3)` is as serious as a failing `fwrite(3)`.
Previously the code ignored `fclose(3)` failures. This PR improves that by changing all users of `AutoFile` that use it to write data to explicitly close the file and handle a possible error.
---
Other alternatives are:
1. `fflush(3)` after each write to the file (and throw if it fails from the `AutoFile::write()` method) and hope that `fclose(3)` will then always succeed. Assert that it succeeds from the destructor 🙄. Will hurt performance.
2. Throw nevertheless from the destructor. Exception within the exception in C++ I think results in terminating the program without a useful message.
3. (this is implemented in the latest incarnation of this PR) Redesign `AutoFile` so that its destructor cannot fail. Adjust _all_ its users 😭. For example, if the file has been written to, then require the callers to explicitly call the `AutoFile::fclose()` method before the object goes out of scope. In the destructor, as a sanity check, assume/assert that this is indeed the case. Defeats the purpose of a RAII wrapper for `FILE*` which automatically closes the file when it goes out of scope and there are a lot of users of `AutoFile`.
4. Pass a new callback function to the `AutoFile` constructor which will be called from the destructor to handle `fclose()` errors, as described in https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2243842400. My thinking is that if that callback is going to only log a message, then we can log the message directly from the destructor without needing a callback. If the callback is going to do more complicated error handling then it is easier to do that at the call site by directly calling `AutoFile::fclose()` instead of getting the `AutoFile` object out of scope (so that its destructor is called) and inspecting for side effects done by the callback (e.g. set a variable to indicate a failed `fclose()`).
ACKs for top commit:
l0rinc:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
achow101:
ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
hodlinator:
re-ACK c10e382d2a3b76b70ebb8a4eb5cd99fc9f14d702
Tree-SHA512: 3994ca57e5b2b649fc84f24dad144173b7500fc0e914e06291d5c32fbbf8d2b1f8eae0040abd7a5f16095ddf4e11fe1636c6092f49058cda34f3eb2ee536d7ba
Trim internally builds an approximate dependency graph of the merged cluster,
replacing all existing dependencies within existing clusters with a simple
linear chain of dependencies. This helps keep the complexity of the merging
operation down, but may result in cycles to appear in the general case, even
though in real scenarios (where Trim is called for stitching re-added mempool
transactions after a reorg back to the existing mempool transactions) such
cycles are not possible.
Add a test that specifically targets Trim() but in scenarios where it is
guaranteed not to have any cycles. It is a special case, is much more a
whitebox test than a blackbox test, and relies on randomness rather than
fuzz input. The upside is that somewhat stronger properties can be tested.
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
During reorganisations, it is possible that dependencies get add which
result in clusters that violate limits (count, size), when linking the
new from-block transactions to the old from-mempool transactions.
Unlike RBF scenarios, we cannot simply reject these policy violations
when they are due to received blocks. To accomodate this, add a Trim()
function to TxGraph, which removes transactions (including descendants)
in order to make all resulting clusters satisfy the limits.
In the initial version of the function added here, the following approach
is used:
- Lazily compute a naive linearization for the to-be-merged cluster (using
an O(n log n) algorithm, optimized for far larger groups of transactions
than the normal linearization code).
- Initialize a set of accepted transactions to {}
- Iterate over the transactions in this cluster one by one:
- If adding the transaction to the set makes it exceed the max cluster size
or count limit, stop.
- Add the transaction to the set.
- Remove all transactions from the cluster that were not included in the set
(note that this necessarily includes all descendants too, because they
appear later in the naive linearization).
Co-authored-by: Greg Sanders <gsanders87@gmail.com>
This removes the restriction added in the previous commit that individual
transactions do not exceed the max cluster size limit.
With this change, the responsibility for enforcing cluster size limits can
be localized purely in TxGraph, without callers (and in particular, tests)
needing to duplicate the enforcement for individual transactions.
This is integrated with the oversized property: the graph is oversized when
any connected component within it contains more than the cluster count limit
many transactions, or when their combined size/weight exceeds the cluster size
limit.
It becomes disallowed to call AddTransaction with a size larger than this limit,
though this limit will be lifted in the next commit.
In addition, SetTransactionFeeRate becomes SetTransactionFee, so that we do not
need to deal with the case that a call to this function might affect the
oversizedness.
a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf miniscript: Make `operator""_mst` `consteval` (Pieter Wuille)
14052162b19ac22f465f7db7880a6ab5d588a98c Revert "miniscript: make operator_mst consteval" (Hennadii Stepanov)
Pull request description:
Same as https://github.com/bitcoin/bitcoin/pull/28657, but without the refactoring required to work around [fixed](https://github.com/bitcoin/bitcoin/pull/28657#discussion_r2095743353) MSVC bugs.
The second commit has been taken from https://github.com/bitcoin/bitcoin/pull/29167.
ACKs for top commit:
sipa:
ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
hodlinator:
re-ACK a34fb9ad6c6cb4ffafdcefefa1ab957a430b69cf
Tree-SHA512: 8b531f9d6c450a8a5218865da05ffb5093d09ce2c0bee9874c0160795c4b1713928730d894ea3cd0b12b133346971ae3a00ed2fe8d9fd8a50b67a74ef81fde98
28299ce77636d7563ec545d043cf1b61bd2f01c1 p2p: remove vestigial READ_STATUS_CHECKBLOCK_FAILED (Greg Sanders)
bac9ee4830664c86c1cb3d38a5b19c722aae2f54 p2p: Add witness mutation check inside FillBlock (Greg Sanders)
Pull request description:
Since #29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside `PartiallyDownloadedBlock::FillBlock`, immediately before returning `READ_STATUS_OK`.
ACKs for top commit:
Crypt-iQ:
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
achow101:
ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
stratospher:
ACK 28299ce7.
dergoegge:
Code review ACK 28299ce77636d7563ec545d043cf1b61bd2f01c1
Tree-SHA512: 883d7c12ca096234b425e6fe12e46b0611607600916e6ac8d1c8112224aa76924b7b074754910163ac2ec15379075d618a9ece3642649ac7629cddb0d4e432ea
95969bc58ae0cd928e536d7cb8541de93e8c7205 test: added fuzz coverage to consensus/merkle.cpp (kevkevinpal)
Pull request description:
### Summary
This adds a new fuzz target "merkle" which adds fuzz coverage to `consensus/merkle.cpp`
I can also add this to an existing fuzz target if that is preferable
Before:
After:
ACKs for top commit:
marcofleon:
ReACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
Prabhat1308:
ACK [`95969bc`](95969bc58a)
maflcko:
lgtm ACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
achow101:
ACK 95969bc58ae0cd928e536d7cb8541de93e8c7205
Tree-SHA512: e1fe8b69444733516bfa6cf2adaa199fde4c7c5582b7b908408f9313ed0f2e8cb803d27d707a1716d49606d5eaef8c1e722990bbc3cffc30fa91fe73d2233e9d
This reverts commit 63317103c9f2b0635558da814567bb79c17ae851.
operator""_mst has been manually adjusted according to commit
faf21625652fd0d4bbf9b86fd9ebedb5857505ea
There is no way to report a close error from `AutoFile` destructor.
Such an error could be serious if the file has been written to because
it may mean the file is now corrupted (same as if write fails).
So, change all users of `AutoFile` that use it to write data to
explicitly close the file and handle a possible error.
Rather than this exhaustive linearization check happening inline inside
clusterlin_simple_linearize, abstract it out into a Linearize()-like
function for clarity.
Note that this isn't exactly a refactor, because the old code would compare the
found linearization against all (valid) permutations, while the new code instead
first computes the best linearization from all valid permutations, and then
compares it with the found one.
In several call sites for ReadTopologicalSubset, a non-empty result is
expected, necessitating a special case at the call site for empty results.
Fix this by adding a bool non_empty argument, which does this special
casing (more efficiently) inside ReadTopologicalSubset itself.
Whenever a non-topological permutation is encountered, fast forward to the
last permutation with the same non-topological prefix, skipping over
potentially many permutations that are non-topological for the same reason.
With that, increase the checking of all permutations to clusters of size 8
instead of 7.
The separates the existing fuzz test into:
* clusterlin_linearize: establishes the correctness of Linearize() using the
simpler SimpleLinearize() function.
* clusterlin_simple_linearize: establishes the correctness of SimpleLinearize() by
comparing with all valid linearizations computed by
std::next_permutation.
rather than combining the first two into a single fuzz test.
This separates the existing fuzz test into:
* clusterlin_search_finder: establishes SearchCandidateFinder's correctness using the
simpler SimpleCandidateFinder.
* clusterlin_simple_finder: establishes SimpleCandidateFinder's correctness using the
(even) simpler ExhaustiveCandidateFinder.
rather than trying to do both at once.
Only count the number of actual new subsets added. If the queue contains
a work item that completely covers a component, no transaction can be added
to it without creating a disconnected component. In this case, also don't
count it as an iteration.
With this, the number of iterations performed by SimpleCandidateFinder is
bounded by the number of distinct connected topologically-valid subsets of
the cluster.
IsValid() also returns false for blocks that have not been
validated yet up to the default validity level of BLOCK_VALID_TRANSACTIONS but
are not marked as invalid - e.g. if we only know the header.
Here, we specifically want to filter for invalid blocks.
Also removes the default arg from IsValid() which is now unused outside
of tests, to prevent this kind of misuse for the future.
Co-authored-by: TheCharlatan <seb.kung@gmail.com>
fa9ca13f35be0a023aeed78775ad66f95717b28b refactor: Sort includes of touched source files (MarcoFalke)
facb152697b8d7b75a9e6108f8896f774b06b35f scripted-diff: Bump copyright headers after include changes (MarcoFalke)
fae71d30f7227594e2f59499cf7f7f9420284e04 clang-tidy: Apply modernize-deprecated-headers (MarcoFalke)
Pull request description:
Bitcoin Core is written in C++, so it is confusing to sometimes use the deprecated C headers (with the `.h` extension). For example, it is less clear whether `string.h` refers to the file in this repo or the cstring stdlib header (https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797).
The check is currently disabled for headers, to exclude subtree headers.
ACKs for top commit:
l0rinc:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
achow101:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
janb84:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
stickies-v:
ACK fa9ca13f35be0a023aeed78775ad66f95717b28b
Tree-SHA512: 6639608308c598d612e24435aa519afe92d71b955874b87e527245291fb874b67f3ab95d3a0a5125c6adce5eb41c0d62f6ca488fbbfd60a94f2063d734173f4d
Since #29412, we have not allowed mutated blocks to continue
being processed immediately the block is received, but this
is only done for the legacy BLOCK message.
Extend these checks as belt-and-suspenders to not allow
similar mutation strategies to affect relay by honest peers
by applying the check inside
PartiallyDownloadedBlock::FillBlock, immediately before
returning READ_STATUS_OK.
This also removes the extraneous CheckBlock call.
a189d636184b1c28fa4a325b56c1fab8f44527b1 add release note for datacarriersize default change (Greg Sanders)
a141e1bf501bb2660f3a62083a65678250085e56 Add more OP_RETURN mempool acceptance functional tests (Peter Todd)
0b4048c73385166144d0b3e76beb9a2ac4cc1eca datacarrier: deprecate startup arguments for future removal (Greg Sanders)
63091b79e70b8e230a122fa6fb3dac91c80638e7 test: remove unnecessary -datacarriersize args from tests (Greg Sanders)
9f36962b07eff2369577a17c8adeaa0433697e1c policy: uncap datacarrier by default (Greg Sanders)
Pull request description:
Retains the `-datacarrier*` args, marks them as deprecated, and does not require another startup argument for multiple OP_RETURN outputs.
If a user has set `-datacarriersize` the value is "budgeted" across all seen OP_RETURN output scriptPubKeys. In other words the total script bytes stays the same, but can be spread across any number of outputs. This is done to not introduce an additional argument to support multiple outputs.
I do not advise people use the option with custom arguments and it is marked as deprecated to not mislead as a promise to offer it forever. The argument itself can be removed in some future release to clean up the code and minimize footguns for users.
ACKs for top commit:
stickies-v:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
Sjors:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
polespinasa:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
hodlinator:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
ajtowns:
reACK a189d636184b1c28fa4a325b56c1fab8f44527b1
mzumsande:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
petertodd:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
theStack:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
1440000bytes:
re-ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
willcl-ark:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
dergoegge:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
fanquake:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
murchandamus:
ACK a189d636184b1c28fa4a325b56c1fab8f44527b1
darosior:
Concept ACK a189d636184b1c28fa4a325b56c1fab8f44527b1.
Tree-SHA512: 3da2f1ef2f50884d4da7e50df2121bf175cb826edaa14ba7c3068a6d5b2a70beb426edc55d50338ee1d9686b9f74fdf9e10d30fb26a023a718dd82fa1e77b038
cfc42ae5b7ef6d3892907cfe36dc3ae923495d37 fuzz: add a target for the coins database (Antoine Poinsot)
46e14630f7fe8e2d51adc18a4f50345eb26970c7 fuzz: move the coins_view target's body into a standalone function (Antoine Poinsot)
56d878c4650cc46ce54d1d79db054ac44b486605 fuzz: avoid underflow in coins_view target (Antoine Poinsot)
Pull request description:
This reopens https://github.com/bitcoin/bitcoin/pull/28216.
The current `coins_view` target only tests `CCoinsViewCache` using a basic `CCoinsView` instance. The addition of the `coins_view_db` target enables testing with an actual `CCoinsViewDB` as the backend.
ACKs for top commit:
maflcko:
lgtm ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
l0rinc:
code review ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
TheCharlatan:
ACK cfc42ae5b7ef6d3892907cfe36dc3ae923495d37
Tree-SHA512: d3a92f122629f075767453a1abd9819a1c9716db53b997418993fef62d27683324740d0a8f84df76d8a7a45e508ccadeb69553b6f69e29a1238cd7c0be5276ca
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~0 )
-END VERIFY SCRIPT-
This can be reproduced according to the developer notes with something
like
( cd ./src/ && ../contrib/devtools/run-clang-tidy.py -p ../bld-cmake -fix -j $(nproc) )
Also, the header related changes were done manually.
Datacarrier output script sizes and output counts are now
uncapped by default.
To avoid introducing another startup argument, we modify the
OP_RETURN accounting to "budget" the spk sizes.
If a user has set a custom default, this results in that
budget being spent over the sum of all OP_RETURN outputs'
scripts in the transaction, no longer capping the number
of OP_RETURN outputs themselves. This should allow a
superset of current behavior while respecting the passed
argument in terms of total arbitrary data storage.
Co-authored-by: Anthony Towns <aj@erisian.com.au>
It reuses the logic from the `coins_view` target, except it uses an
in-memory CCoinsViewDB as the backend.
Note `CCoinsViewDB` will assert the best block hash is never null, so we
slightly modify the coins_view fuzz logic to take care of this.
fd290730f530a8b76a9607392f49830697cdd7c5 validation: clean up and clarify CheckInputScripts logic (Cory Fields)
1a37507895402ee08b1f248262701d4f848647e1 validation: use a lock for CCheckQueueControl (Cory Fields)
c3b0e6c7f4828291cd136717fddf1df878f3ca20 validation: make CCheckQueueControl's CCheckQueue non-optional (Cory Fields)
4c8c90b5567a3f31444bf0b151c3109e85ac2329 validation: only create a CCheckQueueControl if it's actually going to be used (Cory Fields)
11fed833b3ed6d5c96957de5addc4f903b2cee6c threading: add LOCK_ARGS macro (Cory Fields)
Pull request description:
As part of an effort to cleanup our threading primitives and add safe `SharedMutex`/`SharedLock` impls, I'd like to get rid of the last of our legacy `ENTER_CRITICAL_SECTION`/`LEAVE_CRITICAL_SECTION` usage. This, along with a follow-up [after fixing REVERSE_LOCK](https://github.com/bitcoin/bitcoin/pull/32465) will allow us to do that.
This replaces the old macros with an RAII lock, while simplifying `CCheckQueueControl`. It now requires a `CCheckQueue`, and optionality is handled externally. In the case of validation, it is wrapped in a `std::optional`.
It also adds an `LOCK_ARGS` macro for `UniqueLock` initialization which may be helpful elsewhere.
ACKs for top commit:
fjahr:
re-ACK fd290730f530a8b76a9607392f49830697cdd7c5
ryanofsky:
Code review ACK fd290730f530a8b76a9607392f49830697cdd7c5, just removing assert since last review. Thanks for considering all the comments and feedback!
TheCharlatan:
Re-ACK fd290730f530a8b76a9607392f49830697cdd7c5
Tree-SHA512: 54b9db604ee1bda7d11bce1653a88d3dcbc4f525eed6a85abdd4d6409138674af4bb8b00afa4e0d3d29dadd045a3a39de253a45f0ef9c05f56cba1aac5b59303
785e1407b0a39fef81a7b25554aab88d4cecd66b wallet: Use util::Error throughout AddWalletDescriptor (Ava Chow)
Pull request description:
#32023 changed `AddWalletDescriptor` to return `util::Error`, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use `util::Error` so that all callers handle `AddWalletDescriptor` errors in the same way.
The encapsulated return type is changed from `ScriptPubKeyMan*` to `std::reference_wrapper<DescriptorScriptPubKeyMan>`. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of `ScriptPubKeyMan` that can come out of `AddWalletDescriptor` is a `DescriptorScriptPubKeyMan` anyways.
ACKs for top commit:
Sjors:
utACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
ryanofsky:
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
furszy:
Code review ACK 785e1407b0a39fef81a7b25554aab88d4cecd66b
Tree-SHA512: 52a48263c8d4161a8c0419b7289c25b0986f8e3bcd10b639eeeb0b6862d08b6c5e70998d20070ab26b39ecd90ab83dc8b71c65d85f70626282cf8cc6abff50e7
