Abstract out CTransaction-specific signing into SignatureCreator #5208

pull sipa wants to merge 1 commits into bitcoin:master from sipa:abstractsign changing 2 files +90 −40

sipa commented at 6:11 PM on November 4, 2014: member

Uses ideas from @jtimon's #4694/#4695/#4989, to build an abstraction class BaseSignatureCreator that doesn't depend on the data type being signed, and a SignatureCreator specific to CTransaction's. Instead of doing the abstraction at the hash compution level, do it at the signature creation level, which means a slight bit more code duplication, but less code changes.

Full disclosure: this is useful for sidechains-related work.
sipa renamed this:
~~Abstract out Ctransaction-specific signing into SignatureCreator~~
Abstract out CTransaction-specific signing into SignatureCreator
on Nov 4, 2014
sipa force-pushed on Nov 4, 2014
sipa force-pushed on Nov 4, 2014
sipa force-pushed on Nov 4, 2014
theuni commented at 2:07 AM on November 5, 2014: member

Wouldn't this would be more flexible without keystore in the base class? Removing that would make this conducive to a future signing lib as well. Quick hack: https://github.com/theuni/bitcoin/commit/a61f6b103cd00c3540ea9d1d33f9810107713447
sipa commented at 6:16 AM on November 5, 2014: member

@theuni Yes, I actually started off that way, but if you need something that can look up private keys, public keys, and scripts... then you pretty much already have CKeyStore (the abstract base) already again.
theuni commented at 6:23 AM on November 5, 2014: member

@sipa fair enough. I've been debating whether CKeyStore should just drop its app deps and become available to libs. I guess you would argue "yes" on that one. In that case, it's only necessary to bump CKeyStore's CCriticalSection up to CBasicKeyStore, and move CKeyStore to a new file as a stateless base class. If you agree, I'll PR that change.

Edit: Err.. stateless in the sense that it depends on no app globals
sipa commented at 6:25 AM on November 5, 2014: member

@theuni An alternative would be to make BaseSignatureCreator have the methods you suggest, and make CKeyStore derive from it: it effectively is something that is able to create signatures, but in addition, is able to recognize payments to itself.
theuni commented at 6:30 AM on November 5, 2014: member

@sipa Yes, that would make sense. That way SignatureCreator could use a small subset of CKeyStore, and that subset would be available to a lib as well. +1
sipa force-pushed on Nov 8, 2014
sipa commented at 4:08 PM on November 8, 2014: member

@theuni After thinking a bit longer, I think I prefer the solution with simplifying CKeyStore if necessary, as that means the same keystore class could be reused for different types of signing.
theuni commented at 7:51 PM on November 8, 2014: member

@sipa ok. Taking another look, for that we'd also need to rid CKeyStore (the interface class) of CKey usage, since that's not lib safe. Clunky as that sounds at first, it may make a little bit of sense if you consider that a lib's keystore implementation may want to maintain its own backing store and avoid copying data around with getters/setters.
jtimon commented at 11:53 PM on November 9, 2014: contributor

