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

Lint

Lint Not Applicable

Unit

Tests Not Applicable

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 ↗	(On Diff #13322)	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 ↗	(On Diff #13322)	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 ↗	(On Diff #13322)	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 ↗	(On Diff #13322)	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 ↗	(On Diff #13322)	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 ↗	(On Diff #13322)	here

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

src/script/interpreter.cpp
1485 ↗	(On Diff #13322)	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.