Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors #22793

1. 
   achow101 commented at 8:22 pm on August 24, 2021: member

   BaseSignatureChecker has virtual functions which are overridden by its child classes. These virtual functions have a default implementation. However because of the default implementation, bugs can be introduced by failing to implement one of them (e.g. #21151). To avoid this, those functions are made into pure virtual functions so that child classes must implement those functions. There are a few places where a BaseSignatureChecker was being created; these instances have been replaced with the DUMMY_CHECKER.

   GenericTransactionSignatureChecker has two constructors, one which takes a PrecomputedTransactionData and one which does not. It can be difficult to understand which one to use, and whether a particular GenericTransactionSignatureChecker has a PrecomputedTransactionData. This has lead to some bugs as well (e.g. #22784). Looking through the codebase, the only place where PrecomputedTransactionData does not need to be provided is in the unit tests. As such, this PR changes GenericTransactionSignatureChecker to have only one constructor which requires PrecomputedTransactionData to be provided. This has a side effect of requiring the same for MutableTransactionSignatureCreator, which also had two constructors, and the one that did not require PrecomputedTransactionData was only used in tests.

2. DrahtBot added the label Consensus on Aug 24, 2021
3. DrahtBot added the label RPC/REST/ZMQ on Aug 24, 2021
4. DrahtBot added the label TX fees and policy on Aug 24, 2021
5. DrahtBot added the label Utils/log/libs on Aug 24, 2021
6. Herrytheeagle approved
7. 
   theStack commented at 0:15 am on September 1, 2021: contributor

   Concept ACK
8. 
   glozow commented at 1:54 pm on September 1, 2021: member

   Concept ACK
9. 
   DrahtBot commented at 1:05 am on September 16, 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:

   • #26101 (script: create V1SigVersion for functions which should only accept taproot/tapscript by theuni)
   • #24058 (BIP-322 basic support by kallewoof)
   • #13525 (policy: Report reason inputs are nonstandard from AreInputsStandard by Empact)

   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.

10. DrahtBot added the label Needs rebase on Nov 17, 2021
11. achow101 force-pushed on Nov 18, 2021
12. achow101 force-pushed on Nov 18, 2021
13. DrahtBot removed the label Needs rebase on Nov 18, 2021
14. DrahtBot added the label Needs rebase on Jan 25, 2022
15. achow101 force-pushed on Jan 25, 2022
16. DrahtBot removed the label Needs rebase on Jan 25, 2022
17. maflcko removed the label TX fees and policy on May 6, 2022
18. maflcko removed the label RPC/REST/ZMQ on May 6, 2022
19. maflcko removed the label Consensus on May 6, 2022
20. maflcko removed the label Utils/log/libs on May 6, 2022
21. in src/script/sign.h:46 in 71bf6d5310 outdated

    44+    const PrecomputedTransactionData& m_txdata;
    45 
    46 public:
    47-    MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, int hash_type);
    48-    MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData* txdata, int hash_type);
    49+    MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData& txdata, int hash_type);
    

    ---

    ---

    

    maflcko commented at 9:29 am on May 6, 2022:

    0    MutableTransactionSignatureCreator(const CMutableTransaction* tx, unsigned int input_idx, const CAmount& amount, const PrecomputedTransactionData& txdata LIFETIMEBOUND, int hash_type);
    

    Maybe add LIFETIMEBOUND while touching this line?

    ---

    

    achow101 commented at 3:49 pm on May 16, 2022:

    Done
22. DrahtBot added the label Refactoring on May 6, 2022
23. DrahtBot added the label Needs rebase on May 6, 2022
24. achow101 force-pushed on May 16, 2022
25. DrahtBot removed the label Needs rebase on May 16, 2022
26. DrahtBot added the label Needs rebase on May 18, 2022
27. achow101 force-pushed on May 18, 2022
28. DrahtBot removed the label Needs rebase on May 18, 2022
29. in src/script/interpreter.h:329 in 30d8600e1d outdated

    324+    bool CheckECDSASignature(const std::vector<unsigned char>& scriptSig, const std::vector<unsigned char>& vchPubKey, const CScript& scriptCode, SigVersion sigversion) const override { return true; }
    325+    bool CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey, SigVersion sigversion, ScriptExecutionData& execdata, ScriptError* serror) const override { return true; }
    326+    bool CheckLockTime(const CScriptNum& nLockTime) const override { return false; }
    327+    bool CheckSequence(const CScriptNum& nSequence) const override { return false; }
    328+};
    329+const DummySignatureChecker DUMMY_CHECKER;
    

    ---

    ---

    

    david-bakin commented at 2:26 am on June 6, 2022:

    Add inline keyword to this variable defn in a header? I know this class doesn’t have any data members - in fact all it has is a virtual table pointer. So the fact that it (right now) has internal linkage and is thus duplicated wherever interpreter.h is included doesn’t cost much. At all. But, maybe its better to mark it properly.

    (Builds properly both without and the inline keyword …)

30. 
    david-bakin commented at 3:22 am on June 6, 2022: contributor

    ACK 30d8600 - code reviewed (looks v.g. but left one comment), tested by building (without GUI) and running unit tests only.

    I also merged in #25097 which is open and which mocks BaseSignatureChecker in several places and the merge, though it required changes, was straightforward (and I’m prepared to make it there if this PR lands first).

31. DrahtBot added the label Needs rebase on Aug 22, 2022
32. Make BaseSignatureChecker abstract

    Instead of having BaseSignatureChecker implement default behavior for
    its member functions, make those functions completely virtual and force
    subclasses to implement all functions. The users of BaseSignatureChecker
    are changed to use DummySignatureChecker which have similar
    implementations of those functions.

    449e7a6641
33. Pass PrecomputedTransactionData to DataFromTransaction

    Without a PrecomputedTransactionData, DataFromTransaction will be
    missing data and unable to verify some kinds of transactions, thereby
    making it unable to extract data from the transaction.

    ff9aba9ce2
34. Require PrecomputedTransactionData in SignatureCreator

    MutableTransactionSignatureCreator now requires a
    PrecomputedTransactionData.

    840682a514
35. Require PrecomputedTransactionData in TransactionSignatureChecker

    GenericTransactionSignatureChecker now requires a
    PrecomputedTransactionData.

    1f428783bb
36. achow101 force-pushed on Aug 22, 2022
37. DrahtBot removed the label Needs rebase on Aug 22, 2022
38. achow101 closed this on Oct 12, 2022

    ---

39. bitcoin locked this on Oct 12, 2023

Contributors
achow101 Herrytheeagle theStack glozow DrahtBot maflcko david-bakin

Labels
Refactoring