Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it

achow101 commented at 7:21 pm on February 12, 2021: member

Previously SignatureExtractorChecker took a MutableTransactionSignatureChecker and passed through function calls to that. However not all functions were implemented so not everything passed through as it should have. To solve this, SignatureExctractorChecker now implements all of those functions via a new class - DeferredSignatureChecker. DeferredSignatureChecker is introduced to allow for future signature checkers which use another SignatureChecker but need to be able to do somethings outside of just the signature checking.

Fixes #21151

in src/script/sign.cpp:271 in 9194ef68a8 outdated

266@@ -267,6 +267,10 @@ class SignatureExtractorChecker final : public BaseSignatureChecker
267         }
268         return false;
269     }
270+    // These methods just pass through to checker
271+    bool CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey, SigVersion sigversion, const ScriptExecutionData& execdata, ScriptError* serror = nullptr) const { return checker.CheckSchnorrSignature(sig, pubkey, sigversion, execdata, serror); }

MarcoFalke commented at 7:38 pm on February 12, 2021:

0/usr/bin/ccache g++ -m64 -std=c++17 -DHAVE_CONFIG_H -I. -I../src/config   -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 -I. -I./secp256k1/include  -DBOOST_SP_USE_STD_ATOMIC -DBOOST_AC_USE_STD_ATOMIC -isystem /tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include -I./leveldb/include -I./leveldb/helpers/memenv -I./univalue/include -I/tmp/cirrus-ci-build/depends/x86_64-pc-linux-gnu/include/  -DHAVE_BUILD_INFO -D__STDC_FORMAT_MACROS -DPROVIDE_MAIN_FUNCTION  -fstack-reuse=none -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection   -Werror=vla -Werror=switch -Werror=unused-variable -Werror=date-time -Werror=return-type -Werror=sign-compare -Werror=suggest-override   -fPIE -pipe -O2  -fno-extended-identifiers -c -o script/libbitcoin_common_a-sign.o `test -f 'script/sign.cpp' || echo './'`script/sign.cpp
1script/sign.cpp:271:10: error: ‘virtual bool {anonymous}::SignatureExtractorChecker::CheckSchnorrSignature(Span<const unsigned char>, Span<const unsigned char>, SigVersion, const ScriptExecutionData&, ScriptError*) const’ can be marked override [-Werror=suggest-override]
2  271 |     bool CheckSchnorrSignature(Span<const unsigned char> sig, Span<const unsigned char> pubkey, SigVersion sigversion, const ScriptExecutionData& execdata, ScriptError* serror = nullptr) const { return checker.CheckSchnorrSignature(sig, pubkey, sigversion, execdata, serror); }
3      |          ^~~~~~~~~~~~~~~~~~~~~
4cc1plus: some warnings being treated as errors

achow101 commented at 7:40 pm on February 12, 2021:

Changed the approach entirely, shouldn’t be a problem.

achow101 force-pushed on Feb 12, 2021

achow101 renamed this:
~~Pass through SignatureExtractorChecker methods to base~~
Have SignatureExtractorChecker subclass MutableTransactionSignatureChecker
on Feb 12, 2021

jonatack commented at 7:41 pm on February 12, 2021: member

Some test coverage might be good (if this solves the issue).

achow101 force-pushed on Feb 12, 2021

achow101 commented at 9:03 pm on February 12, 2021: member

I’ve added test cases (one CSV, one CLTV) for this.

DrahtBot added the label Tests on Feb 12, 2021

MarcoFalke removed the label Tests on Feb 15, 2021

MarcoFalke added the label Wallet on Feb 15, 2021

DrahtBot commented at 11:31 am on March 2, 2021: member

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

Conflicts

No conflicts as of last run.

sipa commented at 2:04 am on March 5, 2021: member

I think it would be a bit cleaner and more flexible to have a DeferringSignatureChecker or so in script/sign which just forwards all calls to another BaseSignatureChecker it holds a pointer to, and then have the SignatureExtractorChecker extend that, overriding just what it needs (similar to CCoinsViewBacked in coins).

That has the advance of just being able to use the SignatureCreator’s Checker() as object deferred to (which in #21365 is extended to hold PrecomputedTransactionData; doing the same with the approach in this PR would mean passing down and/or reconstructing more).

Introduce DeferringSignatureChecker and inherit with SignatureExtractor

Introduces a DeferringSignatureChecker which simply takes a
BaseSignatureChecker and passes through everything.
SignatureExtractorChecker now subclasses DeferringSignatureChecker. This
allows for all BaseSignatureChecker functions to be implemented for
SignatureExtractorChecker, while allowing for future signature checkers
which opreate similarly to SignatureExtractorChecker.

6965456c10

Test that signrawtx works when a signed CSV and CLTV inputs are present a97a9298ce

achow101 commented at 3:18 am on March 5, 2021: member

Implemented @sipa’s suggestion

achow101 force-pushed on Mar 5, 2021

sipa commented at 3:52 am on March 6, 2021: member

utACK a97a9298cea085858e1a65a5e9b20d7a9e0f7303

MarcoFalke added this to the milestone 0.21.1 on Mar 6, 2021

MarcoFalke added the label Needs backport (0.21) on Mar 6, 2021

MarcoFalke commented at 7:19 am on March 6, 2021: member

Marked for backport

achow101 renamed this:
~~Have SignatureExtractorChecker subclass MutableTransactionSignatureChecker~~
Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it
on Mar 12, 2021

in src/script/interpreter.h:297 in 6965456c10 outdated

292+
293+    bool CheckLockTime(const CScriptNum& nLockTime) const override
294+    {
295+        return m_checker.CheckLockTime(nLockTime);
296+    }
297+    bool CheckSequence(const CScriptNum& nSequence) const override

meshcollider commented at 10:36 pm on March 25, 2021:

style nit: newline

meshcollider commented at 11:36 pm on March 25, 2021: contributor

Code review ACK a97a9298cea085858e1a65a5e9b20d7a9e0f7303

Confirmed that the test fails on master but passes with 6965456c10c9c4025c71c5e24fa5b27b15e5933a

fanquake requested review from instagibbs on Apr 6, 2021

instagibbs approved

instagibbs commented at 3:04 am on April 7, 2021: member

utACK a97a9298cea085858e1a65a5e9b20d7a9e0f7303

fanquake merged this on Apr 7, 2021

fanquake closed this on Apr 7, 2021

sidhujag referenced this in commit 976ed65d4d on Apr 7, 2021

achow101 referenced this in commit 7de019bc61 on Apr 8, 2021

achow101 referenced this in commit f79189ca54 on Apr 8, 2021

achow101 commented at 10:33 pm on April 8, 2021: member

Backported in #21640

fanquake removed the label Needs backport (0.21) on Apr 8, 2021

fanquake referenced this in commit e358b43f7d on Apr 12, 2021

fanquake deleted a comment on Jun 12, 2021

fanquake locked this on Jun 12, 2021

Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it #21166

Conflicts