ffff4a293ad878494e12f8f00108cc99ee2b713e bench: Update span-serialize comment (MarcoFalke)
fa4d6ec97bcb1790a7cd4363a13fda7c80c3dd90 refactor: Avoid false-positive gcc warning (MarcoFalke)
fa942332b40c97375af0722f32f7575bca3af819 scripted-diff: Bump copyright headers after std::span changes (MarcoFalke)
fa0c6b7179c062b7ca92d120455ce02a9f4e9e19 refactor: Remove unused Span alias (MarcoFalke)
fade0b5e5e6e80e3da1ab6448b6212244bafa5d3 scripted-diff: Use std::span over Span (MarcoFalke)
fadccc26c03db00a2be3f703aa7e5eec4312bd2e refactor: Make Span an alias of std::span (MarcoFalke)
fa27e36717ec18d64b7ff7bba71b8f0c202ba31d test: Fix broken span_tests (MarcoFalke)
fadf02ef8bf96ad5b3b8e34fd425b31b555f4371 refactor: Return std::span from MakeUCharSpan (MarcoFalke)
fa720b94be17fa9e7c91188710e6a04939ceab11 refactor: Return std::span from MakeByteSpan (MarcoFalke)
Pull request description:
`Span` has some issues:
* It does not support fixed-size spans, which are available through `std::span`.
* It is confusing to have it available and in use at the same time with `std::span`.
* It does not obey the standard library iterator build hardening flags. See https://github.com/bitcoin/bitcoin/issues/31272 for a discussion. For example, this allows to catch issues like the one fixed in commit fabeca3458b38a3d8930cb0cbc866388c3f120f1.
Both types are type-safe and can even implicitly convert into each other in most contexts.
However, exclusively using `std::span` seems less confusing, so do it here with a scripted-diff.
ACKs for top commit:
l0rinc:
reACK ffff4a293ad878494e12f8f00108cc99ee2b713e
theuni:
ACK ffff4a293ad878494e12f8f00108cc99ee2b713e.
Tree-SHA512: 9cc2f1f43551e2c07cc09f38b1f27d11e57e9e9bc0c6138c8fddd0cef54b91acd8b14711205ff949be874294a121910d0aceffe0e8914c4cff07f1e0e87ad5b8
21e9d39a3725cd6107b742f0cb97f65b3640201b docs: add release notes for 31603 (brunoerg)
a8b548d75d9a376c9bb66e06bb918c876416d615 test: `getdescriptorinfo`/`importdescriptors` with whitespace in pubkeys (brunoerg)
c7afca3d62cf5d3ea9b98d5a76e4e54cac07bc3c test: descriptor: check whitespace into keys (brunoerg)
cb722a3cea16a04844c83e56fd6deaa1f0dc0a7e descriptor: check whitespace in ParsePubkeyInner (brunoerg)
50856695ef6c02ecbaa0cf448567355b6b86b510 test: fix descriptors in `ismine_tests` (brunoerg)
Pull request description:
Currently, we successfully parse descriptors which contains spaces in the beginning or end of the public/private key within a fragment (e.g. `pk( KEY)`, `pk(KEY )` or `pk( KEY )`). I have noticed that one of the reasons is that the `DecodeBase58` function simply ignore these whitespaces.
This PR changes the `ParsePubkeyInner ` to reject pubkeys that contain a whitespace at the beginning and/or at the end. We will only check the whitespace in some RPCs (e.g. `importdescriptors`), but an already imported descriptor won't be affected by this check, especially because we store descriptors from `ToString`.
For context: https://github.com/brunoerg/bitcoinfuzz/issues/72
ACKs for top commit:
rkrux:
tACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
darosior:
re-ACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
sipa:
utACK 21e9d39a3725cd6107b742f0cb97f65b3640201b
Tree-SHA512: 54f48a89a235517e5cdc29a46dceeb7dabbee93c7616a166288ff3f90131808eb0ece43b0797a11fe827a5f7bd51d65e3e75c16789b0a42020934cabb684cc8f
Historically, the headers have been bumped some time after a file has
been touched. Do it now to avoid having to touch them again in the
future for that reason.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's;( 20[0-2][0-9])(-20[0-2][0-9])? The Bitcoin Core developers;\1-present The Bitcoin Core developers;g' $( git show --pretty="" --name-only HEAD~1 )
-END VERIFY SCRIPT-
The following scripted-diff commit will replace ParseHex("...") with "..."_hex_u8, but this replacement will not work in cases where vectors are needed instead of arrays, and is not ideal in cases where std::byte is accepted.
For example, it is currently necessary to use _hex_v_u8 when calling CScript operator<< because that operator does not currently support std::array or std::byte.
Conversely, it is incorrect to use _hex_v instead of _hex in net_processing.cpp for the MakeAndPushMessage argument, because if the argument is a std::vector it is considered variable-length and serialized with a size prefix, but if the argument is a std::array or Span is it considered fixed length and serialized without a prefix.
By the same logic, it is also safe to change the NUMS_H constant in pubkey.cpp from a std::vector to std::array because it is never serialized.
a0abcbd3822bd17a1d73c42ccd5b040a150b0501 doc: Mention multipath specifier (Ava Chow)
0019f61fc546b4d5f42eb4086f42560863fe0efb tests: Test importing of multipath descriptors (Ava Chow)
f97d5c137d605ac48f1122a836c9aa5f834957ba wallet, rpc: Allow importdescriptors to import multipath descriptors (Ava Chow)
32dcbca3fb918bc899a0637f876db31c3419aafd rpc: Allow importmulti to import multipath descriptors correctly (Ava Chow)
64dfe3ce4bed9ac168d0b08def8af7485db94ef1 wallet: Move internal to be per key when importing (Ava Chow)
16922455253f47fae0466c4ec6c3adfadcfe9182 tests: Multipath descriptors for scantxoutset and deriveaddresses (Ava Chow)
cddc0ba9a9dca3ca5873d768b3b504cdb2ab947b rpc: Have deriveaddresses derive receiving and change (Ava Chow)
360456cd221501fde3efe11bdba5c6d999dbb323 tests: Multipath descriptors for getdescriptorinfo (Ava Chow)
a90eee444c965bbd7bcddf9656eca9cee14c3aec tests: Add unit tests for multipath descriptors (Ava Chow)
1bbf46e2dae4599d04c79aaacf7c5db00b2e707f descriptors: Change Parse to return vector of descriptors (Ava Chow)
0d640c6f02bc20e5c1be773443dd74d8806d953b descriptors: Have ParseKeypath handle multipath specifiers (Ava Chow)
a5f39b103461a98689fd5d382e8da29037f55bea descriptors: Change ParseScript to return vector of descriptors (Ava Chow)
0d55deae157f4f8226b2419d55e7dc0dfb6e4aec descriptors: Add DescriptorImpl::Clone (Ava Chow)
7e86541f723d62c7ec6768f7f592c09ba2047d9e descriptors: Add PubkeyProvider::Clone (Ava Chow)
Pull request description:
It is convenient to have a descriptor which specifies both receiving and change addresses in a single string. However, as discussed in https://github.com/bitcoin/bitcoin/issues/17190#issuecomment-895515768, it is not feasible to use a generic multipath specification like BIP 88 due to combinatorial blow up and that it would result in unexpected descriptors.
To resolve that problem, this PR proposes a targeted solution which allows only a single pair of 2 derivation indexes to be inserted in the place of a single derivation index. So instead of two descriptor `wpkh(xpub.../0/0/*)` and `wpkh(xpub.../0/1/*)` to represent receive and change addresses, this could be written as `wpkh(xpub.../0/<0;1>/*)`. The multipath specifier is of the form `<NUM;NUM>`. Each `NUM` can have its own hardened specifier, e.g. `<0;1h>` is valid. The multipath specifier can also only appear in one path index in the derivation path.
This results in the parser returning two descriptors. The first descriptor uses the first `NUM` in all pairs present, and the second uses the second `NUM`. In our implementation, if a multipath descriptor is not provided, a pair is still returned, but the second element is just `nullptr`.
The wallet will not output the multipath descriptors (yet). Furthermore, when a multipath descriptor is imported, it is expanded to the two descriptors and each imported on its own, with the second descriptor being implicitly for internal (change) addresses. There is no change to how the wallet stores or outputs descriptors (yet).
Note that the path specifier is different from what was proposed. It uses angle brackets and the semicolon because these are unused characters available in the character set and I wanted to avoid conflicts with characters already in use in descriptors.
Closes#17190
ACKs for top commit:
darosior:
re-ACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
mjdietzx:
reACK a0abcbd3822bd17a1d73c42ccd5b040a150b0501
pythcoiner:
reACK a0abcbd
furszy:
Code review ACK a0abcbd
glozow:
light code review ACK a0abcbd3822
Tree-SHA512: 84ea40b3fd1b762194acd021cae018c2f09b98e595f5e87de5c832c265cfe8a6d0bc4dae25785392fa90db0f6301ddf9aea787980a29c74f81d04b711ac446c2
There are no changes to behavior. Changes in this commit are all additions, and
are easiest to review using "git diff -U0 --word-diff-regex=." options.
Motivation for this change is to keep util functions with really generic names
like "Split" and "Join" out of the global namespace so it is easier to see
where these functions are defined, and so they don't interfere with function
overloading, especially since the util library is a dependency of the kernel
library and intended to be used with external code.
This will help move the miniscript / descriptor parsing functions out of the
util library in an upcoming commit, so they are not exposed to libbitcoinkernel
applications. Moving the Split functions should also make them more
discoverable since they now close to related functions like Join.
The functions are moved verbatim without any changes.
We make the Satisfier a base in which to store the common methods
between the Tapscript and P2WSH satisfier, and from which they both
inherit.
A field is added to SignatureData to be able to satisfy pkh() under
Tapscript context (to get the pubkey hash preimage) without wallet data.
For instance in `finalizepsbt` RPC. See also the next commits for a
functional test that exercises this.
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.
Instead of using the dummysigner to compute a placeholder satisfaction,
infer a descriptor on the scriptPubKey of the coin being spent and use
the estimation of the satisfaction size given by the descriptor
directly.
Note this (almost, see next paragraph) exactly conserves the previous
behaviour. For instance CalculateMaximumSignedInputSize was previously
assuming the input to be spent in a transaction that spends at least one
Segwit coin, since it was always accounting for the serialization of the
number of witness elements.
In this commit we use a placeholder for the size of the serialization of
the witness stack size (1 byte). Since the logic in this commit is
already tricky enough to review, and that it is only a very tiny
approximation not observable through the existing tests, it is addressed
in the next commit.
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()`).
Tests vectors were calculated by running the same tests on
v25. Which was the last release prior to introducing the
diff in the descriptor's string representation ('h' format).
Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>
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.
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.
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
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 b16f93cadd
achow101:
re-ACK b16f93caddcd3254eaf3dc43e09adf2142a9c40a
furszy:
ACK b16f93ca, only change is the `IsSolvable` helper function removal.
Tree-SHA512: 137068157ce90210b710b1bf9ac3c400e2ff5af1112f892094b69875ea473d6a899f52adb51e5030cb907dee517602059cd1661107808558efa5de842ba12b41
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>
36012ef143917f97179d3ba6599ef36a26a9a014 qa: test descriptors with mixed xpubs and const pubkeys (Antoine Poinsot)
Pull request description:
Writing unit tests for Miniscript descriptors i noticed that `test/descriptor_tests`'s `DoCheck()` assumes that a descriptor would either contain only extended keys or only const pubkeys: if it detects an xpub in the descriptor it would assert the number of cached keys is equal to the number of keys in the descriptor, which does not hold if the descriptor also contains const (raw?) public keys since we only cache parent xpubs.
ACKs for top commit:
achow101:
ACK 36012ef143917f97179d3ba6599ef36a26a9a014
Tree-SHA512: 2ede67a6dff726bcad3e260f3deb25c9b77542ed1880eb4ad136730b741014ce950396c69c7027225de1ef27108d609bafd055188b88538ace0beb13c7e34b0b
