Miniscript integration follow-ups #24860

pull darosior wants to merge 14 commits into bitcoin:master from darosior:miniscript_updates changing 6 files +481 −306
  1. darosior commented at 8:57 am on April 15, 2022: member

    The Miniscript repository and the Miniscript integration PR here have been a moving target for the past months, and some final cleanups were done there that were not included here. I initially intended to add some small followup commits to #24148 but i think there are enough of them to be worth a followup PR on its own.

    Some parts of the code did not change since it was initially written in 2019, and the code could use some modernization. (Use std::optional instead of out args, remove old compiler workarounds). We refactored the helpers to be more meaningful, and also did some renaming. A new fuzz target was also added and both were merged in a single file. 2 more will be added in #24149 that will be contained in this file too.

    The only behaviour change in this PR is to rule out Miniscript with duplicate keys from sane Miniscripts. In a P2WSH context, signatures can be rebounded (Miniscript does not use CODESEPARATOR) and it’s reasonable to assume that reusing keys across the Script drops the malleability guarantees. It was previously assumed such Miniscript would never exist in the first place since a compiler should never create them. We finally agreed that if one were to exist (say, written by hand or from a buggy compiler) it would be very confusing if an imported Miniscript descriptor (after #24148) with duplicate keys was deemed sane (ie, “safe to use”) by Bitcoin Core. We now check for duplicate keys in the constructor.

    This is (still) joint work with Pieter Wuille. (Actually he entirely authored the cleanups and code modernization.)

  2. DrahtBot added the label Build system on Apr 15, 2022
  3. fanquake commented at 9:41 am on April 15, 2022: member

    Fuzzer failure:

     04.0K	/tmp/cirrus-ci-build/depends/sdk-sources/
 14.0K	/tmp/cirrus-ci-build/releases/x86_64-pc-linux-gnu
 2184 fuzz target(s) found: addition_overflow addr_info_deserialize address_deserialize_v1_notime address_deserialize_v1_withtime address_deserialize_v2 addrman addrman_deserialize addrman_serdeser asmap asmap_direct autofile banman base_encode_decode bech32 block block_deserialize block_file_info_deserialize block_filter_deserialize block_header block_header_and_short_txids_deserialize blockfilter blockheader_deserialize blocklocator_deserialize blockmerkleroot blocktransactions_deserialize blocktransactionsrequest_deserialize blockundo_deserialize bloom_filter bloomfilter_deserialize buffered_file chain checkqueue coins_deserialize coins_view coinselection connman crypto crypto_aes256 crypto_aes256cbc crypto_chacha20 crypto_chacha20_poly1305_aead crypto_common crypto_diff_fuzz_chacha20 crypto_hkdf_hmac_sha256_l32 crypto_poly1305 cuckoocache data_stream_addr_man decode_tx descriptor_parse diskblockindex_deserialize eval_script fee_rate fee_rate_deserialize fees flat_file_pos_deserialize flatfile float golomb_rice hex http_request i2p integer inv_deserialize key key_io key_origin_info_deserialize kitchen_sink load_external_block_file locale merkle_block_deserialize merkleblock message messageheader_deserialize miniscript_script miniscript_string minisketch muhash multiplication_overflow net net_permissions netaddr_deserialize netaddress netbase_dns_lookup node_eviction out_point_deserialize p2p_transport_serialization parse_hd_keypath parse_iso8601 parse_numbers parse_script parse_univalue partial_merkle_tree_deserialize partially_signed_transaction_deserialize policy_estimator policy_estimator_io pow prefilled_transaction_deserialize prevector primitives_transaction process_message process_message_addr process_message_addrv2 process_message_block process_message_blocktxn process_message_cfcheckpt process_message_cfheaders process_message_cfilter process_message_cmpctblock process_message_feefilter process_message_filteradd process_message_filterclear process_message_filterload process_message_getaddr process_message_getblocks process_message_getblocktxn process_message_getcfcheckpt process_message_getcfheaders process_message_getcfilters process_message_getdata process_message_getheaders process_message_headers process_message_inv process_message_mempool process_message_merkleblock process_message_notfound process_message_ping process_message_pong process_message_sendaddrv2 process_message_sendcmpct process_message_sendheaders process_message_tx process_message_verack process_message_version process_message_wtxidrelay process_messages protocol psbt psbt_input_deserialize psbt_output_deserialize pub_key_deserialize random rbf rolling_bloom_filter rpc script script_bitcoin_consensus script_descriptor_cache script_deserialize script_flags script_format script_interpreter script_ops script_sigcache script_sign scriptnum_ops secp256k1_ec_seckey_import_export_der secp256k1_ecdsa_signature_parse_der_lax service_deserialize signature_checker signet snapshotmetadata_deserialize socks5 span spanparsing str_printf string system timedata torcontrol transaction tx_in tx_in_deserialize tx_out tx_pool tx_pool_standard txoutcompressor_deserialize txrequest txundo_deserialize uint160_deserialize uint256_deserialize utxo_snapshot validation_load_mempool versionbits wallet_notifications
 3184 of 184 detected fuzz target(s) selected: addition_overflow addr_info_deserialize address_deserialize_v1_notime address_deserialize_v1_withtime address_deserialize_v2 addrman addrman_deserialize addrman_serdeser asmap asmap_direct autofile banman base_encode_decode bech32 block block_deserialize block_file_info_deserialize block_filter_deserialize block_header block_header_and_short_txids_deserialize blockfilter blockheader_deserialize blocklocator_deserialize blockmerkleroot blocktransactions_deserialize blocktransactionsrequest_deserialize blockundo_deserialize bloom_filter bloomfilter_deserialize buffered_file chain checkqueue coins_deserialize coins_view coinselection connman crypto crypto_aes256 crypto_aes256cbc crypto_chacha20 crypto_chacha20_poly1305_aead crypto_common crypto_diff_fuzz_chacha20 crypto_hkdf_hmac_sha256_l32 crypto_poly1305 cuckoocache data_stream_addr_man decode_tx descriptor_parse diskblockindex_deserialize eval_script fee_rate fee_rate_deserialize fees flat_file_pos_deserialize flatfile float golomb_rice hex http_request i2p integer inv_deserialize key key_io key_origin_info_deserialize kitchen_sink load_external_block_file locale merkle_block_deserialize merkleblock message messageheader_deserialize miniscript_script miniscript_string minisketch muhash multiplication_overflow net net_permissions netaddr_deserialize netaddress netbase_dns_lookup node_eviction out_point_deserialize p2p_transport_serialization parse_hd_keypath parse_iso8601 parse_numbers parse_script parse_univalue partial_merkle_tree_deserialize partially_signed_transaction_deserialize policy_estimator policy_estimator_io pow prefilled_transaction_deserialize prevector primitives_transaction process_message process_message_addr process_message_addrv2 process_message_block process_message_blocktxn process_message_cfcheckpt process_message_cfheaders process_message_cfilter process_message_cmpctblock process_message_feefilter process_message_filteradd process_message_filterclear process_message_filterload process_message_getaddr process_message_getblocks process_message_getblocktxn process_message_getcfcheckpt process_message_getcfheaders process_message_getcfilters process_message_getdata process_message_getheaders process_message_headers process_message_inv process_message_mempool process_message_merkleblock process_message_notfound process_message_ping process_message_pong process_message_sendaddrv2 process_message_sendcmpct process_message_sendheaders process_message_tx process_message_verack process_message_version process_message_wtxidrelay process_messages protocol psbt psbt_input_deserialize psbt_output_deserialize pub_key_deserialize random rbf rolling_bloom_filter rpc script script_bitcoin_consensus script_descriptor_cache script_deserialize script_flags script_format script_interpreter script_ops script_sigcache script_sign scriptnum_ops secp256k1_ec_seckey_import_export_der secp256k1_ecdsa_signature_parse_der_lax service_deserialize signature_checker signet snapshotmetadata_deserialize socks5 span spanparsing str_printf string system timedata torcontrol transaction tx_in tx_in_deserialize tx_out tx_pool tx_pool_standard txoutcompressor_deserialize txrequest txundo_deserialize uint160_deserialize uint256_deserialize utxo_snapshot validation_load_mempool versionbits wallet_notifications
 4Fuzzing harnesses lacking a corpus: coinselection miniscript_script miniscript_string wallet_notifications
 5Please consider adding a fuzz corpus at https://github.com/bitcoin-core/qa-assets
 6Run address_deserialize_v2 with args ['/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-runs=1', '/tmp/cirrus-ci-build/ci/scratch/qa-assets/fuzz_seed_corpus/address_deserialize_v2']fuzz: key.cpp:392: void ECC_Start(): Assertion `secp256k1_context_sign == nullptr' failed.
 7
 8fuzz: key.cpp:392: void ECC_Start(): Assertion `secp256k1_context_sign == nullptr' failed.
 9
10Target "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/test/fuzz/fuzz -runs=1 /tmp/cirrus-ci-build/ci/scratch/qa-assets/fuzz_seed_corpus/address_deserialize_v2" failed with exit code -6
  4. Sjors commented at 10:29 am on April 15, 2022: member

    Did some very light code review, it looks sane. I didn’t look at the fuzz changes.

    Disallowing duplicate keys seems reasonable; I would not at all be surprised if people generate miniscript by hand or even from Sipa’s website, and then paste whatever keys they need into the result. E.g. if you just need a timelock fallback, which you can’t do with current descriptors, you probably won’t go through the trouble of finding and installing a compiler, figuring out how to put xpub’s into it, etc.

    Nit: can you make b9a36d3c6ce77c98e5a6e76d0ed223c410f323d2 a scripted diff?

  5. darosior force-pushed on Apr 15, 2022
  6. darosior commented at 10:46 am on April 15, 2022: member

    Thanks for the notification about the fuzzer failure, i fixed it. See also discussion about it in https://github.com/sipa/miniscript/pull/106.

    Regarding the scripted diff, sure. I’ll push it soon.

  7. darosior force-pushed on Apr 15, 2022
  8. in src/script/miniscript.cpp:284 in e9828f2635 outdated 
    279@@ -282,16 +280,15 @@ size_t ComputeScriptLen(Fragment nodetype, Type sub0typ, size_t subsize, uint32_
280     return 0;
281 }
282 
283-bool DecomposeScript(const CScript& script, std::vector<std::pair<opcodetype, std::vector<unsigned char>>>& out)
284+std::optional<std::vector<std::pair<opcodetype, std::vector<unsigned char>>>> DecomposeScript(const CScript& script)
    vincenzopalazzo commented at 9:22 pm on April 15, 2022:

    I’m not a fan of typedef, but I think that one typedef, in this case, can make the return type cleaner from a reader’s viewpoint?

    0// FIXME: move this declaration in some better place!
1typedef std::pair<opcodetype, std::vector<unsigned char>> OPCode
2
3std::optional<std::vector<OPCode>> DecomposeScript(const CScript& script)
    darosior commented at 1:25 pm on May 16, 2022:
    Done
  9. vincenzopalazzo commented at 9:24 pm on April 15, 2022: none
    Concept ACK, with some opinionable comments!
  10. DrahtBot commented at 6:51 am on April 16, 2022: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24149 (Signing support for Miniscript Descriptors by darosior)
    • #24148 (Miniscript support in Output Descriptors by darosior)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  11. darosior commented at 1:42 pm on April 19, 2022: member
    cc @sanket1729
  12. DrahtBot added the label Needs rebase on Apr 19, 2022
  13. darosior force-pushed on Apr 19, 2022
  14. DrahtBot removed the label Needs rebase on Apr 19, 2022
  15. darosior force-pushed on Apr 20, 2022
  16. miniscript: remove a workaround for a GCC 4.8 bug 
    Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    c5f65db0f0
  17. scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' 
    The 'Fragment' type was previously named 'Nodetype'. For clarity, name
the variables the same.

-BEGIN VERIFY SCRIPT-
sed -i 's/nodetype/fragment/g' src/script/miniscript.*
-END VERIFY SCRIPT-

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    5922c662c0
  18. miniscript: make equality operator non-recursive 
    Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    1ab8d89fd1
  19. miniscript: use optional instead of bool/outarg 
    Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    ed45ee3882
  20. miniscript: introduce a CheckTimeLocksMix helper 
    This helps to have finer-grained descriptor parsing errors.
    a0f064dc14
  21. miniscript: split ValidSatisfactions from IsSane 
    This makes IsSane clearer. It is useful to differentiate between 'potential non-malleable satisfactions are valid' and 'such satisfactions exist' for testing.

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    5cea85f12c
  22. miniscript: tiny doc fixups 
    Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    7eb70f0ac0
  23. fuzz: rename and improve the Miniscript Script roundtrip target 
    Parse also key hashes using the Key type. Make this target the first of
the 4 Miniscript fuzz targets in a single `miniscript` file.

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    be34d5077b
  24. fuzz: add a Miniscript target for string representation roundtripping 
    Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
    8c0f8bf7bc
  25. darosior force-pushed on Apr 28, 2022
  26. in src/script/miniscript.h:782 in 5cea85f12c outdated 
    778@@ -779,8 +779,11 @@ struct Node {
779     //! Check whether there is no satisfaction path that contains both timelocks and heightlocks
780     bool CheckTimeLocksMix() const { return GetType() << "k"_mst; }
781 
782-    //! Do all sanity checks.
783-    bool IsSane() const { return IsValid() && IsNonMalleable() && CheckTimeLocksMix() && CheckOpsLimit() && CheckStackSize(); }
784+    //! Whether successful non-malleable satisfactions are guaranteed to be valid.
    sanket1729 commented at 6:08 pm on April 29, 2022:
    I think you mean all satisfactions here? Not only the non-malleable ones?
    sipa commented at 3:02 pm on May 1, 2022:
    Only the non-malleable ones, because CheckOpsLimit and CheckStackSize only take the logic of the non-malleable satisfaction algorithm into account.
  27. in src/script/miniscript.h:786 in 5cea85f12c outdated 
    783-    bool IsSane() const { return IsValid() && IsNonMalleable() && CheckTimeLocksMix() && CheckOpsLimit() && CheckStackSize(); }
784+    //! Whether successful non-malleable satisfactions are guaranteed to be valid.
785+    bool ValidSatisfactions() const { return IsValid() && CheckOpsLimit() && CheckStackSize(); }
786+
787+    //! Whether the apparent policy of this node matches its script semantics.
788+    bool IsSane() const { return ValidSatisfactions() && IsNonMalleable() && CheckTimeLocksMix(); }
    sanket1729 commented at 6:10 pm on April 29, 2022:

    Technically, all timelocks are malleable if there is no signature. If there is no signature, anyone can malleate nLockTime/nSequence to any value perhaps not matching the script semantics?

    Why is the NeedsSignature check only on top-level? and not a general condition for sanity?

    darosior commented at 11:58 am on May 16, 2022:

    You are right, n:older(12) shouldn’t be called “sane”. But to call it “sane” one should use IsSaneTopLevel().

    I think that the check for a fragment’s sanity should be context-less, and the requirement for signature depends on the context. To take back the example from above n:older(12) shouldn’t be called sane on its own, but it should in the context of and_b(n:older(12),s:pk(A)). So it should only be called insane if not ’s’ and is the top level fragment. That’s what IsSaneTopLevel() is. It’s not purely theoretical, for instance we use this in #24148 to find the deepest insane sub of an insane Miniscript: https://github.com/bitcoin/bitcoin/pull/24148/files#diff-a55760aaec4bce216663f5ebf65823516347356a8320d30459427149f7bbc2c5R824-R831. If the IsSane() check is context-dependent it gets messy.

    Now, maybe there is a point for the need for a better naming of these methods.

    sipa commented at 3:17 pm on May 17, 2022:

    Yeah, I think this is right.

    IsSane() being false for n:older(12) would be strange, because while indeed it would be bad to use that as a standalone script, it is perfectly reasonable (and even expected) to occur as a subexpression inside other scripts.

    What about swapping the naming around?

    • IsSane() -> IsSaneSubexpression()
    • IsSaneTopLevel() -> IsSane()
    darosior commented at 3:35 pm on May 17, 2022:
    Done.
  28. in src/script/miniscript.h:780 in 038f367494 outdated 
    761@@ -746,6 +762,30 @@ struct Node {
762         return {{}, {}};
763     }
764 
765+    //! Returns true if the node contains at least one duplicate key.
766+    bool ContainsDuplicateKey() const {
767+        auto upfn = [this](const Node& node, Span<std::set<Key>> subs) -> std::optional<std::set<Key>> {
768+            if (&node != this && node.duplicate_key) return {};
    sanket1729 commented at 6:42 pm on April 29, 2022:
    I am confused by why we need the node = this here. Should upfn always not operate on this node?
    sipa commented at 7:36 pm on April 29, 2022:
    this->duplicate_key isn’t yet initialized when ContainsDuplicateKey is being called, so we need to skip inspecting it.
  29. sanket1729 approved
  30. sanket1729 commented at 6:44 pm on April 29, 2022: contributor
    LGTM. Few questions.
  31. darosior force-pushed on May 16, 2022
  32. darosior commented at 2:24 pm on May 16, 2022: member

    I think i addressed all comments here. It’s ready for another round of review. :)

    I had previously made the rename a scripted diff as asked by @Sjors. In the last push i:

    • Added a typedef for the (operand, data pushed) pair as suggested by @vincenzopalazzo
    • Added a CheckDuplicateKey() which is useful for unit tests and for granular error detection in #24148
    • Addressed the nits from @sanket1729 review of #24148
  33. in src/script/miniscript.h:182 in a58f35f1ea outdated 
    178@@ -179,6 +179,8 @@ inline constexpr Type operator"" _mst(const char* c, size_t l) {
179     return typ;
180 }
181 
182+typedef std::pair<opcodetype, std::vector<unsigned char>> OpCode;
    sipa commented at 3:13 pm on May 17, 2022:

    Nit: the “using” notation is considered more modern than typedef (though for non-templated types there is no functional difference):

    0using Opcode = std::pair<opcodetype, std::vector<unsigned char>>;
    darosior commented at 3:35 pm on May 17, 2022:
    Done, thanks
  34. darosior force-pushed on May 17, 2022
  35. darosior force-pushed on May 26, 2022
  36. darosior commented at 10:59 am on May 26, 2022: member
    Added an explicit requirement on the context to provide a KeyCompare method instead of implicitly relying on operator< for checking key duplicates. Suggested by @sipa in #24148 (comment).
  37. darosior force-pushed on May 26, 2022
  38. in src/script/miniscript.h:771 in 478c46cad9 outdated 
    762@@ -746,6 +763,36 @@ struct Node {
763         return {{}, {}};
764     }
765 
766+    /** Check whether any key is repeated.
767+     * This uses a custom key comparator provided by the context in order to still detect duplicates
768+     * for more complicated types.
769+     */
770+    template<typename Ctx> bool ContainsDuplicateKey(const Ctx& ctx) const {
771+        using set = std::set<Key, std::function<bool(const Key& a, const Key& b)>>;
    sipa commented at 2:46 pm on May 27, 2022:

    std::function may have allocation overhead, and results in dynamic binding (functions pointers rather than compile-time resolving). Here is an alternative, though it’s not as elegant as I’d like:

     0--- a/src/script/miniscript.h
 1+++ b/src/script/miniscript.h
 2@@ -767,14 +767,20 @@ public:
 3      * for more complicated types.
 4      */
 5     template<typename Ctx> bool ContainsDuplicateKey(const Ctx& ctx) const {
 6-        using set = std::set<Key, std::function<bool(const Key& a, const Key& b)>>;
 7+        // We cannot use a lambda here, as lambdas are non assignable, and the set operations
 8+        // below require moving the comparators around.
 9+        struct Comp {
10+            const Ctx* ctx_ptr;
11+            Comp(const Ctx& ctx) : ctx_ptr(&ctx) {}
12+            bool operator()(const Key& a, const Key& b) const { return ctx_ptr->KeyCompare(a, b); }
13+        };
14+        using set = std::set<Key, Comp>;
15 
16         auto upfn = [this, &ctx](const Node& node, Span<set> subs) -> std::optional<set> {
17             if (&node != this && node.duplicate_key) return {};
18 
19             size_t keys_count = node.keys.size();
20-            auto comp = [&ctx](const Key& a, const Key& b) -> bool { return ctx.KeyCompare(a, b); };
21-            set key_set{node.keys.begin(), node.keys.end(), comp};
22+            set key_set{node.keys.begin(), node.keys.end(), Comp(ctx)};
23             if (key_set.size() != keys_count) return {};
24 
25             for (auto& sub: subs) {
    darosior commented at 2:59 pm on May 30, 2022:
    TIL, thanks. Done.
  39. sipa commented at 3:48 pm on May 27, 2022: member

    utACK f552056225f2205bcb4af875e42730a357ceb916 (with the caveat that a lot of it is my own code).

    With or without addressing #24860 (review)

  40. miniscript: mark nodes with duplicate keys as insane 
    As stated on the website, duplicate keys make it hard to reason about
malleability as a single signature may unlock multiple paths.

We use a custom KeyCompare function instead of operator< to be explicit
about the requirement.
    7a549c6c59
  41. miniscript: add an OpCode typedef for readability 
    Suggested-by: Vincenzo Palazzo
    8323e4249d
  42. miniscript: explicit the threshold size computation in multi() 7bbaca9d8d
  43. miniscript: nit: don't return after assert(false) c5fe5163dc
  44. miniscript: rename IsSane and IsSaneSubexpression to prevent misuse f3a50c9dfe
  45. darosior force-pushed on May 30, 2022
  46. sipa commented at 3:21 pm on May 31, 2022: member
    utACK f3a50c9dfe645c548713e44e0eaf26ea9917a379 (with the caveat that a lot of it is my own code)
  47. sanket1729 approved
  48. sanket1729 commented at 8:04 am on June 2, 2022: contributor
    code review ACK f3a50c9dfe645c548713e44e0eaf26ea9917a379. Did not review the fuzz tests.
  49. fanquake merged this on Jun 4, 2022
  50. fanquake closed this on Jun 4, 2022
  51. sidhujag referenced this in commit da822e0083 on Jun 5, 2022
  52. darosior deleted the branch on Jun 6, 2022
  53. MarcoFalke commented at 6:08 pm on June 8, 2022: member
    Renamed the fuzz target input folder to preserve the inputs: https://github.com/bitcoin-core/qa-assets/commit/3366f7bad1144804e372afd88ee3eae6ca140dfa
  54. darosior commented at 7:42 pm on June 8, 2022: member

    I’ve been populating corpora for the two targets here. I’ll PR them to qa-assets soon. ——- Original Message ——- Le mercredi 8 juin 2022 à 8:09 PM, MacroFake @.***> a écrit :

    Renamed the fuzz target input folder to preserve the inputs: @.***(https://github.com/bitcoin-core/qa-assets/commit/3366f7bad1144804e372afd88ee3eae6ca140dfa)

    — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

  55. in src/test/fuzz/miniscript.cpp:146 in f3a50c9dfe 
    141+/* Fuzz tests that test parsing from a string, and roundtripping via string. */
142+FUZZ_TARGET_INIT(miniscript_string, FuzzInit)
143+{
144+    FuzzedDataProvider provider(buffer.data(), buffer.size());
145+    auto str = provider.ConsumeRemainingBytesAsString();
146+    auto parsed = miniscript::FromString(str, PARSER_CTX);
    MarcoFalke commented at 6:17 am on June 9, 2022:

    Looks like this may take quite a while. For example:

    clusterfuzz-testcase-miniscript_string-5717444439703552.bin.txt

    takes 10 seconds

    With the flame graph:

    Screenshot from 2022-06-09 08-14-22

    darosior commented at 11:14 am on June 9, 2022:

    Yeah calling it in the constructor was not exactly bright since we’ll re-do the same computation as we pile more nodes in the parsers. On the other hand it’s hard not to do so and to keep the constness of the NodeRef. Here are different approaches i tried:

    sipa commented at 2:31 pm on June 9, 2022:

    @darosior What about this idea:

    • Make the current miniscript::Node constructors private, and make them not compute the duplicate_key value.
    • Add a forwarding public constructor which invokes the private ones, and also computes the duplicate_key value (so now any publicly constructed Node object will have duplicate_key set, but it’s possible to bypass this computation by using the private ones, if you have access to them).
    • FromScript and FromString are made friend, and thus can use the private constructors.
    • Make the duplicate_key variable mutable, so ContainsDuplicateKey can modify it (and is changed so it sets the value for all children of the called node too).
    darosior commented at 8:54 am on June 10, 2022:
    Sounds much better than all my approaches. I’ll implement and PR this.
    MarcoFalke commented at 12:38 pm on July 4, 2022:
    See also https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47994
    darosior commented at 12:39 pm on July 4, 2022:
    I’m about to PR the fix, doing final testing at the moment.
  56. glozow referenced this in commit 55e1deb745 on Sep 19, 2022
  57. sidhujag referenced this in commit a06877b1b4 on Sep 20, 2022
  58. DrahtBot locked this on Jul 4, 2023


darosior fanquake Sjors vincenzopalazzo DrahtBot sanket1729 sipa MarcoFalke

Labels
Build system

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me