Make merkle signer SNARK friendly #3263

id-ms · 2021-12-01T10:27:18Z

Summary

In this PR we change how the Merkle key store is being hashed.

The index of the merkle leaf is now part of the signature
we no longer hash a msgpack representation of the MSS's structs (use of an arbitrary hash input is bad for the SNARK verifier).
integrate an updated falcon lib which uses two formats of signature
when committing on falcon signature we commit on the CT format

Test Plan

codecov-commenter · 2021-12-01T11:36:26Z

Codecov Report

Merging #3263 (70f0dd2) into feature/dilithium-scheme-integration (ca330c6) will decrease coverage by 0.00%.
The diff coverage is 62.27%.

@@                           Coverage Diff                            @@
##           feature/dilithium-scheme-integration    #3263      +/-   ##
========================================================================
- Coverage                                 47.84%   47.83%   -0.01%     
========================================================================
  Files                                       381      382       +1     
  Lines                                     61651    61643       -8     
========================================================================
- Hits                                      29496    29489       -7     
+ Misses                                    28734    28732       -2     
- Partials                                   3421     3422       +1

Impacted Files	Coverage Δ
crypto/invalidsigner.go	`55.55% <0.00%> (-15.88%)`	⬇️
crypto/merklearray/merkle.go	`87.61% <ø> (ø)`
crypto/msgp_gen.go	`36.22% <ø> (ø)`
crypto/sig_abstractions.go	`80.95% <ø> (ø)`
data/basics/ccertpart.go	`0.00% <0.00%> (ø)`
ledger/ledgercore/votersForRound.go	`0.00% <0.00%> (ø)`
crypto/merklekeystore/keystore.go	`59.52% <36.00%> (-15.16%)`	⬇️
crypto/compactcert/builder.go	`65.51% <50.00%> (+0.68%)`	⬆️
crypto/merklekeystore/msgp_gen.go	`51.96% <56.52%> (+0.47%)`	⬆️
crypto/compactcert/verifier.go	`65.00% <60.00%> (-4.24%)`	⬇️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca330c6...70f0dd2. Read the comment docs.

crypto/compactcert/builder_test.go

algonautshant · 2021-12-01T19:23:46Z

crypto/compactcert/builder_test.go

@@ -36,8 +37,7 @@ import (

 type TestMessage string

-// TODO: change to CurrentVersion when updated
-var CompactCertRounds = config.Consensus[protocol.ConsensusFuture].CompactCertRounds
+const CompactCertRoundsForTests = 128

 func (m TestMessage) ToBeHashed() (protocol.HashID, []byte) {


TestMessage and it's functions do not need to be exported. Use small case for the first letter.

I've made TestMessage private as you've recommended.
However, ToBeHash is an interface implmenation function so it can't be chagned.

algonautshant · 2021-12-02T13:30:30Z

crypto/compactcert/structs.go

+
+	sigBytes := ssc.Sig.Signature.GetSerializedSignature()
+
+	sigSlotCommitment := make([]byte, 0, len(binaryLValue)+len(sigBytes))


The intend here is to get a fixed sized byte array. However, this function makes no compiler enforced commitment to a fixed size array.
It very difficult to know if GetSerializedSignature will indeed return a fixed size array every time.

algonautshant · 2021-12-02T13:38:12Z

crypto/sig_abstractions.go

+	// GetVerificationBytes returns a serialized version of the public key data (without the use of the msgpack).
+	GetVerificationBytes() []byte
+	// GetSerializedSignature returns a serialized version of the signature
+	GetSerializedSignature(signature ByteSignature) []byte


The problem with implementing this function as an interface is that the caller of GetSerializedSignature will not know what size array it will be receiving.

In some cases, it is important to receive a fixed size array every time. This interface will not permit the compiler to to that enforcement, and in the future someone might change a function implementation which breaks that requirement.

algonautshant · 2021-12-02T13:41:59Z

data/basics/ccertpart.go

 }

 // ToBeHashed implements the crypto.Hashable interface.
+// In order to create a more SNARK-friendly commitments on the signature we must avoid using the msgpack infrastructure.


Why this change for all uses of ToBeHashed?
Shouldn't this less efficient implementation be restricted only when the target is an SNARK commitment?
If this function is only used in the SNARK context, maybe the name should indicate that this is different, and may not be as efficient to transport as the msgp serialization.

Agree; see also my comment above about the naming of GetSerializedSignature. The purpose of that function is much narrower than general serialization.

…blic keys.

algonautshant

Looks great!
I have some comments, mainly clarifications/comment requests.
Also, I added some comments in #2990 which could be addressed here, since the changes here may be impacted by those.

crypto/falconWrapper.go

crypto/falconWrapper_test.go

algonautshant · 2022-01-07T01:02:17Z

crypto/merklekeystore/keystore.go


 	// Signature is a byte signature on a crypto.Hashable object,
 	// crypto.GenericVerifyingKey and includes a merkle proof for the key.
 	Signature struct {
 		_struct              struct{} `codec:",omitempty,omitemptyarray"`
 		crypto.ByteSignature `codec:"bsig"`

-		Proof        Proof                      `codec:"prf"`
-		VerifyingKey crypto.GenericVerifyingKey `codec:"vkey"`
+		MerkleArrayIndex uint64                     `codec:"idx"`


Can you please update the comment above (line 30)?
Will be nice to improve the comment by adding some more explanation on Proof, VerifyingKey and MerkleArrayIndex.

agree. let me know if it looks better now.

crypto/merklekeystore/keystore.go

algonautshant · 2022-01-08T05:56:58Z

crypto/merklekeystore/keystore.go

 	)
 	if err != nil {
 		return err
 	}

-	return sig.VerifyingKey.GetVerifier().Verify(obj, sig.ByteSignature)
+	return sig.VerifyingKey.GetVerifier().Verify(msg, sig.ByteSignature)


In this location, GetVerifier can it return Ed25519Type or it always need to be FalconPublicKey?

the GetVerifier could use ed25519 crypto type. this module is built to support it.

algonautshant · 2022-01-08T05:58:11Z

crypto/merklekeystore/keystore.go

 	)
 	if err != nil {
 		return err
 	}

