aboutsummaryrefslogtreecommitdiff
path: root/src/test
AgeCommit message (Collapse)Author
2020-05-27Merge #16127: More thread safety annotation coverageMarcoFalke
5478d6c099e76fe070703cc5383cba7b91468b0f logging: thread safety annotations (Anthony Towns) e685ca19928eec4e687c66f5edfcfff085a42c27 util/system.cpp: add thread safety annotations for dir_locks (Anthony Towns) a7887899480db72328784009181d93904e6d479d test/checkqueue_tests: thread safety annotations (Anthony Towns) 479c5846f7477625ec275fbb8a076c7ef157172b rpc/blockchain.cpp: thread safety annotations for latestblock (Anthony Towns) 8b5af3d4c1270267ad85e78f661bf8fab06f3aad net: fMsgProcWake use LOCK instead of lock_guard (Anthony Towns) de7c5f41aba860751ef7824245e6d9d5088a1200 wallet/wallet.h: Remove mutexScanning which was only protecting a single atomic bool (Anthony Towns) c3cf2f55013c4ea1c1ef4a878fc7ff8e92f2c42d rpc/blockchain.cpp: Remove g_utxosetscan mutex that is only protecting a single atomic variable (Anthony Towns) Pull request description: In a few cases we need to use `std::mutex` rather than the sync.h primitives. But `std::lock_guard<std::mutex>` doesn't include the clang thread safety annotations unless you also use clang's C library, which means you can't indicate when variables should be guarded by `std::mutex` mutexes. This adds an annotated version of `std::lock_guard<std::mutex>` to threadsafety.h to fix that, and modifies places where `std::mutex` is used to take advantage of the annotations. It's based on top of #16112, and turns the thread safety comments included there into annotations. It also changes the RAII classes in wallet/wallet.h and rpc/blockchain.cpp to just use the atomic<bool> flag for synchronisation rather than having a mutex that doesn't actually guard anything as well. ACKs for top commit: MarcoFalke: ACK 5478d6c099e76fe070703cc5383cba7b91468b0f πŸ—Ύ hebasto: re-ACK 5478d6c099e76fe070703cc5383cba7b91468b0f, only renamed s/`MutexGuard`/`LockGuard`/, and dropped the commit "test/util_threadnames_tests: add thread safety annotations" since the [previous](https://github.com/bitcoin/bitcoin/pull/16127#pullrequestreview-414184113) review. ryanofsky: Code review ACK 5478d6c099e76fe070703cc5383cba7b91468b0f. Thanks for taking suggestions! Only changes since last review are dropping thread rename test commit d53072ec730d8eec5a5b72f7e65a54b141e62b19 and renaming mutex guard to lock guard Tree-SHA512: 7b00d31f6f2b5a222ec69431eb810a74abf0542db3a65d1bbad54e354c40df2857ec89c00b4a5e466c81ba223267ca95f3f98d5fbc1a1d052a2c3a7d2209790a
2020-05-27Merge #19004: refactor: Replace const char* to std::stringMarcoFalke
c57f03ce1741b38af448bec7b22ab9f8ac21f067 refactor: Replace const char* to std::string (Calvin Kim) Pull request description: Rationale: Addresses #19000 Some functions should be returning std::string instead of const char*. This commit changes that. Main benefits/reasoning: 1. The functions never return nullptr, so returning a string makes code at call sites easier to review (reviewers don't have to read the source code to verify that a nullptr is never returned) 2. All call sites convert to string anyway ACKs for top commit: MarcoFalke: re-ACK c57f03ce17 (no changes since previous review) πŸšƒ Empact: Fair enough, Code Review ACK https://github.com/bitcoin/bitcoin/pull/19004/commits/c57f03ce1741b38af448bec7b22ab9f8ac21f067 practicalswift: ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067 -- patch looks correct hebasto: re-ACK c57f03ce1741b38af448bec7b22ab9f8ac21f067 Tree-SHA512: 9ce99bb38fe399b54844315048204cafce0f27fd8f24cae357fa7ac6f5d8094d57bbf5f5c1f5878a65f2d35e4a3f95d527eb17f49250b690c591c0df86ca84fd
2020-05-26Merge #19032: Serialization improvements: final stepWladimir J. van der Laan
71f016c6eb42e1ac2c905e04ba4d20c2009e533f Remove old serialization primitives (Pieter Wuille) 92beff15d3ae2646c00bd78146d7592a7097ce9c Convert LimitedString to formatter (Pieter Wuille) ef17c03e074b6c3f185afa4eff572ba687c2a171 Convert wallet to new serialization (Pieter Wuille) 65c589e45e8b8914698a0fd25cd5aafdda30869c Convert Qt to new serialization (Pieter Wuille) Pull request description: This is the final step πŸ₯³ of the serialization improvements extracted from #10785. It converts the LimitedString wrapper to a new-style formatter, and updates the wallet and Qt code to use the new serialization framework. Finally all remaining old primitives are removed. ACKs for top commit: jonatack: ACK 71f016c6eb42e1ac2 reviewed diff, builds/tests/re-fuzzed. laanwj: Code review ACK 71f016c6eb42e1ac2c905e04ba4d20c2009e533f Tree-SHA512: d952194bc73259f6510bd4ab1348a1febbbf9862af30f905991812fb0e1f23f15948cdb3fc662be54d648e8f6d95b11060055d2e7a8c2cb5bf008224870b1ea1
2020-05-25tests: Add fuzzing harness for CCoinsViewCachepracticalswift
2020-05-24Convert LimitedString to formatterPieter Wuille
2020-05-23Merge #18698: Make g_chainman internal to validationMarcoFalke
fab6b9d18fd48bbbd1939b1173723bc04c5824b5 validation: Mark g_chainman DEPRECATED (MarcoFalke) fa1d97b25686a5caca623599f6d608fd08616fe8 validation: Make ProcessNewBlock*() members of ChainstateManager (MarcoFalke) fa24d4909864096934577abc26cfa9be47f634ba validation: Make PruneOneBlockFile() a member of ChainstateManager (MarcoFalke) fa84b1cd846f6499b741710fd478ec9ad49b5120 validation: Make LoadBlockIndex() a member of ChainstateManager (MarcoFalke) fa05fdf0f19fa4b557cc5e9ba436e3215b83c4e6 net: Pass chainman into PeerLogicValidation (MarcoFalke) fa7b626d7a150e5cbd4d163d2dab6f8a55fc2cc4 node: Add chainman alias for g_chainman (MarcoFalke) Pull request description: The global `g_chainman` has recently been introduced in #17737. The chainstate manager is primarily needed for the assumeutxo feature, but it can also simplify testing in the future. The goal of this pull is to make the global chainstate manager internal to validation, so that all external code does not depend on globals and that unit or fuzz tests can pass in their (potentially mocked) chainstate manager. I suggest reviewing the pull request commit-by-commit. It should be relatively straightforward refactoring that does not change behavior at all. ACKs for top commit: ryanofsky: Code review ACK fab6b9d18fd48bbbd1939b1173723bc04c5824b5. Had to be rebased but still looks good Tree-SHA512: dcbf114aeef4f8320d466369769f22ce4dd8f46a846870354df176c3de9ff17c64630fbd777e7121d7470d7a8564ed8d37b77168746e8df7489c6877e55d7b4f
2020-05-22tests: Don't limit fuzzing inputs to 1 MB for afl-fuzz (now: ∞ βˆ€ fuzzers)practicalswift
2020-05-22refactor: Replace const char* to std::stringCalvin Kim
Some functions should be returning std::string instead of const char*. This commit changes that.
2020-05-21validation: Make ProcessNewBlock*() members of ChainstateManagerMarcoFalke
2020-05-21net: Pass chainman into PeerLogicValidationMarcoFalke
2020-05-21node: Add chainman alias for g_chainmanMarcoFalke
2020-05-21Merge #18740: Remove g_rpc_node globalMarcoFalke
b3f7f375efb9a9ca9a7a4f2caf41fe3df2262520 refactor: Remove g_rpc_node global (Russell Yanofsky) ccb5059ee89f6e8dc31ba5b82830b384890bb65e scripted-diff: Remove g_rpc_node references (Russell Yanofsky) 6fca33b2edc09ed62dab2323c780b31585de1750 refactor: Pass NodeContext to RPC and REST methods through util::Ref (Russell Yanofsky) 691c817b340d10e806dc3b1834d2a8fcc5e681fd Add util::Ref class as temporary alternative for c++17 std::any (Russell Yanofsky) Pull request description: This PR removes the `g_rpc_node` global, to get same benefits we see removing other globals and make RPC code more testable, modular, and reusable. This uses a hybrid of the approaches suggested in #17548. Instead of using `std::any`, which isn't available in c++11, or `void*`, which isn't type safe, it uses a small new `util::Ref` helper class, which acts like a simplified `std::any` that only holds references, not values. Motivation for writing this was to provide an simpler alternative to #18647 by Harris BrakmiΔ‡ (brakmic) which avoids some shortcomings of that PR (https://github.com/bitcoin/bitcoin/pull/18647#issuecomment-617878826) ACKs for top commit: MarcoFalke: re-ACK b3f7f375ef, only change is adding back const and more tests 🚾 ajtowns: ACK b3f7f375efb9a9ca9a7a4f2caf41fe3df2262520 Tree-SHA512: 56292268a001bdbe34d641db1180c215351503966ff451e55cc96c9137f1d262225d7d7733de9c9da7ce7d7a4b34213a98c2476266b58c89dbbb0f3cb5aa5d70
2020-05-20Merge #18317: Serialization improvements step 6 (all except wallet/gui)MarcoFalke
f9ee0f37c28f604bc82dab502ce229c66ef5b3b9 Add comments to CustomUintFormatter (Pieter Wuille) 4eb5643e3538863c9d2ff261f49a9a1b248de243 Convert everything except wallet/qt to new serialization (Pieter Wuille) 2b1f85e8c52c8bc5a17eae4c809eaf61d724af98 Convert blockencodings_tests to new serialization (Pieter Wuille) 73747afbbeb013669faf4c4d2c0903cec4526fb0 Convert merkleblock to new serialization (Pieter Wuille) d06fedd1bc26bf5bf2b203d4445aeaebccca780e Add SER_READ and SER_WRITE for read/write-dependent statements (Russell Yanofsky) 6f9a1e5ad0a270d3b5a715f3e3ea0911193bf244 Extend CustomUintFormatter to support enums (Russell Yanofsky) 769ee5fa0011ae658770586442715452a656559d Merge BigEndian functionality into CustomUintFormatter (Pieter Wuille) Pull request description: The next step of changes from #10785. This: * Adds support for enum serialization to `CustomUintFormatter`, used in `CAddress` for service flags. * Merges `BigEndian` into `CustomUintFormatter`, used in `CNetAddr` for port numbers. * Converts everything (except wallet and gui) to use the new serialization framework. ACKs for top commit: MarcoFalke: re-ACK f9ee0f37c2, only change is new documentation commit for CustomUintFormatter πŸ“‚ ryanofsky: Code review ACK f9ee0f37c28f604bc82dab502ce229c66ef5b3b9. Just new commit adding comment since last review jonatack: Code review re-ACK f9ee0f37c28f604bc82dab502ce229c6 only change since last review is an additional commit adding Doxygen documentation for `CustomUintFormatter`. Tree-SHA512: e7a0a36afae592d5a4ff8c81ae04d858ac409388e361f2bc197d9a78abca45134218497ab2dfd6d031e0cce0ca586cf857077b7c6ce17fccf67e2d367c1b6cd4
2020-05-19test/checkqueue_tests: thread safety annotationsAnthony Towns
2020-05-17Merge #18938: tests: Fill fuzzing coverage gaps for functions in ↡MarcoFalke
consensus/validation.h, primitives/block.h and util/translation.h cd34038cbda4864e4770734c44b18d3e01aa2a28 Switch from Optional<T> to std::optional<T> (C++17). Run clang-format. (practicalswift) fb559c1170773360afb9d05daaccd57d18636ee9 tests: Fill fuzzing coverage gaps for functions in util/translation.h (practicalswift) b74f3d6c452d9ad7013c70a91216220917978f66 tests: Fill fuzzing coverage gaps for functions in consensus/validation.h (practicalswift) c0bbf8193d92ba85d62092c4fd886ff4461f65bf tests: Fill fuzzing coverage gaps for functions in primitives/block.h (practicalswift) Pull request description: * Fill fuzzing coverage gaps for functions in `consensus/validation.h` * Fill fuzzing coverage gaps for functions in `primitives/block.h` * Fill fuzzing coverage gaps for functions in `util/translation.h` * Switch from `Optional<T>` to `std::optional<T>` (C++17). Run `clang-format`. See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for 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 :) Top commit has no ACKs. Tree-SHA512: d6aa4634c3953ade173589a8239bd230eb317ef897835a8557acb73df01b25e5e17bf46f837838e59ec04c1f3d3b7d1309ba68c8a264d17b938215512c9e6085
2020-05-15Merge #18781: Add templated GetRandDuration<>MarcoFalke
0000ea32656833efa3d2ffd9bab66c88c83334f0 test: Add test for GetRandMillis and GetRandMicros (MarcoFalke) fa0e5b89cf742df56c6c8f49fe9b3c54d2970a66 Add templated GetRandomDuration<> (MarcoFalke) Pull request description: A naive implementation of this template is dangerous, because the call site might accidentally omit the template parameter: ```cpp template <typename D> D GetRandDur(const D& duration_max) { return D{GetRand(duration_max.count())}; } BOOST_AUTO_TEST_CASE(util_time_GetRandTime) { std::chrono::seconds rand_hour = GetRandDur(std::chrono::hours{1}); // Want seconds to be in range [0..1hour), but always get zero :(((( BOOST_CHECK_EQUAL(rand_hour.count(), 0); } ``` Luckily `std::common_type` is already specialised in the standard lib for `std::chrono::duration` (https://en.cppreference.com/w/cpp/chrono/duration/common_type). And its effect seem to be that the call site must always specify the template argument explicitly. So instead of implementing the function for each duration type by hand, replace it with a templated version that is safe to use. ACKs for top commit: laanwj: Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 promag: Code review ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0. jonatack: ACK 0000ea3 thanks for the improved documentation. Code review, built, ran `src/test/test_bitcoin -t random_tests -l test_suite` for the new unit tests, `git diff fa05a4c 0000ea3` since previous review: hebasto: ACK 0000ea32656833efa3d2ffd9bab66c88c83334f0 with non-blocking [nit](https://github.com/bitcoin/bitcoin/pull/18781#discussion_r424924671). Tree-SHA512: e89d46e31452be6ea14269ecbbb2cdd9ae83b4412cd14dff7d1084283092722a2f847cb501e8054394e4a3eff852f9c87f6d694fd008b3f7e8458cb5a3068af7
2020-05-14Switch from Optional<T> to std::optional<T> (C++17). Run clang-format.practicalswift
2020-05-14tests: Fill fuzzing coverage gaps for functions in util/translation.hpracticalswift
2020-05-14tests: Fill fuzzing coverage gaps for functions in consensus/validation.hpracticalswift
2020-05-14tests: Fill fuzzing coverage gaps for functions in primitives/block.hpracticalswift
2020-05-14test: Remove const to work around compiler error on xenialWladimir J. van der Laan
Fix the following error in travis: test/validationinterface_tests.cpp:26:36: error: default initialization of an object of const type 'const BlockValidationState' without a user-provided default constructor const BlockValidationState state_dummy;
2020-05-14Merge #18742: miner: Avoid stack-use-after-return in validationinterfacefanquake
7777f2a4bb1f9d843bc50a4e35085cfbb2808780 miner: Avoid stack-use-after-return in validationinterface (MarcoFalke) fa5ceb25fce2200edf6b8ebfa6d4f01ed6774b95 test: Remove UninterruptibleSleep from test and replace it by SyncWithValidationInterfaceQueue (MarcoFalke) fa770ce7fe67685c43780e219d8232efbee0bb8e validationinterface: Rework documentation, Rename pwalletIn to callbacks (MarcoFalke) fab6d060ce5f580db538070beec1c5518c8c777c test: Add unregister_validation_interface_race test (MarcoFalke) Pull request description: When a validationinterface has itself unregistered in one thread, but is about to get executed in another thread [1], there is a race: * The validationinterface destructing itself * The validationinterface getting dereferenced for execution [1] https://github.com/bitcoin/bitcoin/blob/64139803f1225dab26197a20314109d37fa87d5f/src/validationinterface.cpp#L82-L83 This happens in the miner. More generally it happens everywhere where at least one thread is generating notifications and another one is unregistering a validationinterface. This issue has been fixed in commit ab31b9d6fe7b39713682e3f52d11238dbe042c16, but the fix has not been applied to the miner. Example where this happened in practice: https://travis-ci.org/github/bitcoin/bitcoin/jobs/675322230#L4414 ACKs for top commit: promag: Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780. laanwj: Code review ACK 7777f2a4bb1f9d843bc50a4e35085cfbb2808780 Tree-SHA512: 8087119243c71ba18a823a63515f3730d127162625d8729024278b447af29e2ff206f4840ee3d90bf84f93a2c5ab73b76c7e7044c83aa93b5b51047a166ec3d3
2020-05-13miner: Avoid stack-use-after-return in validationinterfaceMarcoFalke
This is achieved by switching to a shared_ptr. Also, switch the validationinterfaces in the tests to use shared_ptrs for the same reason.
2020-05-13test: Remove UninterruptibleSleep from test and replace it by ↡MarcoFalke
SyncWithValidationInterfaceQueue For the purpose of this test the two have the same outcome, but this one is shorter and avoids a sleep for 0.1 seconds.
2020-05-13test: Add unregister_validation_interface_race testMarcoFalke
This commit is (intentionally) adding a broken test. The test is broken because it registering a subscriber object that can go out of scope while events are still being sent. To run the broken test and reproduce the bug: - Remove comment /** and */ - ./configure --with-sanitizers=address - export ASAN_OPTIONS=detect_leaks=0 - make - while ./src/test/test_bitcoin -t validationinterface_tests/unregister_validation_interface_race --catch_system_errors=no ; do true; done
2020-05-13refactor: Remove g_rpc_node globalRussell Yanofsky
This commit does not change behavior
2020-05-13refactor: Pass NodeContext to RPC and REST methods through util::RefRussell Yanofsky
This commit does not change behavior
2020-05-13Add util::Ref class as temporary alternative for c++17 std::anyRussell Yanofsky
This commit does not change behavior
2020-05-12test: use p2p constants in denial of service testsfanquake
2020-05-11Merge #18216: test, build: Enable -Werror=sign-comparefanquake
68537275bd91d1dc14a69609ae443f955bfdbd64 build: Enable -Werror=sign-compare (Ben Woosley) eac6a3080d38cfd4eb7204ecd327df213958e51a refactor: Rework asmap Interpret to avoid ptrdiff_t (Ben Woosley) df37377e30678ac9b8338ea920e50b7296da6bd5 test: Fix outstanding -Wsign-compare errors (Ben Woosley) Pull request description: Disallowing sign-comparison mismatches can help to prevent the introduction of overflow and interpretation bugs. In this case, ~all~ most existing violations are in the tests, and most simply required annotating the literal as unsigned for comparison. This was previously prevented by violations in leveldb which were fixed upstream and merged in #17398. You can test that by building this branch against: 22d11187ee3c7abfe9d43c9eb68f102498cc2b9a vs 75fb37ce68289eb7e00e2ccdd2ef7f9271332545 ACKs for top commit: fjahr: re-ACK 68537275bd91d1dc14a69609ae443f955bfdbd64 practicalswift: ACK 68537275bd91d1dc14a69609ae443f955bfdbd64 Tree-SHA512: 14b5daa38c496fb51548feb30fb4dd179e6f76a8d355f52bc8e2a18f2f9340f0bc98dcf36d8b3d6521045d013891c3103749a4eda88ceef00202a6a0cf93f73c
2020-05-09fuzz: use std::optional for sep_pos variableHarris
Co-authored-by: MarcoFalke <falke.marco@gmail.com>
2020-05-08fuzz: fix vector size problem in system fuzzerHarris
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
2020-05-08test: Fix outstanding -Wsign-compare errorsBen Woosley
2020-05-06Merge #18512: Improve asmap checks and add sanity checkWladimir J. van der Laan
748977690e0519110cda9628162a7ccf73a5934b Add asmap_direct fuzzer that tests Interpreter directly (Pieter Wuille) 7cf97fda154ba837933eb05be5aeecfb69a06641 Make asmap Interpreter errors fatal and fuzz test it (Pieter Wuille) c81aefc5377888c7ac4f29f570249fd6c2fdb352 Add additional effiency checks to sanity checker (Pieter Wuille) fffd8dca2de39ad4a683f0dce57cdca55ed2f600 Add asmap sanity checker (Pieter Wuille) 5feefbe6e7b6cdd809eba4074d41dc95a7035f7e Improve asmap Interpret checks and document failures (Pieter Wuille) 2b3dbfa5a63cb5a6625ec00294ebd933800f0255 Deal with decoding failures explicitly in asmap Interpret (Pieter Wuille) 1479007a335ab43af46f527d0543e254fc2a8e86 Introduce Instruction enum in asmap (Pieter Wuille) Pull request description: This improves/documents the failure cases inside the asmap interpreter. None of the changes are bug fixes (they only change behavior for corrupted asmap files), but they may make things easier to follow. In a second step, a sanity checker is added that effectively executes every potential code path through the asmap file, checking the same failure cases as the interpreter, and more. It takes around 30 ms to run for me for a 1.2 MB asmap file. I've verified that this accepts asmap files constructed by https://github.com/sipa/asmap/blob/master/buildmap.py with a large dataset, and no longer accepts it with 1 bit changed in it. ACKs for top commit: practicalswift: ACK 748977690e0519110cda9628162a7ccf73a5934b modulo feedback below. jonatack: ACK 748977690e0519110cda9628162a7ccf73a5934b code review, regular build/tests/ran bitcoin with -asmap, fuzz build/ran both fuzzers overnight. fjahr: ACK 748977690e0519110cda9628162a7ccf73a5934b Tree-SHA512: d876df3859735795c857c83e7155ba6851ce839bdfa10c18ce2698022cc493ce024b5578c1828e2a94bcdf2552c2f46c392a251ed086691b41959e62a6970821
2020-05-06Merge #18853: wallet: Fix typo in assert that is compile-time trueWladimir J. van der Laan
fa47cf9d95dc2c2822fc96df16f179176935bf96 wallet: Fix typo in assert that is compile-time true (MarcoFalke) Pull request description: Commit 92bcd70808b9cac56b184903aa6d37baf9641b37 presumably added a check that a `dest` of type `CNoDestination` implies an empty `scriptChange`. However, it accidentally checked for `boost::variant::empty`, which always returns false: https://www.boost.org/doc/libs/1_72_0/doc/html/boost/variant.html#id-1_3_46_5_4_1_1_16_2-bb ACKs for top commit: Sjors: utACK fa47cf9d95dc2c2822fc96df16f179176935bf96 Tree-SHA512: 9626b1e2947039853703932a362c2ee204e002d3344856eb93eef0e0f833401336f2dfa80fd43b83c8ec6eac624e6302aee771fb67aec436ba6483be02b8d615
2020-05-06Merge #18806: net: remove is{Empty,Full} flags from CBloomFilter, clarify ↡fanquake
CVE fix 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 net: remove is{Empty,Full} flags from CBloomFilter, clarify CVE fix (Sebastian Falbesoner) Pull request description: The BIP37 bloom filter class `CBloomFilter` contains two flags `isEmpty`/`isFull` together with an update method with the purpose to, according to the comments, "avoid wasting cpu", i.e. the mechanism should serve as an optimization for the trivial cases of empty (all bits zero) or full (all bits one) filters. However, the real reason of adding those flags (introduced with commit https://github.com/bitcoin/bitcoin/commit/37c6389c5a0ca63ae3573440ecdfe95d28ad8f07 by gmaxwell) was a _covert fix_ of [CVE-2013-5700](https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2013-5700), a vulnerability that allowed a divide-by-zero remote node crash. According to gmaxwell himself (https://github.com/bitcoin/bitcoin/pull/9060#issuecomment-257749165): > the IsEmpty/IsFull optimizations were largely a pretextual optimization intended to make unexploitable a remote crash vulnerability (integer division by zero) that existed in the original bloom filtering code without disclosing it. I'm doubtful that they are all that useful. :) For more information on how to trigger this crash, see PR https://github.com/bitcoin/bitcoin/pull/18515 which contains a detailled description and a regression test. It has also been discussed on a [recent PR club meeting on fuzzing](https://bitcoincore.reviews/18521.html). The covert fix code already led to issues and PR based on the wrong assumption that the flags are there for optimization reasons (see #16886 and #16922). This PR gets rid of the flags and the update method and just focuses on the CVE fix itself, i.e. it can be seen as a revert of the covert fix commit modulo the actual fix. ACKs for top commit: meshcollider: utACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 laanwj: Concept and code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 jkczyz: ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 MarcoFalke: ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 fjahr: Code review ACK 1ad8ea2b73134bdd8d6b50704a019d47ad2191d8 Tree-SHA512: 29f7ff9faece0285e11e16c024851f5bcb772dec64118ccc3f9067ec256267ec8e1b1e3105c7de2a72fd122c3b085e8fc840ab8f4e49813f1cc7a444df1867f7
2020-05-04wallet: Fix typo in assert that is compile-time trueMarcoFalke
2020-05-04Merge #18783: tests: Add fuzzing harness for MessageSign, MessageVerify and ↡MarcoFalke
other functions in util/message.h 38e49ded8bd079f8da8b270b39f81cc5cf3ada11 tests: Add fuzzing harness for MessageSign, MessageVerify and other functions in util/message.h (practicalswift) Pull request description: Add fuzzing harness for `MessageSign`, `MessageVerify` and other functions in `util/message.h`. See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for 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: vasild: utACK 38e49ded8bd079f8da8b270b39f81cc5cf3ada11 Tree-SHA512: 4f83718365d9c7e772a4ccecb31817bf17117efae2bfaf6e9618ff17908def0c8b97b5fa2504d51ab38b2e6f82c046178dd751495cc37ab4779c0b1ac1a4d211
2020-05-04Merge #18859: Remove CCoinsViewCache::GetValueIn(...)MarcoFalke
b56607a89ba112083f2b0a7b64ab18d66b26e2be Remove CCoinsViewCache::GetValueIn(...) (practicalswift) Pull request description: Remove `CCoinsViewCache::GetValueIn(...)`. Fixes #18858. It seems like `GetValueIn` was added in #748 ("Pay-to-script-hash (OP_EVAL replacement)", merged in 2012) and the last use in validation code was removed in #8498 ("Near-Bugfix: Optimization: Minimize the number of times it is checked that no money...", merged in 2017). `CCoinsViewCache::GetValueIn(…)` performs money summation like this: ```c++ CAmount CCoinsViewCache::GetValueIn(const CTransaction& tx) const { if (tx.IsCoinBase()) return 0; CAmount nResult = 0; for (unsigned int i = 0; i < tx.vin.size(); i++) nResult += AccessCoin(tx.vin[i].prevout).out.nValue; return nResult; } ``` Note that no check is done to make sure that the resulting `nResult` is such that it stays within the money bounds (`MoneyRange(nResult)`), or that the summation does not trigger a signed integer overflow. Proof of concept output: ``` coins.cpp:243:17: runtime error: signed integer overflow: 9223200000000000000 + 2100000000000000 cannot be represented in type 'long' GetValueIn = -9221444073709551616 ``` Proof of concept code: ```c++ CMutableTransaction mutable_transaction; mutable_transaction.vin.resize(4393); Coin coin; coin.out.nValue = MAX_MONEY; assert(MoneyRange(coin.out.nValue)); CCoinsCacheEntry coins_cache_entry; coins_cache_entry.coin = coin; coins_cache_entry.flags = CCoinsCacheEntry::DIRTY; CCoinsView backend_coins_view; CCoinsViewCache coins_view_cache{&backend_coins_view}; CCoinsMap coins_map; coins_map.emplace(COutPoint{}, std::move(coins_cache_entry)); coins_view_cache.BatchWrite(coins_map, {}); const CAmount total_value_in = coins_view_cache.GetValueIn(CTransaction{mutable_transaction}); std::cout << "GetValueIn = " << total_value_in << std::endl; ``` ACKs for top commit: MarcoFalke: ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be promag: Code review ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be. jb55: ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be hebasto: ACK b56607a89ba112083f2b0a7b64ab18d66b26e2be, I have not tested the code, but I have reviewed it and it looks OK, I agree it can be merged. Tree-SHA512: 2c8402b5753ec96703d12c57c3eda8eccf999ed3519134a87faaf0838cfe44b94ef384296af2a524c06c8756c0245418d181af9083548e360905fac9d79215e6
2020-05-03Remove CCoinsViewCache::GetValueIn(...)practicalswift
2020-05-02Merge #18413: script: prevent UB when computing abs value for num opcode ↡fanquake
serialize 2748e8793267126c5b40621d75d1930e358f057e script: prevent UB when computing abs value for num opcode serialize (pierrenn) Pull request description: This was reported by practicalswift here #18046 It seems that the original author of the line used a reference to glibc `abs`: https://github.com/lattera/glibc/blob/master/stdlib/abs.c However depending on some implementation details this can be undefined behavior for unusual values. A detailed explanation of the UB is provided here : https://stackoverflow.com/questions/17313579/is-there-a-safe-way-to-get-the-unsigned-absolute-value-of-a-signed-integer-with (by [Billy O'Neal](https://twitter.com/malwareminigun)) Simple relevant godbolt example : https://godbolt.org/z/yRwtCG Thanks! ACKs for top commit: sipa: ACK 2748e8793267126c5b40621d75d1930e358f057e MarcoFalke: ACK 2748e8793267126c5b40621d75d1930e358f057e, only checked that the bitcoind binary does not change with clang -O2 πŸŽ“ practicalswift: ACK 2748e8793267126c5b40621d75d1930e358f057e Tree-SHA512: 539a34c636c2674c66cb6e707d9d0dfdce63f59b5525610ed88da10c9a8d59d81466b111ad63b850660cef3750d732fc7755530c81a2d61f396be0707cd86dec
2020-04-30tests: Clarify how we avoid hitting the signed integer overflow in ↡practicalswift
CFeeRate::GetFeePerK() when fuzzing
2020-04-30tests: Add fuzzing harness for IsRBFOptIn(...)practicalswift
2020-04-30tests: Add fuzzing harness for CBlockPolicyEstimatorpracticalswift
2020-04-30test: Add test for GetRandMillis and GetRandMicrosMarcoFalke
2020-04-30Merge #18825: test: fix message for ECC_InitSanityCheck testMarcoFalke
06e434d7d96b5ebddd2ee829995101a62fa8da4e test: fix message for ECC_InitSanityCheck test (fanquake) Pull request description: OpenSSL is long gone. ACKs for top commit: laanwj: Good catch. ACK 06e434d7d96b5ebddd2ee829995101a62fa8da4e Tree-SHA512: 1a920fd6493e0374ca00633407e0130f987b136bc68d2062402747bda16a1e588a12bd8b0b8cdef828c9911f210386cfbdb25d478cb9b684d52769d197032064
2020-04-30Merge #18780: validation: add const for minimum witness commitment sizefanquake
692f8307fc1449299b90182e7d79efb81a55d7ab test: add test for witness commitment index (fanquake) 06442549f8b725f46c1c727e9eb6fde6b843503c validation: Add minimum witness commitment size constant (fanquake) Pull request description: https://github.com/bitcoin/bitcoin/commit/16101de5f33be494019a3f81755e204d00c22347: Per [BIP 141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#Commitment_structure), the witness commitment structure is at least 38 bytes, OP_RETURN (0x6a) + 36 (0x24) + 4 byte header (0xaa21a9ed) + 32 byte SHA256 hash. It can be longer, however any additional data has no consensus meaning. https://github.com/bitcoin/bitcoin/commit/54f8c48d6ac973024df35c4db038791b7958a51d: As per BIP 141, if there is more than 1 pubkey that matches the witness commitment structure, the one with the highest output index should be chosen. This adds a sanity check that we are doing that, which will fail if anyone tries to "optimize" GetWitnessCommitmentIndex() by returning early. ACKs for top commit: MarcoFalke: ACK 692f8307fc1449299b90182e7d79efb81a55d7ab 🌡 jonatack: Code review ACK 692f830 ajtowns: ACK 692f8307fc1449299b90182e7d79efb81a55d7ab jnewbery: utACK 692f8307fc1449299b90182e7d79efb81a55d7ab laanwj: ACK 692f8307fc1449299b90182e7d79efb81a55d7ab Tree-SHA512: 7af3fe4b8a52fea2cdd0aec95f7bb935351a77b73d934bc88d6625a3503311b2a062cba5190b2228f97caa76840db3889032d910fc8e318ca8e7810a8afbafa0
2020-04-30test: fix message for ECC_InitSanityCheck testfanquake
OpenSSL is long gone.
2020-04-29Merge #18736: test: Add fuzzing harnesses for various classes/functions in util/MarcoFalke
32b6b386a5499b1f8439f80d8fc1ee573bc31a53 tests: Sort fuzzing harnesses (practicalswift) e1e181fad1a73e9dee38a2bd74518e1b8d446930 tests: Add fuzzing coverage for JSONRPCTransactionError(...) and RPCErrorFromTransactionError(...) (practicalswift) 103b6ecce0f8e6d1366962c8748794067b2485fe tests: Add fuzzing coverage for TransactionErrorString(...) (practicalswift) dde508b8b03a4a144331cb1ff97f1349b491c402 tests: Add fuzzing coverage for ParseFixedPoint(...) (practicalswift) 1532259fcae8712777e1cedefc91224ee60a6aaa tests: Add fuzzing coverage for FormatHDKeypath(...) and WriteHDKeypath(...) (practicalswift) 90b635e84e432e5a3682864f15274dba6acfbded tests: Add fuzzing coverage for CHECK_NONFATAL(...) (practicalswift) a4e3d13df6a6f48974f541de0b5b061e8078ba9a tests: Add fuzzing coverage for StringForFeeReason(...) (practicalswift) a19598cf9851cb238a4b5caa04f9ae7281532352 tests: Add fuzzing harness for functions in system.h (ArgsManager) (practicalswift) Pull request description: Add fuzzing harnesses for various classes/functions in `util/`. See [`doc/fuzzing.md`](https://github.com/bitcoin/bitcoin/blob/master/doc/fuzzing.md) for 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 :) Top commit has no ACKs. Tree-SHA512: d27947220850c2a202c7740f44140c17545f45522596912452ccab0c2f5379abeb07cc769982c7855cb465059425206371a2b75ee1c285b03984161c9619d0b0
2020-04-29test: add test for witness commitment indexfanquake
As per BIP 141, if there is more than 1 pubkey that matches the witness commitment structure, the one with the highest output index should be chosen. This adds a sanity check that we are doing that, which will fail if anyone trys to "optimise" GetWitnessCommitmentIndex() be returning early.
generated by cgit v1.2.3 (git 2.26.2) at 2024-11-01 14:30:33 +0000