Generic TransactionSignatureCreator works with both CTransaction and CMutableTransaction
ClosedPublic
Actions

Authored by deadalnix on Oct 2 2019, 16:04.

Details

Reviewers

jasonbcox
Fabien

Group Reviewers

Restricted Project

Commits

rSTAGING8f425f2d3013: Generic TransactionSignatureCreator works with both CTransaction and…
rABC8f425f2d3013: Generic TransactionSignatureCreator works with both CTransaction and…

Summary

Templated version so that no copying of CMutableTransaction into a CTransaction is
necessary. This speeds up the test case transaction_tests/test_big_witness_transaction
from 7.9 seconds to 3.1 seconds on my machine.

This is a backport of Core PR13309

Test Plan

make check
./src/bench/bench_bitcoin

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr13309

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 7668
Build 13375: Bitcoin ABC Buildbot (legacy)
Build 13374: arc lint + arc unit

Event Timeline

deadalnix created this revision.Oct 2 2019, 16:04

Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 2 2019, 16:04

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B7668: Diff 13322.Oct 2 2019, 16:20

deadalnix added a child revision: D4203: Drop UpdateTransaction in favor of UpdateInput.Oct 2 2019, 22:08

jasonbcox requested changes to this revision.Oct 3 2019, 00:39

jasonbcox added a subscriber: jasonbcox.

jasonbcox added inline comments.

src/script/interpreter.cpp
1485	reference

This revision now requires changes to proceed.Oct 3 2019, 00:39

deadalnix requested review of this revision.Oct 3 2019, 11:54

deadalnix added inline comments.

src/script/interpreter.cpp
1485	Can you point me to the place in the original PR where this was changed from a value to a reference, or maybe to the newly added code that require this to be passed as a reference?

I'm sure you didn't run the test from the summary. Please make it clear that you are quoting core PR description.

src/script/sign.cpp
18	Any reason for passing `amountIn` by reference here ?

This revision now requires changes to proceed.Oct 3 2019, 19:59

deadalnix requested review of this revision.Oct 3 2019, 22:11

deadalnix added inline comments.

src/script/sign.cpp
18	It just makes the whole CheckSig business easier to backport. It doesn't change much in practice as this is const anyways and Amount is 64 bits.

jasonbcox added inline comments.Oct 3 2019, 23:28

src/script/interpreter.cpp
1485	It seemed like a good idea to follow the same pattern used in the rest of this diff (see one example marked 'here') and to bring it up-to-date with Core: https://github.com/bitcoin/bitcoin/pull/13309/files#diff-be2905e2f5218ecdbe4e55637dac75f3R1234
src/script/interpreter.h
67	here

deadalnix added inline comments.Oct 4 2019, 12:26

src/script/interpreter.cpp
1485	This must surely have tremendous value as you judged it was worth blocking 5 patches for something completely unrelated to what they are doing and that do not affect in anyway the way the code is behaving. Yet I still fail to see it.