| Age | Commit message (Collapse) | Author |
|---|
|
b16f93caddcd3254eaf3dc43e09adf2142a9c40a script/sign: remove needless IsSolvable() utility (Antoine Poinsot)
c232ef20c0fd2e3b55355e52684091cad3af5247 outputtype: remove redundant check for uncompressed keys in AddAndGetDestinationForScript (Antoine Poinsot)
Pull request description:
Now that we have descriptors there is no need to try to sign for a scriptPubKey using dummy signatures, and using a mocked verification of this witness against the interpreter, just to make sure we know how to spend such a Script. Just try to infer a solvable descriptor: any scriptPubKey that we can sign for can be inferred as such.
This came up in #24149 but i think it's worth it on its own.
ACKs for top commit:
instagibbs:
ACK https://github.com/bitcoin/bitcoin/pull/25664/commits/b16f93caddcd3254eaf3dc43e09adf2142a9c40a
achow101:
re-ACK b16f93caddcd3254eaf3dc43e09adf2142a9c40a
furszy:
ACK b16f93ca, only change is the `IsSolvable` helper function removal.
Tree-SHA512: 137068157ce90210b710b1bf9ac3c400e2ff5af1112f892094b69875ea473d6a899f52adb51e5030cb907dee517602059cd1661107808558efa5de842ba12b41
|
|
It was used back when we didn't have a concept of descriptor. Now we
can check for solvability using descriptors.
|
|
at a too large depth
fb9faffae3a26b8aed8b671864ba679747163019 extended keys: fail to derive too large depth instead of wrapping around (Antoine Poinsot)
8dc6670ce159c2b080e9f735c6603a601d40b6ac descriptor: don't assert success of extended key derivation (Antoine Poinsot)
50cfc9e7613d6cf6b534df6e551238b80678c70d (pubk)key: mark Derive() as nodiscard (Antoine Poinsot)
0ca258a5ace798c4e54308aa8a09b1ab3302cd7e descriptor: never ignore the return value when deriving an extended key (Antoine Poinsot)
d3599c22bd4c6b3cfaaadd675e95ebe3b3cb1749 spkman: don't ignore the return value when deriving an extended key (Antoine Poinsot)
Pull request description:
We would previously silently wrap the derived child's depth back to `0`. Instead, explicitly fail when trying to derive an impossible depth, and handle the error in callers.
An extended fuzzing corpus of `descriptor_parse` triggered this behaviour, which was reported by MarcoFalke.
Fixes #25751.
ACKs for top commit:
achow101:
re-ACK fb9faffae3a26b8aed8b671864ba679747163019
instagibbs:
utACK https://github.com/bitcoin/bitcoin/pull/25642/commits/fb9faffae3a26b8aed8b671864ba679747163019
Tree-SHA512: 9f75c23572ce847239bd15e5497df2960b6bd63c61ea72347959d968b5c4c9a4bfeee284e76bdcd7bacbf9eeb70feee85ffd3e316f353ca6eca30e93aafad343
|
|
(tweaked) output key
544b4332f0e122167bdb94dc963405422faa30cb Add wallet tests for spending rawtr() (Pieter Wuille)
e1e3081200a71b6c9b0dcf236bc2a37ed1aa7552 If P2TR tweaked key is available, sign with it (Pieter Wuille)
8d9670ccb756592bddb2a269bf5078d62658537b Add rawtr() descriptor for P2TR with unknown tweak (Pieter Wuille)
Pull request description:
It may be useful to be able to represent P2TR outputs in descriptors whose script tree and/or internal key aren't known. This PR does that, by adding a `rawtr(KEY)` descriptor, where the KEY represents the output key directly. If the private key corresponding to that output key is known, it also permits signing with it.
I'm not convinced this is desirable, but presumably "tr(KEY)" sounds more intended for direct use than "rawtr(KEY)".
ACKs for top commit:
achow101:
ACK 544b4332f0e122167bdb94dc963405422faa30cb
sanket1729:
code review ACK 544b4332f0e122167bdb94dc963405422faa30cb
w0xlt:
reACK https://github.com/bitcoin/bitcoin/pull/23480/commits/544b4332f0e122167bdb94dc963405422faa30cb
Tree-SHA512: 0de08de517468bc22ab0c00db471ce33144f5dc211ebc2974c6ea95709f44e830532ec5cdb0128c572513d352120bd651c4559516d4500b5b0a3d257c4b45aca
|
|
It might already fail, and we'll add another failure case.
|
|
In some cases we asserted it succeeded, in others we were just ignoring it
|
|
...also move the 0-clamping logic to ApplyArgsManOptions, where it
belongs.
|
|
Also:
- Make DEFAULT_MAX_SIG_CACHE_SIZE into constexpr
DEFAULT_MAX_SIG_CACHE_BYTES to utilize the compile-time integer
arithmetic overflow checking available to constexpr.
- Fix comment (MiB instead of MB) for DEFAULT_MAX_SIG_CACHE_BYTES.
- Pass in max_size_bytes parameter to InitS*Cache(), modify log line to
no longer allude to maxsigcachesize being split evenly between the two
validation caches.
- Fix possible integer truncation and add a comment.
[META] I've kept the integer types as int64_t in order to not introduce
unintended behaviour changes, in the next commit we will make
them size_t.
|
|
This fixes an potential overflow which existed prior to this patchset.
If CuckooCache::cache<Element, Hash>::setup_bytes is called with a
`size_t bytes` which, when divided by sizeof(Element), does not fit into
an uint32_t, the implicit conversion to uint32_t in the call to setup
will result in an overflow.
At least on x86_64, this overflow is possible:
static_assert(std::numeric_limits<size_t>::max() / 32 <= std::numeric_limits<uint32_t>::max());
static_assert(std::numeric_limits<size_t>::max() / 4 <= std::numeric_limits<uint32_t>::max());
This commit detects such cases and signals to callers that the `size_t
bytes` input is too large.
|
|
1. -maxsigcachesize is a DEBUG_ONLY option
2. Almost 7 years has passed since its semantics change in
830e3f3d027ba5c8121eed0f6a9ce99961352572 from "number of entries" to
"number of mebibytes"
3. A std::new_handler was added to the codebase after the original PR
which introduced this limit, which will terminate immediately instead
of causing trouble by being caught somewhere unexpected.
|
|
Returning the approximate total size eliminates the need for
InitS*Cache() to do nElems*sizeof(uint256). The cuckoocache has a better
idea of this information.
|
|
|
|
|
|
The counter is an optimization over calling `ret.empty()`. It was
suggested that the compiler would realize `cnt` is only `0` on the first
iteration, and not actually emit the check and conditional.
This optimization was actually not triggered at all, since we
incremented `cnt` at the beginning of the first iteration. Fix it by
incrementing at the end instead.
This was reported by Github user "Janus".
|
|
|
|
|
|
|
|
Miniscript descriptors are defined under P2WSH context (either `wsh()`
or `sh(wsh())`).
Only sane Miniscripts are accepted, as insane ones (although valid by
type) can have surprising behaviour with regard to malleability
guarantees and resources limitations.
As Miniscript descriptors are longer and more complex than "legacy"
descriptors, care was taken in error reporting to help a user determine
for what reason a provided Miniscript is insane.
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
|
|
This is helpful for finer grained descriptor parsing error: when there
are multiple errors to report in a Miniscript descriptor start with the
"smallest" fragments: the ones closer to be a leaf.
Co-Authored-By: Pieter Wuille <pieter@wuille.net>
|
|
Letting the caller perform the checks allows for finer-grained error
reporting.
|
|
The taproot spenddata stored in a sigdata is the combination of data
existing previously (e.g. in a PSBT) and the data stored in a
SigningProvider. In order to use the external data when signing, we need
to be using the sigdata's spenddata.
|
|
Actually use pre-existing signatures in CreateTaprootScriptSig if a
signature is found for the given key and leaf hash.
|
|
|
|
GetSpendData needs to be finalized in order to be used. To avoid future
bugs, assert `!m_output_key.IsNull()` as m_output_key is only set during
Finalize.
|
|
TaprootSpendData can be gotten from TaprootBuilder, however for PSBT, we
also need TaprootBuilders directly (for the outputs). So we store the
TaprootBuilder in the FlatSigningProvider and when the TaprootSpendData
is needed, we generate it on the fly using the stored builder.
|
|
|
|
|
|
GetTreeTuples returns the leaves in DFS order as tuples of depth, leaf
version, and script. This is a representation of the tree that can be
serialized.
|
|
f3a50c9dfe645c548713e44e0eaf26ea9917a379 miniscript: rename IsSane and IsSaneSubexpression to prevent misuse (Antoine Poinsot)
c5fe5163dc31db939c44129f2ff8283b290a9330 miniscript: nit: don't return after assert(false) (Antoine Poinsot)
7bbaca9d8d355a17348a8d01e3e2521c5de466b0 miniscript: explicit the threshold size computation in multi() (Antoine Poinsot)
8323e4249db50d46ae4f43c1d8a50666549ae938 miniscript: add an OpCode typedef for readability (Antoine Poinsot)
7a549c6c59e6babbae76af008433426c6fa38fe2 miniscript: mark nodes with duplicate keys as insane (Antoine Poinsot)
8c0f8bf7bc3750fad648af1a548517a272114bca fuzz: add a Miniscript target for string representation roundtripping (Antoine Poinsot)
be34d5077b2fede7404de7706362f5858c443525 fuzz: rename and improve the Miniscript Script roundtrip target (Antoine Poinsot)
7eb70f0ac0a54adabc566e2b93bbf6b2beb54a79 miniscript: tiny doc fixups (Antoine Poinsot)
5cea85f12cba5dcfe3a298eddfa711f582adffac miniscript: split ValidSatisfactions from IsSane (Antoine Poinsot)
a0f064dc1474a048e236bfff12f4def3aa11daf3 miniscript: introduce a CheckTimeLocksMix helper (Antoine Poinsot)
ed45ee3882e69266d550b56ff69388e071f0ad1b miniscript: use optional instead of bool/outarg (Antoine Poinsot)
1ab8d89fd1bdb3c0f2a506b4a10df6c23ba21c48 miniscript: make equality operator non-recursive (Antoine Poinsot)
5922c662c08a061b3b3d5ac34a31f9f9d4640d47 scripted-diff: miniscript: rename 'nodetype' variables to 'fragment' (Antoine Poinsot)
c5f65db0f03b52bc4525acae944173829290ce6f 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 #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.)
ACKs for top commit:
sipa:
utACK f3a50c9dfe645c548713e44e0eaf26ea9917a379 (with the caveat that a lot of it is my own code)
sanket1729:
code review ACK f3a50c9dfe645c548713e44e0eaf26ea9917a379. Did not review the fuzz tests.
Tree-SHA512: c043325e4936fe25e8ece4266b46119e000c6745f88cea530fed1edf01c80f03ee6f9edc83b6e9d42ca01688d184bad16bfd967c5bb8037744e726993adf3deb
|
|
|
|
|
|
|
|
Suggested-by: Vincenzo Palazzo
|
|
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.
|
|
BIP 341 specifies constraints on the size of the control block _c_ used
to compute the taproot merkle root.
> The last stack element is called the control block _c_, and must have
> length _33 + 32m_, for a value of m that is an integer between 0 and
> 128, inclusive. Fail if it does not have such a length.
(See BIP-341 "Script Validation Rules" here: https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#script-validation-rules)
|
|
Since the removal of NODISCARD in 81d5af42f4dba5b68a597536cad7f61894dc22a3,
the only attributes def is LIFETIMEBOUND, and it's included in many more
places that it is used.
This removes all includes which do not have an associated use of LIFETIMEBOUND,
and adds it to the following files, due to their use of the same:
* src/validationinterface.h
* src/script/standard.h
|
|
|
|
|
|
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
|
|
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>
|
|
This helps to have finer-grained descriptor parsing errors.
|
|
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
|
|
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>
|
|
|
|
The value it leaves on the stack depends on the last element on the
stack. However, we can't make sure this element is OP_1 (which would
give us the 'u' property) without the MINIMALIF rule.
MINIMALIF is only policy for P2WSH, therefore giving 'd:' the 'u'
property breaks consensus soundness: it makes it possible (by consensus
but not policy) for instance to satisfy a thresh() without satisfying
at least k of its subs.
This bug was found and reported by Andrew Poelstra.
|
|
|
|
2da94a4c6f55f7a3621f4a6f70902c52f735c868 fuzz: add a fuzz target for Miniscript decoding from Script (Antoine Poinsot)
f8369996e76dbc41a12f7b7eea14a7e7990a81c1 Miniscript: ops limit and stack size computation (Pieter Wuille)
2e55e88f86d0dd49b35d04af3f57e863498aabae Miniscript: conversion from script (Pieter Wuille)
1ddaa66eae67b102f5e37d212d366a5dcad4aa26 Miniscript: type system, script creation, text notation, tests (Pieter Wuille)
4fe29368c0ded0e62f437cab3a7c904f7fd3ad67 script: expose getter for CScriptNum, add a BuildScript helper (Antoine Poinsot)
f4e289f384efdda6c3f56e1e1c30820a91ac2612 script: move CheckMinimalPush from interpreter to script.h (Antoine Poinsot)
31ec6ae92a5d9910a26d90a6ff20bab27dee5826 script: make IsPushdataOp non-static (Antoine Poinsot)
Pull request description:
Miniscript is a language for writing (a subset of) Bitcoin Scripts in a structured way.
Miniscript permits:
- To safely extend the Output Descriptor language to many more scripting features thanks to the typing system (composition).
- Statical analysis of spending conditions, maximum spending cost of each branch, security properties, third-party malleability.
- General satisfaction of any correctly typed ("valid" [0]) Miniscript. The satisfaction itself is also analyzable.
- To extend the possibilities of external signers, because of all of the above and since it carries enough metadata.
Miniscript guarantees:
- That for any statically-analyzed as "safe" [0] Script, a witness can be constructed in the bounds of the consensus and standardness rules (standardness complete).
- That unless the conditions of the Miniscript are met, no witness can be created for the Script (consensus sound).
- Third-party malleability protection for the satisfaction of a sane Miniscript, which is too complex to summarize here.
For more details around Miniscript (including the specifications), please refer to the [website](https://bitcoin.sipa.be/miniscript/).
Miniscript was designed by Pieter Wuille, Andrew Poelstra and Sanket Kanjalkar.
This PR is an updated and rebased version of #16800. See [the commit history of the Miniscript repository](https://github.com/sipa/miniscript/commits/master) for details about the changes made since September 2019 (TL;DR: bugfixes, introduction of timelock conflicts in the type system, `pk()` and `pkh()` aliases, `thresh_m` renamed to `multi`, all recursive algorithms were made non-recursive).
This PR is also the first in a series of 3:
- The first one (here) integrates the backbone of Miniscript.
- The second one (#24148) introduces support for Miniscript in Output Descriptors, allowing for watch-only support of Miniscript Descriptors in the wallet.
- The third one (#24149) implements signing for these Miniscript Descriptors, using Miniscript's satisfaction algorithm.
Note to reviewers:
- Miniscript is currently defined only for P2WSH. No Taproot yet.
- Miniscript is different from the policy language (a high-level logical representation of a spending policy). A policy->Miniscript compiler is not included here.
- The fuzz target included here is more interestingly extended in the 3rd PR to check a script's satisfaction against `VerifyScript`. I think it could be further improved by having custom mutators as we now have for multisig (see https://github.com/bitcoin/bitcoin/issues/23105). A minified corpus of Miniscript Scripts is available at https://github.com/bitcoin-core/qa-assets/pull/85.
[0] We call "valid" any correctly-typed Miniscript. And "safe" any sane Miniscript, ie one whose satisfaction isn't malleable, which requires a key for any spending path, etc..
ACKs for top commit:
jb55:
ACK 2da94a4c6f55f7a3621f4a6f70902c52f735c868
laanwj:
Light code review ACK 2da94a4c6f55f7a3621f4a6f70902c52f735c868 (mostly reviewed the changes to the existing code and build system)
Tree-SHA512: d3ef558436cfcc699a50ad13caf1e776f7d0addddb433ee28ef38f66ea5c3e581382d8c748ccac9b51768e4b95712ed7a6112b0e3281a6551e0f325331de9167
|
|
|
