Currently when a user uses `signrawtransaction` on a transaction that is
already partially signed with SIGHASH_RANGEPROOF signatures, the existing
signatures will be deleted because they are not recognized as valid. This
makes it impossible to collabratively sign transactions using this RPC.
Fix this.
The logic for computing the uncached version of the sighash changed during
the 0.21 rebase, such that it no longer matched the cached version. As the
changed hash is used during signing (not verification!), this was not a
forking change (and our existing functional test would have caught such a
forking change since it uses Python to independently compute the hash).
But it still broke signing.
Test in next commit.
Messy merge conflicts because this PR backported some ad-hoc stuff from
upstream while keeping a few things that upstream deleted. Hopefully
reviewing is easier than doing this in the first place, since ultimately
all I did was delete code from one side or another of the conflicts.
(Ok, I also changed some boost optional stuff to std::optional, and had
to patch up a test file for test framework changes.)
When reviewing the detailed crypto, bear in mind that taptweaks, like all
hashes are the kind of crypto that cannot be subtly wrong -- it will either
fail very hard or be correct. And we have independent implementations in
C++ and Python that cross-check each other, si it's unlikely to be the former.
This forward-ports the new Taproot sighash but does not fix a couple
22-blocked TODOs related to the MissingDataBehavior enum. Should be fixed
in a followup commit.
One nontrivial change I had to make was feeding the genesis hash to SignTransaction
(the "global" one in script/sign.cpp) so that it could correctly compute
the sighash at signing time.
Surprisingly easy to do. Almost all of the diff resolution was mechanically
* replacing boost::variant with std::variant
* replacing Optional with std::optional
* then replacing `nullopt` with `std::nullopt`
* updating the RPC functions for the new RPCArg::Default type
* update the tests/ directory to make new (since 22) tests use arrays for
createrawtransaction outputs
* other ad-hoc changes to function parameters etc (not too many of these)
I had to "really" change the code in PrecomputePSBTData, which was introduced
in 22.0 and affected by PSET, but this function was like 8 lines long so it
was easy.
Reviewing the diff may be a bit difficult because of the mix of mechanical
changes and ad-hoc things. Probably the most straightforward thing to do
is to redo the merge, `sed -i` to fix the boost::variant and Optional stuff,
then diff the remaining conflicts against this commit.
TODO: grep for `blindpsbt` and you will see that this RPC is still referenced
in documentation and help text even though it was deleted. Need to fix this
in 0.21 in a separate PR.
That results in a much safer interface (making the tweak commit
to the key implicitly using a fixed tag means it can't be used for
unrelated tweaking).
https://github.com/bitcoin/bitcoin/pull/22051 (5/9)
We actually preserve the "unrelated tweaking" method so we can
use it in OP_TWEAKVERIFY
The places where we need to lookup information for a XOnlyPubKey
currently implement a hack which makes both serializations of the full
pubkey in order to try the CKeyIDs for the lookup functions. Instead of
duplicating this everywhere it is needed, we can consolidate the CKeyID
generation into a function, and then have wrappers around GetPubKey,
GetKey, and GetKeyOrigin which takes the XOnlyPubKey, retrieves all of
the CKeyIDs (using the new GetKeyIDs() function in XOnlyPubKey), and
tries their respective underlying lookup function.
08f57a0057 Assert that IsComplete() in GetSpendData() (Pieter Wuille)
d8f4b976d5 Remove default nHashTypeIn arguments to MutableTransactionSignatureCreator (Pieter Wuille)
c7048aae95 Simplify SignTransaction precomputation loop (Pieter Wuille)
addb9b5a71 Improve comments in taproot signing logic (Pieter Wuille)
Pull request description:
This addresses a few review comments from #21365 that were left at the time of merge (as well as some from #22166 applying to the commit it shared with #21365).
I do not think any are blockers for a 22.0 release.
ACKs for top commit:
theStack:
re-ACK 08f57a0057🌴
Zero-1729:
crACK 08f57a0
jonatack:
Code review ACK 08f57a0057 per `git range-diff e9d6eb1 9336670 08f57a0` followed by re-code review per commit to swap context back into memory and debug build/run unit tests + feature_taproot.py as a sanity check
Tree-SHA512: 968750109ba8d6faf3016035a38f81c6aefb724c632a3cab0bbf43cf31b9d187b6b0fddd8772768f57338df11eb07ab9c4c6dacdf3cf35b71f29699c67a301ea
As XOnlyPubKey has a Span-based constructor, that can be used directly
without needing to first convert the byte sequence into a vector, only
to convert that to a uint256, which only then can then be passed as a
span to the constructor.
92993aa5cf Change SignTransaction's input_errors to use bilingual_str (Andrew Chow)
171366e89b Use bilingual_str for address fetching functions (Andrew Chow)
9571c69b51 Add bilingual_str::clear() (Andrew Chow)
Pull request description:
In a couple of places in the wallet, errors are `std::string`. In order for these errors to be translated, change them to use `bilingual_str`.
ACKs for top commit:
hebasto:
re-ACK 92993aa5cf, only rebased since my [previous](https://github.com/bitcoin/bitcoin/pull/22337#pullrequestreview-694542729) review, verified with
klementtan:
Code review ACK 92993aa5cf
meshcollider:
Code review ACK 92993aa5cf
Tree-SHA512: 5400e419dd87db8c49b67ed0964de2d44b58010a566ca246f2f0760ed9ef6a9b6f6df7a6adcb211b315b74c727bfe8c7d07eb5690b5922fa5828ceef4c83461f
This introduces Taproot wallet support. I fixed all the merge conflicts
and ensured that the tests pass, but this is still using the old sighash
(before Russell/Sanket/I redid it) so is not actually production ready.
Will be fixed when we bring Elements #1002 in.
c3e111a7da Reduced number of validations in `tx_validationcache_tests` to keep the run time reasonable. (lucash-dev)
Pull request description:
Following a suggestion in the comments, changed `ValidateCheckInputsForAllFlags` from testing all possible flag combinations to testing a random subset. Also created a new enum constant for the highest flag, so that this test doesn’t keep testing an incomplete subset in case a new flag is added.
Timing for `checkinputs_test`:
```
Before: 6.8s
After: 3.7s
----------------
Saved: 3.1s (45%)
```
This PR was split from #13050. Also see #10026.
ACKs for top commit:
leonardojobim:
tACK https://github.com/bitcoin/bitcoin/pull/13533/commits/c3e111a7daf5800026dda4455c737de0412528f1.
kallewoof:
ACK c3e111a7da
theStack:
re-ACK c3e111a7da
Tree-SHA512: bef49645bdd4f61ec73cc77a9f028b95d9856db9446d2e7fc9a48867a6f0e94c2c9f150cb771a30fe852db0efb0a1bd15d38b00d712651793ccb59ff6157a7b4
Does the bare minimum to introduce Taproot wallet support with CT; just
adds a CPubKey blinding_pubkey to the taproot destination variant and
updates some visitors.
In future when we define blech32 we will need to make sure we are using
that encoding and using the pubkey.
007910388b [Refactor] Rename scriptPubKey -> exec_script (sanket1729)
Pull request description:
Rename scriptPubKey to witness_script in ExecuteWitnessScript() function to correctly reflect which script is being executed.
For example in segwitv0, this scriptPubKey refers to the script of the form `OP_0 <script_hash>`, but witness_script refers to the script that actually hashes to the `script_hash`.
If there is a reason why it's named this way, I would love to know
ACKs for top commit:
MarcoFalke:
review ACK 007910388b🖖
theStack:
ACK 007910388b
lsilva01:
Code Review https://github.com/bitcoin/bitcoin/pull/22428/commits/007910388b54abc97057e44a7a8f7241e83c203b ACK
Tree-SHA512: 768e10e656b60b1293beb560fb7adbc2c1495e6db1f54f0c2c63109692ae0c579c856b194b33f72afd0d332159a9796c0e2bd99b79ea5c4b1803469a81301fd6
fa621ededd refactor: Pass script verify flags as uint32_t (MarcoFalke)
Pull request description:
The flags are cast to unsigned in the interpreter anyway, so avoid the confusion (and fuzz crashes) by just passing them as unsigned from the beginning.
Also, the flags are often inverted bit-wise with the `~` operator, which also works on signed integers, but might cause confusion as the sign bit is flipped.
Fixes#22233
ACKs for top commit:
theStack:
Concept and code review ACK fa621ededd
kristapsk:
ACK fa621ededd
jonatack:
ACK fa621ededd
Tree-SHA512: ea0720f32f823fa7f075309978672aa39773c6019d12b6c1c9d611fc1983a76115b7fe2a28d50814673bb6415c311ccc05b99d6e871575fb6900faf75ed17769
