Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with missing data in signature hashes more consistently #21330

Merged
merged 4 commits into from
Apr 13, 2021

Conversation

sipa
Copy link
Member

@sipa sipa commented Mar 2, 2021

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

Copy link
Contributor

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 1aba748 . Verified all non test call sites for SignatureHash are covered in 82e242d .

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2021

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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.

@achow101
Copy link
Member

ACK 1aba748

src/script/interpreter.h Outdated Show resolved Hide resolved
sipa added 4 commits March 15, 2021 17:29
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.
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
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).
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.
@sipa sipa force-pushed the 202103_missing_data_verify branch from 1aba748 to 725d7ae Compare March 16, 2021 00:29
Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 725d7ae

@sanket1729
Copy link
Contributor

reACK 725d7ae

@Sjors
Copy link
Member

Sjors commented Mar 19, 2021

ACK 725d7ae

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:

return CAmount(-1);

@achow101
Copy link
Member

achow101 commented Apr 9, 2021

re-ACK 725d7ae

Copy link
Contributor

@fjahr fjahr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 725d7ae

Obviously, feel free to ignore my nit.

@@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@fanquake fanquake merged commit bd65a76 into bitcoin:master Apr 13, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 13, 2021
…consistently

725d7ae Use PrecomputedTransactionData in signet check (Pieter Wuille)
497718b Treat amount<0 also as missing data for P2WPKH/P2WSH (Pieter Wuille)
3820090 Make all SignatureChecker explicit about missing data (Pieter Wuille)
b77b0cc 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 725d7ae
  Sjors:
    ACK 725d7ae
  achow101:
    re-ACK 725d7ae
  benthecarman:
    ACK 725d7ae
  fjahr:
    Code review ACK 725d7ae

Tree-SHA512: d67dc51bae9ca7ef6eb9acccefd682529f397830f77d74cd305500a081ef55aede0e9fa380648c3a8dd4857aa7eeb1ab54fe808979d79db0784ac94ceb31b657
@maflcko
Copy link
Member

maflcko commented Apr 25, 2021

Follow-up: #21773

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 2, 2021
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.

Github-Pull: bitcoin#21330
Rebased-From: b77b0cc
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 2, 2021
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

Github-Pull: bitcoin#21330
Rebased-From: 3820090
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 2, 2021
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).

Github-Pull: bitcoin#21330
Rebased-From: 497718b
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Sep 2, 2021
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.

Github-Pull: bitcoin#21330
Rebased-From: 725d7ae
@fanquake
Copy link
Member

fanquake commented Sep 2, 2021

Backported to 0.21 in #22858.

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants