highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-19 06:43:45 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge #21330: Deal with missing data in signature hashes more consistently

Browse Source 
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
This commit is contained in:
fanquake
2021-04-13 09:03:31 +08:00
parent 89b72ce045 725d7ae049
commit bd65a76b9d
14 changed files with 74 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/script/sign.cpp
View File

@@ -14,7 +14,7 @@
typedef std::vector<unsigned char> valtype;
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn) {}
MutableTransactionSignatureCreator::MutableTransactionSignatureCreator(const CMutableTransaction* txToIn, unsigned int nInIn, const CAmount& amountIn, int nHashTypeIn) : txTo(txToIn), nIn(nInIn), nHashType(nHashTypeIn), amount(amountIn), checker(txTo, nIn, amountIn, MissingDataBehavior::FAIL) {}
bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& address, const CScript& scriptCode, SigVersion sigversion) const
{
@@ -26,6 +26,9 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid
if (sigversion == SigVersion::WITNESS_V0 && !key.IsCompressed())
return false;
// Signing for witness scripts needs the amount.
if (sigversion == SigVersion::WITNESS_V0 && amount < 0) return false;
uint256 hash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion);
if (!key.Sign(hash, vchSig))
return false;
@@ -292,7 +295,7 @@ SignatureData DataFromTransaction(const CMutableTransaction& tx, unsigned int nI
Stacks stack(data);
// Get signatures
MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue);
MutableTransactionSignatureChecker tx_checker(&tx, nIn, txout.nValue, MissingDataBehavior::FAIL);
SignatureExtractorChecker extractor_checker(data, tx_checker);
if (VerifyScript(data.scriptSig, txout.scriptPubKey, &data.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, extractor_checker)) {
data.complete = true;
@@ -499,7 +502,7 @@ bool SignTransaction(CMutableTransaction& mtx, const SigningProvider* keystore,
}
ScriptError serror = SCRIPT_ERR_OK;
if (!VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount), &serror)) {
if (!VerifyScript(txin.scriptSig, prevPubKey, &txin.scriptWitness, STANDARD_SCRIPT_VERIFY_FLAGS, TransactionSignatureChecker(&txConst, i, amount, MissingDataBehavior::FAIL), &serror)) {
if (serror == SCRIPT_ERR_INVALID_STACK_OPERATION) {
// Unable to sign input and verification failed (possible attempt to partially sign).
input_errors[i] = "Unable to sign input, invalid stack size (possibly missing key)";