SerializeToVector, UnserializeFromVector, DeserializeHDKeypaths, and SerializeHDKeypaths
were in sign.h where PSBT was originally implemented. Since all of the PSBT serialization
has moved to its own file, these functions should follow.
libsecp256k1's secp256k1_schnorrsig_sign only follows BIP340 exactly
if an aux_rand32 argument is passed. When no randomness is used
(as is the case in the current codebase here), there is no impact
on security between not providing aux_rand32 at all, or providing
an empty one. Yet, for repeatability/testability it is simpler
to always use an all-zero one.
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.
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.
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.
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.
This provides a means to pass in a PrecomputedTransactionData object to
the MutableTransactionSignatureCreator, allowing the prevout data to be
passed into the signature hashers. It is also more efficient.
725d7ae049 Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b467 Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090bd6 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc507 Add MissingDataBehavior and make TransactionSignatureChecker handle it (Pieter Wuille)
Pull request description:
Currently we have 2 levels of potentially-missing data in the transaction signature hashes:
* P2WPKH/P2WSH hashes need the spent amount
* P2TR hashes need all spent outputs (amount + scriptPubKey)
Missing amounts are treated as -1 (thus leading to unexpected signature failures), while missing outputs in P2TR validation cause assertion failure. This is hard to extend for signing support, and also quite ugly in general.
In this PR, an explicit configuration option to {Mutable,}TransactionSignatureChecker is added (MissingDataBehavior enum class) to either select ASSERT_FAIL or FAIL. Validation code passes ASSERT_FAIL (as at validation time all data should always be passed, and anything else is a serious bug in the code), while signing code uses FAIL.
The existence of the ASSERT_FAIL option is really just an abundance of caution. Always using FAIL should be just fine, but if there were for some reason a code path in consensus code was introduced that misses certain data, I think we prefer as assertion failure over silently introducing a consensus change.
Potentially useful follow-ups (not for this PR, in my preference):
* Having an explicit script validation error code for missing data.
* Having a MissingDataBehavior::SUCCEED option as well, for use in script/sign.cpp DataFromTransaction (if a signature is present in a witness, and we don't have enough data to fully validate it, we should probably treat it as valid and not touch it).
ACKs for top commit:
sanket1729:
reACK 725d7ae049
Sjors:
ACK 725d7ae049
achow101:
re-ACK 725d7ae049
benthecarman:
ACK 725d7ae049
fjahr:
Code review ACK 725d7ae049
Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
Several conflicts in the C++ code related to the new `flags` parameter
to `CheckSignature` and the corresponding function being renamed upstream
to `CheckSignatureECDSA`.
Several conflicts in the test harness as Steven sorta pulled the new
upstream ECKey module into the Python code, and the actual upstream
code was slightly different. Also needed to update the feature_taproot
code to always use the non-RANGEPROOF sighash since dynafed is not
enabled in the Taproot test.
Also had to pull the `set_wif` method out of `ECKey` and inline it because
otherwise it triggers a "circular inclusion" error between script.py (which
would pull in `base58_to_bytes` from address.py) and address.py (which now
pulls in some taproot EC related stuff from script.py).
Noticed that #960 does not test the "sighash rangeproof flag set but no
witnesses" case.
Historically lack of amount data has been treated as amount==-1. Change
this and treat it as missing data, as introduced in the previous commits.
To be minimally invasive, do this at SignatureHash() call sites rather
than inside SignatureHash() (which currently has no means or returning
a failure code).
Remove the implicit MissingDataBehavior::ASSERT_FAIL in the
*TransationSignatureChecker constructors, and instead specify
it explicit in all call sites:
* Test code uses ASSERT_FAIL
* Validation uses ASSERT_FAIL (through CachingTransactionSignatureChecker)
(including signet)
* libconsensus uses FAIL, matching the existing behavior of the
non-amount API (and the extended required data for taproot validation
is not available yet)
* Signing code uses FAIL
Introduces a DeferringSignatureChecker which simply takes a
BaseSignatureChecker and passes through everything.
SignatureExtractorChecker now subclasses DeferringSignatureChecker. This
allows for all BaseSignatureChecker functions to be implemented for
SignatureExtractorChecker, while allowing for future signature checkers
which opreate similarly to SignatureExtractorChecker.
fa650ca7f1 Use -Wswitch for TxoutType where possible (MarcoFalke)
fa59e0b5bd test: Add missing script_standard_Solver_success cases (MarcoFalke)
Pull request description:
This removes unused `default:` cases for all `switch` statements on `TxoutType` and adds the cases (`MULTISIG`, `NULL_DATA`, `NONSTANDARD`) to `ExtractDestination` for clarity.
Also, the compiler is now able to use `-Wswitch`.
ACKs for top commit:
practicalswift:
cr ACK fa650ca7f1: patch looks correct and `assert(false);` is better than UB :)
hebasto:
ACK fa650ca7f1, I have reviewed the code and it looks OK, I agree it can be merged.
Tree-SHA512: 282458b6523bd8923a0c0f5c423d1db2dce2a2d1b1d1dae455415c6fc995bb41ce82c1f9b0a1c0dcc6d874d171e04c30eca585f147582f52c7048c140358630a
