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

kallewoof · 2019-08-19T08:03:23Z

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.

DrahtBot · 2019-08-19T08:15:24Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Implement BIP 340-342 validation (Schnorr/taproot/tapscript) #17977 (Implement BIP 340-342 validation (Schnorr/taproot/tapscript) by sipa)
Make script interpreter independent from storage type CScript #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 · 2019-08-19T08:27:32Z

ACK 02a1db1

Sjors · 2019-08-19T10:33:43Z

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

cvengler · 2019-08-19T16:12:24Z

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

kallewoof · 2019-08-20T03:21:46Z

@emilengler Did you read the PR description?

kallewoof · 2019-08-20T05:49:06Z

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

cvengler · 2019-08-20T14:27:57Z

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

kallewoof · 2019-08-21T02:35:45Z

lol good trolling there

Sjors · 2019-08-21T10:07:26Z

@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 · 2019-08-21T14:07:22Z

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

cvengler · 2019-08-21T14:16:00Z

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

sdaftuar · 2019-08-21T14:58:41Z

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 · 2019-08-22T03:29:58Z

@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):

bitcoin/src/signet.cpp

Lines 31 to 36 in 55d7c4e

    
           bool CheckBlockSolution(const uint256& signet_hash, const std::vector<uint8_t>& signature, const Consensus::Params& params) 
        
           { 
        
               SimpleSignatureChecker bsc(signet_hash); 
        
               CScript solution = CScript(signature.begin(), signature.end()); 
        
               return VerifyScript(solution, g_signet_blockscript, nullptr, MANDATORY_SCRIPT_VERIFY_FLAGS, bsc); 
        
           }

which gets the hash from

bitcoin/src/signet.cpp

Lines 54 to 58 in 55d7c4e

    
           uint256 GetSignetHash(const CBlock& block) 
        
           { 
        
               if (block.vtx.size() == 0) return block.GetHash(); 
        
               return (CHashWriter(SER_DISK, PROTOCOL_VERSION) << block.nVersion << block.hashPrevBlock << BlockSignetMerkleRoot(block) << block.nTime << block.nBits).GetHash(); 
        
           }

and here (#16440):

bitcoin/src/script/proof.h

Lines 226 to 233 in 203464e

    
           void GenerateSingleProof(const SignMessage& challenge, SigningProvider* sp, const uint256& sighash, const CScript& scriptPubKey, const std::string& message) override { 
        
               SimpleSignatureCreator sc(sighash); 
        
               SignatureData sigdata; 
        
               if (!ProduceSignature(*sp, sc, scriptPubKey, sigdata)) { 
        
                   throw signing_error("Failed to produce a signature"); 
        
               } 
        
               m_proof.m_items.emplace_back(sigdata); 
        
           }

which gets the hash from

bitcoin/src/script/proof.cpp

Lines 15 to 18 in 203464e

    
           CHashWriter hw(SER_DISK, 0); 
        
           std::string s = strMessageMagic + message; 
        
           hw << m_scriptpubkey << LimitedString<65536>(s); 
        
           sighash_out = hw.GetHash();

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

sdaftuar

@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.

sdaftuar · 2019-09-10T19:57:09Z

src/script/interpreter.h

@@ -162,6 +162,18 @@ class BaseSignatureChecker
    virtual ~BaseSignatureChecker() {}
 };

+/** A general purpose signature checker. Note that this lacks assumptions about the `sighash`, as the preimage is not known to the signer. */


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

/** 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). */

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

jtimon · 2019-10-23T23:20:05Z

utACK 0bd274f

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:

explicit 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.

theStack

Concept ACK

theStack · 2020-01-24T15:12:09Z

src/script/interpreter.h

+
+public:
+    const uint256& GetHash() const { return hash; }
+    explicit SimpleSignatureChecker(const uint256& hash_in) : hash(hash_in) {}


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

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

src/script/sign.h

src/test/simplesigner_tests.cpp

instagibbs · 2020-04-08T13:31:56Z

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

kallewoof · 2020-07-31T03:00:08Z

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

