bbbb51877a fuzz: Speed up transaction fuzz target (MarcoFalke)
Pull request description:
`hashBlock` and `include_addresses` are orthogonal, so no need to do an exhaustive "search".
Might fix https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34491
ACKs for top commit:
practicalswift:
cr ACK bbbb51877a: patch looks correct, and `TxToUniv` surprisingly wide in the `transaction_fuzz_target` flame graph! Putting it on a diet makes sense.
Tree-SHA512: 1e7c30c7fecf96364a9a1597c0a22139389fdeb67db59f3c2c6fc088196e3332877b2865991a957980d542f99a2f48cc066dd7cc16c695a5113190fe06205089
393992b049 fuzz: Terminate immediately if a fuzzing harness ever tries to create a TCP socket (belt and suspenders) (practicalswift)
Pull request description:
Terminate immediately if a fuzzing harness ever to create a TCP socket (belt and suspenders).
Obviously this _should_ never happen, but if it _does_ happen we want immediate termination instead of a TCP socket :)
ACKs for top commit:
MarcoFalke:
ACK 393992b049
Tree-SHA512: 5bbff1f7e9a58b3eae24f742b7daf3fc870424c985f29bed5931e47a708d9c0984bfd8762f43658cffa9c69d32f86d56deb48bc7e43821e3398052174b6a160e
ce6bca88e8 doc: release note for getnodeaddresses by network (Jon Atack)
3f89c0e990 test: improve getnodeaddresses coverage, test by network (Jon Atack)
6c98c09991 rpc: enable filtering getnodeaddresses by network (Jon Atack)
80ba294854 p2p: allow CConnman::GetAddresses() by network, add doxygen (Jon Atack)
a49f3ddbba p2p: allow CAddrMan::GetAddr() by network, add doxygen (Jon Atack)
c38981e748 p2p: pull time call out of loop in CAddrMan::GetAddr_() (João Barbosa)
d35ddca91e p2p: enable CAddrMan::GetAddr_() by network, add doxygen (Jon Atack)
Pull request description:
This patch allows passing a network argument to CAddrMan::GetAddr(), CConnman::GetAddresses(), and rpc getnodeaddresses to return only addresses of that network.
It also contains a performance optimisation by promag.
ACKs for top commit:
laanwj:
Code review and lightly tested ACK ce6bca88e8
vasild:
ACK ce6bca88e8
Tree-SHA512: 40e700d97091248429c73cbc0639a1f03ab7288e636a7b9026ad253e9708253c6b2ec98e7d9fb2d56136c0f762313dd648915ac98d723ee330d713813a43f99d
e286cd0d7b net: flag relevant Sock methods with [[nodiscard]] (Vasil Dimov)
Pull request description:
Flag relevant Sock methods with `[[nodiscard]]` to avoid issues like the one fixed in https://github.com/bitcoin/bitcoin/pull/21631.
ACKs for top commit:
practicalswift:
cr ACK e286cd0d7b: the only changes made are additions of `[[nodiscard]]` and `(void)` where appropriate
laanwj:
Code review ACK e286cd0d7b
Tree-SHA512: addc361968d24912bb625b42f4db557791556bf0ffad818252a89a32d76ac22758ec70f8282dcfbfd77eebec20a8e6bb7557c8ed08d50a58de95378c34955973
7075f604e8 scripted-diff: update noban documentation in net_processing.cpp (Jon Atack)
a95540cf43 scripted-diff: rename NetPermissionFlags enumerators (Jon Atack)
810d0929c1 p2p, refactor: make NetPermissionFlags a uint32 enum class (Jon Atack)
7b55a94497 p2p: NetPermissions::HasFlag() pass flags param by value (Jon Atack)
91f6e6e6d1 scripted-diff: add NetPermissionFlags scopes where not already present (Jon Atack)
Pull request description:
While reviewing #20196, I noticed the `NetPermissionFlags` enums are frequently called as if they were scoped, yet are still global. This patch upgrades `NetPermissionFlags` to a scoped class enum and updates the enumerator naming, similarly to #19771. See https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#enum-enumerations for more info.
This change would eliminate the class of bugs like https://github.com/bitcoin/bitcoin/pull/20196#discussion_r610770148 and #21644, as only defined operations on the flags would compile.
ACKs for top commit:
laanwj:
Code review ACK 7075f604e8
vasild:
ACK 7075f604e8
Tree-SHA512: 7fcea66ee499f059efc78c934b5f729b3c8573fe304dee2c27c837c2f662b89324790568246d75b2a574cf9f059b42d3551d928996862f4358055eb43521e6f4
fae814c9a6 fuzz: Remove incorrect float round-trip serialization test (MarcoFalke)
Pull request description:
It tests the wrong way of the round-trip: `int -> float -> int`, but only `float -> int -> float` is allowed and used. See also `src/test/fuzz/float.cpp`.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34118
ACKs for top commit:
laanwj:
Anyhow, ACK fae814c9a6
Tree-SHA512: 8412a7985be2225109f382b7c7ea6d6fcfbea15711671fdf2f41dd1a9adbb3b4489592863751d78bedaff98e9b0b13571d9cae06ffd92db8fbf7ce0f47874a41
- drop redundant PF_ permission flags prefixes
- drop ALL_CAPS naming per https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps
- rename IsImplicit to Implicit
-BEGIN VERIFY SCRIPT-
s() { git grep -l "$1" src | xargs sed -i "s/$1/$2/g"; }
s 'PF_NONE' 'None'
s 'PF_BLOOMFILTER' 'BloomFilter'
s 'PF_RELAY' 'Relay'
s 'PF_FORCERELAY' 'ForceRelay'
s 'PF_DOWNLOAD' 'Download'
s 'PF_NOBAN' 'NoBan'
s 'PF_MEMPOOL' 'Mempool'
s 'PF_ADDR' 'Addr'
s 'PF_ISIMPLICIT' 'Implicit'
s 'PF_ALL' 'All'
-END VERIFY SCRIPT-
99993f0664 fuzz: Avoid excessively large min fee rate in tx_pool (MarcoFalke)
Pull request description:
Any fee rate above 1 BTC / kvB is clearly nonsense, so no need to fuzz this.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34078
ACKs for top commit:
practicalswift:
cr ACK 99993f0664: patch looks correct despite no `fa` prefix in commit hash
Tree-SHA512: bd3651d354b13d889ad1708d2b385ad0479de036de74a237346eefad5dbfb1df76ec02b55ec00487ec598657ef6102f992302b14c4e47f913a9962f81f4157e6
fa95555a49 fuzz: Limit max insertions in timedata fuzz test (MarcoFalke)
Pull request description:
It is debatable whether a size of the median filter other than `200` (the only size used in production) should be fuzzed. For now add a minimal patch to cap the max insertions. Otherwise the complexity is N^2 log(N), where N is the size of the fuzz input.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34167
ACKs for top commit:
practicalswift:
cr ACK fa95555a49: patch looks correct
Tree-SHA512: be7737e9f4c906053e355641de84dde31fed37ed6be4c5e92e602ca7675dffdaf06b7063b9235ef541b05d3d5fd689c99479317473bb15cb5271b8baabffd0f2
36fb036d25 p2p: allow NetPermissions::ClearFlag() only with PF_ISIMPLICIT (Jon Atack)
4e0d5788ba test: add net permissions noban/download unit test coverage (Jon Atack)
dde69f20a0 p2p, bugfix: use NetPermissions::HasFlag() in CConnman::Bind() (Jon Atack)
Pull request description:
This is a bugfix follow-up to #16248 and #19191 that was noticed in #21506. Both v0.21 and master are affected.
Since #19191, noban is a multi-flag that implies download, so the conditional in `CConnman::Bind()` using a bitwise AND on noban will return the same result for both the noban status and the download status. This means that download peers are incorrectly not being added to local addresses because they are mistakenly seen as noban peers.
The second commit adds unit test coverage to illustrate and test the noban/download relationship and the `NetPermissions` operations involving them.
The final commit adds documentation and disallows calling `NetPermissions::ClearFlag()` with any second param other than `NetPermissionFlags` "implicit" -- per current usage in the codebase -- because `ClearFlag()` should not be called with any second param that is a subflag of a multiflag, e.g. "relay" or "download," as that would leave the result in an invalid state corresponding to none of the existing NetPermissionFlags. Thanks to Vasil Dimov for noticing this.
ACKs for top commit:
theStack:
re-ACK 36fb036d25☕
vasild:
ACK 36fb036d25
hebasto:
ACK 36fb036d25, I have reviewed the code and it looks OK, I agree it can be merged.
kallewoof:
Code review ACK 36fb036d25
Tree-SHA512: 5fbc7ddbf31d06b35bf238f4d77ef311e6b6ef2e1bb9893f32f889c1a0f65774a3710dcb21d94317fe6166df9334a9f2d42630809e7fe8cbd797dd6f6fc49491
facfc0f65d fuzz: Remove strprintf test cases that are known to fail (MarcoFalke)
Pull request description:
They are still waiting to be fixed (see https://github.com/c42f/tinyformat/issues/70 ), so no need for us to carry them around in our source code. They can be added back once upstream is fixed.
Hopefully fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=34082
ACKs for top commit:
laanwj:
Code review ACK facfc0f65d
Tree-SHA512: d9d3d35555b6d58740a041ae45797ca85149f60990e2ed632c5dadf363e1d2362d2447681d7ceaa1fbffcd6e7bc8da5bc15d3923b68829a86c25b364a599afc8
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
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
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
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
