Make BaseSignatureCreator a pure interface #12803

sipa · 2018-03-27T20:41:51Z

Removes the m_provider field from BaseSignatureCreator. Instead both a SigningProvider (which provides keys and scripts) and a BaseSignatureCreator (which implements the transaction-specific (or other) signing logic) are passed into and down in ProduceSignature, making the two concepts orthogonal.
Makes BaseSignatureCreator a pure interface without constructor, making it easier to implement new derivations of it (for example for message signing).
As DummySignatureCreator now becomes a stateless object, turn it into a singleton DUMMY_SIGNATURE_CREATOR.

Empact · 2018-03-27T21:38:59Z

utACK 6d4bf33

promag

utACK 6d4bf33. I like 2nd commit as it makes DummySignatureCreator a "private" class.

Travis error seams unrelated:

feature_blocksdir.py failed, Duration: 1 s
stdout:
2018-03-28T00:22:53.604000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_test_runner_20180328_001902/feature_blocksdir_15
2018-03-28T00:22:54.024000Z TestFramework (INFO): Starting with non exiting blocksdir ...
2018-03-28T00:22:54.024000Z TestFramework (ERROR): Unexpected exception caught during testing
Traceback (most recent call last):
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/test_framework/test_framework.py", line 126, in main
    self.run_test()
  File "/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/test/functional/feature_blocksdir.py", line 24, in run_test
    self.assert_start_raises_init_error(0, ["-blocksdir="+self.options.tmpdir+ "/blocksdir"], "Specified blocks director")
AttributeError: 'BlocksdirTest' object has no attribute 'assert_start_raises_init_error'

promag · 2018-03-28T12:51:08Z

src/script/sign.cpp

+class DummySignatureCreator final : public BaseSignatureCreator {
+public:
+    const BaseSignatureChecker& Checker() const override;
+    bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override final;


Remove final since class definition is final? Or add final in DummySignatureChecker::CheckSig above?

sipa · 2018-04-01T21:24:59Z

@kallewoof You may be interested in this for message script signing.

kallewoof

Very nice clean up! I believe this will make things easier to do the signmessage proofs for sure.

utACK b56e9cf2b743c1f135606e08ccdaa6bf43b09f62

ryanofsky

utACK b56e9cf2b743c1f135606e08ccdaa6bf43b09f62. Interface change seems good. I don't understand the reason for introducing a dummy_signature_creator global though. Since the object doesn't have any state, it doesn't seem like a performance optimization, and it doesn't seem like there are other advantages, so it's not clear why you'd introduce a global singleton that wasn't actually needed.

ryanofsky · 2018-04-03T19:09:21Z

src/script/sign.cpp

-    vchSig[6 + 33 + 32] = SIGHASH_ALL;
-    return true;
-}
+const BaseSignatureCreator& dummy_signature_creator = DummySignatureCreator();


In commit "Make DummySignatureCreator a singleton"

New global variable should probably have g_ prefix.

Actually, it's a constant.

sipa · 2018-04-03T19:27:23Z

@ryanofsky My preference for having a singleton is because it reduces the amount of code in header files.

ryanofsky · 2018-04-03T19:30:07Z

My preference for having a singleton is because it reduces the amount of code in header files.

Sounds good, thanks for explanation.

sipa · 2018-04-03T19:58:13Z

Updated to use LOUD_SCREAMING_NOTATION for global constants.

promag · 2018-04-03T20:57:10Z

Updated to use LOUD_SCREAMING_NOTATION for global constants.

Why? Isn't it a global variable?

ryanofsky

utACK 9ea9521fc882b93a51e426cb10f04f530dedb813. Only change since last review is constant names.

Why? Isn't it a global variable?

It is in the sense that a global constant is a type of global variable. Either notation would seem fine to use in this case.

promag · 2018-04-03T22:05:46Z

utACK 9ea9521.

Either notation would seem fine to use in this case.

Usually I see upper case names as #define macros.

sipa · 2018-04-03T22:20:10Z

Usually I see upper case names as #define macros.

From https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md:

Constant names are all uppercase, and use _ to separate words.

promag · 2018-04-03T22:32:54Z

@sipa the first thing I did was looking the developer notes, hence the utACK. I still see that as macros 😄

promag · 2018-04-03T22:33:40Z

@sipa PR description needs to be updated.

sipa · 2018-04-03T22:42:57Z

Done!

jtimon