kallewoof force-pushed the 2019-08-genpursigs branch 2 times, most recently from 2098ca9 to 02a1db1 Compare August 19, 2019 08:04

laanwj added the Tests label Aug 19, 2019

laanwj added the Validation label Aug 19, 2019

maflcko removed the Tests label Aug 19, 2019

kallewoof force-pushed the 2019-08-genpursigs branch 2 times, most recently from cb50c61 to a2b4370 Compare August 20, 2019 05:52

kallewoof mentioned this pull request Aug 21, 2019

BIP-322: Generic signed message format #16440

Closed

kallewoof force-pushed the 2019-08-genpursigs branch from a2b4370 to 9b910a1 Compare August 22, 2019 16:26

sdaftuar reviewed Sep 10, 2019

View reviewed changes

kallewoof force-pushed the 2019-08-genpursigs branch from 9b910a1 to 0bd274f Compare September 24, 2019 08:16

ch4ot1c mentioned this pull request Oct 17, 2019

doc: Update list of valid PR areas #17172

Closed

kallewoof force-pushed the 2019-08-genpursigs branch 2 times, most recently from 7704285 to 7ba973a Compare January 15, 2020 08:08

theStack reviewed Jan 24, 2020

View reviewed changes

kallewoof force-pushed the 2019-08-genpursigs branch from 7ba973a to b872f8c Compare January 25, 2020 02:00

DrahtBot mentioned this pull request Mar 5, 2020

BIP-325: Signet [consensus] #18267

Merged

DrahtBot added the Needs rebase label Mar 13, 2020

kallewoof force-pushed the 2019-08-genpursigs branch from b872f8c to f808d72 Compare March 24, 2020 04:06

DrahtBot removed the Needs rebase label Mar 24, 2020

DrahtBot mentioned this pull request Mar 24, 2020

Make VerifyWitnessProgram use a Span stack #18388

Merged

DrahtBot added the Needs rebase label Mar 27, 2020

kallewoof added 2 commits March 30, 2020 17:01

add simple signature support (checker)

14860a4

script: add simple signature support (creator)

da3d2a7

kallewoof force-pushed the 2019-08-genpursigs branch from f808d72 to 58bc964 Compare March 30, 2020 08:02

DrahtBot removed the Needs rebase label Mar 30, 2020

kallewoof force-pushed the 2019-08-genpursigs branch from 58bc964 to ffe9624 Compare March 31, 2020 02:46

test: add simple signer/checker tests

cdcb377

kallewoof force-pushed the 2019-08-genpursigs branch from ffe9624 to cdcb377 Compare April 1, 2020 00:17

kallewoof closed this Jul 31, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

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

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

Conversation

kallewoof commented Aug 19, 2019

Uh oh!

DrahtBot commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

NicolasDorier commented Aug 19, 2019

Uh oh!

Sjors commented Aug 19, 2019

Uh oh!

cvengler commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kallewoof commented Aug 20, 2019

Uh oh!

kallewoof commented Aug 20, 2019

Uh oh!

cvengler commented Aug 20, 2019

Uh oh!

kallewoof commented Aug 21, 2019

Uh oh!

Sjors commented Aug 21, 2019

Uh oh!

NicolasDorier commented Aug 21, 2019

Uh oh!

cvengler commented Aug 21, 2019

Uh oh!

sdaftuar commented Aug 21, 2019

Uh oh!

kallewoof commented Aug 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdaftuar left a comment

Choose a reason for hiding this comment

Uh oh!

sdaftuar Sep 10, 2019

Choose a reason for hiding this comment

Uh oh!

kallewoof Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 23, 2019

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

theStack Jan 24, 2020

Choose a reason for hiding this comment

Uh oh!

kallewoof Jan 25, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Apr 8, 2020

Uh oh!

kallewoof commented Jul 31, 2020

Uh oh!

Uh oh!

DrahtBot commented Aug 19, 2019 •

edited

Loading

cvengler commented Aug 19, 2019 •

edited

Loading

kallewoof commented Aug 22, 2019 •

edited

Loading