Skip to content

refactor: privatize SignatureExtractorChecker [moveonly] #13266

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
Merged
merged 1 commit into from
Oct 2, 2019
Merged

refactor: privatize SignatureExtractorChecker [moveonly] #13266

merged 1 commit into from
Oct 2, 2019

Conversation

Empact
Copy link
Contributor

@Empact Empact commented May 18, 2018 •
edited
Loading

If we add a CTxIn constructor to SignatureData, then constructing the
SignatureData directly is no more verbose than calling DataFromTransaction,
and grants the caller additional flexibiliy in how to provide the CTxIn.

A simple change to enhance encapsulation.

@promag
Copy link
Contributor

promag commented May 18, 2018

utACK 61f178b.

@Empact Empact force-pushed the data-from-transaction branch from 61f178b to 7815ac1 Compare June 6, 2018 09:23
@Empact
Copy link
Contributor Author

Empact commented Jun 6, 2018

Rebased for #13269

@DrahtBot DrahtBot mentioned this pull request Jun 14, 2018
Merged
@DrahtBot DrahtBot mentioned this pull request Jun 28, 2018
Merged
@Empact Empact force-pushed the data-from-transaction branch from 7815ac1 to 8b680d4 Compare July 6, 2018 04:09
@Empact Empact changed the title refactoring: Inline DataFromTransaction via new SignatureData constructor refactoring: Convert DataFromTransaction to a SignatureData constructor, privatize and drop dead code from helpers Jul 6, 2018
@Empact
Copy link
Contributor Author

Empact commented Jul 6, 2018 •
edited
Loading

This has developed into a bit of a follow-up on #13425. @achow101 would you take a look?

@Empact Empact force-pushed the data-from-transaction branch from 8b680d4 to 1f85582 Compare July 11, 2018 09:34
@Empact Empact mentioned this pull request Jul 11, 2018
Merged
@Empact Empact changed the title refactoring: Convert DataFromTransaction to a SignatureData constructor, privatize and drop dead code from helpers refactoring: Convert DataFromTransaction to a SignatureData constructor, and privatize helpers Jul 11, 2018
sipa
sipa reviewed Jul 14, 2018
View reviewed changes
src/script/sign.h Outdated
@@ -70,6 +70,8 @@ struct SignatureData {

SignatureData() {}
explicit SignatureData(const CScript& script) : scriptSig(script) {}
explicit SignatureData(const CMutableTransaction& tx, unsigned int nIn, const CTxOut& txout);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep SignatureData independent from the data being signed.

@Empact Empact force-pushed the data-from-transaction branch from 1f85582 to ef7457d Compare July 15, 2018 16:35
@Empact Empact changed the title refactoring: Convert DataFromTransaction to a SignatureData constructor, and privatize helpers refactoring: privatize SignatureExtractorChecker Jul 15, 2018
@Empact Empact force-pushed the data-from-transaction branch from ef7457d to 4012fe1 Compare July 15, 2018 21:05
@sipa
Copy link
Member

sipa commented Jul 17, 2018

utACK 4012fe1be85ee00d55951a5bb69bd451159a9ea0

@Empact Empact force-pushed the data-from-transaction branch from 4012fe1 to a0a0df3 Compare July 19, 2018 04:53
@Empact
Copy link
Contributor Author

Empact commented Jul 19, 2018

Rebased for #13557

@DrahtBot DrahtBot mentioned this pull request Aug 27, 2018
Closed
@maflcko
Copy link
Member

maflcko commented Sep 7, 2018

utACK a0a0df3

@maflcko maflcko closed this Sep 7, 2018
@maflcko maflcko reopened this Sep 7, 2018
@DrahtBot DrahtBot mentioned this pull request Sep 7, 2018
Closed
@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 7, 2018 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #13360 ([Policy] Reject SIGHASH_SINGLE with output out of bound by jl2012)

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.

@Empact Empact changed the title refactoring: privatize SignatureExtractorChecker [moveonly] privatize SignatureExtractorChecker Jan 17, 2019
@Empact
Make SignatureExtractorChecker private to its own file
73aaf4e 
It's only used here in DataFromTransaction
@Empact Empact force-pushed the data-from-transaction branch from a0a0df3 to 73aaf4e Compare March 4, 2019 09:19
@Empact
Copy link
Contributor Author

Empact commented Mar 4, 2019

Rebased. git show --color-moved=dimmed_zebra --color-moved-ws=allow-indentation-change eases review.

@maflcko
Copy link
Member

maflcko commented Mar 4, 2019

utACK 73aaf4e

@Empact
Copy link
Contributor Author

Empact commented Jun 5, 2019

This has 3 utACKs, 1 current.

2 conflicting PRs:
#14079: no ACKs
#13360: 1 concept ACK

@laanwj laanwj changed the title [moveonly] privatize SignatureExtractorChecker refactor: privatize SignatureExtractorChecker [moveonly] Oct 2, 2019
@laanwj
Copy link
Member

laanwj commented Oct 2, 2019

ACK 73aaf4e

laanwj added a commit that referenced this pull request Oct 2, 2019
@laanwj
Merge #13266: refactor: privatize SignatureExtractorChecker [moveonly]
752debd 
73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
@laanwj laanwj merged commit 73aaf4e into bitcoin:master Oct 2, 2019
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 2, 2019
@laanwj
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
2eacb4a 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 24, 2020
@Empact @PiRK
Make SignatureExtractorChecker private to its own file
1fd789f 
Summary:
It's only used here in sign.cpp in DataFromTransaction(), so this enhances encapsulation.

This is a backport of Core [[bitcoin/bitcoin#13266 | PR13266]]

Test Plan: `ninja && ninja check`

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D8088
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
eb4aa49 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
1adb390 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 12, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
c549bb7 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 12, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
4ce8dc2 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 12, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
87cb735 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 14, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
f010f43 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 14, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
5d1c945 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 15, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#13266: refactor: privatize SignatureExtractorChecker [m…
8a3389e 
…oveonly]

73aaf4e Make SignatureExtractorChecker private to its own file (Ben Woosley)

Pull request description:

  ~If we add a CTxIn constructor to SignatureData, then constructing the
  SignatureData directly is no more verbose than calling DataFromTransaction,
  and grants the caller additional flexibiliy in how to provide the CTxIn.~

  A simple change to enhance encapsulation.

ACKs for top commit:
  MarcoFalke:
    utACK 73aaf4e
  laanwj:
    ACK 73aaf4e

Tree-SHA512: f7eafbce22b0e9917a8487e88d1f5a1061f2a0959ae1a097cbd9c8ea0d774edfb807da56813cb5fb26f6ca98499a0604a8ff024c198a7c8dc755164de66d972a
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sipa sipa sipa left review comments

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Empact @promag @sipa @maflcko @DrahtBot @laanwj @fanquake