utACK 37ce177e36063acc11d92fd76e72e2d312452e20
~~Regarding 9ea9521fc882b93a51e426cb10f04f530dedb813 I'm not convinced of the value of making it a singleton, but if others see it, I guess I'm not opposed to it.~~
utACK 9ea9521fc882b93a51e426cb10f04f530dedb813

jtimon · 2018-04-03T23:08:26Z

src/script/sign.cpp

-    vchSig[6 + 33 + 32] = SIGHASH_ALL;
-    return true;
-}
+const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR = DummySignatureCreator();


why not just use a std::unique_ptr ? when is this being destroyed?

That seems unnecessarily complicated. This is just a constant, with no storage associated with it, that exists at the start of the program and is always there.

What is the scope of the object that this reference points to? It's not just this line, I hope?

Initializing a const reference with a temporary extends the lifetime of the temporary to that of the reference. The goal here is avoiding the need to have the DummySignatureCreator type externally visible (the reference type can be different from the dynamic type). An alternative would be to have a static object of type DummySignatureCreator, and then const BaseSignatureCreator& exported referencing it. This approach combines the two into one line.

kallewoof

re-utACK 9ea9521fc882b93a51e426cb10f04f530dedb813

instagibbs · 2018-04-04T21:27:49Z

utACK 9ea9521

Though now I do wish SigningProvider was renamed SigningSupporter to reduce confusion ;)

laanwj · 2018-04-12T20:04:01Z

utACK be67831

be67831 Make DummySignatureCreator a singleton (Pieter Wuille) 190b8d2 Make BaseSignatureCreator a pure interface (Pieter Wuille) Pull request description: * Removes the `m_provider` field from `BaseSignatureCreator`. Instead both a `SigningProvider` (which provides keys and scripts) and a `BaseSignatureCreator` (which implements the transaction-specific (or other) signing logic) are passed into and down in `ProduceSignature`, making the two concepts orthogonal. * Makes `BaseSignatureCreator` a pure interface without constructor, making it easier to implement new derivations of it (for example for message signing). * As `DummySignatureCreator` now becomes a stateless object, turn it into a singleton `DUMMY_SIGNATURE_CREATOR`. Tree-SHA512: 5f1f4512e4ea7d02a31df7b9ede55008efa716c5b74a2630ca1c2fc6599584d8bf5f5641487266127f4b3788033803539fbd22b03ef1219c83c10da2d3da3dcd

Summary: be67831 Make DummySignatureCreator a singleton (Pieter Wuille) 190b8d2 Make BaseSignatureCreator a pure interface (Pieter Wuille) Pull request description: * Removes the `m_provider` field from `BaseSignatureCreator`. Instead both a `SigningProvider` (which provides keys and scripts) and a `BaseSignatureCreator` (which implements the transaction-specific (or other) signing logic) are passed into and down in `ProduceSignature`, making the two concepts orthogonal. * Makes `BaseSignatureCreator` a pure interface without constructor, making it easier to implement new derivations of it (for example for message signing). * As `DummySignatureCreator` now becomes a stateless object, turn it into a singleton `DUMMY_SIGNATURE_CREATOR`. Tree-SHA512: 5f1f4512e4ea7d02a31df7b9ede55008efa716c5b74a2630ca1c2fc6599584d8bf5f5641487266127f4b3788033803539fbd22b03ef1219c83c10da2d3da3dcd Backport of Core PR12803 bitcoin/bitcoin#12803 Depends on D3888 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3889

be67831 Make DummySignatureCreator a singleton (Pieter Wuille) 190b8d2 Make BaseSignatureCreator a pure interface (Pieter Wuille) Pull request description: * Removes the `m_provider` field from `BaseSignatureCreator`. Instead both a `SigningProvider` (which provides keys and scripts) and a `BaseSignatureCreator` (which implements the transaction-specific (or other) signing logic) are passed into and down in `ProduceSignature`, making the two concepts orthogonal. * Makes `BaseSignatureCreator` a pure interface without constructor, making it easier to implement new derivations of it (for example for message signing). * As `DummySignatureCreator` now becomes a stateless object, turn it into a singleton `DUMMY_SIGNATURE_CREATOR`. Tree-SHA512: 5f1f4512e4ea7d02a31df7b9ede55008efa716c5b74a2630ca1c2fc6599584d8bf5f5641487266127f4b3788033803539fbd22b03ef1219c83c10da2d3da3dcd

