Page MenuHomePhabricator

Generic TransactionSignatureCreator works with both CTransaction and CMutableTransaction
ClosedPublic

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

Details

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
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

deadalnix created this revision.Wed, Oct 2, 16:04
Herald added a reviewer: Restricted Project. · View Herald TranscriptWed, Oct 2, 16:04
jasonbcox requested changes to this revision.Thu, Oct 3, 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.Thu, Oct 3, 00:39
deadalnix requested review of this revision.Thu, Oct 3, 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?

Fabien requested changes to this revision.Thu, Oct 3, 19:59
Fabien added a subscriber: Fabien.

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.Thu, Oct 3, 19:59
deadalnix requested review of this revision.Thu, Oct 3, 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.Thu, Oct 3, 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.Fri, Oct 4, 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.

Fabien accepted this revision.Fri, Oct 4, 15:47
jasonbcox accepted this revision.Fri, Oct 4, 16:21
This revision is now accepted and ready to land.Fri, Oct 4, 16:21
  1. Reduced code ownership.
  2. Reduced cognitive load on merge conflicts.

But it's not worth my time to try and convince you over this. You would have red'd the diff if this was done by anyone else on the team.