| Age | Commit message (Collapse) | Author |
|---|
|
Co-authored-by: stickies-v <stickies-v@protonmail.com>
Co-authored-by: Gloria Zhao <gloriajzhao@gmail.com>
|
|
|
|
|
|
|
|
InferPubkey can return a nullptr, so check it's result before continuing
with creating the inferred descriptor.
|
|
|
|
64-hex-characters public keys are valid in Miniscript key expressions
within a Tapscript context.
Keys under a Tapscript context always serialize as 32-bytes x-only
public keys (and that's what get hashed by OP_HASH160 on the stack too).
|
|
We'll need to get a compressed key out of an x-only one in other places.
Avoid duplicating the code.
|
|
We are going to introduce Tapscript support in Miniscript, for which
some of Miniscript rules and properties change (new or modified
fragments, different typing rules, different resources consumption, ..).
|
|
The descriptor documentation (doc/descriptors.md) and BIP380 explicitly
require that hex-encoded public keys start with 02 or 03 (compressed) or
04 (uncompressed). However, the current parsing/inference code permit 06
and 07 (hybrid) encoding as well. Fix this.
|
|
When estimating the maximum size of an input, we were assuming the
number of elements on the witness stack could be encode in a single
byte. This is a valid approximation for all the descriptors we support
(including P2WSH Miniscript ones), but may not hold anymore once we
support Miniscript within Taproot descriptors (since the max standard
witness stack size of 100 gets lifted).
It's a low-hanging fruit to account for it correctly, so just do it now.
|
|
In the wallet code, we are currently estimating the size of a signed
input by doing a dry run of the signing logic. This is unnecessary as
all outputs we are able to sign for can be represented by a descriptor,
and we can derive the size of a satisfaction ("signature") from the
descriptor itself directly.
In addition, this approach does not scale: getting the size of a
satisfaction through a dry run of the signing logic is only possible for
the most basic scripts.
This commit introduces the computation of the size of satisfaction per
descriptor. It's a bit intricate for 2 main reasons:
- We want to conserve the behaviour of the current dry-run logic used by
the wallet that sometimes assumes ECDSA signatures will be low-r,
sometimes not (when we don't create them).
- We need to account for the witness discount. A single descriptor may
sometimes benefit of it, sometimes not (for instance `pk()` if used as
top-level versus if used inside `wsh()`).
|
|
Since script/standard only contains things that are used by the Solver
and its callers, rename the files to script/solver.
|
|
TaprootSpendData and TaprootBuilder are used in signing in
SigningProvider contexts, so they should live near that.
|
|
as sub descriptors
dd9633b516d6936ac4e23a40f9b0bea120117d35 test: wallet, add coverage for watch-only raw sh script migration (furszy)
cc781a21800a6ce13875feefd0cb14ab0a84524c descriptor: InferScript, do not return top-level only func as sub descriptor (furszy)
286e0c7d5e9538198b28b792c5168b8fafa1534f wallet: loading, log descriptor parsing error details (furszy)
Pull request description:
Linked to #28057.
Currently, the `InferScript` function returns an invalid descriptor when it tries to infer a p2sh-p2pkh script whose pubkey is not known by the wallet.
This behavior occurs because the inference process bypasses the `pkh` subscript when the pubkey is not contained by the wallet (no pubkey provider), interpreting it as a `sh(addr(ADDR))` descriptor. Then, the failure arises because the `addr()` function is restricted to being used only at the top level.
For reviewers, would recommend to start by examining the functional test to understand the context and the circumstances on which this can result in a fatal error (e.g. during the migration process).
ACKs for top commit:
achow101:
ACK dd9633b516d6936ac4e23a40f9b0bea120117d35
darosior:
utACK dd9633b516d6936ac4e23a40f9b0bea120117d35
Tree-SHA512: 61e763206c604c372019d2c36e31684f3dddf81f8b154eb9aba5cd66d8d61bda457ed4e591613eb6ce6c76cf7c3f11764abc6cd727a7c2b6414f1065783be032
|
|
e.g. sh(addr(ADDR)) or sh(raw(HEX)) are invalid descriptors.
Making sh and wsh top level functions to return addr/raw descriptors when
the subscript inference fails.
|
|
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
|
|
|
|
It's possible for some unsatisfiable miniscripts to be considered sane.
Make sure we refuse to import those, as they would be unspendable.
|
|
As we update the descriptor's db record every time that
the wallet is loaded (at `TopUp` time), if the spkm ID differs
from the one in db, the wallet will enter in an unrecoverable
corruption state, and no soft version will be able to open
it anymore.
Because we cannot change the past, to stay compatible between
releases, we need to always use the apostrophe version for the
spkm IDs.
|
|
This allows us to verify the descriptor ID on the descriptors
unit tests in different software versions without requiring to
use the entire DescriptorScriptPubKeyMan machinery.
Note:
The unit test changes are introduced after the bugfix commit
but this commit + the unit test commit can be cherry-picked
on top of the v25 branch to verify IDs correctness. IDs must
be the same for v25 and after the bugfix commit.
|
|
fe49f06c0e91b96feb8d8f1bd478c3173f14782c doc: clarify PR 26076 release note (Sjors Provoost)
bd13dc2f46ea10302a928fcf0f53b7aed77ad260 Switch hardened derivation marker to h in descriptors (Sjors Provoost)
Pull request description:
This makes it easier to handle descriptor strings manually, especially when importing from another Bitcoin Core wallet.
For example the `importdescriptors` RPC call is easiest to use `h` as the marker: `'["desc": ".../0h/..."]'`, avoiding the need for escape characters. With this change `listdescriptors` will use `h`, so you can copy-paste the result, without having to add escape characters or switch `'` to 'h' manually.
Both markers can still be parsed.
The `hdkeypath` field in `getaddressinfo` is also impacted by this change, except for legacy wallets. The latter is to prevent accidentally breaking ancient software that uses our legacy wallet.
See discussion in #15740
ACKs for top commit:
achow101:
ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
darosior:
re-ACK fe49f06c0e91b96feb8d8f1bd478c3173f14782c
Tree-SHA512: f78bc873b24a6f7a2bf38f5dd58f2b723e35e6b10e4d65c36ec300e2d362d475eeca6e5afa04b3037ab4bee0bf8ebc93ea5fc18102a2111d3d88fc873c08dc89
|
|
This is an extraction of ArgsManager related functions from util/system
into their own common file.
Config file related functions are moved to common/config.cpp.
The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
|
|
This makes it easier to handle descriptor strings manually. E.g. an RPC call that takes an array of descriptors can now use '["desc": ".../0h/..."]'.
Both markers can still be parsed. The default for new descriptors is changed to h. In normalized form h is also used. For private keys the chosen marker is preserved in a round trip.
The hdkeypath field in getaddressinfo is also impacted by this change.
|
|
https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html
|
|
6c7a17a8e0eec377f83ed1399f003ae70b898270 psbt: support externally provided preimages for Miniscript satisfaction (Antoine Poinsot)
840a396029316896beda46600aec3c1af09a899c qa: add a "smart" Miniscript fuzz target (Antoine Poinsot)
17e3547241d593bc92c5c6b36c54284d9d9f3feb qa: add a fuzz target generating random nodes from a binary encoding (Antoine Poinsot)
611e12502a5887ffb751bb92fadaa334d484824b qa: functional test Miniscript signing with key and timelocks (Antoine Poinsot)
d57b7f2021d2369f6e88cdf0f562aab27c51beaf refactor: make descriptors in Miniscript functional test more readable (Antoine Poinsot)
0a8fc9e200b5018c1efd6f9126eb405ca0beeea3 wallet: check solvability using descriptor in AvailableCoins (Antoine Poinsot)
560e62b1e221832ae99ff8684559a7b8f9df84a7 script/sign: signing support for Miniscripts with hash preimage challenges (Antoine Poinsot)
a2f81b6a8f1ff3b0750711409c7538812a52ef40 script/sign: signing support for Miniscript with timelocks (Antoine Poinsot)
61c6d1a8440db09c44d7fd367a6f2c641ea93d40 script/sign: basic signing support for Miniscript descriptors (Antoine Poinsot)
4242c1c52127df3a24be0c15b88d4fc463af04fc Align 'e' property of or_d and andor with website spec (Pieter Wuille)
f5deb417804b9f267830bd40177677987df4526d Various additional explanations of the satisfaction logic from Pieter (Pieter Wuille)
22c5b00345063bdeb8b6d3da8b5692d18f92bfb7 miniscript: satisfaction support (Antoine Poinsot)
Pull request description:
This makes the Miniscript descriptors solvable.
Note this introduces signing support for much more complex scripts than the wallet was previously able to solve, and the whole tooling isn't provided for a complete Miniscript integration in the wallet. Particularly, the PSBT<->Miniscript integration isn't entirely covered in this PR.
ACKs for top commit:
achow101:
ACK 6c7a17a8e0eec377f83ed1399f003ae70b898270
sipa:
utACK 6c7a17a8e0eec377f83ed1399f003ae70b898270 (to the extent that it's not my own code).
Tree-SHA512: a71ec002aaf66bd429012caa338fc58384067bcd2f453a46e21d381ed1bacc8e57afb9db57c0fb4bf40de43b30808815e9ebc0ae1fbd9e61df0e7b91a17771cc
|
|
Try to solve a script using the Miniscript satisfier if the legacy
solver fails under P2WSH context. Only solve public key and public key
hash challenges for now.
We don't entirely replace the raw solver and especially rule out trying to
solve CHECKMULTISIG-based multisigs with the Miniscript satisfier since
some features, such as the transaction input combiner, rely on the
specific behaviour of the former.
|
|
|
|
6879be691bf636a53208ef058f2ebe18bfa8017c refactor: Extract RIPEMD160 (Ben Woosley)
Pull request description:
To directly return a CRIPEMD160 hash from data.
Simplifies the call sites.
ACKs for top commit:
achow101:
ACK 6879be691bf636a53208ef058f2ebe18bfa8017c
theStack:
re-ACK 6879be691bf636a53208ef058f2ebe18bfa8017c
MarcoFalke:
review ACK 6879be691bf636a53208ef058f2ebe18bfa8017c 🏔
Tree-SHA512: 6ead85d8060c2ac6afd43ec716ff5a82d6754c4132fe7df3b898541fa19f1dfd8b301b2b66ae7cb7594b1b1a8c7f68bce3790a8c610d4a1164e995d89bc5ae34
|
|
To directly return a CRIPEMD160 hash from data.
Incidentally, decoding this acronym:
* RIPEMD -> RIPE Message Digest
* RIPE -> RACE Integrity Primitives Evaluation
* RACE -> Research and Development in Advanced Communications Technologies in Europe
|
|
Prevents use of CScript methods until the tapleaf is known to be a tapscript.
|
|
wallets
53e7ed075c49f853cc845afc7b2f058cabad0cb0 doc: Release notes and other docs for migration (Andrew Chow)
9c44bfe244f35f08ba576d8b979a90dcd68d2c77 Test migratewallet (Andrew Chow)
0b26e7cdf2659fd8b54d21fd2bd749f9f3e87af8 descriptors: addr() and raw() should return false for ToPrivateString (Andrew Chow)
31764c3f872f4f01b48d50585f86e97c41554954 Add migratewallet RPC (Andrew Chow)
0bf7b38bff422e7413bcd3dc0abe2568dd918ddc Implement MigrateLegacyToDescriptor (Andrew Chow)
e7b16f925ae5b117e8b74ce814b63e19b19b50f4 Implement MigrateToSQLite (Andrew Chow)
5b62f095e790a0d4e2a70ece89465b64fc68358a wallet: Refactor SetupDescSPKMs to take CExtKey (Andrew Chow)
22401f17e026ead4bc3fe96967eec56a719a4f75 Implement LegacyScriptPubKeyMan::DeleteRecords (Andrew Chow)
35f428fae68ad974abdce0fa905148f620a9443c Implement LegacyScriptPubKeyMan::MigrateToDescriptor (Andrew Chow)
ea1ab390e4dac128e3a37d4884528c3f4128ed83 scriptpubkeyman: Implement GetScriptPubKeys in Legacy (Andrew Chow)
e664af29760527e75cd7e290be5f102b6d29ebee Apply label to all scriptPubKeys of imported combo() (Andrew Chow)
Pull request description:
This PR adds a new `migratewallet` RPC which migrates a legacy wallet to a descriptor wallet. Migrated wallets will need a new backup. If a wallet has watchonly stuff in it, a new watchonly descriptor wallet will be created containing those watchonly things. The related transactions, labels, and descriptors for those watchonly things will be removed from the original wallet. Migrated wallets will not have any of the legacy things be available for fetching from `getnewaddress` or `getrawchangeaddress`. Wallets that have private keys enabled will have newly generated descriptors. Wallets with private keys disabled will not have any active `ScriptPubKeyMan`s.
For the basic HD wallet case of just generated keys, in addition to the standard descriptor wallet descriptors using the master key derived from the pre-existing hd seed, the migration will also create 3 descriptors for each HD chain in: a ranged combo external, a ranged combo internal, and a single key combo for the seed (the seed is a valid key that we can receive coins at!). The migrated wallet will then have newly generated descriptors as the active `ScriptPubKeyMan`s. This is equivalent to creating a new descriptor wallet and importing the 3 descriptors for each HD chain. For wallets containing non-HD keys, each key will have its own combo descriptor.
There are also tests.
ACKs for top commit:
Sjors:
tACK 53e7ed075c49f853cc845afc7b2f058cabad0cb0
w0xlt:
reACK https://github.com/bitcoin/bitcoin/commit/53e7ed075c49f853cc845afc7b2f058cabad0cb0
Tree-SHA512: c0c003694ca2e17064922d08e8464278d314e970efb7df874b4fe04ec5d124c7206409ca701c65c099d17779ab2136ae63f1da2a9dba39b45f6d62cf93b5c60a
|
|
They don't have any private data and they can't be nested so they
should return false for ToPrivateString.
|
|
|
|
416ceb8661117235c76f2985512473ebbc64956b descriptor: check if `rawtr` has only one key. (w0xlt)
Pull request description:
If I understand `rawtr` descriptor correctly, it should only allow `rawtr(KEY)`, not `rawtr(KEY1, KEY2, ...)` or other concatenations.
On master branch, `rawtr(KEY1, KEY2, ...)` will produce the `rawtr(KEY1)` descriptor ignoring the `KEY2, ...` with no error messages or warnings.
For example, the code below will print `rawtr(tprv8ZgxMBicQKsPefef2Doobbq3xTCaVTHcDn6me82KSXY1vY9AJAWD5u7SDM4XGLfc4EoXRMFrJKpp6HNmQWA3FTMRQeEmMJYJ9RPqe9ne2hU/*)#lx9qryfh`
for the supposedly invalid descriptor
`rawtr(tprv8ZgxMBicQKsPefef2Doobbq3xTCaVTHcDn6me82KSXY1vY9AJAWD5u7SDM4XGLfc4EoXRMFrJKpp6HNmQWA3FTMRQeEmMJYJ9RPqe9ne2hU/*, tprv8ZgxMBicQKsPezQ2KGArMRovTEbCGxaLgBgaVcTvEx8mby8ogX2bgC4HBapH4yMwrz2FpoCuA17eocuUVMgEP6fnm83YpwSDTFrumw42bny/*)`
```python
self.nodes[1].createwallet(wallet_name="rawtr_multi", descriptors=True, blank=True)
rawtr_multi = self.nodes[1].get_wallet_rpc("rawtr_multi")
rawtr_multi_desc = "rawtr(tprv8ZgxMBicQKsPefef2Doobbq3xTCaVTHcDn6me82KSXY1vY9AJAWD5u7SDM4XGLfc4EoXRMFrJKpp6HNmQWA3FTMRQeEmMJYJ9RPqe9ne2hU/*, tprv8ZgxMBicQKsPezQ2KGArMRovTEbCGxaLgBgaVcTvEx8mby8ogX2bgC4HBapH4yMwrz2FpoCuA17eocuUVMgEP6fnm83YpwSDTFrumw42bny/*)#uv78hkt0"
result = rawtr_multi.importdescriptors([{"desc": rawtr_multi_desc, "active": True, "timestamp": "now"}])
print(rawtr_multi.listdescriptors(True))
```
This PR adds a check that prevents `rawtr` descriptors from being created if more than one key is entered, shows an error message, and adds a test for this case.
ACKs for top commit:
achow101:
ACK 416ceb8661117235c76f2985512473ebbc64956b
sipa:
ACK 416ceb8661117235c76f2985512473ebbc64956b
Tree-SHA512: a2009e91f1bca6ee79cc68f65811caa6a21fc8b80acd8dc58e283f424b41fe53b0db7ce3693b1c7e2184ff571e6d1fbb9f5ccde89b65d3026726f3393c492044
|
|
|
|
|
|
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
|
|
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
|
|
|
|
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>
|
|
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.
|
|
context information
9b526727000509dc6ef90f2ce6a9049edebf959c For descriptor pubkey parse errors, include context information (Ben Woosley)
Pull request description:
This adds readily-available context information to the error string, for further disambiguation.
This is a revival of #16123 which was largely addressed in #16542.
Note 'Multi:' is used rather than 'multi():' as it also encompasses 'sortedmulti():'
ACKs for top commit:
achow101:
ACK 9b526727000509dc6ef90f2ce6a9049edebf959c
theStack:
ACK 9b526727000509dc6ef90f2ce6a9049edebf959c
Tree-SHA512: 96533ea8c3ac7010f9b62e75b4bd20b65aff843030eb91c7a88312975acecaaf17909b7d1841f45edc86dbf7fa402d208adb85f0673bd79b857dbebacb8c9395
|
|
multisig inside tr
4828d53eccd52a67631c64cef0ba7df90dff138d Add (sorted)multi_a descriptors to doc/descriptors.md (Pieter Wuille)
b5f33ac1f82aea290b4653af36ac2ad1bf1cce7b Simplify wallet_taproot.py functional test (Pieter Wuille)
eb0667ea96d52db9135514a5e95ab943f6abd8a6 Add tests for (sorted)multi_a derivation/signing (Pieter Wuille)
c17c6aa08df81aa0086d80b50187c8cd60ecc222 Add signing support for (sorted)multi_a scripts (Pieter Wuille)
3eed6fca57d1fa7544f372e6e7de0a9ae1b5715a Add multi_a descriptor inference (Pieter Wuille)
79728c4a3d8a74f276daf1e72abbdecdab85a5d8 Add (sorted)multi_a descriptor and script derivation (Pieter Wuille)
25e95f9ff89a97b87ce218f28274c3c821b2d54d Merge/generalize IsValidMultisigKeyCount/GetMultisigKeyCount (Pieter Wuille)
Pull request description:
This adds a new `multi_a(k,key_1,key_2,...,key_n)` (and corresponding `sortedmulti_a`) descriptor for k-of-n policies inside `tr()`. Semantically it is very similar to the existing `multi()` descriptor, but with the following changes:
* The corresponding script is `<key1> OP_CHECKSIG <key2> OP_CHECKSIGADD <key3> OP_CHECKSIGADD ... <key_n> OP_CHECKSIGADD <k> OP_NUMEQUAL`, rather than the traditional `OP_CHECKMULTISIG`-based script, making it usable inside the `tr()` descriptor.
* The keys can optionally be specified in x-only notation.
* Both the number of keys and the threshold can be as high as 999; this is the limit due to the consensus stacksize=1000 limit
I expect that this functionality will later be replaced with a miniscript-based implementation, but I don't think it's necessary to wait for that.
Limitations:
* The wallet code will for not estimate witness size incorrectly for script path spends, which may result in a (dramatic) fee underpayment with large multi_a scripts.
* The multi_a script construction is (slightly) suboptimal for n-of-n (where a `<key1> OP_CHECKSIGVERIFY ... <key_n-1> OP_CHECKSIGVERIFY <key_n> OP_CHECKSIG` would be better). Such a construction is not included here.
ACKs for top commit:
achow101:
ACK 4828d53eccd52a67631c64cef0ba7df90dff138d
gruve-p:
ACK https://github.com/bitcoin/bitcoin/pull/24043/commits/4828d53eccd52a67631c64cef0ba7df90dff138d
sanket1729:
code review ACK 4828d53eccd52a67631c64cef0ba7df90dff138d
darosior:
Code review ACK 4828d53eccd52a67631c64cef0ba7df90dff138d
Tree-SHA512: 5dcd434b79585f0ff830f7d501d27df5e346f5749f47a3109ec309ebf2cbbad0e1da541eec654026d911ab67fd7cf7793fab0f765628d68d81b96ef2a4d234ce
|
|
Note 'Multi:' is used rather than 'multi():' as it also encompasses 'sortedmulti():'
|
|
|
|
|
|
|
|
|
