aboutsummaryrefslogtreecommitdiff
path: root/src/test
AgeCommit message (Collapse)Author
2021-02-05Merge #20839: fuzz: Avoid extraneous copy of input data, using Span<>Wladimir J. van der Laan
faf7d7418cf01cb04cd457bcc630654da958a777 fuzz: Avoid extraneous copy of input data, using Span<> (MarcoFalke) Pull request description: Seeing speedup here in the fuzz framework part (non-fuzz-target part). Speedup is only visible for input data larger than 100kB. ACKs for top commit: practicalswift: cr ACK faf7d7418cf01cb04cd457bcc630654da958a777: patch looks correct :) laanwj: Code review ACK faf7d7418cf01cb04cd457bcc630654da958a777 Tree-SHA512: 41af7118846e0dfee237a6d5269a6c7cfbc775d7bd1cc2a85814cb60f6c2b37fe7fd35f1a788d4f08e6e0202c48b71054b67d2931160c445c79fc59e5347dadf
2021-02-04Merge #20715: util: Add ArgsManager::GetCommand() and use it in bitcoin-walletMarcoFalke
fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke) 7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke) fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke) fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke) Pull request description: This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937 Fixes: #20902 ACKs for top commit: ajtowns: ACK fa61b9d1a68820758f9540653920deaeae6abe79 fjahr: Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79 Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b
2021-02-03fuzz: Avoid extraneous copy of input data, using Span<>MarcoFalke
2021-02-01Merge #20464: refactor: Treat CDataStream bytes as uint8_tWladimir J. van der Laan
fa292724598c273867bc6dbf311f1440fe2541ba Remove redundant MakeUCharSpan wrappers (MarcoFalke) faf4aa2f47c0de4f3a0c5f5fe5b3ec32f611eefd Remove CDataStream::Init in favor of C++11 member initialization (MarcoFalke) fada14b948cac147198e3b685b5dd8cb72dc2911 Treat CDataStream bytes as uint8_t (MarcoFalke) fa8bdb048e65cae2d26bea3f991717a856e2fb39 refactor: Drop CDataStream constructors in favor of one taking a Span of bytes (MarcoFalke) faa96f841fe45bc49ebb6e07ac82a129fa9c40bf Remove unused CDataStream methods (MarcoFalke) Pull request description: Using `uint8_t` for raw bytes has a style benefit: * The signedness is clear from reading the code, as it does not depend on the architecture Other clean-ups in this pull include: * Remove unused methods * Constructor is simplified with `Span` * Remove `Init()` member in favor of C++11 member initialization ACKs for top commit: laanwj: code review ACK fa292724598c273867bc6dbf311f1440fe2541ba theStack: ACK fa292724598c273867bc6dbf311f1440fe2541ba 🍾 Tree-SHA512: 931ee28bd99843d7e894b48e90e1187ffb0278677c267044b3c0c255069d9bbd9298ab2e539b1002a30b543d240450eaec718ef4ee95a7fd4be0a295e926343f
2021-02-01Merge #21016: refactor: remove boost::thread_group usageWladimir J. van der Laan
dc8be12510c2fd5a809d9a82d2c14b464b5e5a3f refactor: remove boost::thread_group usage (fanquake) Pull request description: Post #18710, there isn't much left using `boost::thread_group`, so should just be able to replace it with the standard library. This also removes the last use of `boost::thread_interrupted`. After this change, last piece of Boost Thread we'd be using is `boost::shared_mutex`. See the commentary [here](https://github.com/bitcoin/bitcoin/issues/16684#issuecomment-726214696) as to why it may be non-trivial to swap that for `std::shared_mutex` in the near future. Closes #17307 ACKs for top commit: laanwj: Code review re-ACK dc8be12510c2fd5a809d9a82d2c14b464b5e5a3f MarcoFalke: review ACK dc8be12510c2fd5a809d9a82d2c14b464b5e5a3f 🔁 jonatack: Non-expert code review ACK dc8be12510c2fd5a809d9a82d2c14b464b5e5a3f, also checked range-diff since last review and that local debug build is clean with gcc 10.2.1-6 on Debian Tree-SHA512: 5510e2d760cce824234207dc86b1551ca8f21cbf3a2ce753c0254a0d03ffd83c94e449aec202fb7bd76e6fc64df783a6b70a736b0add9ece3734bb9c8ce8fc2f
2021-02-01Merge #20749: [Bundle 1/n] Prune g_chainman usage related to ::LookupBlockIndexWladimir J. van der Laan
67c9a83df19c6e2a2edb32336879204e7770b4a7 style-only: Remove redundant sentence in ActivateBestChain comment (Carl Dong) b8e95658d5909f93dfc7d1e6532661db8919e5b7 style-only: Make TestBlockValidity signature readable (Carl Dong) 0cdad753903640ff4240b715dec9d62f68e51407 validation: Use accessible chainstate in ChainstateManager::ProcessNewBlock (Carl Dong) ea4fed90219be17160136313c68c06d84176af08 validation: Use existing chainstate in ChainstateManager::ProcessNewBlockHeaders (Carl Dong) e0dc3057277c9576ddbfb8541599db0149e08bb6 validation: Move LoadExternalBlockFile to CChainState (Carl Dong) 5f8cd7b3a527999512161956db4d718688cb956f validation: Remove global ::ActivateBestChain (Carl Dong) 2a696472a1423e877bfa83f016f66c7e45be7369 validation: Pass in chainstate to ::NotifyHeaderTip (Carl Dong) 9c300cc8b3ce3d82874982fbf3087e48a6ac0ef2 validation: Pass in chainstate to TestBlockValidity (Carl Dong) 0e17c833cda67cdba5338bd7409061772b6d5edb validation: Make CChainState.m_blockman public (Carl Dong) d363d06bf7d6c3736140672ba8a7f82f4d6fb6ab validation: Pass in blockman to ContextualCheckBlockHeader (Carl Dong) f11d11600ddb0ddff6538250ae2a35df6112c3db validation: Move GetLastCheckpoint to BlockManager (Carl Dong) e4b95eefbc700ebc915bec312f77477ce3e87a7e validation: Move GetSpendHeight to BlockManager (Carl Dong) b026e318c39f59a06e29f1b25c7f577e01b25ccb validation: Move FindForkInGlobalIndex to BlockManager (Carl Dong) 3664a150ac7547c9336b571557af223d9e31aac9 validation: Remove global LookupBlockIndex (Carl Dong) eae54e6e60d7ed05b29d8345c0bb055397149ce8 scripted-diff: Use BlockManager::LookupBlockIndex (Carl Dong) 15d20f40e1321b24963b40c12958c7d30ad112ec validation: Move LookupBlockIndex to BlockManager (Carl Dong) f92dc6557a153b390a1ae1d0808ff7ed5d02c66e validation: Guard the active_chainstate with cs_main (Carl Dong) Pull request description: Overall PR: #20158 (tree-wide: De-globalize ChainstateManager) 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: jnewbery: utACK 67c9a83df19c6e2a2edb32336879204e7770b4a7 laanwj: re-ACK 67c9a83df19c6e2a2edb32336879204e7770b4a7 ryanofsky: Code review ACK 67c9a83df19c6e2a2edb32336879204e7770b4a7. Changes since last review: Tree-SHA512: 8744aba2dd57a40cd2fedca809b0fe24d771bc60da1bffde89601999384aa0df428057a86644a3f72fbeedbc8b04db6c4fd264ea0db2e73c279e5acc6d056cbf
2021-01-30fuzz: Avoid designated initialization (C++20) in fuzz testspracticalswift
2021-01-29refactor: remove boost::thread_group usagefanquake
2021-01-28validation: Move LoadExternalBlockFile to CChainStateCarl Dong
[META] This commit should be followed up by removing the comments and assertions meant only to show that the change is correct. LoadExternalBlockFile mainly acts on CChainState.
2021-01-28validation: Remove global ::ActivateBestChainCarl Dong
Instead use CChainState::ActivateBestChain, which is what the global one calls anyway.
2021-01-28scripted-diff: Use BlockManager::LookupBlockIndexCarl Dong
[META] In a previous commit, we moved ::LookupBlockIndex to become a member function of BlockManager. This commit is split out from that one since it can be expressed nicely as a scripted-diff. -BEGIN VERIFY SCRIPT- find_regex='LookupBlockIndex' \ && git grep -l -E "$find_regex" -- src \ | grep -v '^src/validation\.\(cpp\|h\)$' \ | xargs sed -i -E "s@${find_regex}@g_chainman.m_blockman.LookupBlockIndex@g" -END VERIFY SCRIPT-
2021-01-28Merge #20995: fuzz: Avoid initializing version to less than ↵MarcoFalke
MIN_PEER_PROTO_VERSION fad3d7625aa1c2b6c343946e709e87e7168f9d9d fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSION (MarcoFalke) fa99e33aebed0109630474e11183b0726b410c2e fuzz: move-only FillNode implementation to cpp file (MarcoFalke) Pull request description: This fixes a fuzz bug introduced in #20881. Previously the nodes in the fuzz tests had their version initialized to a constant (`PROTOCOL_VERSION`). After #20881, the nodes have their version initialized to an arbitrary signed integer. This is problematic for several reasons: * Both `nVersion` and `m_greatest_common_version` may be initialized to `0`. If a `version` message is processed, this leads to a crash, because `m_greatest_common_version` must be `INIT_PROTO_VERSION` while the `version` message is processed. See #20138 * The "valid" range for `nVersion` is `[MIN_PEER_PROTO_VERSION, std::numeric_limits<int32_t>::max()]` (see check in net_processing) * The "valid" range for `m_greatest_common_version` is `std::min(nVersion, PROTOCOL_VERSION)` (see net_processing) Fix all issues by initializing `nVersion` and `m_greatest_common_version` to their valid ranges. ----- The crashers, if someone wants to try this at home: ``` ( echo 'dmVyc2lvbgAWFhYWFhYWFhYWFhYWFhYWFhYWFhZp/29uAPX//xYWFhYWFhYWFhYWFhYWFhYWFhYW FhYWFhYWaW9uAOr1//8WFhYWFha0ZXJzaW9uAPX//wAAAAAAABAAAAAAAAAAAAC0ZXJzaW9uAPX/ /wBPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT08AAAAAABAAAAAAAAAAAAAAAAAA AAAAAAAAAAAAAAAAAAAAAAAAAAAACgAAAAAAAAAAgAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA AAAAAAAAAAAAAAB2ZXJzaW9uAACDJIO9vXYKAAAAAAAAAAAAAAAAAAAAAAB2ZfS1qmu1qhUVFWs=' | base64 --decode > /tmp/a ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/a ``` ``` ( echo 'dmVyc2lvbgD//wAhTmiqN///NDcAAACENDL/iv//8DYAAHL///////79/RtcAJqamhqa/QEAAAD/ ///+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZyq8SaGVhZGVycwAAAAD/NDcAAACENDL/iv// 8DYAAHL///////79/RtcAJqamhqa/QEAAAD////+/f1oZWFkZXJzAAAAAM8BAAAAIAYibkYRGgtZ yq8SaGVhZGVycwAAAADPAQAAACAGIm5GERoLWS1wb3J061u/KMNPOkwFXqZ///b5IgIAAD+5ubkb XD5hZGRyAJqamhqasP0BAAAAAAAAAP0BAAAAIf39/R0dHQAAAAAAMgAA///7//+gXqZ///b5IgIA AD+5ubm5ubm5AAAAAAAAAAAAAAAAAAAAAAAAAACAAAAAAAAAFgAAAAAAAAAAAAlBmv39/f1/f39B f39hZGRyAG5vAACaLgAdGzY2zwEAAAAgBiJuRhEaC1ktcG9ydOtbvyjDTzpMBV6mf//2+SICAAA/ ubm5G1w+YWRkcgCampoamrD9AQAAAAAAAAD9AQAAACH9/f0dHR0AAAAAADIAAP//+///oF6mf//2 +SICAAA/ubm5ubm5uQAAAAAAAAAAAAAAAAAAAAAAAAAAgAAAAAAAABYAAAAAAAAAAAAJQZr9/f39 f39/QX9/YWRkcgBubwAAmi4AHRs2NjY2NjY2NjYCAgI2NgIA/f39/f39Nv39/TUmABxc' | base64 --decode > /tmp/b ) && FUZZ=process_message_version ./src/test/fuzz/fuzz /tmp/b ``` ACKs for top commit: practicalswift: cr ACK fad3d7625aa1c2b6c343946e709e87e7168f9d9d Tree-SHA512: ea64ee99b94d8e619e3949d2d21252c1236412c0e40f44f2b73595ca70cd2da0bdab005fb1a54f65fb291e7b07fdd33577ce4a3a078ca933246b511ebcb0e52a
2021-01-26Merge bitcoin-core/gui#183: Add include for std::bind.MarcoFalke
2a39ccf1334ef3c48c6f9969a0fc916b9e10aae1 Add include for std::bind. (sinetek) Pull request description: Hi, this patch adds in <functional> because the GUI code makes use of std::bind. That's all. ACKs for top commit: jonasschnelli: utACK 2a39ccf1334ef3c48c6f9969a0fc916b9e10aae1 Tree-SHA512: fb5ac07d9cd5d006182b52857b289a9926362a2f1bfa4f7f1c78a088670e2ccf39ca28214781df82e8de3909fa3e69685fe1124a7e3ead758575839f5f2277a9
2021-01-26Merge #21000: fuzz: Add UBSan suppressions needed for fuzz tests to not warn ↵MarcoFalke
under -fsanitize=integer f0f8b1a076c362c6e26570a2129809f4d6a0abad fuzz: Add UBSan suppressions needed for fuzz tests to not warn under -fsanitize=integer (practicalswift) 58232e3ffb319d27b41cfe040cfea8e43a3fb7e9 fuzz: Avoid -fsanitize=integer warnings in fuzzing harnesses (practicalswift) Pull request description: Add UBSan suppressions needed for fuzz tests to not warn under `-fsanitize=integer`. Avoid `-fsanitize=integer` warnings in fuzzing harnesses. Suppressed warnings (excluding warnings from `src/crypto/` and `src/test/`): ``` addrman.cpp:306:24: runtime error: implicit conversion from type 'long' of value 5190149478 (64-bit, signed) to type 'uint32_t' (aka 'unsigned int') changed the value to 895182182 (32-bit, unsigned) addrman.h:446:43: runtime error: implicit conversion from type 'int' of value -22 (32-bit, signed) to type 'const uint8_t' (aka 'const unsigned char') changed the value to 234 (8-bit, unsigned) arith_uint256.cpp:32:35: runtime error: left shift of 1712128 by 24 places cannot be represented in type 'uint32_t' (aka 'unsigned int') arith_uint256.cpp:47:39: runtime error: left shift of 4294966784 by 31 places cannot be represented in type 'uint32_t' (aka 'unsigned int') chain.cpp:151:12: runtime error: implicit conversion from type 'int' of value -1 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551615 (64-bit, unsigned) coins.cpp:114:22: runtime error: unsigned integer overflow: 0 - 96 cannot be represented in type 'unsigned long' compressor.cpp:162:33: runtime error: unsigned integer overflow: 15617702637291228364 * 10 cannot be represented in type 'unsigned long' compressor.cpp:188:11: runtime error: unsigned integer overflow: 2265760372865400000 * 10 cannot be represented in type 'unsigned long' hash.cpp:13:15: runtime error: left shift of 1692305888 by 15 places cannot be represented in type 'uint32_t' (aka 'unsigned int') pubkey.h:152:23: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'unsigned int' streams.h:570:31: runtime error: left shift of 350879 by 52 places cannot be represented in type 'uint64_t' (aka 'unsigned long') util/bip32.cpp:57:36: runtime error: left shift of 3241096244 by 1 places cannot be represented in type 'unsigned int' util/strencodings.cpp:562:38: runtime error: implicit conversion from type 'unsigned char' of value 255 (8-bit, unsigned) to type 'char' changed the value to -1 (8-bit, signed) util/strencodings.h:164:24: runtime error: implicit conversion from type 'int' of value -74 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551542 (64-bit, unsigned) ``` The warnings above happen here: https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/addrman.cpp#L306 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/addrman.h#L446 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/arith_uint256.cpp#L32 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/arith_uint256.cpp#L47 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/chain.cpp#L151 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/coins.cpp#L114 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/compressor.cpp#L162 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/compressor.cpp#L188 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/hash.cpp#L13 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/pubkey.h#L152 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/streams.h#L570 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/util/bip32.cpp#L57 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/util/strencodings.cpp#L562 https://github.com/bitcoin/bitcoin/blob/32b191fb66e644c690c94cbfdae6ddbc754769d7/src/util/strencodings.h#L164 ACKs for top commit: MarcoFalke: review ACK f0f8b1a076c362c6e26570a2129809f4d6a0abad 🤚 Tree-SHA512: a8f04f7cc055d03653161de1d9d14d106a6280cea1e86a1243abcd57cf8e61dcf5f731d0ab0da5b390790e816022ff7a70759a641463bc7e3303076b8667009f
2021-01-26refactor: remove straggling boost::mutex usagefanquake
After the merge of #18710, the linter is warning: ```bash A new Boost dependency in the form of "boost/thread/mutex.hpp" appears to have been introduced: src/sync.cpp:#include <boost/thread/mutex.hpp> src/test/sync_tests.cpp:#include <boost/thread/mutex.hpp> ^---- failure generated from test/lint/lint-includes.sh ``` the interim #19337 was merged, which introduced more `boost::mutex` usage. Given we no longer use `boost::mutex`, just remove the double lock test and remaining includes.
2021-01-25fuzz: Avoid -fsanitize=integer warnings in fuzzing harnessespracticalswift
2021-01-25Merge #18710: Add local thread pool to CCheckQueueWladimir J. van der Laan
bb6fcc75d1ec94b733d1477c816351c50be5faf9 refactor: Drop boost::thread stuff in CCheckQueue (Hennadii Stepanov) 6784ac471bb32b6bb8e2de60986f123eb4990706 bench: Use CCheckQueue local thread pool (Hennadii Stepanov) dba30695fc42f45828db008e7e5b81cb2b5d8551 test: Use CCheckQueue local thread pool (Hennadii Stepanov) 01511776acb0c7ec216dc9c8112531067763f1cb Add local thread pool to CCheckQueue (Hennadii Stepanov) 0ef938685b5c079a6f5a98daf0e3865d718d817b refactor: Use member initializers in CCheckQueue (Hennadii Stepanov) Pull request description: This PR: - gets rid of `boost::thread_group` in the `CCheckQueue` class - allows thread safety annotation usage in the `CCheckQueue` class - is alternative to #14464 (https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-616618525, https://github.com/bitcoin/bitcoin/pull/18710#issuecomment-617291612) Also, with this PR (I hope) it could be easier to resurrect a bunch of brilliant ideas from #9938. Related: #17307 ACKs for top commit: laanwj: Code review ACK bb6fcc75d1ec94b733d1477c816351c50be5faf9 LarryRuane: ACK bb6fcc75d1ec94b733d1477c816351c50be5faf9 jonatack: Code review ACK bb6fcc75d1ec94b733d1477c816351c50be5faf9 and verified rebase to master builds cleanly with unit/functional tests green Tree-SHA512: fddeb720d5a391b48bb4c6fa58ed34ccc3f57862fdb8e641745c021841c8340e35c5126338271446cbd98f40bd5484f27926aa6c3e76fa478ba1efafe72e73c1
2021-01-23fuzz: Avoid initializing version to less than MIN_PEER_PROTO_VERSIONMarcoFalke
2021-01-23fuzz: move-only FillNode implementation to cpp fileMarcoFalke
This allows to modify the implementation without having to recompile all fuzz targets. Can be reviewed with --color-moved=dimmed-zebra
2021-01-21util: Add ArgsManager::GetCommand() and use it in bitcoin-walletMarcoFalke
Co-Authored-by: Anthony Towns <aj@erisian.com.au>
2021-01-21Merge #20932: refactor: Replace fs::absolute calls with AbsPathJoin callsMarcoFalke
da9caa1cedd69702aea44cb44b2fd0a2d6d56916 Replace fs::absolute calls with AbsPathJoin calls (Kiminuo) 66576c4fd532ac18b8b355ea93d25581a2c15654 test: Clear forced -walletdir setting after wallet init_tests (Kiminuo) Pull request description: This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17. This PR doesn't change behavior aside from adding an assert and fixing a test bug. ACKs for top commit: jonatack: Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 only doxygen improvements since my last review per `git diff d867d7a da9caa1` MarcoFalke: review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯 ryanofsky: Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916. Just comment and test tweaks since previous review. Tree-SHA512: c940ee60f3ba374d4927cf34cf12d27c4c735c94af591fbc0ca408c641b30f8f8fbcfe521d66bfbddf9877a1fc8cd99bd8a47ebcd2fa59789de6bd87a7b9cf4d
2021-01-21Merge #20946: fuzz: Consolidate fuzzing TestingSetup initializationMarcoFalke
abb6fa728598c4cc8874eae1c3c5e587e36424cd fuzz: Initialize a full TestingSetup where appropriate (Carl Dong) 713314abfa224efceb7ba6fa7fef37c449902936 fuzz: Consolidate fuzzing TestingSetup initialization (Carl Dong) Pull request description: ``` Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways: 1. Calling InitializeFuzzingContext, which implicitly constructs a static const BasicTestingSetup 2. Directly constructing a static const BasicTestingSetup in the initialize_* function 3. Directly constructing a static TestingSetup and reproducing the initialization arguments (I'm assuming because InitializeFuzzingContext only initializes a BasicTestingSetup) The new, relatively-simple MakeFuzzingContext function allows us to consolidate these methods of initialization by being flexible enough to be used in all situations. It: 1. Is templated so that we can choose to initialize any of the *TestingSetup classes 2. Has sane defaults which are often used in fuzzers but are also easily overridable 3. Returns a unique_ptr, explicitly transferring ownership to the caller to deal with according to its situation ``` ~~Question for fuzzing people: was it intentional that `src/test/fuzz/net.cpp` would directly instantiate the `BasicTestingSetup` and thus omit the `"-nodebuglogfile"` flag?~~ [Answered](https://github.com/bitcoin/bitcoin/pull/20946#issuecomment-761537108) ACKs for top commit: MarcoFalke: ACK abb6fa728598c4cc8874eae1c3c5e587e36424cd Tree-SHA512: 96a5ca6f4cd5ea0e9483b60165b31ae3e9003918c700a7f6ade48010f419f2a6312e10b816b3187f1d263798827571866e4c4ac0bbfb2e0c79dfad254cda68e7
2021-01-21fuzz: Initialize a full TestingSetup where appropriateCarl Dong
A full TestingSetup is required for both coins_view and load_external_block_file as they interact with the active chainstate.
2021-01-21fuzz: Consolidate fuzzing TestingSetup initializationCarl Dong
Previously, the {Basic,}TestingSetup for fuzzers were set up in many ways: 1. Calling InitializeFuzzingContext, which implicitly constructs a static const BasicTestingSetup 2. Directly constructing a static const BasicTestingSetup in the initialize_* function 3. Directly constructing a static TestingSetup and reproducing the initialization arguments (I'm assuming because InitializeFuzzingContext only initializes a BasicTestingSetup) The new, relatively-simple MakeFuzzingContext function allows us to consolidate these methods of initialization by being flexible enough to be used in all situations. It: 1. Is templated so that we can choose to initialize any of the *TestingSetup classes 2. Has sane defaults which are often used in fuzzers but are also easily overridable 3. Returns a unique_ptr, explicitly transferring ownership to the caller to deal with according to its situation
2021-01-15Replace fs::absolute calls with AbsPathJoin callsKiminuo
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
2021-01-15Merge #20908: fuzz: Use mocktime in process_message* fuzz targetsMarcoFalke
fa0a864b383a794910cdb215bb836dae12357bc6 fuzz: Use mocktime in process_message* fuzz targets (MarcoFalke) Pull request description: Use mocktime to allow time to advance deterministically during execution of a fuzz input. This also allows to drop the call to `JumpOutOfIbd`. ACKs for top commit: practicalswift: cr ACK fa0a864b383a794910cdb215bb836dae12357bc6 Tree-SHA512: e92fc70ec6bd49760173cb202549f364304e22b3f7127b9a4da8447cf9341008e477ad42c2599c2fde167bbcbc0e2d139709b4ef6371788bc2c1c3b7f589e11d
2021-01-14Merge #20828: fuzz: Introduce CallOneOf helper to replace switch-caseMarcoFalke
fa75d40ef866ef9ff8dc115e239ca6763aa23b06 fuzz: Introduce CallOneOf helper to replace switch-case (MarcoFalke) Pull request description: The current `switch (fuzzed_data_provider.ConsumeIntegralInRange<int>(0, nn)) { case 0: ... case 1: ... case nn: ...` has several problems: * It makes it hard to review newly added targets, because it requires manual counting of cases * It makes it hard to update a target, because updating all case labels is trivial, but tedious to review and causes merge conflicts * ~~Updating the target raises the question whether the case labels should be preserved to not invalidate the existing fuzz inputs format. Fuzz input format might already change implicitly on every commit, so this isn't something worthwhile to pursue.~~ Edit: This pull doesn't fix this problem. Fix all issues by adding a new `CallOneOf` helper ACKs for top commit: ajtowns: ACK fa75d40ef866ef9ff8dc115e239ca6763aa23b06 - code review only jnewbery: utACK fa75d40ef866ef9ff8dc115e239ca6763aa23b06 Tree-SHA512: 2daa602b240b86c8e85a024e008f03a57ba60349377eed771f4d21a97a9dba9b66e93fff16ff1992018d4330be7a1a276944c3dfdf698748ce135626c380e563
2021-01-13Merge #20811: refactor: move net_processing implementation details out of headerWladimir J. van der Laan
c97f70c861ac6959b8116a9bca3031edeb2b2aaa net_processing: move Peer definition to .cpp (Anthony Towns) e0f2e6d2df7117a8dbf17c63c5149fc53a6fe2b2 net_processing: move PeerManagerImpl into cpp file (Anthony Towns) a568b82febb3ecbd5ebb7c3f9da27e762b0c68f6 net_processing: split PeerManager into interface and implementation classes (Anthony Towns) 0df3d3fd6bbbd0e06116797177ba797580553250 net_processing: make more of PeerManager private (Anthony Towns) 0d246a59b606c51728d10cb70004a6eedb951bca net, net_processing: move NetEventsInterface method docs to net.h (Anthony Towns) Pull request description: Moves the implementation details of `PeerManager` and all of `struct Peer` into net_processing.cpp. ACKs for top commit: jnewbery: ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa Sjors: ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa laanwj: Code review ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa vasild: ACK c97f70c861ac6959b8116a9bca3031edeb2b2aaa Tree-SHA512: 2e081e491d981c61bd78436a6a6c2eb41d3c2d86a1a8ef9d4d6f801b82cb64a8bf707a96db3429418d1704cffb60a657d1037a0336cbc8173fb79aef9eb72001
2021-01-13fuzz: Use mocktime in process_message* fuzz targetsMarcoFalke
2021-01-13Add include for std::bind.sinetek
2021-01-12Merge #20495: sync: Use decltype(auto) return type for WITH_LOCKfanquake
3eb94ec81b72b14f72a1f6ce5c9aa24476df755a sync: Use decltype(auto) return type for WITH_LOCK (Carl Dong) Pull request description: > Now that we're using C++17, we can use the decltype(auto) return type > for functions and lambda expressions. > > As demonstrated in this commit, this can simplify cases where previously > the compiler failed to deduce the correct return type. > > Just for reference, for the "assign to ref" cases fixed here, there are > 3 possible solutions: > > - Return a pointer and immediately deref as used before this commit > - Make sure the function/lambda returns declspec(auto) as used after > this commit > - Class& i = WITH_LOCK(..., return std::ref(...)); > > ----- > > References: > 1. https://en.cppreference.com/w/cpp/language/function#Return_type_deduction > 2. https://en.cppreference.com/w/cpp/language/template_argument_deduction#Other_contexts > 3. https://en.cppreference.com/w/cpp/language/auto > 4. https://en.cppreference.com/w/cpp/language/decltype > > Explanations: > 1. https://stackoverflow.com/a/21369192 > 2. https://stackoverflow.com/a/21369170 Thanks to sipa and ryanofsky for helping me understand this ACKs for top commit: jnewbery: utACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a hebasto: ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a, I have reviewed the code and it looks OK, I agree it can be merged. I have verified possible warnings: ryanofsky: Code review ACK 3eb94ec81b72b14f72a1f6ce5c9aa24476df755a Tree-SHA512: 5f55c7722aeca8ea70e5c1a8db93e93ba0e356e8967e7f607ada38003df4b153d73c29bd2cea8d7ec1344720d37d857ea7dbfd2a88da1d92e0e9cbb9abd287df
2021-01-11Merge #20787: Use C++17 std::array deduction for OUTPUT_TYPES, ↵fanquake
ALL_FEE_ESTIMATE_HORIZONS aaaa9878405f3f38f4f61c00feca110d7f9ca481 refactor: Use C++17 std::array deduction for ALL_FEE_ESTIMATE_HORIZONS (MarcoFalke) fa39cdd072c91eac70cda04b8b26681611f94cb7 refactor: Use C++17 std::array deduction for OUTPUT_TYPES (MarcoFalke) Pull request description: With the new C++17 array deduction rules, an array encompassing all values in an enum can be specified in the same header file that specifies the enum. This is useful to avoid having to repeatedly enumerate all enum values in the code. E.g. the RPC code, but also the fuzz code. ACKs for top commit: theStack: cr ACK aaaa9878405f3f38f4f61c00feca110d7f9ca481 ⚙️ fanquake: ACK aaaa9878405f3f38f4f61c00feca110d7f9ca481 Tree-SHA512: b71bd98f3ca07ddfec385735538ce89a4952e418b52dc990fb160187ccef1fc7ebc139d42988b6f7b48df24823af61f803b83d47fb7a3b82475f0c0b109bffb7
2021-01-11Merge #20852: net: allow CSubNet of non-IP networksWladimir J. van der Laan
39b43298d9c54f9c18bef36f3d5934f57aefd088 test: add test for banning of non-IP addresses (Vasil Dimov) 94d335da7f8232bc653c9b08b0a33b517b4c98ad net: allow CSubNet of non-IP networks (Vasil Dimov) Pull request description: Allow creation of valid `CSubNet` objects of non-IP networks and only match the single address they were created from (like /32 for IPv4 or /128 for IPv6). This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)` and in `BanMan` which assume that creating a subnet from any address using the `CSubNet(CNetAddr)` constructor would later match that address only. Before this change a non-IP subnet would be invalid and would not match any address. ACKs for top commit: jonatack: Code review re-ACK 39b43298d9c54f9c18bef36f3d5934f57aefd088 per `git diff 5e95ce6 39b4329`; only change since last review is improvements to the functional test; verified the test fails on master @ 616eace0 where expected (`assert(self.is_banned(node, tor_addr))` fails and unban unfails) laanwj: code review ACK 39b43298d9c54f9c18bef36f3d5934f57aefd088 Tree-SHA512: 3239b26d0f2fa2d1388b4fdbc1d05ce4ac1980be699c6ec46049409baefcb2006b1e72b889871e2210e897f6725c48e873f68457eea7e6e4958ab4f959d20297
2021-01-11fuzz: Introduce CallOneOf helper to replace switch-caseMarcoFalke
Can be reviewed with --ignore-all-space
2021-01-11Merge #20480: Replace boost::variant with std::variantfanquake
faa8f68943615785a2855676cf96e0e96f3cc6bd Replace boost::variant with std::variant (MarcoFalke) Pull request description: Now that we can use std::variant from the vanilla standard library, drop the third-party boost variant dependency ACKs for top commit: fjahr: Code review ACK faa8f68943615785a2855676cf96e0e96f3cc6bd fanquake: ACK faa8f68943615785a2855676cf96e0e96f3cc6bd Tree-SHA512: 6e3aecd33b00c2e31a763f999247944d5b2ce5e3018f1965c516c1000cd08ff6703a8d50fb0be64883153da2925ae72986b8a6b96586db74057bd05d6f4986e6
2021-01-10net: allow CSubNet of non-IP networksVasil Dimov
Allow creation of valid `CSubNet` objects of non-IP networks and only match the single address they were created from (like /32 for IPv4 or /128 for IPv6). This fixes a deficiency in `CConnman::DisconnectNode(const CNetAddr& addr)` and in `BanMan` which assume that creating a subnet from any address using the `CSubNet(CNetAddr)` constructor would later match that address only. Before this change a non-IP subnet would be invalid and would not match any address.
2021-01-10Merge #20881: fuzz: net permission flags in net processingMarcoFalke
fad327ca65ef30cee2027f1e62d597f0b1c1b641 fuzz: net permission flags in net processing (MarcoFalke) Pull request description: to increase coverage ACKs for top commit: Crypt-iQ: cr ACK fad327c practicalswift: ACK fad327ca65ef30cee2027f1e62d597f0b1c1b641 Tree-SHA512: f8643d1774ff13524ab97ab228ad070489e080435e5742af26e6e325fd002e4c1fd78b9887e11622e79d6fe0c4daaddce5e033e6cd4b32e50fd68b434aab7333
2021-01-09net_processing: split PeerManager into interface and implementation classesAnthony Towns
2021-01-08fuzz: Add missing muhash registrationMarcoFalke
2021-01-07fuzz: net permission flags in net processingMarcoFalke
2021-01-07Merge #19055: Add MuHash3072 implementationWladimir J. van der Laan
9815332d5158d69a94abeaf465a2c07bd8e43359 test: Change MuHash Python implementation to match cpp version again (Fabian Jahr) 01297fb3ca57e4b8cbc5a89fc7c6367de33b0bc6 fuzz: Add MuHash consistency fuzz test (Fabian Jahr) b111410914041b72961536c3e4037eba103a8085 test: Add MuHash3072 fuzz test (Fabian Jahr) c1225273857f9fa2e2276396e3f8b3ea48306df3 bench: Add Muhash benchmarks (Fabian Jahr) 7b1242229d1fcc9277238a3aefb3431061c82bfa test: Add MuHash3072 unit tests (Fabian Jahr) adc708c98dbf03b1735edc91f813a36580781a95 crypto: Add MuHash3072 implementation (Fabian Jahr) 0b4d290bf5b0a4d156c523431bf89aaa9ffe92e5 crypto: Add Num3072 implementation (Fabian Jahr) 589f958662a2dcaacdb9a66f1088c74828a39577 build: Check for 128 bit integer support (Fabian Jahr) Pull request description: This is the first split of #18000 which implements the Muhash algorithm and uses it to calculate the UTXO set hash in `gettxoutsetinfo`. ACKs for top commit: laanwj: Code review ACK 9815332d5158d69a94abeaf465a2c07bd8e43359 Tree-SHA512: 4bc090738f0e3d80b74bdd8122e24a8ce80121120fd37c7e4335a73e7ba4fcd7643f2a2d559e2eebf54b8e3a3bd5f12cfb27ba61ded135fda210a07a233eae45
2021-01-07Merge #20789: fuzz: Rework strong and weak net enum fuzzingMarcoFalke
eeee43bc48ea7fbacd3c5e3f076f01f04744adb8 fuzz: Use ConsumeWeakEnum for ServiceFlags (MarcoFalke) fa9949b91414ee0da376a322cee32ba4e3989d8c fuzz: Add ConsumeWeakEnum helper, Extract ALL_NET_PERMISSION_FLAGS (MarcoFalke) faaef9434c19e3643322ee442c240c166af5adbd fuzz: [refactor] Extract ALL_CONNECTION_TYPES constant (MarcoFalke) fa42da2d5424c0aeccfae4b49fde2bea330b63dc fuzz: Use ConsumeNode in process_message target (MarcoFalke) fa121f058fdc5f09dd11678480f551246cb3c5e2 fuzz: Use ConsumeNode in process_messages target (MarcoFalke) Pull request description: The fuzz tests have several problems: * The array passed to the fuzz engine to pick `net_permission_flags` is outdated * The process_message* targets has the service flags as well as connection type hardcoded, limiting potential coverage * The service flags deserialization from the fuzz engine doesn't allow for easy "exact matches". The fuzz engine has to explore a 64-bit space to hit an "exact match" (only one bit set) Fix all issues in the commits in this pull ACKs for top commit: mzumsande: ACK eeee43bc48ea7fbacd3c5e3f076f01f04744adb8 after rebase. Tree-SHA512: 1ad9520c7e708b7f4994ae8f77886ffca33d7c542756e2a3e07dbbbe59e360f9fcaccf2e2fb57d9bc731d4aeb4938fb1c5c546e9d2744b007af5626f5cb377fe
2021-01-07Merge #20584: Declare de facto const reference variables/member functions as ↵MarcoFalke
const 31b136e5802e1b1e5f9a9589736afe0652f34da2 Don't declare de facto const reference variables as non-const (practicalswift) 1c65c075ee4c7f98d9c1fac5ed7576b96374d4e9 Don't declare de facto const member functions as non-const (practicalswift) Pull request description: _Meta: This is the second and final part of the `const` refactoring series (part one: #20581). **I promise: no more refactoring PRs from me in a while! :)** I'll now go back to focusing on fuzzing/hardening!_ Changes in this PR: * Don't declare de facto const member functions as non-const * Don't declare de facto const reference variables as non-const Awards for finding candidates for the above changes go to: * `clang-tidy`'s [`readability-make-member-function-const`](https://clang.llvm.org/extra/clang-tidy/checks/readability-make-member-function-const.html) check ([list of `clang-tidy` checks](https://clang.llvm.org/extra/clang-tidy/checks/list.html)) * `cppcheck`'s `constVariable` check ([list of `cppcheck` checks](https://sourceforge.net/p/cppcheck/wiki/ListOfChecks/)) See #18920 for instructions on how to analyse Bitcoin Core using Clang Static Analysis, `clang-tidy` and `cppcheck`. ACKs for top commit: ajtowns: ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 jonatack: ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 theStack: ACK 31b136e5802e1b1e5f9a9589736afe0652f34da2 :snowflake: Tree-SHA512: f58f8f00744219426874379e9f3e9331132b9b48e954d24f3a85cbb858fdcc98009ed42ef7e7b4619ae8af9fc240a6d8bfc1c438db2e97b0ecd722a80dcfeffe
2021-01-05Merge #19846: build: enable unused member function diagnosticMarcoFalke
819d03b932134ee91df3b0fe98a481a331ce57bf refactor: took out unused member functions (Zero) ed69213c2b2a99023bdee5168614cb8b71990f5f build: enable unused member function diagnostic (Zero) Pull request description: This PR enables the `-Wunused-member-function` compiler diagnostic, as discussed in #19702. > **Notice**: The `unused-member-function` diagnostic is only available on clang. Therefore, clang should be used to test this PR. - [x] Include the `-Wunused-member-function`diagnostic in `./configure.ac`. (ed69213c2b2a99023bdee5168614cb8b71990f5f) - [x] Resolve the reported warnings. (819d03b932134ee91df3b0fe98a481a331ce57bf) Currently, enabling this flag no longer reports the following warnings: > **Note**: output from `make 2>&1 | grep "warning: unused member function" | sort | uniq -c` ``` 1 index/blockfilterindex.cpp:54:5: warning: unused member function 'DBHeightKey' [-Wunused-member-function] 2 script/bitcoinconsensus.cpp:50:9: warning: unused member function 'GetType' [-Wunused-member-function] 1 test/util_tests.cpp:1975:14: warning: unused member function 'operator=' [-Wunused-member-function] ``` All tests have passed locally (from `make check` & `src/test/test_bitcoin`). This PR closes #19702. ACKs for top commit: practicalswift: ACK 819d03b932134ee91df3b0fe98a481a331ce57bf - patch still looks correct :) MarcoFalke: ACK 819d03b932134ee91df3b0fe98a481a331ce57bf pox: Tested ACK 819d03b932134ee91df3b0fe98a481a331ce57bf with clang after `make clean`. No unused member function warnings. theStack: tested ACK 819d03b932134ee91df3b0fe98a481a331ce57bf Tree-SHA512: 5fdfbbb02b3dc618a90a874a5caa5e01e596fc1d14a209e75a6981f01b253f9bca0cfac8fdd758dd7151986609fb76571c3745124a29cfd4f8cbb8d82a07272e
2021-01-05Replace boost::variant with std::variantMarcoFalke
2021-01-04doc: fix various typosIkko Ashimine
Co-authored-by: Peter Yordanov <ppyordanov@yahoo.com>
2021-01-03Merge #20760: test: Set correct nValue for multi-op-return policy checkMarcoFalke
fad140e311028f904635126e3c77352afac1b75e test: Set correct nValue for multi-op-return policy check (MarcoFalke) Pull request description: `CTxOut::nValue` is default-initialized to `-1`. The dust-threshold for `OP_RETURN` outputs is `0`. Thus, the policy failure would be `dust` instead of `multi-op-return`. The test only passes because the dust check is currently not run. Avoid that confusion by setting the value to `0`, to ensure the dust check passes. ACKs for top commit: theStack: ACK fad140e311028f904635126e3c77352afac1b75e Tree-SHA512: f0c7a68eb2c573d6595b2b129fa8fa2a34fa35c17691f448bf1c54ccf66059c37562e7480cde7b51c4de677038d7717873da4257147a5f60acc8bbcd25fb7e3f
2021-01-03refactor: Use C++17 std::array deduction for ALL_FEE_ESTIMATE_HORIZONSMarcoFalke
2021-01-03Merge #20765: fuzz: check that certain script TxoutType are nonstandardMarcoFalke
efaf80e9bb0afeca2955720bfe6c225d7864036b fuzz: check that certain script TxoutType are nonstandard (Michael Dietz) Pull request description: - Every transaction of type NONSTANDARD must not be a standard script - The only know types of nonstandard scripts are NONSTANDARD and certain NULL_DATA and MULTISIG scripts When reviewing https://github.com/bitcoin/bitcoin/pull/20761 I figured this is very similar and might also be good to have ACKs for top commit: MarcoFalke: ACK efaf80e9bb0afeca2955720bfe6c225d7864036b Tree-SHA512: 6f563ee3104ea9d2633aad95f1d003474bea759d0f22636c37aa91b5536a6ff0800c42447285ca8ed12f1b3699bf781dae1e5e0a3362da578749cd3164a06ea4
2021-01-02fuzz: Use ConsumeWeakEnum for ServiceFlagsMarcoFalke
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-15 16:44:57 +0000