script: add simple signature support (checker/creator)

kallewoof commented at 8:03 am on August 19, 2019: member

The simple signature takes a sighash as argument and verifies a signature and pubkey against it.

It is used in signet (#16411) for verifying blocks and in BIP-322 implementation (#16440) for verifying messages.

kallewoof force-pushed on Aug 19, 2019

DrahtBot commented at 8:15 am on August 19, 2019: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17977 (Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
#13062 (Make script interpreter independent from storage type CScript by sipa)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

NicolasDorier commented at 8:27 am on August 19, 2019: contributor

ACK 02a1db1607ec59b6c54072cd3ffb606422eea3a4

laanwj added the label Tests on Aug 19, 2019

Sjors commented at 10:33 am on August 19, 2019: member

Concept ACK. It strictly adds new functions so that seems a pretty safe approach. This PR needs tests though.

laanwj added the label Validation on Aug 19, 2019

MarcoFalke removed the label Tests on Aug 19, 2019

emilengler commented at 4:12 pm on August 19, 2019: contributor

Concept NACK until it is being used actively. I don’t think unused code is good, it just makes the binary bigger

kallewoof commented at 3:21 am on August 20, 2019: member

@emilengler Did you read the PR description?

kallewoof commented at 5:49 am on August 20, 2019: member

@Sjors Added tests. The multisig ones are based on the multisig_tests.cpp file, but adapted to use the new creators/checkers.

kallewoof force-pushed on Aug 20, 2019

emilengler commented at 2:27 pm on August 20, 2019: contributor

@kallewoof I did but until nothing is merged I don’t think this should be added. I prefer a chronological order.

kallewoof commented at 2:35 am on August 21, 2019: member

lol good trolling there

Sjors commented at 10:07 am on August 21, 2019: member

@kallewoof thanks for adding the tests. They pass for me on macOS. I haven’t followed the BIP-322 discussion in great detail, so can’t really comment on the cryptography itself or how likely that is to change. It would be nice if it remained consistent with signet. @emilengler when you built the binary, how many bytes were added? Even if this difference is non-trivial, it only matters if for some reason this PR makes it into the the 0.19 release and neither of the two followup PRs make the cut-off. This PR seems well worth the reduced review burden and the improved coordination between both projects.

NicolasDorier commented at 2:07 pm on August 21, 2019: contributor

On my side I am fine both about either having it in this PR or on the same PR as signet.

emilengler commented at 2:16 pm on August 21, 2019: contributor

@Sjors Sure thing but why isn’t this being added to the original PR? @kallewoof

sdaftuar commented at 2:58 pm on August 21, 2019: member

This interface, where the caller provides the hash of a message, and then “verifies” that the signature is valid, seems problematic to me at this layer of our code – my understanding is that without knowing the preimage of the hash (ie the message being signed), this doesn’t prove that the signature could only have come from someone with knowledge of the private key.

I know at some layer of our cryptography code we will have verification methods like this, but at this layer it seems inconsistent with all our other signature checkers, which are operating under different assumptions (well, except maybe for the DummySignatureChecker which does not do signature verifications, but hopefully that is clear to any callers). So my initial thought is that this is not a good place to have what is effectively a lower-level API call to a crypto library function.

I’m not very familiar with the broader work this is part of, so if this is in fact necessary or expedient from an implementation perspective, then I’d say at a minimum that we should at least include comments explaining that the assumptions here are different than for the other signature checkers.

kallewoof commented at 3:29 am on August 22, 2019: member

@sdaftuar

Maybe I’m misunderstanding, but I’m not sure if it’s that inconsistent in the end. The “CreateSig” methods right now

Get the private key from the signing provider.
Generate the sighash based on internal data.
Sign the sighash using the aforementioned private key.

This is consistent across all of these, and in fact, the other implementations could become sub-classes of this one (perform step 2 into internal sighash then have master do the rest; edit: I actually tried to do this, but it turns out a bit messy as the checker has to be re-constructed for every CreateSig call).

To pass the preimage rather than the hash itself would only lock this down to using some specific type of hasher, and would have no practical meaning for the actual implementation. The two (current) uses of these are here (#16411):

https://github.com/bitcoin/bitcoin/blob/55d7c4ef3ccf9ad84d62592969de35c4ab84af2c/src/signet.cpp#L31-L36

which gets the hash from

https://github.com/bitcoin/bitcoin/blob/55d7c4ef3ccf9ad84d62592969de35c4ab84af2c/src/signet.cpp#L54-L58

and here (#16440):

https://github.com/bitcoin/bitcoin/blob/203464efb4302d4c21bf2bc1503487f116c8837e/src/script/proof.h#L226-L233

which gets the hash from

https://github.com/bitcoin/bitcoin/blob/203464efb4302d4c21bf2bc1503487f116c8837e/src/script/proof.cpp#L15-L18

I made a note that these lack some assumptions that are normally present about the validity of the signature.

kallewoof force-pushed on Aug 22, 2019

in src/script/interpreter.h:165 in 9b910a1a42 outdated

161@@ -162,6 +162,18 @@ class BaseSignatureChecker
162     virtual ~BaseSignatureChecker() {}
163 };
164 
165+/** A general purpose signature checker. Note that this lacks assumptions about the `sighash`, as the preimage is not known to the signer. */

sdaftuar commented at 7:57 pm on September 10, 2019:

This should emphasize that the issue about not knowing the hash preimage is for the verifier, not the signer. Perhaps:

0/** A low level signature checker.  Note that the caller must verify that the hash passed in relates to a known message (unlike for the other signature checkers). */

kallewoof commented at 8:14 am on September 24, 2019:

Sorry for delay. Yes, that looks great, thanks. Adopting.

sdaftuar commented at 7:58 pm on September 10, 2019: member

@kallewoof Thanks for providing that context, now I understand a bit better why this is being implemented in this way. My only suggestion is that we improve the comments to explain the implications of this for verification.

kallewoof force-pushed on Sep 24, 2019

jtimon commented at 11:20 pm on October 23, 2019: contributor

utACK 0bd274fe8544c896616268a21edd06b2d927fc0f

An alternative that perhaps @sdaftuar would like more given his reasonings, is having a templated class that will work for any object implementing a GetHash() method. Something like:

0explicit SimpleSignatureChecker(const T& hashable) : hash(hashable.GetHash()) {}

But that would require a wrapper class for signet and another one for the signed messages, I think. And if a comment solves the concerns, that’s certainly easier. Just thinking out loud.

kallewoof force-pushed on Jan 15, 2020

in src/script/interpreter.h:172 in 0a202e0b26 outdated

167+private:
168+    uint256 hash;
169+
170+public:
171+    const uint256& GetHash() const { return hash; }
172+    explicit SimpleSignatureChecker(const uint256& hash_in) : hash(hash_in) {}

theStack commented at 3:12 pm on January 24, 2020:

nit: Move constructor one line up, immediately after public (like for the other class SimpleSignatureCreator and for most classes in general I’d say)

kallewoof commented at 1:47 am on January 25, 2020:

I think it’s fine as is, and there are plenty of examples of the constructor not being right below public:.

in src/script/sign.h:41 in 0a202e0b26 outdated

35+class SimpleSignatureCreator : public BaseSignatureCreator
36+{
37+    SimpleSignatureChecker checker;
38+
39+public:
40+    explicit SimpleSignatureCreator(const uint256& hashIn) : BaseSignatureCreator(), checker(hashIn) {};

theStack commented at 3:13 pm on January 24, 2020:

Is there any reason why the constructor of this class calls its base class constructor BaseSignatureCreator(), while the constructor of SimpleSignatureChecker() doesn’t?

kallewoof commented at 1:45 am on January 25, 2020:

Nope. Shouldn’t change anything in practice, but consistency is nice. Unifying.

in src/test/simplesigner_tests.cpp:21 in 7ba973ac3f outdated

16+
17+BOOST_FIXTURE_TEST_SUITE(simplesigner_tests, BasicTestingSetup)
18+
19+BOOST_AUTO_TEST_CASE(simplesigner_base_tests)
20+{
21+    uint256 message;

theStack commented at 3:41 pm on January 24, 2020:

Is it intended that here (and in all following testcases) message is (implicitely) set to all-zero?

kallewoof commented at 1:48 am on January 25, 2020:

It shouldn’t really matter what the message is, but then there’s that saying about bridges, so I’ll set it to something, sure.

theStack commented at 3:51 pm on January 24, 2020: member

Concept ACK

kallewoof force-pushed on Jan 25, 2020

DrahtBot added the label Needs rebase on Mar 13, 2020

kallewoof force-pushed on Mar 24, 2020

DrahtBot removed the label Needs rebase on Mar 24, 2020

DrahtBot added the label Needs rebase on Mar 27, 2020

add simple signature support (checker) 14860a4e80

script: add simple signature support (creator) da3d2a7b19

kallewoof force-pushed on Mar 30, 2020

DrahtBot removed the label Needs rebase on Mar 30, 2020

kallewoof force-pushed on Mar 31, 2020

test: add simple signer/checker tests cdcb3776bf

kallewoof force-pushed on Apr 1, 2020

instagibbs commented at 1:31 pm on April 8, 2020: member

concept and approach ACK. It’s ok sometimes to have really small tooling to support not-yet-merged uses, provided there are tests

kallewoof commented at 3:00 am on July 31, 2020: member

Closing this as it’s no longer certain whether it will be used, now that BIP-322 switched to a tx based version.

kallewoof closed this on Jul 31, 2020

DrahtBot locked this on Feb 15, 2022

script: add simple signature support (checker/creator) #16653

Conflicts