Deal with missing data in signature hashes more consistently #21330

1. 
   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).
2. DrahtBot added the label Consensus on Mar 2, 2021
3. sanket1729 approved
4. 
   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 .
5. 
   DrahtBot commented at 10:09 am on March 2, 2021: member

   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.

6. 
   achow101 commented at 1:36 am on March 12, 2021: member

   ACK 1aba748de0ba88db320a15c43f5f39d564d63539
7. 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

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

   ---

   

   sipa commented at 0:29 am on March 16, 2021:

   Done.
8. 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
9. 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
10. 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
11. 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
12. sipa force-pushed on Mar 16, 2021
13. benthecarman approved
14. 
    benthecarman commented at 9:13 pm on March 18, 2021: contributor

    ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
15. 
    sanket1729 commented at 9:17 pm on March 18, 2021: contributor

    reACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
16. 
    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

17. laanwj added this to the "Bugfixes" column in a project

    ---

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

    ---

19. 
    achow101 commented at 7:48 pm on April 9, 2021: member

    re-ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965
20. 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 0: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.
21. 
    fjahr commented at 0:17 am on April 13, 2021: member

    Code review ACK 725d7ae0494d4a45f5a840bbbd19c008a7363965

    Obviously, feel free to ignore my nit.

22. fanquake merged this on Apr 13, 2021
23. fanquake closed this on Apr 13, 2021

    ---

24. fanquake removed this from the "Blockers" column in a project

    ---

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

    Follow-up: #21773
27. fanquake referenced this in commit eeb9db506d on Sep 2, 2021
28. fanquake referenced this in commit 6a3f25ba9f on Sep 2, 2021
29. fanquake referenced this in commit b1a51c7cf2 on Sep 2, 2021
30. fanquake referenced this in commit 7e63f59eed on Sep 2, 2021
31. 
    fanquake commented at 2:14 am on September 2, 2021: member

    Backported to 0.21 in #22858.
32. DrahtBot locked this on Sep 2, 2022

Contributors
sipa sanket1729 DrahtBot achow101 benthecarman Sjors fjahr MarcoFalke fanquake

Labels
Consensus