fanquake added the Refactoring label Mar 27, 2018

promag reviewed Mar 28, 2018

View reviewed changes

sipa force-pushed the 201803_puresignaturecreator branch 3 times, most recently from bd830ef to b56e9cf Compare March 31, 2018 16:40

kallewoof approved these changes Apr 2, 2018

View reviewed changes

ryanofsky reviewed Apr 3, 2018

View reviewed changes

sipa force-pushed the 201803_puresignaturecreator branch from b56e9cf to 9ea9521 Compare April 3, 2018 19:57

ryanofsky reviewed Apr 3, 2018

View reviewed changes

jtimon reviewed Apr 3, 2018

View reviewed changes

jtimon approved these changes Apr 3, 2018

View reviewed changes

kallewoof reviewed Apr 4, 2018

View reviewed changes

sipa added 2 commits April 10, 2018 09:29

Make BaseSignatureCreator a pure interface

190b8d2

Make DummySignatureCreator a singleton

be67831

sipa force-pushed the 201803_puresignaturecreator branch from 9ea9521 to be67831 Compare April 10, 2018 16:35

laanwj merged commit be67831 into bitcoin:master Apr 12, 2018

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Jun 19, 2021

leftovers of bitcoin#12803

2b2a764

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Jun 24, 2021

leftovers of bitcoin#12803

a989ffc

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Jun 26, 2021

leftovers of bitcoin#12803

4f36944

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Jun 26, 2021

leftovers of bitcoin#12803

db65685

UdjinM6 added a commit to UdjinM6/dash that referenced this pull request Jun 28, 2021

leftovers of bitcoin#12803

99608c8

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make BaseSignatureCreator a pure interface #12803

Make BaseSignatureCreator a pure interface #12803

sipa commented Mar 27, 2018 •

edited

Loading

Empact commented Mar 27, 2018

promag left a comment

promag Mar 28, 2018

sipa commented Apr 1, 2018

kallewoof left a comment

ryanofsky left a comment

ryanofsky Apr 3, 2018

sipa Apr 3, 2018

sipa commented Apr 3, 2018

ryanofsky commented Apr 3, 2018 •

edited

Loading

sipa commented Apr 3, 2018

promag commented Apr 3, 2018

ryanofsky left a comment

promag commented Apr 3, 2018

sipa commented Apr 3, 2018

promag commented Apr 3, 2018

promag commented Apr 3, 2018

sipa commented Apr 3, 2018

jtimon left a comment •

edited

Loading

jtimon Apr 3, 2018

sipa Apr 3, 2018

laanwj Apr 11, 2018

sipa Apr 11, 2018 •

edited

Loading

kallewoof left a comment

instagibbs commented Apr 4, 2018

laanwj commented Apr 12, 2018

Make BaseSignatureCreator a pure interface #12803

Make BaseSignatureCreator a pure interface #12803

Conversation

sipa commented Mar 27, 2018 • edited Loading

Empact commented Mar 27, 2018

promag left a comment

Choose a reason for hiding this comment

promag Mar 28, 2018

Choose a reason for hiding this comment

sipa commented Apr 1, 2018

kallewoof left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Apr 3, 2018

Choose a reason for hiding this comment

sipa Apr 3, 2018

Choose a reason for hiding this comment

sipa commented Apr 3, 2018

ryanofsky commented Apr 3, 2018 • edited Loading

sipa commented Apr 3, 2018

promag commented Apr 3, 2018

ryanofsky left a comment

Choose a reason for hiding this comment

promag commented Apr 3, 2018

sipa commented Apr 3, 2018

promag commented Apr 3, 2018

promag commented Apr 3, 2018

sipa commented Apr 3, 2018

jtimon left a comment • edited Loading

Choose a reason for hiding this comment

jtimon Apr 3, 2018

Choose a reason for hiding this comment

sipa Apr 3, 2018

Choose a reason for hiding this comment

laanwj Apr 11, 2018

Choose a reason for hiding this comment

sipa Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

kallewoof left a comment

Choose a reason for hiding this comment

instagibbs commented Apr 4, 2018

laanwj commented Apr 12, 2018

sipa commented Mar 27, 2018 •

edited

Loading

ryanofsky commented Apr 3, 2018 •

edited

Loading

jtimon left a comment •

edited

Loading

sipa Apr 11, 2018 •

edited

Loading