My preference is the solution in which CKeyStore doesn't have any critical section (it's moved up to CBasicKeyStore) and becomes just a pure interface (well, leaving AddKey() and GetPubKey() as the only non-pure virtual methods, so it's still an abstract class rather than an interface) with only CKey, CPubKey, CKeyID, CScript and CScriptID as dependencies. I implemented that prerequisite in #5251.

As said earlier, it would be good to rename Solver() since it is very different from script/standard::Solver.

Bike-shedding a little bit, I would also change the name of the implementation from SignatureCreator to TxSignatureCreator (IMO SignatureChecker shoud have been named TxSignatureChecker as well). and maybe BaseSignatureCreator to just SignatureCreator.
sipa commented at 11:55 PM on November 9, 2014: member

@jtimon 3x agreed.
sipa force-pushed on Nov 16, 2014
sipa commented at 9:09 PM on November 16, 2014: member

Rebased, and addressed @jtimon's two last nits. I haven't incorporated #5251 yet as that isn't sufficiently up to date with master.
sipa commented at 9:21 PM on November 16, 2014: member

@theuni we should get rid of those globals regardless, I suppose.
sipa force-pushed on Feb 5, 2015
sipa commented at 2:13 AM on February 5, 2015: member

Rebased, and made the class names consistent with #5719.
sipa force-pushed on Feb 5, 2015
theuni commented at 3:04 AM on February 5, 2015: member

Concept ACK, looks ok from a quick glance.
jtimon commented at 8:06 PM on February 5, 2015: contributor

You ended up doing TransactionSignatureCreator and TransactionSignatureChecker instead of TxSignatureCreator and TxSignatureChecker. Longer, but too late to bikeshed that. utACK
in src/script/sign.cpp:None in 38f7dc300e outdated
```
  63 |   * unless whichTypeRet is TX_SCRIPTHASH, in which case scriptSigRet is the redemption script.
  64 |   * Returns false if scriptPubKey could not be completely satisfied.
  65 |   */
  66 | -bool Solver(const CKeyStore& keystore, const CScript& scriptPubKey, uint256 hash, int nHashType,
  67 | -                  CScript& scriptSigRet, txnouttype& whichTypeRet)
  68 | +static bool SignStep(const BaseSignatureCreator& creator, const CScript& scriptPubKey,
```
jtimon commented at 8:11 PM on February 5, 2015:

Why this needs to become static? Can't the compiler find out what it's best (static or inline) by itself?

luke-jr commented at 8:13 PM on February 5, 2015:

Compilers don't have enough information to determine if functions can be static. And non-static functions cannot be inlined.

sipa commented at 8:14 PM on February 5, 2015:

The compiler compiles individual .cpp files. At that stage, it's not yet known whether a function will be used by other modules (.cpp), so no, you need to tell it that it's local to the module.

sipa commented at 8:30 PM on February 5, 2015:

@luke-jr Non-static functions can (and are) inlined just fine. They just can't be omitted from the resulting .o file unless they're 1) static 2) inlined in every call site.

in src/script/sign.cpp:None in 38f7dc300e outdated

 110 | @@ -109,21 +111,29 @@ bool SignSignature(const CKeyStore &keystore, const CScript& fromPubKey, CMutabl
 111 |          // Solver returns the subscript that need to be evaluated;

jtimon commented at 12:15 AM on February 6, 2015:

s/Solver/SignStep

jtimon commented at 4:04 AM on February 6, 2015:

and s/need/needs

sipa commented at 8:33 PM on February 16, 2015: member

@laanwj Care to have a look?
laanwj added the label Improvement on Mar 20, 2015
laanwj commented at 3:35 PM on March 20, 2015: member

Huh - this somehow slipped through. Will take a look.
Abstract out Ctransaction-specific signing into TransactionSignatureCreator 18051c7fbd
in src/script/sign.h:None in 38f7dc300e outdated
```
  16 |  struct CMutableTransaction;
  17 |  
  18 | +/** Virtual base class for signature creators. */
  19 | +class BaseSignatureCreator {
  20 | +protected:
  21 | +    const CKeyStore& keystore;
```
laanwj commented at 3:57 PM on March 20, 2015:

Let's follow #5719 and make this (as well as the argument to BaseSignatureCreator) a pointer instead of a reference. This will avoid any possibility of bugs caused by storing a reference to a temporary, implicit conversion.
sipa force-pushed on Mar 21, 2015
sipa commented at 2:09 PM on March 21, 2015: member

Rebased and updated to use a CKeyStore* pointer instead of reference, except for SignSignature, whose signature I did not change.
laanwj commented at 7:31 AM on March 24, 2015: member

utACK
jgarzik commented at 7:45 AM on March 24, 2015: contributor

ut ACK
sipa merged this on Mar 24, 2015
sipa closed this on Mar 24, 2015
sipa referenced this in commit 2afd919f21 on Mar 24, 2015
random-zebra referenced this in commit 2c11e37c91 on May 20, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#4694 Don't hash what you're not going to sign #4695 Strictly hash what is going to be signed #4989 Replace SignatureHash() with class TxSignatureHasher #5251 Refactor: Separate CKeyStore interface from its basic implementation