Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors #22793
Conversation
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
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. |
achow101
force-pushed
the
refactor-sig-checker
branch
2 times, most recently
from
cdeb66b to
2aefe91
Compare
achow101
force-pushed
the
refactor-sig-checker
branch
from
2aefe91 to
71bf6d5
Compare
achow101
force-pushed
the
refactor-sig-checker
branch
from
71bf6d5 to
be4aab8
Compare
achow101
force-pushed
the
refactor-sig-checker
branch
from
be4aab8 to
30d8600
Compare
src/script/interpreter.h
Outdated
There was a problem hiding this comment.
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 ...)
|
ACK I also merged in #25097 which is open and which mocks |
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.
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.
MutableTransactionSignatureCreator now requires a PrecomputedTransactionData.
GenericTransactionSignatureChecker now requires a PrecomputedTransactionData.
achow101
force-pushed
the
refactor-sig-checker
branch
from
30d8600 to
1f42878
Compare
7 participants
BaseSignatureCheckerhas 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 aBaseSignatureCheckerwas being created; these instances have been replaced with theDUMMY_CHECKER.GenericTransactionSignatureCheckerhas two constructors, one which takes aPrecomputedTransactionDataand one which does not. It can be difficult to understand which one to use, and whether a particularGenericTransactionSignatureCheckerhas aPrecomputedTransactionData. This has lead to some bugs as well (e.g. #22784). Looking through the codebase, the only place wherePrecomputedTransactionDatadoes not need to be provided is in the unit tests. As such, this PR changesGenericTransactionSignatureCheckerto have only one constructor which requiresPrecomputedTransactionDatato be provided. This has a side effect of requiring the same forMutableTransactionSignatureCreator, which also had two constructors, and the one that did not requirePrecomputedTransactionDatawas only used in tests.