Deal with missing data in signature hashes more consistently #21330

pull sipa wants to merge 4 commits into bitcoin:master from sipa:202103_missing_data_verify changing 14 files +74 −42

sipa commented at 2:55 AM on March 2, 2021: member
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).
DrahtBot added the label Consensus on Mar 2, 2021
sanket1729 approved
sanket1729 commented at 8:22 AM on March 2, 2021: contributor

ACK 1aba748de0ba88db320a15c43f5f39d564d63539 . Verified all non test call sites for SignatureHash are covered in 82e242d71f39f6a1af5214656337af4cd2c99d17 .
DrahtBot commented at 10:09 AM on March 2, 2021: contributor


The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #21365 (Basic Taproot signing support for descriptor wallets by sipa)
- #21158 (lib: Add Taproot support to libconsensus by jrawsthorne)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
DrahtBot cross-referenced this on Mar 2, 2021 from issue Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it by achow101
DrahtBot cross-referenced this on Mar 2, 2021 from issue lib: Add Taproot support to libconsensus by jrawsthorne
sipa cross-referenced this on Mar 4, 2021 from issue Basic Taproot signing support for descriptor wallets by sipa
achow101 commented at 1:36 AM on March 12, 2021: member

ACK 1aba748de0ba88db320a15c43f5f39d564d63539

in src/script/interpreter.h:250 in acac9421ae outdated

 246 | @@ -247,11 +247,21 @@ class BaseSignatureChecker
 247 |      virtual ~BaseSignatureChecker() {}
 248 |  };
 249 |  
 250 | +/** Enum to specify what *TransiactionSignatureChecker's behavior should be

achow101 commented at 7:55 PM on March 15, 2021:

In acac9421ae7c8487ec8d311610a60de2b8f84a40 "Add MissingDataBehavior and make TransactionSignatureChecker handle it"

nit: typo

/** Enum to specify what *TransactionSignatureChecker's behavior should be

sipa commented at 12:29 AM on March 16, 2021:

Done.

Add MissingDataBehavior and make TransactionSignatureChecker handle it

This allows specifying how *TransactionSignatureChecker will behave when
presented with missing transaction data such as amounts spent, BIP341 data,
or spent outputs.

As all call sites still (implicitly) use MissingDataBehavior::ASSERT_FAIL,
this commit introduces no change in behavior.

b77b0cc507

Make all SignatureChecker explicit about missing data

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

3820090bd6

Treat amount<0 also as missing data for P2WPKH/P2WSH

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).

497718b467

Use PrecomputedTransactionData in signet check

This is out of an abundance of caution only, as signet currently doesn't
enable taproot validation flags. Still, it seems cleaner to make sure
that all non-test code that passes MissingDataBehavior::ASSERT_FAIL
also actually makes sure no data can be missing.

725d7ae049

sipa force-pushed on Mar 16, 2021
benthecarman approved
benthecarman commented at 9:13 PM on March 18, 2021: contributor

ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
sanket1729 commented at 9:17 PM on March 18, 2021: contributor

reACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
Sjors commented at 9:38 AM on March 19, 2021: member

ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965

Missing amounts are treated as -1 (thus leading to unexpected signature failures)

I found one example of this, but having difficulty finding the other places:

https://github.com/bitcoin/bitcoin/blob/05757aa860215a8fd5002d99b6ec653175c6b734/src/node/psbt.cpp#L100
laanwj added this to the "Bugfixes" column in a project

laanwj moved this from the "Bugfixes" to the "Blockers" column in a project

achow101 commented at 7:48 PM on April 9, 2021: member

re-ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965

in src/script/sign.cpp:30 in 497718b467 outdated

  25 | @@ -26,6 +26,9 @@ bool MutableTransactionSignatureCreator::CreateSig(const SigningProvider& provid
  26 |      if (sigversion == SigVersion::WITNESS_V0 && !key.IsCompressed())
  27 |          return false;
  28 |  
  29 | +    // Signing for witness scripts needs the amount.
  30 | +    if (sigversion == SigVersion::WITNESS_V0 && amount < 0) return false;

fjahr commented at 12:14 AM on April 13, 2021:

nit: might have been interesting to make it available here and use return HandleMissingData(MissingDataBehavior::FAIL) to make this easier to grep. But I can see that it's probably not worth the additional effort.

fjahr commented at 12:17 AM on April 13, 2021: contributor

Code review ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965

Obviously, feel free to ignore my nit.
fanquake merged this on Apr 13, 2021
fanquake closed this on Apr 13, 2021
fanquake removed this from the "Blockers" column in a project

sidhujag referenced this in commit deebd929f6 on Apr 13, 2021
MarcoFalke commented at 8:51 AM on April 25, 2021: member

Follow-up: #21773
roconnor-blockstream cross-referenced this on Aug 23, 2021 from issue Tapscript new opcodes by sanket1729
achow101 cross-referenced this on Aug 23, 2021 from issue combinerawtransaction fails to properly combine transactions containing taproot inputs by achow101
achow101 cross-referenced this on Aug 26, 2021 from issue v0.21.2 Testing by fanquake
fanquake referenced this in commit eeb9db506d on Sep 2, 2021
fanquake referenced this in commit 6a3f25ba9f on Sep 2, 2021
fanquake referenced this in commit b1a51c7cf2 on Sep 2, 2021
fanquake referenced this in commit 7e63f59eed on Sep 2, 2021
fanquake cross-referenced this on Sep 2, 2021 from issue [0.21] Final changes for 0.21.2 by fanquake
fanquake commented at 2:14 AM on September 2, 2021: member

Backported to 0.21 in #22858.
JeremyRubin cross-referenced this on Jan 3, 2022 from issue Implement BIP-119 Validation (CheckTemplateVerify) by JeremyRubin
bitcoin locked this on Sep 2, 2022

Contributors

Labels

Linked (view graph)

#21365 Basic Taproot signing support for descriptor wallets #21702 Implement BIP-119 Validation (CheckTemplateVerify)#21773 fuzz: Ensure prevout is consensus-valid #22720 v0.21.2 Testing #22784 combinerawtransaction fails to properly combine transactions containing taproot inputs #22858 [0.21] Final changes for 0.21.2

Deal with missing data in signature hashes more consistently #21330

Conflicts