Make BaseSignatureCreator a pure interface #12803

pull sipa wants to merge 2 commits into bitcoin:master from sipa:201803_puresignaturecreator changing 7 files +59 −72
  1. sipa commented at 8:41 PM on March 27, 2018: member
    • 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.
  2. Empact commented at 9:38 PM on March 27, 2018: member

    utACK 6d4bf33

  3. fanquake added the label Refactoring on Mar 27, 2018
  4. in src/script/sign.cpp:408 in 6d4bf33afe outdated 
     407 | +const DummySignatureChecker dummy_checker;
 408 | +
 409 | +class DummySignatureCreator final : public BaseSignatureCreator {
 410 | +public:
 411 | +    const BaseSignatureChecker& Checker() const override;
 412 | +    bool CreateSig(const SigningProvider& provider, std::vector<unsigned char>& vchSig, const CKeyID& keyid, const CScript& scriptCode, SigVersion sigversion) const override final;
    promag commented at 12:51 PM on March 28, 2018:

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

  5. promag commented at 12:53 PM on March 28, 2018: member

    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'
  6. sipa force-pushed on Mar 30, 2018
  7. sipa force-pushed on Mar 30, 2018
  8. sipa force-pushed on Mar 31, 2018
  9. sipa commented at 9:24 PM on April 1, 2018: member

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

  10. kallewoof approved
  11. kallewoof commented at 6:32 AM on April 2, 2018: member

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

    utACK b56e9cf2b743c1f135606e08ccdaa6bf43b09f62

  12. in src/script/sign.cpp:425 in b56e9cf2b7 outdated 
     443 | -    vchSig[5 + 33] = 32;
 444 | -    vchSig[6 + 33] = 0x01;
 445 | -    vchSig[6 + 33 + 32] = SIGHASH_ALL;
 446 | -    return true;
 447 | -}
 448 | +const BaseSignatureCreator& dummy_signature_creator = DummySignatureCreator();
    ryanofsky commented at 7:09 PM on April 3, 2018:

    In commit "Make DummySignatureCreator a singleton"

    New global variable should probably have g_ prefix.

    sipa commented at 7:49 PM on April 3, 2018:

    Actually, it's a constant.

  13. ryanofsky commented at 7:18 PM on April 3, 2018: member

    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.

  14. sipa commented at 7:27 PM on April 3, 2018: member

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

  15. ryanofsky commented at 7:30 PM on April 3, 2018: member

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

    Sounds good, thanks for explanation.

  16. sipa force-pushed on Apr 3, 2018
  17. sipa commented at 7:58 PM on April 3, 2018: member

    Updated to use LOUD_SCREAMING_NOTATION for global constants.

  18. promag commented at 8:57 PM on April 3, 2018: member

    Updated to use LOUD_SCREAMING_NOTATION for global constants.

    Why? Isn't it a global variable?

  19. ryanofsky commented at 9:16 PM on April 3, 2018: member

    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.

  20. promag commented at 10:05 PM on April 3, 2018: member

    utACK 9ea9521.

    Either notation would seem fine to use in this case.

    Usually I see upper case names as #define macros.

  21. sipa commented at 10:20 PM on April 3, 2018: member

    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.

  22. promag commented at 10:32 PM on April 3, 2018: member

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

  23. promag commented at 10:33 PM on April 3, 2018: member

    @sipa PR description needs to be updated.

  24. sipa commented at 10:42 PM on April 3, 2018: member

    Done!

  25. in src/script/sign.cpp:425 in 9ea9521fc8 outdated 
     443 | -    vchSig[5 + 33] = 32;
 444 | -    vchSig[6 + 33] = 0x01;
 445 | -    vchSig[6 + 33 + 32] = SIGHASH_ALL;
 446 | -    return true;
 447 | -}
 448 | +const BaseSignatureCreator& DUMMY_SIGNATURE_CREATOR = DummySignatureCreator();
    jtimon commented at 11:08 PM on April 3, 2018:

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

    sipa commented at 11:22 PM on April 3, 2018:

    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.

    laanwj commented at 3:04 PM on April 11, 2018:

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

    sipa commented at 4:17 PM on April 11, 2018:

    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.

  26. jtimon commented at 11:19 PM on April 3, 2018: contributor

    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

  27. jtimon approved
  28. kallewoof commented at 8:38 AM on April 4, 2018: member

    re-utACK 9ea9521fc882b93a51e426cb10f04f530dedb813

  29. instagibbs commented at 9:27 PM on April 4, 2018: member

    utACK https://github.com/bitcoin/bitcoin/pull/12803/commits/9ea9521fc882b93a51e426cb10f04f530dedb813

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

  30. Make BaseSignatureCreator a pure interface 190b8d2dcf
  31. Make DummySignatureCreator a singleton be67831210
  32. sipa force-pushed on Apr 10, 2018
  33. laanwj commented at 8:04 PM on April 12, 2018: member

    utACK be678312102ed9bee66738c4721df1343518e3ea

  34. laanwj merged this on Apr 12, 2018
  35. laanwj closed this on Apr 12, 2018
  36. laanwj referenced this in commit 8480d41e0f on Apr 12, 2018
  37. Fabcien referenced this in commit a79f706a34 on Aug 30, 2019
  38. PastaPastaPasta referenced this in commit a833e42919 on Apr 13, 2021
  39. PastaPastaPasta referenced this in commit 71944f5f94 on Apr 17, 2021
  40. kittywhiskers referenced this in commit 2db03fb391 on Apr 23, 2021
  41. UdjinM6 referenced this in commit 2b2a764ccc on Jun 19, 2021
  42. UdjinM6 referenced this in commit a989ffce21 on Jun 24, 2021
  43. UdjinM6 referenced this in commit 4f3694467f on Jun 26, 2021
  44. UdjinM6 referenced this in commit db656852f8 on Jun 26, 2021
  45. UdjinM6 referenced this in commit 99608c8b99 on Jun 28, 2021
  46. MarcoFalke locked this on Sep 8, 2021
Contributors
sipaEmpactpromagkallewoofryanofskyjtimonlaanwjinstagibbs
Labels
Refactoring
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-19 09:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me