Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it #21166

Merged
merged 2 commits into from Apr 7, 2021
Merged

Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it #21166

merged 2 commits into from Apr 7, 2021

Conversation

achow101
Copy link
Member

@achow101 achow101 commented Feb 12, 2021 •
edited

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

@achow101 achow101 mentioned this pull request Feb 12, 2021
Closed
maflcko
maflcko reviewed Feb 12, 2021
View reviewed changes
src/script/sign.cpp Outdated Show resolved Hide resolved
@achow101 achow101 changed the title Pass through SignatureExtractorChecker methods to base Have SignatureExtractorChecker subclass MutableTransactionSignatureChecker Feb 12, 2021
@jonatack
Copy link
Contributor

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

@achow101
Copy link
Member Author

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

@DrahtBot DrahtBot added the Tests label Feb 12, 2021
@maflcko maflcko added Wallet and removed Tests labels Feb 15, 2021
@DrahtBot DrahtBot mentioned this pull request Mar 2, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 2, 2021 •
edited

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

Conflicts

No conflicts as of last run.

@DrahtBot DrahtBot mentioned this pull request Mar 4, 2021
Merged
@sipa
Copy link
Member

sipa commented Mar 5, 2021 •
edited

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).

@achow101
Introduce DeferringSignatureChecker and inherit with SignatureExtractor
6965456 
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.
@achow101
Test that signrawtx works when a signed CSV and CLTV inputs are present
a97a929
@achow101
Copy link
Member Author

achow101 commented Mar 5, 2021

Implemented @sipa's suggestion

@sipa
Copy link
Member

sipa commented Mar 6, 2021

utACK a97a929

@maflcko maflcko added this to the 0.21.1 milestone Mar 6, 2021
@maflcko
Copy link
Member

maflcko commented Mar 6, 2021

Marked for backport

@achow101 achow101 changed the title Have SignatureExtractorChecker subclass MutableTransactionSignatureChecker Introduce DeferredSignatureChecker and have SignatureExtractorClass subclass it Mar 12, 2021
meshcollider
meshcollider reviewed Mar 25, 2021
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK a97a929

Confirmed that the test fails on master but passes with 6965456

src/script/interpreter.h
{
return m_checker.CheckLockTime(nLockTime);
}
bool CheckSequence(const CScriptNum& nSequence) const override
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: newline

@fanquake fanquake requested a review from instagibbs April 6, 2021 09:52
instagibbs
instagibbs approved these changes Apr 7, 2021
View reviewed changes
Copy link
Member

@instagibbs instagibbs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK a97a929

@fanquake fanquake merged commit 245a5cd into bitcoin:master Apr 7, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 7, 2021
@fanquake
Merge bitcoin#21166: Introduce DeferredSignatureChecker and have Sign…
976ed65 
…atureExtractorClass subclass it

a97a929 Test that signrawtx works when a signed CSV and CLTV inputs are present (Andrew Chow)
6965456 Introduce DeferringSignatureChecker and inherit with SignatureExtractor (Andrew Chow)

Pull request description:

  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 bitcoin#21151

ACKs for top commit:
  sipa:
    utACK a97a929
  meshcollider:
    Code review ACK a97a929
  instagibbs:
    utACK a97a929

Tree-SHA512: bca784c75c2fc3fcb74e81f4e3ff516699e8debaa2db81e12843abdfe9cf265dac11db8619751cb9b3e9bbe779805d029fabe5f3cbca5e86bfd72de3664b0b94
achow101 added a commit to achow101/bitcoin that referenced this pull request Apr 8, 2021
@achow101
Introduce DeferringSignatureChecker and inherit with SignatureExtractor
7de019b 
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.

Github-Pull: bitcoin#21166
Rebased-From: 6965456
achow101 added a commit to achow101/bitcoin that referenced this pull request Apr 8, 2021
@achow101
Test that signrawtx works when a signed CSV and CLTV inputs are present
f79189c 
Github-Pull: bitcoin#21166
Rebased-From: a97a929
@achow101 achow101 mentioned this pull request Apr 8, 2021
Merged
@achow101
Copy link
Member Author

achow101 commented Apr 8, 2021

Backported in #21640

fanquake added a commit that referenced this pull request Apr 12, 2021
@fanquake
Merge #21640: [0.21] Introduce DeferredSignatureChecker and have Sign…
e358b43 
…atureExtractorClass subclass it

f79189c Test that signrawtx works when a signed CSV and CLTV inputs are present (Andrew Chow)
7de019b Introduce DeferringSignatureChecker and inherit with SignatureExtractor (Andrew Chow)

Pull request description:

  Backport of #21166

ACKs for top commit:
  MarcoFalke:
    checked this is a clean cherry-pick did not review ACK f79189c 🐖
  instagibbs:
    ACK f79189c

Tree-SHA512: 51e945c9b353713423d3886c557066c66a6517d2300523832e5a5471ab91a8943385096d9bf5b46910477cb4c47470431690cf3da09b9f6956fe030f13ddff51
@bitcoin bitcoin deleted a comment from curtcurt871 Jun 12, 2021
@bitcoin bitcoin locked and limited conversation to collaborators Jun 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@meshcollider meshcollider meshcollider left review comments

@maflcko maflcko maflcko left review comments

@instagibbs instagibbs instagibbs approved these changes

Assignees
No one assigned
Labels
Wallet
Projects
None yet
Milestone
0.21.1
Development

Successfully merging this pull request may close these issues.

signrawtransactionwithwallet fails with signed non-wallet inputs with many witness stack elements
8 participants
@achow101 @jonatack @DrahtBot @sipa @maflcko @meshcollider @instagibbs @fanquake