aboutsummaryrefslogtreecommitdiff
path: root/src/test/fuzz
AgeCommit message (Collapse)Author
2023-08-14Move CTxDestination to its own fileAndrew Chow
CTxDestination is really our internal representation of an address and doesn't really have anything to do with standard script types, so move them to their own file.
2023-08-14Move CScriptID to script.{h/cpp}Andrew Chow
CScriptID should be next to CScript just as CKeyID is next to CPubKey
2023-08-03Merge bitcoin/bitcoin#27918: fuzz: addrman, avoid `ConsumeDeserializable` ↵fanquake
when possible 025fda0a76893d09d19ec9a6c0ba86ad11c466f7 fuzz: addrman, avoid `ConsumeDeserializable` when possible (brunoerg) Pull request description: Using specific functions like `ConsumeService`, `ConsumeAddress` and `ConsumeNetAddr` may be more effective than using `ConsumeDeserializable`. They always return some value while `ConsumeDeserializable` may return `std::nullopt`. E.g.: In this part of the code, if `op_net_addr` is `std::nullopt`, we basically generated the addresses (if so) unnecessarily, because we won't be able to use them: ```cpp std::vector<CAddress> addresses; LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 10000) { const std::optional<CAddress> opt_address = ConsumeDeserializable<CAddress>(fuzzed_data_provider); if (!opt_address) { break; } addresses.push_back(*opt_address); } const std::optional<CNetAddr> opt_net_addr = ConsumeDeserializable<CNetAddr>(fuzzed_data_provider); if (opt_net_addr) { addr_man.Add(addresses, *opt_net_addr, std::chrono::seconds{ConsumeTime(fuzzed_data_provider, 0, 100000000)}); } ``` Also, if we are not calling `Add` effectively, it would also be affect other functions that may "depend" on it. ACKs for top commit: dergoegge: Code review ACK 025fda0a76893d09d19ec9a6c0ba86ad11c466f7 Tree-SHA512: 02450bec0b084c15ba0cd1cbdfbac067c8fea4ccf27be0c86d54e020f029a6c749a16d8e0558f9d6d35a7ca9db8916f180c872f09474702b5591129e9be0d192
2023-08-01Merge bitcoin/bitcoin#28194: test: python E721 and flake8 updatesfanquake
bee2d57a655645dbfaf0242e85c5af034023a2fb script: update flake8 to 6.1.0 (Jon Atack) 38c3fd846bff163eb7c50bd77efcdcf8fcbc7f43 test: python E721 updates (Jon Atack) Pull request description: Update our functional tests per [E721](https://www.flake8rules.com/rules/E721.html) enforced by [flake8 6.1.0](https://flake8.pycqa.org/en/latest/release-notes/6.1.0.html), and update our CI lint task to use that release. This makes the following linter output on current master with flake8 6.1.0 green. ``` $ ./test/lint/lint-python.py ; ./test/lint/lint-spelling.py test/functional/p2p_invalid_locator.py:35:16: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` test/functional/test_framework/siphash.py:34:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` test/functional/test_framework/siphash.py:64:12: E721 do not compare types, for exact checks use `is` / `is not`, for instance checks use `isinstance()` src/test/fuzz/descriptor_parse.cpp:88: occurences ==> occurrences ^ Warning: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt ``` ACKs for top commit: MarcoFalke: lgtm ACK bee2d57a655645dbfaf0242e85c5af034023a2fb Tree-SHA512: f3788a543ca98e44eeeba1d06c32f1b11eec95d4aef068aa1b6b5c401261adfa3fb6c6d6c769f3fe6839d78e74a310d5c926867e7c367d6513a53d580fd376f3
2023-07-31Merge bitcoin/bitcoin#27746: Rework validation logic for assumeutxoRyan Ofsky
a733dd79e29068ad1e0532ac42a45188a040a7b9 Remove unused function `reliesOnAssumedValid` (Suhas Daftuar) d4a11abb1972b54f0babdccfbb2fde97ab885933 Cache block index entry corresponding to assumeutxo snapshot base blockhash (Suhas Daftuar) 3556b850221bc0e597d7dd749d4d47ab58dc8083 Move CheckBlockIndex() from Chainstate to ChainstateManager (Suhas Daftuar) 0ce805b632dcb98944a931f758f76f530f5ce5f2 Documentation improvements for assumeutxo (Ryan Ofsky) 768690b7ce551cd403f8e2a099372915f6022ad4 Fix initialization of setBlockIndexCandidates when working with multiple chainstates (Suhas Daftuar) d43a1f1a2fa35d377c7a9ad7ab92d1ae325bde3d Tighten requirements for adding elements to setBlockIndexCandidates (Suhas Daftuar) d0d40ea9a6478d81d7531b7cfc52a8bdaa0883d6 Move block-storage-related logic to ChainstateManager (Suhas Daftuar) 3cfc75366e6596942cbc84f354f42dfd7fc5c073 test: Clear block index flags when testing snapshots (Suhas Daftuar) 272fbc370c4e133d31d9f1d34e327cc265c5fad2 Update CheckBlockIndex invariants for chains based on an assumeutxo snapshot (Suhas Daftuar) 10c05710ce1602d932037f72dc6c4bbc3f6f34ba Add wrapper for adding entries to a chainstate's block index candidates (Suhas Daftuar) 471da5f6e74bac71aeffe2ebc5faff145a6cbcea Move block-arrival information / preciousblock counters to ChainstateManager (Suhas Daftuar) 1cfc887d00c5d1d4281107e3b3ff4641c6c34631 Remove CChain dependency in node/blockstorage (Suhas Daftuar) fe86a7cd480b32463da900db764d2d11a2bea095 Explicitly track maximum block height stored in undo files (Suhas Daftuar) Pull request description: This PR proposes a clean up of the relationship between block storage and the chainstate objects, by moving the decision of whether to store a block on disk to something that is not chainstate-specific. Philosophically, the decision of whether to store a block on disk is related to validation rules that do not require any UTXO state; for anti-DoS reasons we were using some chainstate-specific heuristics, and those have been reworked here to achieve the proposed separation. This PR also fixes a bug in how a chainstate's `setBlockIndexCandidates` was being initialized; it should always have all the HAVE_DATA block index entries that have more work than the chain tip. During startup, we were not fully populating `setBlockIndexCandidates` in some scenarios involving multiple chainstates. Further, this PR establishes a concept that whenever we have 2 chainstates, that we always know the snapshotted chain's base block and the base block's hash must be an element of our block index. Given that, we can establish a new invariant that the background validation chainstate only needs to consider blocks leading to that snapshotted block entry as potential candidates for its tip. As a followup I would imagine that when writing net_processing logic to download blocks for the background chainstate, that we would use this concept to only download blocks towards the snapshotted entry as well. ACKs for top commit: achow101: ACK a733dd79e29068ad1e0532ac42a45188a040a7b9 jamesob: reACK a733dd79e29068ad1e0532ac42a45188a040a7b9 ([`jamesob/ackr/27746.5.sdaftuar.rework_validation_logic`](https://github.com/jamesob/bitcoin/tree/ackr/27746.5.sdaftuar.rework_validation_logic)) Sjors: Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. ryanofsky: Code review ACK a733dd79e29068ad1e0532ac42a45188a040a7b9. Just suggested changes since the last review. There are various small things that could be followed up on, but I think this is ready for merge. Tree-SHA512: 9ec17746f22b9c27082743ee581b8adceb2bd322fceafa507b428bdcc3ffb8b4c6601fc61cc7bb1161f890c3d38503e8b49474da7b5ab1b1f38bda7aa8668675
2023-07-31script: update flake8 to 6.1.0Jon Atack
and touch up the spelling returned by lint-spelling.py
2023-07-31Merge bitcoin/bitcoin#28091: fuzz: use `ConnmanTestMsg` in `connman`fanquake
ecfe507e07e9bdab210e04ebd3fc3b8ae9d6a094 fuzz: use `ConnmanTestMsg` in `connman` (brunoerg) Pull request description: Fixes #27980 Using `ConnmanTestMsg` we can add nodes and be more effective fuzzing functions like `DisconnectNode`, `FindNode`, `GetNodeStats` and other ones. ACKs for top commit: MarcoFalke: review ACK ecfe507e07e9bdab210e04ebd3fc3b8ae9d6a094 dergoegge: utACK ecfe507e07e9bdab210e04ebd3fc3b8ae9d6a094 Tree-SHA512: 97c363b422809f2e9755c082d1102237347abfab72c7baca417bd8975f8a595ddf3a085f8353dbdb9f17fb98fbfe830792bfc0b83451168458018faf6c239efa
2023-07-28Merge bitcoin/bitcoin#28162: refactor: Revert additional univalue check in ↵fanquake
ParseSighashString 06199a995f20c55583f6948cfe99e608679fcdf1 refactor: Revert addition of univalue sighash string check (TheCharlatan) 0b47c1621524a96b79cbdc3c45ee5ad36e7ba541 doc: Correct release-notes for sighashtype exceptions (TheCharlatan) Pull request description: This is a follow up for #28113. The string type check is already done by the rpc parser / RPCHelpMan. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274568557). Also correct the release note for the correct sighashtype exception change. There is no change in the handling of non-string sighashtype arugments. Pointed out in this [comment](https://github.com/bitcoin/bitcoin/pull/28113/files#r1274567555). ACKs for top commit: MarcoFalke: lgtm ACK 06199a995f20c55583f6948cfe99e608679fcdf1 jonatack: Tested ACK 06199a995f20c55583f6948cfe99e608679fcdf1 stickies-v: ACK 06199a995f20c55583f6948cfe99e608679fcdf1 Tree-SHA512: 3faa6b3d2247624c0973df8d79c09fbf1f90ffb99f1be484e359b528f485c31affea45976759bd206e4c81cbb54ebba5ad0ef4127d1deacbfe2a58153fcc94ee
2023-07-27Merge bitcoin/bitcoin#27888: Fuzz: a more efficient descriptor parsing targetAndrew Chow
131314b62e899f95d1863083d303b489b3212b16 fuzz: increase coverage of the descriptor targets (Antoine Poinsot) 90a24741e79cbf20d4456050f0fe39c3f88f5246 fuzz: add a new, more efficient, descriptor parsing target (Antoine Poinsot) d60229ede54e05724d444eaba02a9ed72f5ada02 fuzz: make the parsed descriptor testing into a function (Antoine Poinsot) Pull request description: The current descriptor parsing fuzz target requires valid public or private keys to be provided. This is unnecessary as we are only interested in fuzzing the descriptor parsing logic here (other targets are focused on fuzzing keys serializations). And it's pretty inefficient, especially for formats that need a checksum (`xpub`, `xprv`, WIF). This introduces a new target that mocks the keys as an index in a list of precomputed keys. Keys are represented as 2 hex characters in the descriptor. The key type (private, public, extended, ..) is deterministically based on this one-byte value. Keys are deterministically generated at target initialization. This is much more efficient and also largely reduces the size of the seeds. TL;DR: for instance instead of requiring the fuzzer to generate a `pk(xpub6DdBu7pBoyf7RjnUVhg8y6LFCfca2QAGJ39FcsgXM52Pg7eejUHLBJn4gNMey5dacyt4AjvKzdTQiuLfRdK8rSzyqZPJmNAcYZ9kVVEz4kj)` to parse a valid descriptor, it just needs to generate a `pk(03)`. Note we only mock the keys themselves, not the entire descriptor key expression. As we want to fuzz the real code that parses the rest of the key expression (origin, derivation paths, ..). This is a target i used for reviewing #17190 and #27255, and figured it was worth PR'ing on its own since the added complexity for mocking the keys is minimal and it could help prevent introducing bugs to the descriptor parsing logic much more efficiently. ACKs for top commit: MarcoFalke: re-ACK 131314b62e899f95d1863083d303b489b3212b16 🐓 achow101: ACK 131314b62e899f95d1863083d303b489b3212b16 Tree-SHA512: 485a8d6a0f31a3a132df94dc57f97bdd81583d63507510debaac6a41dbbb42fa83c704ff3f2bd0b78c8673c583157c9a3efd79410e5e79511859e1470e629118
2023-07-27refactor: Revert addition of univalue sighash string checkTheCharlatan
This check is already done by the rpc parser. Re-doing it is adding dead code. Instead, throwing an exception when the assumption does not hold is the already correct behavior. To make the fuzz test more accurate and not swallow all runtime errors, add a check that the passed in UniValue sighash argument is either a string or null. Co-authored-by: stickies-v <stickies-v@protonmail.com>
2023-07-25Merge bitcoin/bitcoin#28113: kernel: Remove UniValue from kernel libraryAndrew Chow
6960c81cbfa6208d4098353e53b313e13a21cb49 kernel: Remove Univalue from kernel library (TheCharlatan) 10eb3a9faa977371facacee937b2e6dc26f008e0 kernel: Split ParseSighashString (TheCharlatan) Pull request description: Besides the build system changes, this is a mostly move-only change for moving the few UniValue-related functions out of kernel files. UniValue is not required by any of the kernel components and a JSON library should not need to be part of a consensus library. ACKs for top commit: achow101: ACK 6960c81cbfa6208d4098353e53b313e13a21cb49 theuni: Re-ACK 6960c81cbfa6208d4098353e53b313e13a21cb49 stickies-v: re-ACK https://github.com/bitcoin/bitcoin/commit/6960c81cbfa6208d4098353e53b313e13a21cb49 Tree-SHA512: d92e4cb4e12134c94b517751bd746d39f9b8da528ec3a1c94aaedcce93274a3bae9277832e8a7c0243c13df0397ca70ae7bbb24ede200018c569f8d81103c1da
2023-07-25kernel: Remove Univalue from kernel libraryTheCharlatan
It is not required by any of the kernel components. A JSON library should not need to be part of a consensus library.
2023-07-25kernel: Split ParseSighashStringTheCharlatan
This split is done in preparation for the next commit where the dependency on UniValue in the kernel library is removed.
2023-07-22fuzz: use `ConnmanTestMsg` in `connman`brunoerg
Using `ConnmanTestMsg` we can add nodes and be more effective fuzzing functions like `DisconnectNode`, `FindNode`, `GetNodeStats` and other ones.
2023-07-21fuzz: increase coverage of the descriptor targetsAntoine Poinsot
Once a descriptor is successfully parsed, execute more of its methods. There is probably still room for improvements by checking for some invariants, but this is a low hanging fruit that significantly increases the code coverage of these targets.
2023-07-21fuzz: add a new, more efficient, descriptor parsing targetAntoine Poinsot
This new target focuses on fuzzing the actual descriptor parsing logic by not requiring the fuzzer to produce valid keys (nor a valid checksum for that matter). This should make it much more efficient to find bugs we could introduce moving forward. Using a character as a marker (here '%') to be able to search and replace in the string without having to mock the actual descriptor parsing logic was an insight from Pieter Wuille.
2023-07-21Move block-storage-related logic to ChainstateManagerSuhas Daftuar
Separate the notion of which blocks are stored on disk, and what data is in our block index, from what tip a chainstate might be able to get to. We can use chainstate-agnostic data to determine when to store a block on disk (primarily, an anti-DoS set of criteria) and let the chainstates figure out for themselves when a block is of interest for being a candidate tip. Note: some of the invariants in CheckBlockIndex are modified, but more work is needed (ie to move CheckBlockIndex to ChainstateManager, as most of what CheckBlockIndex is doing is checking the consistency of the block index, which is outside of Chainstate).
2023-07-21fuzz: make the parsed descriptor testing into a functionAntoine Poinsot
We'll be reusing it in the new target.
2023-07-19Merge bitcoin/bitcoin#28085: refactor: use Span for SipHash::WriteAndrew Chow
7d92b1430a6fd42c4438810640576830d0ff8d13 refactor: use Span for SipHash::Write (Sebastian Falbesoner) Pull request description: This simple refactoring PR changes the interface for the `SipHash` arbitrary-data `Write` method to take a `Span<unsigned char>` instead of having to pass data and length. (`Span<std::byte>` seems to be more modern, but vectors of `unsigned char` are still used prety much everywhere where SipHash is called, and I didn't find it very appealing having to clutter the code with `Make(Writable)ByteSpan` helpers). ACKs for top commit: sipa: utACK 7d92b1430a6fd42c4438810640576830d0ff8d13 MarcoFalke: lgtm ACK 7d92b1430a6fd42c4438810640576830d0ff8d13 achow101: ACK 7d92b1430a6fd42c4438810640576830d0ff8d13 Tree-SHA512: f17a27013c942aead4b09f5a64e0c3ff8dbc7e83fe63eb9a2e3ace8be9921c9cbba3ec67e3e83fbe3332ca941c42370efd059e702c060f9b508307e9657c66f2
2023-07-17Merge bitcoin/bitcoin#27997: Descriptors: rule out unspendable miniscript ↵Andrew Chow
descriptors c7db88af71b3204171f33399aa4f33b40a4f7cd9 descriptor: assert we never parse a sane miniscript with no pubkey (Antoine Poinsot) a49402a9ec7431c286139b76f8759719a99a8551 qa: make sure we don't let unspendable Miniscript descriptors be imported (Antoine Poinsot) 639e3b6c9759a7a582c5c86fdbfa5ea99cb7bb16 descriptor: refuse to parse unspendable miniscript descriptors (Antoine Poinsot) e3280eae1b53006d74d11f3cf9d7a9dc7ff2c39e miniscript: make GetStackSize() and GetOps() return optionals (Antoine Poinsot) Pull request description: `IsSane()` in Miniscript does not ensure a Script is actually spendable. This is an issue as we would accept any sane Miniscript when parsing a descriptor. Fix this by explicitly checking a Miniscript descriptor is both sane and spendable when parsing it. This bug was exposed due to a check added in #22838 (https://github.com/bitcoin/bitcoin/pull/22838#discussion_r1226859880) that triggered a fuzz crash (https://github.com/bitcoin/bitcoin/pull/22838#issuecomment-1612510057). ACKs for top commit: sipa: utACK c7db88af71b3204171f33399aa4f33b40a4f7cd9 achow101: ACK c7db88af71b3204171f33399aa4f33b40a4f7cd9 Tree-SHA512: e79bc9f7842e98a4e8f358f05811fca51b15b4b80a171c0d2b17cf4bb1f578a18e4397bc2ece9817d392e0de0196ee6a054b7318441fd3566dd22e1f03eb64a5
2023-07-17Merge bitcoin/bitcoin#27993: Make poly1305 support incremental computation + ↵Andrew Chow
modernize 4e5c933f6a40c07d1c68115f7979b89a5b2ba580 Switch all callers from poly1305_auth to Poly1305 class (Pieter Wuille) 8871f7d1ae096839abcbf25a548319185acc01a2 tests: add more Poly1305 test vectors (Pieter Wuille) 40e6c5b9fce92ffe64e91c2aba38bb2ed57bfbfb crypto: add Poly1305 class with std::byte Span interface (Pieter Wuille) 50269b391fa18556bad72dc8c2fb4e2493a6a054 crypto: switch poly1305 to incremental implementation (Pieter Wuille) Pull request description: Our current Poly1305 code (src/crypto/poly1305.*) only supports computing the entire tag in one go (the `poly1305_auth` function takes a key and message, and outputs the tag). However, the RFC8439 authenticated encryption (as used in BIP324, see #27634) scheme makes use of Poly1305 in a way where the message consists of 3 different pieces: * The additionally authenticated data (AAD), padded to 16 bytes. * The ciphertext, padded to 16 bytes. * The length of the AAD and the length of the ciphertext, together another 16 bytes. Implementing RFC8439 using the existing `poly1305_auth` function requires creating a temporary copy with all these pieces of data concatenated just for the purpose of computing the tag (the approach used in #25361). This PR replaces the poly1305 code with new code from https://github.com/floodyberry/poly1305-donna (with minor adjustments to make it match our coding style and use our utility functions, documented in the commit) which supports incremental operation, and then adds a C++ wrapper interface using std::byte Spans around it, and adds tests that incremental and all-at-once computation match. ACKs for top commit: achow101: ACK 4e5c933f6a40c07d1c68115f7979b89a5b2ba580 theStack: ACK 4e5c933f6a40c07d1c68115f7979b89a5b2ba580 stratospher: tested ACK 4e5c933. Tree-SHA512: df6e9a2a4a38a480f9e4360d3e3def5311673a727a4a85b008a084cf6843b260dc82cec7c73e1cecaaccbf10f3521a0ae7dba388b65d0b086770f7fbc5223e2a
2023-07-17Merge bitcoin/bitcoin#28065: fuzz: Flatten all FUZZ_TARGET macros into onefanquake
fa6dfaaf45bde465969fa7d8fa6b6574a497a6ca scripted-diff: Use new FUZZ_TARGET macro everywhere (MarcoFalke) fa36ad8b091c70190491280dcf0794e94e34a9ed fuzz: Accept options in FUZZ_TARGET macro (MarcoFalke) Pull request description: The `FUZZ_TARGET` macros have many issues: * The developer will have to pick the right macro to pass the wanted option. * Adding a new option requires doubling the number of existing macros in the worst case. Fix all issues by using only a single macro. This refactor does not change behavior. ACKs for top commit: dergoegge: ACK fa6dfaaf45bde465969fa7d8fa6b6574a497a6ca Tree-SHA512: 49a34553867a1734ce89e616b2d7c29b784a67cd8990db6573f0c7b18957636ef0c81d3d0d444a04c12cdc98bc4c4aa7a2ec94e6232dc363620a746e28416444
2023-07-17refactor: use Span for SipHash::WriteSebastian Falbesoner
Co-authored-by: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>
2023-07-17fuzz: Bump FuzzedDataProvider.hMarcoFalke
From https://github.com/llvm/llvm-project/blob/fa8401f9bfe81f4853bf9b67bff42a2cebffc10f/compiler-rt/include/fuzzer/FuzzedDataProvider.h
2023-07-13Merge bitcoin/bitcoin#27549: fuzz: addrman, add coverage for `network` field ↵Andrew Chow
in `Select()`, `Size()` and `GetAddr()` 35a2175ad8bec92b409ae2202c124e39b2f3f838 fuzz: addrman, add coverage for `network` field in `Select()`, `Size()` and `GetAddr()` (brunoerg) Pull request description: This PR adds fuzz coverage for `network` field in `Select()`, `Size()` and `GetAddr()`, there was only call to them without passing a network. https://marcofalke.github.io/b-c-cov/fuzz.coverage/src/addrman.cpp.gcov.html ACKs for top commit: amitiuttarwar: for the record, ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838 - only small changes from the version (previously) proposed in 27213 achow101: ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838 mzumsande: Code Review ACK 35a2175ad8bec92b409ae2202c124e39b2f3f838, haven't tested this yet, but I will let the fuzzer run for a while now. Tree-SHA512: dddb8322298d6c373c8e68d57538470b11825a9a310a355828c351d5c0b19ff6779d024a800e3ea90126d0c050e86f71fd22cd23d1a306c784cef0f82c45e3ca
2023-07-13scripted-diff: Use new FUZZ_TARGET macro everywhereMarcoFalke
-BEGIN VERIFY SCRIPT- ren() { sed --regexp-extended -i "s|$1|$2|g" $(git grep -l --extended-regexp "$1"); } # Replace FUZZ_TARGET_INIT ren 'FUZZ_TARGET_INIT\((.+), (.+)\)' 'FUZZ_TARGET(\1, .init = \2)' # Delete unused FUZZ_TARGET_INIT sed -i -e '37,39d' src/test/fuzz/fuzz.h -END VERIFY SCRIPT-
2023-07-13fuzz: Accept options in FUZZ_TARGET macroMarcoFalke
* This allows to reduce the number of total macros. * Also, adding a new option no longer requires doubling the number of macros in the worst case.
2023-07-13Merge bitcoin/bitcoin#27411: p2p: Restrict self-advertisements with privacy ↵Andrew Chow
networks to avoid fingerprinting e7cf8657e1165ea5da3911a9e543837cd8938f97 test: add unit test for local address advertising (Martin Zumsande) f4754b9dfb84859166843fb2a1888fb3cfebf73c net: restrict self-advertisements with privacy networks (Martin Zumsande) e4d541c7cfa65da77e80e6786fdcb197ab50d04b net, refactor: pass reference for peer address in GetReachabilityFrom (Martin Zumsande) 62d73f5370415f910c95a67b3d9f97bc85487bbe net, refactor: pass CNode instead of CNetAddr to GetLocalAddress (Martin Zumsande) Pull request description: The current logic for self-advertisements works such that we detect as many local addresses as we can, and then, using the scoring matrix from `CNetAddr::GetReachabilityFrom()`, self-advertise with the address that fits best to our peer. It is in general not hard for our peers to distinguish our self-advertisements from other addrs we send them, because we self-advertise every ~24h and because the first addr we send over a connection is likely our self-advertisement. `GetReachabilityFrom()` currently only takes into account actual reachability, but not whether we'd _want_ to announce our identity for one network to peers from other networks, which is not straightforward in connection with privacy networks. While the general approach is to prefer self-advertising with the address for the network our peer is on, there are several special situations in which we don't have one, and as a result could allow self-advertise other local addresses, for example: A) We run i2p and clearnet, use `-i2pacceptincoming=0` (so we have no local i2p address), and we have a local ipv4 address. In this case, we'd advertise the ipv4 address to our outbound i2p peers. B) Our `-discover` logic cannot detect any local clearnet addresses in our network environment, but we are actually reachable over clearnet. If we ran bitcoind clearnet-only, we'd always advertise the address our peer sees us with instead, and could get inbound peers this way. Now, if we also have an onion service running (but aren't using tor as a proxy for clearnet connections), we could advertise our onion address to clearnet peers, so that they would be able to connect our clearnet and onion identities. This PR tries to avoid these situations by 1.) never advertising our local Tor or I2P address to peers from other networks. 2.) never advertising local addresses from non-anonymity networks to peers from Tor or I2P Note that this affects only our own self-advertisements, the rules to forward other people's addrs are not changed. [Edit] after Initial [discussion](https://github.com/bitcoin/bitcoin/pull/27411#issuecomment-1497176155): CJDNS is not being treated like Tor and I2P at least for now, because it has different privacy properties and for the practical reason that it has still very few bitcoin nodes. ACKs for top commit: achow101: ACK e7cf8657e1165ea5da3911a9e543837cd8938f97 vasild: ACK e7cf8657e1165ea5da3911a9e543837cd8938f97 luke-jr: utACK e7cf8657e1165ea5da3911a9e543837cd8938f97 Tree-SHA512: 3db8415dea6f82223d11a23bd6cbb3b8cf68831321280e926034a1f110cbe22562570013925f6fa20d8f08e41d0202fd69c733d9f16217318a660d2a1a21b795
2023-07-12Switch all callers from poly1305_auth to Poly1305 classPieter Wuille
This also removes the old poly1305_auth interface, as it no longer serves any function. The new Poly1305 class based interface is more modern and safe.
2023-07-12crypto: add Poly1305 class with std::byte Span interfacePieter Wuille
2023-07-07crypto: Implement RFC8439-compatible variant of ChaCha20Pieter Wuille
There are two variants of ChaCha20 in use. The original one uses a 64-bit nonce and a 64-bit block counter, while the one used in RFC8439 uses a 96-bit nonce and 32-bit block counter. This commit changes the interface to use the 96/32 split (but automatically incrementing the first 32-bit part of the nonce when the 32-bit block counter overflows, so to retain compatibility with >256 GiB output). Simultaneously, also merge the SetIV and Seek64 functions, as we almost always call both anyway. Co-authored-by: dhruv <856960+dhruv@users.noreply.github.com>
2023-06-29Merge bitcoin/bitcoin#27932: test: Fuzz on macOSfanquake
fae7c50d201726f605938c3511dd9119efeea5ec test: Run fuzz tests on macOS (MarcoFalke) Pull request description: Any reason not to? ACKs for top commit: jamesob: Github ACK https://github.com/bitcoin/bitcoin/pull/27932/commits/fae7c50d201726f605938c3511dd9119efeea5ec dergoegge: utACK fae7c50d201726f605938c3511dd9119efeea5ec Tree-SHA512: e45122d73fafb17cea312258314b826cb0745e08daadd28465f687ec02d4c127d2f8cbe20179a9fff5712038850c02c968abb4838fa088b7555e28709317d3a3
2023-06-29miniscript: make GetStackSize() and GetOps() return optionalsAntoine Poinsot
The value is only set for satisfiable nodes, so it was undefined for non-satisfiable nodes. Make it clear in the interface by returning std::nullopt if the node isn't satisfiable instead of an undefined value.
2023-06-27Use only Span{} constructor for byte-like types where possibleMarcoFalke
This removes bloat that is not needed.
2023-06-26Merge bitcoin/bitcoin#27479: BIP324: ElligatorSwift integrationsAndrew Chow
3168b08043546cd248a81563e21ff096019f1521 Bench test for EllSwift ECDH (Pieter Wuille) 42d759f239d1842ec0c662f8fa9ac0a9ff18a2cb Bench tests for CKey->EllSwift (dhruv) 2e5a8a437cf9ac78548891e61797b394571e27ae Fuzz test for Ellswift ECDH (dhruv) c3ac9f5cf413e263803aac668a90a4ddd7316924 Fuzz test for CKey->EllSwift->CPubKey creation/decoding (dhruv) aae432a764e4ceb7eac305458e585726225c7189 Unit test for ellswift creation/decoding roundtrip (dhruv) eff72a0dff8fa83af873ad9b15dbac50b8d4eca3 Add ElligatorSwift key creation and ECDH logic (Pieter Wuille) 42239f839081bba9a426ebb9f1b7a56e35a2d428 Enable ellswift module in libsecp256k1 (dhruv) 901336eee751de088465e313dd8b500dfaf462b2 Squashed 'src/secp256k1/' changes from 4258c54f4e..705ce7ed8c (Pieter Wuille) Pull request description: This replaces #23432 and part of #23561. This PR introduces all of the ElligatorSwift-related changes (libsecp256k1 updates, generation, decoding, ECDH, tests, fuzzing, benchmarks) needed for BIP324. ElligatorSwift is a special 64-byte encoding format for public keys introduced in libsecp256k1 in https://github.com/bitcoin-core/secp256k1/pull/1129. It has the property that *every* 64-byte array is a valid encoding for some public key, and every key has approximately $2^{256}$ encodings. Furthermore, it is possible to efficiently generate a uniformly random encoding for a given public key or private key. This is used for the key exchange phase in BIP324, to achieve a byte stream that is entirely pseudorandom, even before the shared encryption key is established. ACKs for top commit: instagibbs: reACK https://github.com/bitcoin/bitcoin/pull/27479/commits/3168b08043546cd248a81563e21ff096019f1521 achow101: ACK 3168b08043546cd248a81563e21ff096019f1521 theStack: re-ACK 3168b08043546cd248a81563e21ff096019f1521 Tree-SHA512: 308ac3d33e9a2deecb65826cbf0390480a38de201918429c35c796f3421cdf94c5501d027a043ae8f012cfaa0584656da1de6393bfba3532ab4c20f9533f06a6
2023-06-23Fuzz test for Ellswift ECDHdhruv
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
2023-06-23Fuzz test for CKey->EllSwift->CPubKey creation/decodingdhruv
Co-authored-by: Pieter Wuille <bitcoin-dev@wuille.net>
2023-06-22test: Run fuzz tests on macOSMarcoFalke
Also, fix a few bugs: * Error: RPC command "enumeratesigners" not found in RPC_COMMANDS_SAFE_FOR_FUZZING or RPC_COMMANDS_NOT_SAFE_FOR_FUZZING. Please update test/fuzz/rpc.cpp. * in run_once: ...format(" ".join(result.args), ... TypeError: sequence item 2: expected str instance, PosixPath found
2023-06-21fuzz: Avoid OOM in transaction fuzz targetMarcoFalke
Also fix bug where the json object is reused between two calls.
2023-06-19fuzz: addrman, avoid `ConsumeDeserializable` when possiblebrunoerg
`ConsumeDeserializable` may return `std::nullopt`, prefer to call specific functions such as `ConsumeService`and `ConsumeNetAddr` which always return a value.
2023-06-14tx fees, policy: read stale fee estimates with a regtest-only optionismaelsadeeq
If -acceptstalefeeestimates option is passed stale fee estimates can now be read when operating in regtest environments. Additionally, this commit updates all declarations of the CBlockPolicyEstimator class to include a the second constructor variable.
2023-06-13Merge bitcoin/bitcoin#27806: fuzz: Fix mini_miner_selection running out of coinfanquake
76c5ea703e77d580b6962e60398f4988cbd9b58b fuzz: Fix mini_miner_selection running out of coin (Murch) Pull request description: Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new outputs than the two inputs they each spent. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins which resulted in undefined behavior. Fixed per belt-suspender approach: - assert that available_coins is not empty before generating tx - generate at least two coins per new tx - allow building tx with a single input if only one coin is available ACKs for top commit: MarcoFalke: lgtm ACK 76c5ea703e77d580b6962e60398f4988cbd9b58b dergoegge: reACK 76c5ea703e77d580b6962e60398f4988cbd9b58b Tree-SHA512: 5b7ffd1905a712733ad5364958ad79874dd8c31bd50069b0d3e6f734da0f2d496cb08cbe0afa47115674313e1cb7166a6087f2ccbce289774caddc790583e241
2023-06-12fuzz: Fix mini_miner_selection running out of coinMurch
Fixes a bug in the mini_miner_selection fuzz test found by fuzzing: It was possible for the mini_miner_selection fuzz test to generated transactions that created fewer new spendable outputs than the two inputs they each spend. If the fuzz seed did so consistently, eventually it would cause a `pop_front()` on an empty available_coins. Fixed by: - asserting that available_coins is not empty before generating tx - allowing to build tx with a single coin if only one is available
2023-06-09Merge bitcoin/bitcoin#27576: kernel: Remove args, settings, chainparams, ↵Ryan Ofsky
chainparamsbase from kernel library db77f87c6365cb5f414036d6bfb1a12705772028 scripted-diff: move settings to common namespace (TheCharlatan) c27e4bdc35bc7cedd1ee07e98a52c230241120d1 move-only: Move settings to the common library (TheCharlatan) c2dae5d7d89634fbd771755ce3909719f5462f63 kernel: Remove chainparams, chainparamsbase, args, settings from kernel library (TheCharlatan) 05870b1c92f39d90e5ba6e0caf2f6c2b37955528 refactor: Remove gArgs access from validation.cpp (TheCharlatan) 8789b11114b4bd6c7ee727dffbc75a6bdf20dd27 refactor: Add path argument to FindSnapshotChainstateDir (TheCharlatan) ef95be334f3aec671346372b64606e0fd390979a refactor: Add stop_at_height option in ChainstateManager (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project https://github.com/bitcoin/bitcoin/issues/27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". --- This completes the removal of the node's chainparams, chainparamsbase, args and settings files and their respective classes from the kernel library. This is the last pull request in a long series working towards decoupling the `ArgsManager` and the `gArgs` global from kernel code. These prior pull requests are: https://github.com/bitcoin/bitcoin/pull/26177 https://github.com/bitcoin/bitcoin/pull/27125 https://github.com/bitcoin/bitcoin/pull/25527 https://github.com/bitcoin/bitcoin/pull/25487 https://github.com/bitcoin/bitcoin/pull/25290 ACKs for top commit: MarcoFalke: lgtm ACK db77f87c6365cb5f414036d6bfb1a12705772028 🍄 hebasto: ACK db77f87c6365cb5f414036d6bfb1a12705772028, I have reviewed the code and it looks OK. ryanofsky: Code review ACK db77f87c6365cb5f414036d6bfb1a12705772028. Looks great! Tree-SHA512: cbfbd705d056f2f10f16810d4f869eb152362fff2c5ddae5e1ac6785deae095588e52ad48b29d921962b085e51de1e0ecab6e50f46149ffe3c16250608a2c93a
2023-06-07Merge bitcoin/bitcoin#27810: fuzz: Partially revert #27780fanquake
71200ac390fe5c10f088cbe8053b010b515757b1 [fuzz] Only check duplicate coinbase script when block was valid (dergoegge) Pull request description: Partially revert #27780, because moving the duplicate coinbase check out of the `was_valid` branch leads to non-bug crashes in the fuzz target. For context and further explanation see: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=59516 ACKs for top commit: MarcoFalke: nice lgtm ACK 71200ac390fe5c10f088cbe8053b010b515757b1 Tree-SHA512: 8c38e5ff9de6331016b9a0c5e435d007d46186151b04c09085f617bb31627a28ad56678066fe152372a3ad8656f026439e3e2f9ee61d7ef588072aef8124eaa3
2023-06-07Merge bitcoin/bitcoin#27501: mempool / rpc: add getprioritisedtransactions, ↵Andrew Chow
delete a mapDeltas entry when delta==0 67b7fecacd0489809690982c89ba2d0acdca938c [mempool] clear mapDeltas entry if prioritisetransaction sets delta to 0 (glozow) c1061acb9d502cdf8c6996c818d9a8a281cbe40c [functional test] prioritisation is not removed during replacement and expiry (glozow) 0e5874f0b06114d9b077e0ff582915e4f83059e6 [functional test] getprioritisedtransactions RPC (glozow) 99f8046829f699ff2eace266aa8cea1d9f7cb65a [rpc] add getprioritisedtransactions (glozow) 9e9ca36c80013749faaf2aa777d52bd07d9d24ec [mempool] add GetPrioritisedTransactions (glozow) Pull request description: Add an RPC to get prioritised transactions (also tells you whether the tx is in mempool or not), helping users clean up `mapDeltas` manually. When `CTxMemPool::PrioritiseTransaction` sets a delta to 0, remove the entry from `mapDeltas`. Motivation / Background - `mapDeltas` entries are never removed from mapDeltas except when the tx is mined in a block or conflicted. - Mostly it is a feature to allow `prioritisetransaction` for a tx that isn't in the mempool {yet, anymore}. A user can may resbumit a tx and it retains its priority, or mark a tx as "definitely accept" before it is seen. - Since #8448, `mapDeltas` is persisted to mempool.dat and loaded on restart. This is also good, otherwise we lose prioritisation on restart. - Note the removal due to block/conflict is only done when `removeForBlock` is called, i.e. when the block is received. If you load a mempool.dat containing `mapDeltas` with transactions that were mined already (e.g. the file was saved prior to the last few blocks), you don't delete them. - Related: #4818 and #6464. - There is no way to query the node for not-in-mempool `mapDeltas`. If you add a priority and forget what the value was, the only way to get that information is to inspect mempool.dat. - Calling `prioritisetransaction` with an inverse value does not remove it from `mapDeltas`, it just sets the value to 0. It disappears on a restart (`LoadMempool` checks if delta is 0), but that might not happen for a while. Added together, if a user calls `prioritisetransaction` very regularly and not all those transactions get mined/conflicted, `mapDeltas` might keep lots of entries of delta=0 around. A user should clean up the not-in-mempool prioritisations, but that's currently difficult without keeping track of what those txids/amounts are. ACKs for top commit: achow101: ACK 67b7fecacd0489809690982c89ba2d0acdca938c theStack: Code-review ACK 67b7fecacd0489809690982c89ba2d0acdca938c instagibbs: code review ACK 67b7fecacd0489809690982c89ba2d0acdca938c ajtowns: ACK 67b7fecacd0489809690982c89ba2d0acdca938c code review only, some nits Tree-SHA512: 9df48b622ef27f33db1a2748f682bb3f16abe8172fcb7ac3c1a3e1654121ffb9b31aeaad5570c4162261f7e2ff5b5912ddc61a1b8beac0e9f346a86f5952260a
2023-06-05net, refactor: pass reference for peer address in GetReachabilityFromMartin Zumsande
The address of the peer always exists (because addr is a member of CNode), so it was not possible to pass a nullptr before. Also remove NET_UNKNOWN, which is unused now.
2023-06-03[fuzz] Only check duplicate coinbase script when block was validdergoegge
2023-06-02Merge bitcoin/bitcoin#27256: refactor: rpc: Remove unnecessary uses of ↵fanquake
ParseNonRFCJSONValue() and rename it cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687 refactor: rpc: hide and rename ParseNonRFCJSONValue() (stickies-v) 6c8bde6d54d03224709dce54b8ba32b8c3e37ac7 test: move coverage on ParseNonRFCJSONValue() to UniValue::read() (stickies-v) Pull request description: Follow-up to https://github.com/bitcoin/bitcoin/pull/26612#issuecomment-1453623741. As per https://github.com/bitcoin/bitcoin/pull/26506#pullrequestreview-1211984059, `ParseNonRFCJSONValue()` is no longer necessary and we can use `UniValue::read()` directly: > IIRC before that PR UniValue::read could only parse JSON object and array values, but now it will parse string/number/boolean/null values as well. laanwj pointed this out in https://github.com/bitcoin/bitcoin/issues/9028#issuecomment-257885368 The implementation of `ParseNonRFCJSONValue()` was already [simplified in #26612](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-84c7a7f36362b9724c31e5dec9879b2f81eae0d0addbc9c0933c3558c577de65R259-R263) and [test coverage updated](https://github.com/bitcoin/bitcoin/pull/26612/files#diff-fc0f86b6c3bb23b0e983bcf79d7546d1c9eaa15d6e4d8a7b03b5b85955f585abR292-R312) to ensure behaviour didn't change. To avoid code duplication, we keep the function to throw on invalid input data but rename it to `Parse()` and remove it from the header. The existing test coverage we had on `ParseNonRFCJSONValue()` is moved over to `UniValue::read()`. ACKs for top commit: ryanofsky: Code review ACK cfbc8a623b5133f1d0b0c0c9be73b2b107e0d687. Only change since last review is adding a new test Tree-SHA512: 89be959d2353af7ace0c1348ba1600b9ac1d3c7b3cf7f0b59f6e005b7fb9d695ce3e8720e1be3cf77fe7e318a4017c880df108928e7179ec50447583d13bc781
2023-05-31Merge bitcoin/bitcoin#27780: fuzz: Avoid timeout in utxo_total_supplyfanquake
fafb4da121b19ba1b7bd173e25651c64d1982fb4 fuzz: Avoid timeout in utxo_total_supply (MarcoFalke) Pull request description: Looks like for high block counts it may be better to mock the chain, otherwise a high limit will lead to fuzz input bloat and timeouts, see https://github.com/bitcoin/bitcoin/pull/17860#issuecomment-1538252773. It can be checked that the fuzz target can still find the CVE, see https://github.com/bitcoin/bitcoin/pull/17860#pullrequestreview-1410594057 with a diff of: ```diff diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index f949655909..6f4cfb5f51 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -39,8 +39,6 @@ bool CheckTransaction(const CTransaction& tx, TxValidationState& state) // the underlying coins database. std::set<COutPoint> vInOutPoints; for (const auto& txin : tx.vin) { - if (!vInOutPoints.insert(txin.prevout).second) - return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate"); } if (tx.IsCoinBase()) ``` Also, fix a nit, see https://github.com/bitcoin/bitcoin/pull/17860#discussion_r1186451948 ACKs for top commit: dergoegge: ACK fafb4da121b19ba1b7bd173e25651c64d1982fb4 Tree-SHA512: a28fe9cd6ebb4c9bed5a5b35be76c1c436a87586c8fc3b3c4c8559a4a77ac08098324370da421d794c99579882c0872b6b29415de47ade6a05a08504a3d494c4