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.

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

Reduce unnecessary hashing in signrawtransaction #8118

Merged
merged 1 commit into from Jun 7, 2016

Conversation

Projects
None yet
5 participants
@jonasnick
Contributor

jonasnick commented May 30, 2016

When calling CombineSignatures and VerifyScript inside signrawtransaction with a CMutableTransaction, the tx is converted into a CTransaction which requires hashing.
Because both CombineSignatures and VerifyScript accept the scriptSig created by SignSignature separately from the transaction we can instead convert the mutable tx to CTransaction once and use that one.

Results:
1000 inputs, 75kB before signing: 2.86s vs. 4.88s
Signature concatenation of three 250kB transactions with 1000 inputs: 8.638s vs. 19.142s

There still remains some unnecessary hashing, but fixing this requires a larger refactor: SignSignature requires a CMutableTransaction because it changes the scriptSig in place. But it also immediately creates a CTransaction (costly) for a TransactionSignatureChecker. Using a MutableTransactionSignatureChecker is not an option because it immediately converts the mutable transaction to a CTransaction. Instead, the TransactionSignatureChecker should be able to deal with CMutableTransaction without rehashing.

@laanwj laanwj added the RPC/REST/ZMQ label May 30, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 30, 2016

Member

utACK bd0f413

Member

sipa commented May 30, 2016

utACK bd0f413

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli
Member

jonasschnelli commented Jun 2, 2016

utACK bd0f413

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 2, 2016

Contributor

utACK bd0f413

Contributor

dcousens commented Jun 2, 2016

utACK bd0f413

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jun 2, 2016

Contributor

Probably worth noting, from my searches, it appears that MutableTransactionSignatureChecker is [now] only used by bitcoin-tx...

Contributor

dcousens commented Jun 2, 2016

Probably worth noting, from my searches, it appears that MutableTransactionSignatureChecker is [now] only used by bitcoin-tx...

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jun 7, 2016

Member

utACK bd0f413

Probably worth noting, from my searches, it appears that MutableTransactionSignatureChecker is [now] only used by bitcoin-tx...

Good observation, I suppose it could be moved, or maybe the use there could be avoided too and then removed.

Member

laanwj commented Jun 7, 2016

utACK bd0f413

Probably worth noting, from my searches, it appears that MutableTransactionSignatureChecker is [now] only used by bitcoin-tx...

Good observation, I suppose it could be moved, or maybe the use there could be avoided too and then removed.

@laanwj laanwj merged commit bd0f413 into bitcoin:master Jun 7, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jun 7, 2016

Merge #8118: Reduce unnecessary hashing in signrawtransaction
bd0f413 Reduce unnecessary hashing in signrawtransaction (Jonas Nick)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #8118: Reduce unnecessary hashing in signrawtransaction
bd0f413 Reduce unnecessary hashing in signrawtransaction (Jonas Nick)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #8118: Reduce unnecessary hashing in signrawtransaction
bd0f413 Reduce unnecessary hashing in signrawtransaction (Jonas Nick)

codablock added a commit to codablock/dash that referenced this pull request Dec 22, 2017

Merge #8118: Reduce unnecessary hashing in signrawtransaction
bd0f413 Reduce unnecessary hashing in signrawtransaction (Jonas Nick)

@str4d str4d referenced this pull request Jan 25, 2018

Merged

Overwinter SignatureHash #2903

zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of  #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 8, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of  #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 19, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements zcash/zips#129.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#6915
  - Only the rework of `mempool.check()` calls that the next PR depends on.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2018

Auto merge of #2903 - str4d:1408-sighash, r=<try>
Overwinter SignatureHash

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.

zkbot added a commit to zcash/zcash that referenced this pull request Feb 20, 2018

Auto merge of #2903 - str4d:1408-sighash, r=str4d
Overwinter SignatureHash

Implements ZIP 143.

Includes code cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7276
- bitcoin/bitcoin#7976
- bitcoin/bitcoin#8118
- bitcoin/bitcoin#8149
  - Only amount validation and SignatureHash commits.
- bitcoin/bitcoin#8346
- bitcoin/bitcoin#8524

Part of #2074 and #2254. Closes #1408 and #2584.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment