fa03d0acd6 fuzz: Create a block template in tx_pool targets (MarcoFalke)
fa61ce5cf5 fuzz: Limit mocktime to MTP in tx_pool targets (MarcoFalke)
fab646b8ea fuzz: Use correct variant of ConsumeRandomLengthString instead of hardcoding a maximum size (MarcoFalke)
fae2c8bc54 fuzz: Allow to pass min/max to ConsumeTime (MarcoFalke)
Pull request description:
Relatively simple check to ensure a block can always be created from the mempool
ACKs for top commit:
practicalswift:
Tested ACK fa03d0acd6
Tree-SHA512: e613376ccc88591cbe594db14ea21ebc9b2b191f6325b3aa4ee0cd379695352ad3b480e286134ef6ee30f043d486cf9792a1bc7e44445c41045ac8c3b931c7ff
91d93aac4e validation: remove nchaintx from assumeutxo metadata (James O'Beirne)
931684b24a validation: fix ActivateSnapshot to use hardcoded nChainTx (James O'Beirne)
Pull request description:
This fixes an oversight from the move of nChainTx from the user-supplied
snapshot metadata into the hardcoded assumeutxo chainparams.
Since the nChainTx is now unused in the metadata, it should be removed
in a future commit.
See: https://github.com/bitcoin/bitcoin/pull/19806#discussion_r612165410
ACKs for top commit:
Sjors:
utACK 91d93aac4e
ryanofsky:
Code review ACK 91d93aac4e. No change to previous commit, just new commit removing now unused utxo snapshot field and updating tests.
Tree-SHA512: 445bdd738faf007451f40bbcf360dd1fb4675e17a4c96546e6818c12e33dd336dadd95cf8d4b5f8df1d6ccfbc4bf5496864bb5528e416cea894857b6b732140c
fac30eec42 refactor: Replace &foo[0] with foo.data() (MarcoFalke)
faece47c47 refactor: Avoid &foo[0] on C-Style arrays (MarcoFalke)
face961109 refactor: Use only one temporary buffer in CreateObfuscateKey (MarcoFalke)
fa05dddc42 refactor: Use CPubKey vector constructor where possible (MarcoFalke)
fabb6dfe6e script: Replace address-of idiom with vector data() method (Guido Vranken)
Pull request description:
The main theme of this refactor is to replace `&foo[0]` with `foo.data()`.
The first commit is taken from #21781 with the rationale:
* In CSignatureCache::ComputeEntryECDSA, change the way a vector pointer is resolved to prevent invoking undefined behavior if the vector is empty.
The other commits aim to remove all `&foo[0]`, where `foo` is any kind of byte representation. The rationale:
* Sometimes alternative code without any raw data pointers is easier to read (refer to the respective commit message for details)
* If the raw data pointer is needed, `foo.data()` should be preferred, as pointed out in the developer notes. This addresses the instances that have been missed in commit 592404f03f, and https://github.com/bitcoin/bitcoin/pull/9804
ACKs for top commit:
laanwj:
Code review ACK fac30eec42
practicalswift:
cr ACK fac30eec42: patch looks correct
promag:
Code review ACK fac30eec42.
Tree-SHA512: e7e73146edbc78911a8e8c728b0a1c6b0ed9a88a008e650aa5dbffe72425bd42c76df70199a9cf7e02637448d7593e0eac52fd0f91f59240283e1390ee21bfa5
5252f86eb6 fuzz: Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of #ifdef forests (practicalswift)
54549dda31 fuzz: RPC fuzzer post-merge follow-ups. Remove unused includes. Update list of fuzzed RPC commands. (practicalswift)
Pull request description:
Various RPC fuzzer follow-ups:
* Remove unused includes.
* Update list of fuzzed RPC commands.
* Reduce maintenance requirements by allowing RPC annotations also for conditionally available RPC commands (such as wallet commands) without the fragility of `#ifdef` forests.
Context: https://github.com/bitcoin/bitcoin/pull/21169#pullrequestreview-646723483
ACKs for top commit:
MarcoFalke:
Concept ACK 5252f86eb6
Tree-SHA512: 286d70798131706ffb157758e1c73f7f00ed96ce120c7d9dc849e672b283f1362df47b206cfec9da44d5debb5869225e721761dcd5c38a7d5d1019dc6c912ab2
It does not matter if the tests fail due to a BOOST_CHECK failure or
due to a thrown exception. Prefer the exception because it is less
code.
Example fail with the throwing accessor:
unknown location(0): fatal error: in "script_standard_tests/script_standard_ExtractDestinations": std::bad_variant_access: std::get: wrong index for variant
test/script_standard_tests.cpp(314): last checkpoint
*** 1 failure is detected in the test module "Bitcoin Core Test Suite"
ebd4be43cc doc: add release notes for 20867 (Antoine Poinsot)
5aa50ab9cc rpc/util: multisig: only check redeemScript size is <= 520 for P2SH (Antoine Poinsot)
063df9e897 test/functional: standardness sanity checks for P2(W)SH multisig (Antoine Poinsot)
ae0429d3af script: allow up to 20 keys in wsh() descriptors (Antoine Poinsot)
9fc68faf35 script: match multisigs with up to MAX_PUBKEYS_PER_MULTISIG keys (Antoine Poinsot)
Pull request description:
As described in https://github.com/bitcoin/bitcoin/issues/20620 multisigs are currently limited to 16 keys in descriptors and RPC helpers, even for P2WSH and P2SH-P2WSH.
This adds support for multisig with up to 20 keys (which are already standard) for Segwit v0 context for descriptors (`wsh()`, `sh(wsh())`) and RPC helpers.
Fixes https://github.com/bitcoin/bitcoin/issues/20620
ACKs for top commit:
meshcollider:
re-utACK ebd4be43cc
instagibbs:
re-ACK ebd4be43cc
Tree-SHA512: 36141f10a8288010d17d5c4fe8d24878bcd4533b88a8aba3a44fa8f74ceb3182d70fee01427e0ab7f53ce7fab46c88c1cd3ac3b18ab8a10bd4a6b8b74ed79e46
5f96d7d22d rpc: gettxoutsetinfo rejects hash_serialized_2 for specific height (Fabian Jahr)
23fe50436b test: Add test for coinstatsindex behavior in reorgs (Fabian Jahr)
90c966b0f3 rpc: Allow gettxoutsetinfo and getblockstats for stale blocks (Fabian Jahr)
b9362392ae index, rpc: Add use_index option for gettxoutsetinfo (Fabian Jahr)
bb7788b121 test: Test coinstatsindex robustness across restarts (Fabian Jahr)
e0938c2909 test: Add tests for block_info in gettxoutsetinfo (Fabian Jahr)
2501576ecc rpc, index: Add verbose amounts tracking to Coinstats index (Fabian Jahr)
655d929836 test: add coinstatsindex getindexinfo coverage, improve current tests (Jon Atack)
ca01bb8d68 rpc: Add Coinstats index to getindexinfo (Fabian Jahr)
57a026c30f test: Add unit test for Coinstats index (Fabian Jahr)
6a4c0c09ab test: Add functional test for Coinstats index (Fabian Jahr)
3f166ecc12 rpc: gettxoutsetinfo can be requested for specific blockheights (Fabian Jahr)
3c914d58ff index: Coinstats index can be activated with command line flag (Fabian Jahr)
dd58a4de21 index: Add Coinstats index (Fabian Jahr)
a8a46c4b3c refactor: Simplify ApplyStats and ApplyHash (Fabian Jahr)
9c8a265fd2 refactor: Pass hash_type to CoinsStats in stats object (Fabian Jahr)
2e2648a902 crypto: Make MuHash Remove method efficient (Fabian Jahr)
Pull request description:
This is part of the coinstats index project tracked in #18000
While the review of the new UTXO set hash algorithm (MuHash) takes longer recently #19328 was merged which added the possibility to run `gettxoutsetinfo` with a specific hash type. As the first type it added `hash_type=none` which skips the hashing of the UTXO set altogether. This alone did not make `gettxoutsetinfo` much faster but it allows the use of an index for the remaining coin statistics even before a new hashing algorithm has been added. Credit to Sjors for the idea to take this intermediate step.
Features summary:
- Users can start their node with the option `-coinstatsindex` which syncs the index in the background
- After the index is synced the user can use `gettxoutsetinfo` with `hash_type=none` or `hash_type=muhash` and will get the response instantly out of the index
- The user can specify a height or block hash when calling `gettxoutsetinfo` to see coin statistics at a specific block height
ACKs for top commit:
Sjors:
re-tACK 5f96d7d22d
jonatack:
Code review re-ACK 5f96d7d22d per `git range-diff 13d27b4 07201d3 5f96d7d`
promag:
Tested ACK 5f96d7d22d. Light code review ACK 5f96d7d22d.
Tree-SHA512: cbca78bee8e9605c19da4fbcd184625fb280200718396c694a56c7daab6f44ad23ca9fb5456d09f245d8b8d9659fdc2b3f3ce5e953c1c6cf4003dbc74c0463c2
83a425d25a compressor: use a prevector in compressed script serialization (William Casarin)
Pull request description:
This function was doing millions of unnecessary heap allocations during IBD.
I'm start to catalog unnecessary heap allocations as a pet project of mine: as-zero-as-possible-alloc IBD. This is one small step.
before:
after:
~should I type alias this?~ *I type aliased it*
This is a part of the Zero Allocations Project #18849 (ZAP1). This code came up as a place where many allocations occur.
ACKs for top commit:
Empact:
ACK 83a425d25a
elichai:
tACK 83a425d25a
sipa:
utACK 83a425d25a
Tree-SHA512: f0ffa6ab0ea1632715b0b76362753f9f6935f05cdcc80d85566774401155a3c57ad45a687942a1806d3503858f0bb698da9243746c8e2edb8fdf13611235b0e0
We were previously ruling out 17-20 pubkeys multisig, while they are
only invalid under P2SH context.
This makes multisigs with up to 20 keys be detected as valid by the
solver. This is however *not* a policy change as it would only apply
to bare multisigs, which are already limited to 3 pubkeys.
Note that this does not change the sigOpCount calculation (as it would
break consensus). Therefore 1-16 keys multisigs are counted as 1-16 sigops
and 17-20 keys multisigs are counted as 20 sigops.
Signed-off-by: Antoine Poinsot <darosior@protonmail.com>
545404e7e1 fuzz: Add RPC interface fuzzing. Increase fuzzing coverage from 65% to 70%. (practicalswift)
Pull request description:
Add RPC interface fuzzing.
This PR increases overall fuzzing line coverage from [~65%](https://marcofalke.github.io/btc_cov/fuzz.coverage/) to ~70% 🎉
To test this PR:
```
$ make distclean
$ ./autogen.sh
$ CC=clang CXX=clang++ ./configure --enable-fuzz --with-sanitizers=address,fuzzer,undefined
$ make -C src/ test/fuzz/fuzz
$ FUZZ=rpc src/test/fuzz/fuzz
```
See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for more information on how to fuzz Bitcoin Core. Don't forget to contribute any coverage increasing inputs you find to the [Bitcoin Core fuzzing corpus repo](https://github.com/bitcoin-core/qa-assets).
Happy fuzzing :)
ACKs for top commit:
MarcoFalke:
Concept ACK 545404e7e1
Tree-SHA512: 35fc1b508af42bf480ee3762326b09ff2eecdb7960a1917ad16345fadd5c0c21d666dafe736176e5a848ff6492483c782e4ea914cd9000faf50190df051950fd
This value is no longer used and is instead specified statically
in chainparams. This change means that previously generated
snapshots will no longer be usable.
8c8237a4a1 net, refactor: Fix style in CConnman::StopNodes (Hennadii Stepanov)
229ac1892d net: Combine two loops into one, and update comments (Hennadii Stepanov)
a3d090d110 net: Restrict period when cs_vNodes mutex is locked (Hennadii Stepanov)
Pull request description:
This PR restricts the period when the `cs_vNodes` mutex is locked, prevents the only case when `cs_vNodes` could be locked before the `::cs_main`.
This change makes the explicit locking of recursive mutexes in the explicit order redundant.
ACKs for top commit:
jnewbery:
utACK 8c8237a4a1
vasild:
ACK 8c8237a4a1
ajtowns:
utACK 8c8237a4a1 - logic seems sound
MarcoFalke:
review ACK 8c8237a4a1👢
Tree-SHA512: a8277924339622b188b12d260a100adf5d82781634cf974320cf6007341f946a7ff40351137c2f5369aed0d318f38aac2d32965c9b619432440d722a4e78bb73
0b188b751f Clean up context dependent checks in descriptor parsing (Pieter Wuille)
33275a9649 refactor: move uncompressed-permitted logic into ParsePubkey* (Pieter Wuille)
17e006ff8d refactor: split off subscript logic from ToStringHelper (Pieter Wuille)
6ba5dda0c9 Account for key cache indices in subexpressions (Pieter Wuille)
4441c6f3c0 Make DescriptorImpl support multiple subscripts (Pieter Wuille)
a917478db0 refactor: move population of out.scripts from ExpandHelper to MakeScripts (Pieter Wuille)
84f3939ece Remove support for subdescriptors expanding to multiple scripts (Pieter Wuille)
Pull request description:
These are a few refactors and non-invasive improvements to the descriptors code to prepare for adding Taproot descriptors.
None of the commits change behavior in any way, except the last one which improves error reporting a bit.
ACKs for top commit:
S3RK:
reACK 0b188b7
Sjors:
re-ACK 0b188b7
achow101:
re-ACK 0b188b751f
Tree-SHA512: cb4e999134aa2bace0e13d4883454c65bcf1369e1c8585d93cc6444ddc245f3def5a628d58af7dab577e9d5a4a75d3bb46f766421fcc8cc5c85c01a11f148b3f
fa8eaee6a8 test: Run versionbits_sanity for all chains (MarcoFalke)
faec1e9ee1 test: Address outstanding versionbits_test feedback (MarcoFalke)
fad4167871 test: Check that no versionbits are re-used (MarcoFalke)
Pull request description:
ACKs for top commit:
jnewbery:
Code review ACK fa8eaee6a8
ajtowns:
ACK fa8eaee6a8 code review only
Tree-SHA512: e99ffcca8970921fd07fa9e04cf1ea2515a317409865d34ddfd70be0f0b0616b29d1fad58262d96a3c3418c0cf7018a6a955802a178b8f78f6ecfaa30a37d91c
bb8d1c6e02 Change ClearDataDirPathCache() to ArgsManager.ClearPathCache(). (Kiminuo)
b4190eff72 Change GetBlocksDir() to ArgsManager.GetBlocksDirPath(). (Kiminuo)
83292e2a70 scripted-diff: Modify unit tests to use the ArgsManager in the BasicTestingSetup class instead of implicitly relying on gArgs. (Kiminuo)
55c68e6f01 scripted-diff: Replace m_args with m_local_args in getarg_tests.cpp (Kiminuo)
511ce3a26b BasicTestingSetup: Add ArgsManager. (Kiminuo)
1cb52ba065 Modify "util_datadir" unit test to not use gArgs. (Kiminuo)
1add318704 Move GetDataDir(fNetSpecific) implementation to ArgsManager. (Kiminuo)
70cdf679f8 Move StripRedundantLastElementsOfPath before ArgsManager class. (Kiminuo)
Pull request description:
This PR attempts to contribute to "Remove gArgs" (#21005).
Main changes:
* `GetDataDir()` function is moved to `ArgsManager.GetDataDirPath()`.
* `GetBlocksDir()` function is moved to `ArgsManager.GetBlocksDirPath()`.
ACKs for top commit:
ryanofsky:
Code review ACK bb8d1c6e02. Just minor const/naming changes and splitting/scripting commits since last review
MarcoFalke:
review ACK bb8d1c6e02📓
hebasto:
re-ACK bb8d1c6e02, addressed comments, and two commits made scripted-diffs since my [previous](https://github.com/bitcoin/bitcoin/pull/21244#pullrequestreview-638270583) review.
Tree-SHA512: ba9408c22129d6572beaa103dca0324131766f06d562bb7d6b9e214a0a4d40b0216ce861384562bde24b744003b3fbe6fac239061c8fd798abd3981ebc1b9019
b109bde46a [test] check that mapFlagNames is up to date (glozow)
5d3ced72f9 [test] remove unnecessary OP_1s from invalid tests (glozow)
5aee73d175 [test] minor improvements / followups (glozow)
8a365df558 [test] fix bug in ExcludeIndividualFlags (glozow)
8cac2923f5 [test] remove invalid test from tx_valid.json (glozow)
Pull request description:
This is a followup to #19698.
- There was a bug in the `ExcludeIndividualFlags` function which is fixed here.
- Fixing this bug also showed that there is a test that's supposed to fail (already existing in tx_invalid.json) in tx_valid.json, so I removed it. Other than that, the tests should all pass.
- Also implements a few suggestions I received offline: removing the `OP_1`s from the invalid tests (similar to 19db590d04), comments, and style.
- A few other small fixes, like adding asserts, putting all the flags in `mapFlagNames`, better error messages
ACKs for top commit:
jnewbery:
utACK b109bde46a
Tree-SHA512: 7233a8c0f1ae1172fac8000ea6e05384ecf79074c39948d118464868505c7f02f17e96503c81bd05c07adb2087648a5d93d9899e16fdefa6b7efcb51319444a9
586190f0b4 rpc/rest: Take and reuse local Chain/ChainState obj (Carl Dong)
bc3bd36902 rpc: style: Improve BuriedForkDescPushBack signature (Carl Dong)
f99913969f rpc: Remove unnecessary casting of block height (Carl Dong)
6a3d192020 rpc: Tidy up local references (see commit message) (Carl Dong)
038854f31e rest/rpc: Remove now-unused old Ensure functions (Carl Dong)
6fb65b49f4 scripted-diff: rest/rpc: Use renamed EnsureAny*() (Carl Dong)
1570c7ee98 rpc: Add renamed EnsureAny*() functions (Carl Dong)
306b1cd3ee rpc: Add alt Ensure* functions acepting NodeContext (Carl Dong)
d7824acdb9 rest: Use existing NodeContext (Carl Dong)
3f08934799 rest: Pass in NodeContext to rest_block (Carl Dong)
7be0671b95 rpc/rawtx: Use existing NodeContext (Carl Dong)
60dc05afc6 rpc/mining: Use existing NodeContext (Carl Dong)
d485e815e2 rpc/blockchain: Use existing NodeContext (Carl Dong)
d0abf0bf42 rpc/*,rest: Add review-only assertion to EnsureChainman (Carl Dong)
cced0f46c9 miner: Pass in previous CBlockIndex to RegenerateCommitments (Carl Dong)
Pull request description:
Overall PR: #20158 (tree-wide: De-globalize ChainstateManager)
Based on:
- [x] #21270 | [Bundle 4/n] Prune g_chainman usage in validation-adjacent modules
- [x] #21525 | [Bundle 4.5/n] Followup fixups to bundle 4
Note to reviewers:
1. This bundle may _apparently_ introduce usage of `g_chainman` or `::Chain(state|)Active()` globals, but these are resolved later on in the overall PR. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
2. There may be seemingly obvious local references to `ChainstateManager` or other validation objects which are not being used in callers of the current function in question, this is done intentionally to **_keep each commit centered around one function/method_** to ease review and to make the overall change systematic. We don't assume anything about our callers. Rest assured that once we are considering that particular caller in later commits, we will use the obvious local references. [Commits of overall PR](https://github.com/bitcoin/bitcoin/pull/20158/commits)
3. When changing a function/method that has many callers (e.g. `LookupBlockIndex` with 55 callers), it is sometimes easier (and less error-prone) to use a scripted-diff. When doing so, there will be 3 commits in sequence so that every commit compiles like so:
1. Add `new_function`, make `old_function` a wrapper of `new_function`, divert all calls to `old_function` to `new_function` **in the local module only**
2. Scripted-diff to divert all calls to `old_function` to `new_function` **in the rest of the codebase**
3. Remove `old_function`
ACKs for top commit:
ryanofsky:
Code review ACK 586190f0b4. Since last review, no changes to existing commits, just some simple new commits added: three new commits renaming std::any Ensure functions (scripted diff commit and manual pre/post commits), and one new commit factoring out a repeated `ActiveChain()` call made in a loop. Thanks for the updates!
jnewbery:
utACK 586190f0b4
MarcoFalke:
review ACK 586190f0b4🍯
Tree-SHA512: 64b677fb50141805b55c3f1afe68fcd298f9a071a359bdcd63256d52e334f83e462f31fb3ebee9b630da8f1d912a03a128cfc38179e7aaec29a055744a98478c
63631beef6 test: Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic` (practicalswift)
Pull request description:
Remove intermittently failing and not very meaningful `BOOST_CHECK` in `cnetaddr_basic`.
Fixes #21682.
Rationale from https://github.com/bitcoin/bitcoin/issues/21682#issuecomment-819897122:
> I've looked at that test before and I don't think that specific `BOOST_CHECK` makes much sense TBH :)
>
> 1.) I don't understand why we test if `ToString()` output includes `%zone_index`: it clearly doesn't on some platforms, so we cannot rely on it anyways. Then why test it?
>
> 2.) And perhaps more fundamentally: why would we even _want_ to have `%zone_index` in our textual `ToString()` output? I think the expectation is to get say `fe80::1ff:fe23:4567:890a` (without zone index) and not say `fe80::1ff:fe23:4567:890a%eth2 ` or `fe80::1ff:fe23:4567:890a%3 `when doing `ipv6_addr.ToString()` :)
ACKs for top commit:
MarcoFalke:
review ACK 63631beef6
Tree-SHA512: 06863d1edfb9ad1ca9bcae09cf3f0f47b58bb29d222b70799c3dc059b96452889026e4b99b132782846d9896e3e798d17c7f9406e0e6a0bec1bffc6edb54e9df
549c82ad3a fuzz: use ConsumeBool() instead of !ConsumeBool() (Vasil Dimov)
29ae1c13a5 fuzz: split FuzzedSock interface and implementation (Vasil Dimov)
9668e43d8e fuzz: make FuzzedSock::Wait() sometimes simulate an occurred event (Vasil Dimov)
0c90ff1429 fuzz: set errno from FuzzedSock::Wait() if it simulates a failure (Vasil Dimov)
5198a02de4 style: remove extra white space (Vasil Dimov)
Pull request description:
* split FuzzedSock interface and implementation
* make FuzzedSock::Wait() sometimes simulate an occurred event
* set errno from FuzzedSock::Wait() if it simulates a failure
(this is a followup from https://github.com/bitcoin/bitcoin/pull/21617)
ACKs for top commit:
practicalswift:
cr ACK 549c82ad3a: patch looks correct and touches only `src/test/fuzz/`
MarcoFalke:
re-ACK 549c82ad3a only change is rebase 🎬
Tree-SHA512: 8ba965a8319074ad2ef840219c35c77e37cc79f00fb3926f20ccbf5f58e9616f5a3ac96434ad33996b47d292fa760d5d00a529001ac0d1d254262e5df93f616f
fa40d6a1c4 test: Reset mocktime in the common setup (MarcoFalke)
fa78590a8f test: Use mocktime to avoid intermittent failure (MarcoFalke)
Pull request description:
See https://github.com/bitcoin/bitcoin/pull/21602#discussion_r611176103
ACKs for top commit:
jonatack:
Code review ACK fa40d6a1c4
jarolrod:
ACK fa40d6a1c4
Tree-SHA512: 4967e006f3d2c4eb92f03c9086a6abe3190ad54755d251c30d20422c574bb1a154c06f3d5bcb0d4deaa3c4abfd3864d743b71d84897edd358e829bb42233ad12