-	return sig.VerifyingKey.GetVerifier().Verify(obj, sig.ByteSignature)
+	return sig.VerifyingKey.GetVerifier().Verify(msg, sig.ByteSignature)


Why the Falcon verification is not made only for the root of the merkle tree?

The falcon verification is needed to verify that a given signature is valid under this ephemeral pk.

crypto/compactcert/verifier.go

crypto/falconWrapper.go

cpeikert · 2022-01-10T21:38:45Z

crypto/falconWrapper.go

+	return d.PublicKey[:]
+}
+
+// GetSerializedSignature returns a serialized version of the signature


The name and documentation do not really match what this function does. It simply converts a byte array from a COMPRESSED-format signature to a CT-format signature; this is orthogonal to serialization. So I would suggest a name like ConvertCompressedToCT. Also, a FalconVerifier is not needed here, and it would be best not to require one.

crypto/merklekeystore/keystore.go

cpeikert · 2022-01-10T21:51:21Z

crypto/merklekeystore/keystore.go

-	if err := checkKeystoreParams(firstValid, round, interval); err != nil {
-		return err
-	}
+func (v *Verifier) Verify(round uint64, msg crypto.Hashable, sig Signature) error {


As far as I can tell so far, the logic of this function (which is the most important part) looks sound and SNARKable, assuming that merklearray.Verify is. But I cannot find that function in this PR. Is it somewhere else?

we will have a different PR for that.

crypto/sig_abstractions.go

cpeikert · 2022-01-10T21:56:20Z

crypto/sig_abstractions.go

+	GetVerificationBytes() []byte
+	// GetSerializedSignature returns a serialized version of the signature
+	// muse be const sized
+	GetSerializedSignature(signature ByteSignature) ([]byte, error)


The name and documentation ("serialized") are not really descriptive of the intended purpose here. It’s more that this is a "hashable (fixed-length) representation" for the SNARK.

cpeikert · 2022-01-10T21:57:17Z

data/basics/ccertpart.go

 }

 // ToBeHashed implements the crypto.Hashable interface.
+// In order to create a more SNARK-friendly commitments on the signature we must avoid using the msgpack infrastructure.


Agree; see also my comment above about the naming of GetSerializedSignature. The purpose of that function is much narrower than general serialization.

id-ms force-pushed the make-merkle-signer-snark-friendly branch from 7c23285 to e5f410f Compare December 1, 2021 10:42

