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.
Dropped the default expected_hash parameter from `ReadBlock()`.
In `blockmanager_flush_block_file` tests, we pass {} since the tests would already fail at PoW validation for corrupted blocks.
In `ChainstateManager::LoadExternalBlockFile`, we pass {} when processing child blocks because their hashes aren't known beforehand.
Switch to the index-aware `ReadBlock()` overload in `ComputeFilter` so that filter creation will abort if the stored block header hash doesn't match the expected one.
In the `readwriteblock` benchmark, pass the expected hash to `ReadBlock()` to match the new signature without affecting benchmark performance.
029ba1a21d570f7db6c4366ec9a30a381b56d6fb index: remove CBlockIndex access from CustomAppend() (furszy)
91b7ab6c69264a46f70825546a1574478d9e824a refactor: index, simplify CopyHeightIndexToHashIndex to process single block (furszy)
6f1392cc42cde638773f2b697d7d2c58abcdc860 indexes, refactor: Remove remaining CBlockIndex* uses in index Rewind methods (Ryan Ofsky)
0a248708dc9d465db09168c39b3f12cb4c9465b7 indexes, refactor: Stop requiring CBlockIndex type to call IsBIP30Unspendable (Ryan Ofsky)
331a25cb16632042dd6782a9b62fcc5c8aa6da3b test: indexes, avoid creating threads when sync runs synchronously (furszy)
Pull request description:
Combining common refactors from #24230 and #26966, aiming to move both efforts forward while reducing their size and review burden.
Broadly, #24230 focuses on enabling indexes to run in a separate process, and #26966 aims to parallelize the indexes initial synchronization process. A shared prerequisite for both is ensuring that only the base index class interacts with the node’s chain internals - child index classes should instead operate solely through chain events.
This PR moves disk read lookups from child index classes to the base index class. It also includes a few documentation improvements and a test-only code cleanup.
ACKs for top commit:
maflcko:
review ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb 👡
achow101:
ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb
TheCharlatan:
Re-ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb
davidgumberg:
ACK 029ba1a21d570f7db6c
mzumsande:
Code Review ACK 029ba1a21d570f7db6c4366ec9a30a381b56d6fb
Tree-SHA512: f073af407fc86f228cb47a32c7bcf2241551cc89ff32059317eb81d5b86fd5fda35f228d2567e0aedbc9fd6826291f5fee05619db35ba44108421ae04d11e6fb
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
When splitting a string, sometimes the separator needs to be included.
Split will now optionally include the separator at the end of the left
side of the splits, i.e. it appears at the end of the splits, except
for the last one.
Specifically, for musig() descriptors, Split is used to separate a
musig() from any derivation path that follows it by splitting on the
closing parentheses. Since that parentheses is needed for Func() and
Expr(), Split() needs to preserve the end parentheses instead of
discarding it.
When parsing a descriptor, it is useful to be able to check whether a
string begins with a substring without consuming that substring as
another function such as Func() will be used later which requires that
substring to be present at the beginning.
Specifically, for MuSig2, this modified Const will be used to determine
whether a an expression begins with "musig(" before a subsequent
Func("musig", ...) is used.
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
* Make the methods of `CThreadInterrupt` virtual and store a pointer to
it in `CConnman`, thus making it possible to override with a mocked
instance.
* Initialize `CConnman::m_interrupt_net` from the constructor, making it
possible for callers to supply mocked version.
* Introduce `FuzzedThreadInterrupt` and `ConsumeThreadInterrupt()` and
use them in `src/test/fuzz/connman.cpp` and `src/test/fuzz/i2p.cpp`.
This improves the CPU utilization of the `connman` fuzz test.
As a nice side effect, the `std::shared_ptr` used for
`CConnman::m_interrupt_net` resolves the possible lifetime issues with
it (see the removed comment for that variable).
Now that all network calls done by `CConnman::OpenNetworkConnection()`
are done via `Sock` they can be redirected (mocked) to `FuzzedSocket`
for testing.
`FuzzedSock::Accept()` properly returns a new socket, but it forgot to
set the output argument `addr`, like `accept(2)` is expected to.
This could lead to reading uninitialized data during testing when we
read it, e.g. from `CService::SetSockAddr()` which reads the `sa_family`
member.
Set `addr` to a fuzzed IPv4 or IPv6 address.
The indexes test call StartBackgroundSync(), which spawns a thread to run Sync(),
only for the test thread to wait for it to complete by calling IndexWaitSynced().
So, since the sync is performed synchronously, we can skip the extra thread creation
entirely and call Sync() directly.
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>
This change moves binaries that are not typically invoked directly by users
from the `bin/` directory to the `libexec/` directory in CMake installs and
binary releases. The goal is to simplify the contents of `bin/` for end users
while still making all binaries available when needed. After this change, the
binaries remaining in `bin/` are:
- bitcoin
- bitcoin-cli
- bitcoind
- bitcoin-qt
- bitcoin-tx
- bitcoin-util
- bitcoin-wallet
And the binaries that are moved to `libexec/` are:
- bench_bitcoin
- bitcoin-chainstate(*)
- bitcoin-gui(***)
- bitcoin-node(***)
- test_bitcoin(**)
- test_bitcoin-qt
(*) bitcoin-chainstate was previously missing an install rule and was actually
not installed even when it was enabled.
(**) test_bitcoin is the only libexec/ binary that is currently included in
bitcoin binary releases. The others are only installed when building from
source with relevant cmake options enabled.
(***) bitcoin-node and bitcoin-gui are not currently built by default or
included in binary releases but both of these changes are planned and
implemented in #31802
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.
09ee8b7f278627b917f0784adf23cbc76cae5fa0 node: avoid recomputing block hash in `ReadBlock` (Lőrinc)
2bf173210fa1ba7b28fd312b2e62bc9ff45af598 test: exercise `ReadBlock` hash‑mismatch path (Lőrinc)
Pull request description:
Eliminate one block header hash calculation per block-read by reusing the hash for:
* proof‑of‑work verification;
* (optional) integrity check against the supplied hash.
This part of the code wasn't covered by tests either, so the first commit exercises this part first, before pushing the validation to the delegate method.
ACKs for top commit:
maflcko:
lgtm ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
achow101:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
jonatack:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
pinheadmz:
ACK 09ee8b7f278627b917f0784adf23cbc76cae5fa0
Tree-SHA512: 43fe51b478ea574b6d4c952684b13ca54fb8cbd67c3b6c136f460122d9ee953cc70b88778537117eecea71ccb8d88311faeac21b866e11d117f1145973204ed4
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
faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40 doc: Remove ParseInt mentions in documentation (MarcoFalke)
33332829333b589420f8038541d04ec6970f051d refactor: Remove unused Parse(U)Int* (MarcoFalke)
fa84e6c36cb0accf87123ca4eb98f6219d55fb5e bitcoin-tx: Reject + sign in MutateTxDel* (MarcoFalke)
face2519fac9e840d52f0334d9079e664be7eb28 bitcoin-tx: Reject + sign in vout parsing (MarcoFalke)
fa8acaf0b993c879ee8c516baa36339ff7b72406 bitcoin-tx: Reject + sign in replaceable parsing (MarcoFalke)
faff25a558ab15b5d8eeea5dd4c9c0d76350051b bitcoin-tx: Reject + sign in locktime (MarcoFalke)
dddd9e5fe38b81f1af6b343661b65e16b0de7c60 bitcoin-tx: Reject + sign in nversion parsing (MarcoFalke)
fab06ac03788243847b799a3feaac56bc918fba9 rest: Use SAFE_CHARS_URI in SanitizeString error msg (MarcoFalke)
8888bb499dec79258b1857b404d72f93650503f4 rest: Reject + sign in /blockhashbyheight/ (MarcoFalke)
fafd43c69192fcb48a9e04d52eeb07fff15655d0 test: Reject + sign when parsing regtest deployment params (MarcoFalke)
fa123afa0ef752e8645bf695d121da66d8f1382b Reject + sign when checking -ipcfd (MarcoFalke)
fa479857ed234d54df31d33b60de14c6ffab3d6f Reject + sign in SplitHostPort (MarcoFalke)
fab4c2967d554ddbc15f732cea6cd190c547d89f net: Reject + sign when parsing subnet mask (MarcoFalke)
fa89652e68fc07fb6c9f3d9e34dc11d35f0cc1e1 init: Reject + sign in -*port parsing (MarcoFalke)
fa9c45577dfbae67535e4965b5660288557d3631 cli: Reject + sign in -netinfo level parsing (MarcoFalke)
fa980413257e2004a8d48a8be66c6d67f90b76ad refactor: Use ToIntegral in CreateFromDump (MarcoFalke)
fa23ed7fc24212d85cdc7f52b317906b37a1a120 refactor: Use ToIntegral in ParseHDKeypath (MarcoFalke)
Pull request description:
The legacy int parsing is problematic, because it accepts the `+` sign for unsigned integers. In all cases this is either:
* Useless, because the `+` sign was already rejected.
* Erroneous and inconsistent, when third party parsers reject it. (C.f. https://github.com/bitcoin/bitcoin/pull/32365)
* Confusing, because the `+` sign is neither documented, nor can it be assumed to be present.
Fix all issues by removing the legacy int parsing.
ACKs for top commit:
stickies-v:
re-ACK faf55fc80b
brunoerg:
code review ACK faf55fc80b11f3d9b0b12c1b26a9612ea9ce9b40
Tree-SHA512: a311ab6a58fe02a37741c1800feb3dcfad92377b4bfb61b433b2393f52ba89ef45d00940972b2767b213a3dd7b59e5e35d5b659c586eacdfe4e565a77b12b19f
3a18075aedd7cff6f06b5fe10966d618b6378701 ci: Drop `-DENABLE_EXTERNAL_SIGNER=ON` configure option (Hennadii Stepanov)
719fa9f4ef6886c7c3be3b33d249b16f4e36a7e9 build: Re-enable external signer support for Windows (Hennadii Stepanov)
6e5fc2bf9b18cd492fe994dacb39182f601e9b86 test: Reintroduce Windows support in `system_tests/run_command` test (Hennadii Stepanov)
Pull request description:
This PR partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.h`.
ACKs for top commit:
Sjors:
ACK 3a18075aedd7cff6f06b5fe10966d618b6378701.
theStack:
Light ACK 3a18075aedd7cff6f06b5fe10966d618b6378701
laanwj:
Code review and lightly tested ACK 3a18075aedd7cff6f06b5fe10966d618b6378701
Tree-SHA512: 00d200685906e716750aae7cffa0794cca451653738ea590f50dfa28e1f3c5762a9be0ae0917aa0cf7436f00fe1e565236bff2853896530a5879466f7f45cb25
