-
Notifications
You must be signed in to change notification settings - Fork 36.5k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
ACK 1aba748 |
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.
1aba748
to
725d7ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 725d7ae
reACK 725d7ae |
re-ACK 725d7ae |
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
…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
Follow-up: #21773 |
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
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
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
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
Backported to 0.21 in #22858. |
Currently we have 2 levels of potentially-missing data in the transaction signature hashes:
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):