algonautshant self-requested a review December 1, 2021 18:25

id-ms force-pushed the make-merkle-signer-snark-friendly branch from e5f410f to a99b0de Compare December 2, 2021 08:25

algonautshant reviewed Dec 3, 2021

View reviewed changes

id-ms force-pushed the make-merkle-signer-snark-friendly branch 3 times, most recently from b882c83 to b733a04 Compare December 14, 2021 09:27

id-ms linked an issue Dec 15, 2021 that may be closed by this pull request

The Merklekeystore should not use msgpack serialization for hash inputs #3202

Closed

id-ms force-pushed the make-merkle-signer-snark-friendly branch from b733a04 to da92a85 Compare December 19, 2021 10:07

id-ms marked this pull request as ready for review December 20, 2021 10:42

id-ms linked an issue Dec 20, 2021 that may be closed by this pull request

work with uncompress version of Falcon #3210

Closed

algoidan added 18 commits January 2, 2022 21:58

The index of the merklearray can now be found in the merkle signature

6bbb3fd

The merkle signature scheme commits on a raw representation of the pu…

629c172

…blic keys.

CC uses a non-msgpack representation of signatures

f8a208e

add tests so the pk commitment stays as expected

0b4b072

add test to validate that the part commitment format

4cafc45

add tests to verifiy signature commitments

a65c8f5

add the support for signature format conversion

5f25350

commit on the all the fields in merkle scheme signature

462263d

remove interval from the merkle scheme verification.

acd31f1

CR fixes

145e9f1

integrate with falcon's CT format

0989c68

bind cert creation to falcon scheme

892d991

verify signature's salt version

7a74460

signatureslot refacotr

573f268

refactor participation for CC

44afca7

refactor and renaming

5c132b1

protocol changes

d3ae424

update license

1b3a51b

id-ms force-pushed the make-merkle-signer-snark-friendly branch 2 times, most recently from b277b6d to bba3ba1 Compare January 4, 2022 11:58

add MSS signature verification test

5445566

id-ms force-pushed the make-merkle-signer-snark-friendly branch from bba3ba1 to 5445566 Compare January 4, 2022 12:05

remove the verification of the version in falcon

f33bc88

id-ms force-pushed the make-merkle-signer-snark-friendly branch from ea9a758 to f33bc88 Compare January 4, 2022 17:02

algoidan added 2 commits January 5, 2022 10:51

handle an empty slot in the signature array

2d5a7df

minor changes

70f0dd2

algonautshant reviewed Jan 8, 2022

View reviewed changes

CR fix

657ba71

cpeikert reviewed Jan 10, 2022

View reviewed changes

renaming and comments clarification

04dcd77

id-ms merged commit b93f0e5 into algorand:feature/dilithium-scheme-integration Jan 11, 2022

egieseke mentioned this pull request Feb 16, 2022

Script to generate go-algorand release notes #3520

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make merkle signer SNARK friendly #3263

Make merkle signer SNARK friendly #3263

id-ms commented Dec 1, 2021 •

edited

Loading

codecov-commenter commented Dec 1, 2021 •

edited

Loading

algonautshant Dec 1, 2021

id-ms Dec 13, 2021

algonautshant Dec 2, 2021

algonautshant Dec 2, 2021

algonautshant Dec 2, 2021

cpeikert Jan 10, 2022

algonautshant left a comment

algonautshant Jan 7, 2022

id-ms Jan 9, 2022

algonautshant Jan 8, 2022

id-ms Jan 9, 2022

algonautshant Jan 8, 2022

id-ms Jan 9, 2022

cpeikert Jan 10, 2022

cpeikert Jan 10, 2022

id-ms Jan 11, 2022

cpeikert Jan 10, 2022

cpeikert Jan 10, 2022


		sigBytes := ssc.Sig.Signature.GetSerializedSignature()

		sigSlotCommitment := make([]byte, 0, len(binaryLValue)+len(sigBytes))

Make merkle signer SNARK friendly #3263

Make merkle signer SNARK friendly #3263

Conversation

id-ms commented Dec 1, 2021 • edited Loading

Summary

Test Plan

codecov-commenter commented Dec 1, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algonautshant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

id-ms commented Dec 1, 2021 •

edited

Loading

codecov-commenter commented Dec 1, 2021 •

edited

Loading