Miniscript integration follow-ups #24860

darosior · 2022-04-15T08:57:30Z

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.)

fanquake · 2022-04-15T09:41:59Z

Fuzzer failure:

4.0K	/tmp/cirrus-ci-build/depends/sdk-sources/
4.0K	/tmp/cirrus-ci-build/releases/x86_64-pc-linux-gnu
184 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
184 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
Fuzzing harnesses lacking a corpus: coinselection miniscript_script miniscript_string wallet_notifications
Please consider adding a fuzz corpus at https://github.com/bitcoin-core/qa-assets
Run 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.

fuzz: key.cpp:392: void ECC_Start(): Assertion `secp256k1_context_sign == nullptr' failed.

Target "/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

Sjors · 2022-04-15T10:29:36Z

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 b9a36d3 a scripted diff?

darosior · 2022-04-15T10:46:32Z

Thanks for the notification about the fuzzer failure, i fixed it. See also discussion about it in sipa/miniscript#106.

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

src/script/miniscript.cpp

vincenzopalazzo

Concept ACK, with some opinionable comments!

DrahtBot · 2022-04-16T06:51:04Z

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.

darosior · 2022-04-19T13:42:58Z

cc @sanket1729

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

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>

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

This helps to have finer-grained descriptor parsing errors.

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>

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

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>

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

sanket1729

LGTM. Few questions.

src/script/miniscript.h

sanket1729 · 2022-04-29T18:10:57Z

src/script/miniscript.h

+    bool ValidSatisfactions() const { return IsValid() && CheckOpsLimit() && CheckStackSize(); }
+
+    //! Whether the apparent policy of this node matches its script semantics.
+    bool IsSane() const { return ValidSatisfactions() && IsNonMalleable() && CheckTimeLocksMix(); }


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?

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.

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()

src/script/miniscript.h

darosior · 2022-05-16T14:24:07Z

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 Miniscript support in Output Descriptors #24148
Addressed the nits from @sanket1729 review of Miniscript support in Output Descriptors #24148

src/script/miniscript.h

sipa · 2022-05-17T15:17:33Z

src/script/miniscript.h

+    bool ValidSatisfactions() const { return IsValid() && CheckOpsLimit() && CheckStackSize(); }
+
+    //! Whether the apparent policy of this node matches its script semantics.
+    bool IsSane() const { return ValidSatisfactions() && IsNonMalleable() && CheckTimeLocksMix(); }


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 · 2022-05-26T10:59:23Z

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).

src/script/miniscript.h

sipa · 2022-05-27T15:48:49Z

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

With or without addressing #24860 (comment)

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.

Suggested-by: Vincenzo Palazzo

sipa · 2022-05-31T15:21:08Z

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

sanket1729

code review ACK f3a50c9. Did not review the fuzz tests.

f3a50c9 miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot) c5fe516 miniscript: nit: don't return after assert(false) (Antoine Poinsot) 7bbaca9 miniscript: explicit the threshold size computation in multi() (Antoine Poinsot) 8323e42 miniscript: add an OpCode typedef for readability (Antoine Poinsot) 7a549c6 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot) 8c0f8bf fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot) be34d50 fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot) 7eb70f0 miniscript: tiny doc fixups (Antoine Poinsot) 5cea85f miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot) a0f064d miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot) ed45ee3 miniscript: use optional instead of bool/outarg (Antoine Poinsot) 1ab8d89 miniscript: make equality operator non-recursive (Antoine Poinsot) 5922c66 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot) c5f65db miniscript: remove a workaround for a GCC 4.8 bug (Antoine Poinsot) Pull request description: 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 bitcoin#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 bitcoin#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 bitcoin#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.) ACKs for top commit: sipa: utACK f3a50c9 (with the caveat that a lot of it is my own code) sanket1729: code review ACK f3a50c9. Did not review the fuzz tests. Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb

maflcko · 2022-06-08T18:08:54Z

Renamed the fuzz target input folder to preserve the inputs: bitcoin-core/qa-assets@3366f7b

darosior · 2022-06-08T19:42:48Z

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: ***@***.***(bitcoin-core/qa-assets@3366f7b) — Reply to this email directly, [view it on GitHub](#24860 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AFLK3F2HMLUCLFEVIE3XPQLVODOUHANCNFSM5TP67B6Q). You are receiving this because you authored the thread.Message ID: ***@***.***>

maflcko · 2022-06-09T06:17:39Z

src/test/fuzz/miniscript.cpp

+{
+    FuzzedDataProvider provider(buffer.data(), buffer.size());
+    auto str = provider.ConsumeRemainingBytesAsString();
+    auto parsed = miniscript::FromString(str, PARSER_CTX);


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

clusterfuzz-testcase-miniscript_string-5717444439703552.bin.txt

takes 10 seconds

With the flame graph:

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:

Cache the duplicate key result only once we call it https://github.com/darosior/bitcoin/tree/miniscript_cache_dup_key_check. Needs to drop the constness.

Conditionally skip the check in the constructor, in order to only do it for the top level node https://github.com/darosior/bitcoin/tree/miniscript_skip_dup_key_check. Doesn't drop the constness but not elegant..

Don't cache it at all https://github.com/darosior/bitcoin/tree/miniscript_no_dup_key_cache

@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).

Sounds much better than all my approaches. I'll implement and PR this.

See also https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=47994

I'm about to PR the fix, doing final testing at the moment.

…prevent memory blowup when fuzzing e8cc2e4 Make miniscript string parsing account for exact script size as bound (Pieter Wuille) 4cb8f9a Permit delaying duplicate key check in miniscript::Node construction (Pieter Wuille) Pull request description: As reported in bitcoin/bitcoin#24860 (comment), the current code to construct a `miniscript::Node` could cause a blowup on large fuzzer inputs. This is because: 1. The duplicate key check is redundantly done at parsing time, since we will recursively create miniscript nodes and the constructor will unconditionally look for duplicate across this node's keys and all its sub-nodes'. 2. We don't put an upper bound on the size of the inputs to consider for parsing. To avoid wasteful computation, and prevent the blowup on some fuzzer inputs, limit the size of reasonable inputs and only perform the check for duplicate keys once when parsing. Regarding the duplicate key check bypass in the constructor we iterated on different approaches, and eventually settled on passing a dummy argument. Albeit less elegant, all other approaches required getting rid of `std::make_shared` and adding an allocation *per node created*. This PR contains code from Pieter Wuille (see commits). Fixes bitcoin/bitcoin#25824. ACKs for top commit: darosior: ACK e8cc2e4 -- it's my own PR but most of the code here was written by sipa. I've reviewed and tested it. sipa: ACK e8cc2e4 (for the few parts of the code that aren't mine) Tree-SHA512: c21de39b3eeb484393758629882fcf8694a9bd1b8f15ae22efcec1582efc9c2309c5a0c2d90f361dd8e233d704a07dcd5fb982f4a48a002c4d8789e1d78bb526

…memory blowup when fuzzing e8cc2e4 Make miniscript string parsing account for exact script size as bound (Pieter Wuille) 4cb8f9a Permit delaying duplicate key check in miniscript::Node construction (Pieter Wuille) Pull request description: As reported in bitcoin#24860 (comment), the current code to construct a `miniscript::Node` could cause a blowup on large fuzzer inputs. This is because: 1. The duplicate key check is redundantly done at parsing time, since we will recursively create miniscript nodes and the constructor will unconditionally look for duplicate across this node's keys and all its sub-nodes'. 2. We don't put an upper bound on the size of the inputs to consider for parsing. To avoid wasteful computation, and prevent the blowup on some fuzzer inputs, limit the size of reasonable inputs and only perform the check for duplicate keys once when parsing. Regarding the duplicate key check bypass in the constructor we iterated on different approaches, and eventually settled on passing a dummy argument. Albeit less elegant, all other approaches required getting rid of `std::make_shared` and adding an allocation *per node created*. This PR contains code from Pieter Wuille (see commits). Fixes bitcoin#25824. ACKs for top commit: darosior: ACK e8cc2e4 -- it's my own PR but most of the code here was written by sipa. I've reviewed and tested it. sipa: ACK e8cc2e4 (for the few parts of the code that aren't mine) Tree-SHA512: c21de39b3eeb484393758629882fcf8694a9bd1b8f15ae22efcec1582efc9c2309c5a0c2d90f361dd8e233d704a07dcd5fb982f4a48a002c4d8789e1d78bb526

darosior mentioned this pull request Apr 15, 2022

Miniscript integration #24147

Merged

DrahtBot added the Build system label Apr 15, 2022

darosior force-pushed the miniscript_updates branch from 9071194 to ad634a5 Compare April 15, 2022 10:44

darosior force-pushed the miniscript_updates branch from ad634a5 to e9828f2 Compare April 15, 2022 11:41

vincenzopalazzo reviewed Apr 15, 2022

View reviewed changes

src/script/miniscript.cpp Outdated Show resolved Hide resolved

vincenzopalazzo reviewed Apr 15, 2022

View reviewed changes

This was referenced Apr 16, 2022

Signing support for Miniscript Descriptors #24149

Merged

Miniscript support in Output Descriptors #24148

Merged

Rspigler mentioned this pull request Apr 18, 2022

Awesome multisig PR labyrinth guide #24861

Open

16 tasks

DrahtBot mentioned this pull request Apr 19, 2022

miniscript: the 'd:' wrapper must not be 'u' #24906

Merged

DrahtBot added the Needs rebase label Apr 19, 2022

darosior force-pushed the miniscript_updates branch from e9828f2 to 426848e Compare April 19, 2022 18:59

DrahtBot removed the Needs rebase label Apr 19, 2022

darosior force-pushed the miniscript_updates branch from 426848e to 22b2171 Compare April 20, 2022 12:26

darosior and others added 9 commits April 28, 2022 16:44

miniscript: remove a workaround for a GCC 4.8 bug

c5f65db

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

miniscript: make equality operator non-recursive

1ab8d89

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

miniscript: use optional instead of bool/outarg

ed45ee3

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

miniscript: introduce a CheckTimeLocksMix helper

a0f064d

This helps to have finer-grained descriptor parsing errors.

miniscript: split ValidSatisfactions from IsSane

5cea85f

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>

miniscript: tiny doc fixups

7eb70f0

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

fuzz: rename and improve the Miniscript Script roundtrip target

be34d50

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>

fuzz: add a Miniscript target for string representation roundtripping

8c0f8bf

Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>

darosior force-pushed the miniscript_updates branch from 22b2171 to 038f367 Compare April 28, 2022 14:45

sanket1729 approved these changes Apr 29, 2022

View reviewed changes

darosior force-pushed the miniscript_updates branch from 038f367 to 225d73c Compare May 16, 2022 13:24

sipa reviewed May 17, 2022

View reviewed changes

darosior force-pushed the miniscript_updates branch from 225d73c to 6111d2c Compare May 17, 2022 15:35

notmandatory mentioned this pull request May 18, 2022

The rpc blockchain tests don't work with Bitcoin Core 23.0 bitcoindevkit/bdk#598

Closed

darosior force-pushed the miniscript_updates branch from 6111d2c to 59010da Compare May 26, 2022 10:58

darosior force-pushed the miniscript_updates branch from 59010da to f552056 Compare May 26, 2022 11:13

sipa reviewed May 27, 2022

View reviewed changes

src/script/miniscript.h Outdated Show resolved Hide resolved

darosior added 5 commits May 30, 2022 15:16

miniscript: mark nodes with duplicate keys as insane

7a549c6

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.

miniscript: add an OpCode typedef for readability

8323e42

Suggested-by: Vincenzo Palazzo

miniscript: explicit the threshold size computation in multi()

7bbaca9

miniscript: nit: don't return after assert(false)

c5fe516

miniscript: rename IsSane and IsSaneSubexpression to prevent misuse

f3a50c9

darosior force-pushed the miniscript_updates branch from f552056 to f3a50c9 Compare May 30, 2022 14:58

sanket1729 approved these changes Jun 2, 2022

View reviewed changes

fanquake merged commit 695ca64 into bitcoin:master Jun 4, 2022

darosior deleted the miniscript_updates branch June 6, 2022 13:23

maflcko reviewed Jun 9, 2022

View reviewed changes

darosior mentioned this pull request Jun 9, 2022

Miniscript updates bitcoin-core/qa-assets#89

Merged

maflcko mentioned this pull request Jun 9, 2022

miniscript fuzzer failing after qa-assets update #25323

Closed

darosior mentioned this pull request Jul 4, 2022

miniscript: avoid wasteful computation, prevent memory blowup when fuzzing #25540

Merged

bitcoin locked and limited conversation to collaborators Jul 4, 2023

Miniscript integration follow-ups #24860

Miniscript integration follow-ups #24860

Uh oh!

Conversation

darosior commented Apr 15, 2022

Uh oh!

fanquake commented Apr 15, 2022

Uh oh!

Sjors commented Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

darosior commented Apr 15, 2022

Uh oh!

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

darosior commented Apr 19, 2022

Uh oh!

sanket1729 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darosior commented May 16, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darosior commented May 26, 2022

Uh oh!

Uh oh!

sipa commented May 27, 2022

Uh oh!

sipa commented May 31, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sanket1729 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 8, 2022

Uh oh!

darosior commented Jun 8, 2022 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Apr 15, 2022 •

edited

Loading

DrahtBot commented Apr 16, 2022 •

edited

Loading

sanket1729 left a comment •

edited

Loading

darosior May 16, 2022 •

edited

Loading

sipa commented May 31, 2022 •

edited

Loading