multi: add new v2 version of GKR based on Pedersen commitments #1290

Roasbeef · 2025-01-10T21:40:51Z

In this PR, we add a new internal package that can be used to create
and verify Pedersen commitments. We aim to make the API very simple to
use, while providing configuration that's useful for tests and a general
set of use cases.

We also add property based tests to achieve 100% test coverage of all
relevant invariants.

We then this that to add a new version of the new more flexible GKR type.
This version uses an OP_CHECKSIG with an un-spendable public key. We
derive such a public key by using the Pedersen commitment of the asset
ID, or blank message (for the dummy leaf case). As a Pedersen commitment
uses an aux generator H, that no one knows the private key to, it's
impossible to generate a signature using the commitment point.

An informal sketch of a proof by contradiction goes something like:

Starting with a Pedersen commitment, P = m*G + r*H.
Let's assume that we can actually sign with P. This means that
there's some x, where P = x*G that we know.
If we re-write x in terms of our commitment, we have: P = m*G + r*H.
Wlog, we'll simplify by removing r, the blinding factor, so: P = m*G + H.
In terms of private values we have x*G = m*G + H
If we solve for H, then we have H = (x-m)*G
If this is a Pedersen commitment, then the private key of H must be
unknown to all parties.
If we can sign with P, then this means that we know the value (x-m).
However, since this is a Pedersen commitment, it's impossible to
know the value x, which is a function of the private value H of
the Pedersen commitment (P= (x-m)*G). This is a contradiction, as
we see the that if we know x-m, then we also know h.

dstadulis · 2025-01-10T22:12:15Z

Wlog, we'll simplify by removing r, the blinding factor, so: P = m*G + H.

Does Wlog = WLOG?
aka "without loss of generality"

asset/asset.go

internal/pedersen/commitment.go

dstadulis · 2025-01-10T22:38:30Z

internal/pedersen/commitment.go

+// WithCustomNUMs is a functional option that can be used to set a custom NUMs
+// point.
+func WithCustomNUMs(h btcec.PublicKey) commitOpt {


WRT NUMS style is traditional acronym style preferred of capitalizing every letter of the acronym?

Yes, I think this should be WithCustomNUMS. "Sleeve"

ffranr

I hadn't considered Pedersen commitments as a solution for our need before reviewing this PR. Really interesting subject to review/consider.

I think that we can re-use the existing cryptography functionality to achieve the same goal.

I think we can do:

	nonSpendableKey := input.TaprootNUMSKey
	if data != nil {
		dataHash := chainhash.HashH(data)

		tweakedNumsKey := input.TweakPubKeyWithTweak(
			&input.TaprootNUMSKey, dataHash[:],
		)
		nonSpendableKey = *tweakedNumsKey
	}

	scriptBuilder := txscript.NewScriptBuilder()
	scriptBuilder.AddData(schnorr.SerializePubKey(&nonSpendableKey))
	scriptBuilder.AddOp(txscript.OP_CHECKSIG)
	script, err := scriptBuilder.Script()

IMO we can re-use the existing function input.TweakPubKeyWithTweak. The function is short, so I'll quote it here with extra comments to show that it does the same thing in terms of the Pedersen commitment formula P = m*G + r*H.

func TweakPubKeyWithTweak(pubKey *btcec.PublicKey,
	tweakBytes []byte) *btcec.PublicKey {

	var (
		pubKeyJacobian btcec.JacobianPoint
		tweakJacobian  btcec.JacobianPoint
		resultJacobian btcec.JacobianPoint
	)
	// Form tweak scalar from tweak bytes. This is `m` in the equation
	// `P = m*G + r*H`.
	tweakKey, _ := btcec.PrivKeyFromBytes(tweakBytes)

	// Compute the Jacobian point for the tweak. This is `m*G`.
	// Crucially, the base point is the generator point G.
	btcec.ScalarBaseMultNonConst(&tweakKey.Key, &tweakJacobian)

	// At this point we can just assume `r` is 1, as we're only using `H` as
	// the NUMS point. The public key here is the NUMS key.
	pubKey.AsJacobian(&pubKeyJacobian)
	
	// Perform the addition: m*G + r*H
	// where r = 1 and H is the NUMS point.
	btcec.AddNonConst(&pubKeyJacobian, &tweakJacobian, &resultJacobian)

	// Return P.
	resultJacobian.ToAffine()
	return btcec.NewPublicKey(&resultJacobian.X, &resultJacobian.Y)
}

I think P will remain non-spendable for the reason you've outlined in the PR body. Specifically, the NUMS generator point cannot be expressed as a linear combination of the standard generator point G.

We already use input.TweakPubKeyWithTweak in a few places, so re-using it would help keep our cryptography codebase concise and maintainable. Having a single point to test would benefit all callers that depend on it.

Since we won’t be utilizing the privacy feature of Pedersen commitments, I think we should reconsider using Pedersen commitments altogether. From what I understand after reading Non-Interactive and Information-Theoretic Secure Verifiable Secret Sharing, the privacy aspect seems to be a fundamental part of Pedersen commitments. Referring to Pedersen commitments while essentially creating a NUMS key commitment strikes me as unnecessarily complex.

EDIT: Here's a commit showing what I mean re input.TweakPubKeyWithTweak: d6d7a71

asset/asset.go

Roasbeef · 2025-01-13T23:40:23Z

I think that we can re-use the existing cryptography functionality to achieve the same goal.

It's certainly similar, but that function was created for a different purpose, namely for the randomized per commitment point for a LN commitment. The goal here with the internal package was to make a higher level wrapper with a single purpose that's less error prone (eg: explicit Opening struct, ability to use non-randomized commitment, etc).

the privacy aspect seems to be a fundamental part of Pedersen commitments

A cryptographic commitment can be hiding (no information about the opening revealed from the commitment). It can also be binding (not possbile to commit to one value and prevent an opening of a different value. Both those properties can rely on either a computational or information theoretic assumption.

Pedersen commitments are computationally binding (reduces to the DH problem), and information theoretically (perfect) hiding.

The mini library I created allows the commitment to be optionally non-hiding. In our case, we don't really need the hiding property as when the commitment is presented, we already know what the opening is (the asset ID).

Referring to Pedersen commitments while essentially creating a NUMS key commitment strikes me as unnecessarily complex.

Not sure what you mean, that's the very construction of a Pedersen commitment. It relies on a aux generator H with an unknown discrete log. A NUMS key by definition has an unknown discrete log. This is just a precise implementation of the construct.

coveralls · 2025-01-14T01:36:51Z

Pull Request Test Coverage Report for Build 12814109365

Details

160 of 194 (82.47%) changed or added relevant lines in 4 files are covered.
21 unchanged lines in 5 files lost coverage.
Overall coverage increased (+0.1%) to 40.857%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tapgarden/planter.go	0	4	0.0%
asset/asset.go	95	125	76.0%

Files with Coverage Reduction	New Missed Lines	%
tappsbt/create.go	2	53.22%
internal/test/helpers.go	2	86.95%
asset/asset.go	3	77.01%
tapgarden/caretaker.go	4	68.87%
universe/interface.go	10	52.81%

Totals
Change from base Build 12810137371:	0.1%
Covered Lines:	26666
Relevant Lines:	65266

💛 - Coveralls

ffranr · 2025-01-14T13:21:36Z

Referring to Pedersen commitments while essentially creating a NUMS key commitment strikes me as unnecessarily complex.

Not sure what you mean, that's the very construction of a Pedersen commitment. It relies on a aux generator H with an unknown discrete log. A NUMS key by definition has an unknown discrete log. This is just a precise implementation of the construct.

@Roasbeef What I mean is that a Pedersen commitment includes the separate component of the unknown discrete log, the blinding factor r, which IMO is an unnecessary complexity for our use case since we won't use it.

From my perspective, the only functional change introduced by this PR is the added support for r. Otherwise, input.TweakPubKeyWithTweak already computes exactly (see: d6d7a71) what we need and seems sufficiently general for our purposes.

But I do like that the Pedersen commitment library formalises the G tweak term and NUMS point relationship where as with input.TweakPubKeyWithTweak it just seems more happenstance. input.TweakPubKeyWithTweak could have just assume that the public key had the generator point G for example.

ffranr

How do we feed the non-spend leaf version from the interface to this point? Do we need to serialise it into the proof group key reveal?

Alternatively, instead of adding a non-spend leaf version, we could, during group key reveal verification just attempt to reconstruct the tapscript root using both non-spend leaf versions. We just want one of them to validate, doesn't matter which.

So the next release could have this functionality:

Construct group keys with Pedersen commitment.
Be able to verify group keys with either OP_RETURN or Pedersen commitment leaves. And just tries both verification methods without looking up any non-spend leaf version.

asset/asset.go

Roasbeef · 2025-01-14T23:56:08Z

How do we feed the non-spend leaf version from the interface to this point? Do we need to serialise it into the proof group key reveal?

So what I've done here is just inherit the value from the enclosing struct during decoding: https://github.com/lightninglabs/taproot-assets/pull/1290/files#diff-98586afaee9ef4f1d46ba2b4af7ce1fd31d28bc83e6df2117e6483532221f8a7R1388-R1391. They're the exact same version, which is only really used when we need to make the unspendable leaf.

guggero · 2025-01-15T13:49:59Z

I've pushed up some fixes and additional commits for what we need for the hardware wallet support.

Alternatively, instead of adding a non-spend leaf version, we could, during group key reveal verification just attempt to reconstruct the tapscript root using both non-spend leaf versions. We just want one of them to validate, doesn't matter which.

I think the way the version is encoded now I prefer the more explicit one. And since we already encoded the version anyway, we don't store more data in the proof.

guggero

Provided my review in form of direct patches.

ffranr

Nice!

I think we should change NUMs to NUMS throughout.

ffranr · 2025-01-16T14:15:08Z

internal/pedersen/commitment.go

+// WithCustomNUMs is a functional option that can be used to set a custom NUMs
+// point.
+func WithCustomNUMs(h btcec.PublicKey) commitOpt {


Yes, I think this should be WithCustomNUMS. "Sleeve"

ffranr · 2025-01-16T14:21:45Z

.../TestPedersenCommitmentProperties/TestPedersenCommitmentProperties-20250110122316-67631.fail

+# 2025/01/10 12:23:16.878522 [TestPedersenCommitmentProperties] [rapid] draw bytes: []byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
+# 2025/01/10 12:23:16.878526 [TestPedersenCommitmentProperties] [rapid] draw bytes: []byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x2}


.fail files don't need to be committed? or do they?

Not 100% sure, but this commit makes me think it behaves the same way as fuzz corporae, that if files like that exist they act as a regression test case: flyingmutant/rapid@4db076b

asset/asset.go

ffranr · 2025-01-16T14:37:26Z

asset/asset.go

+		var msg [sha256.Size]byte
+		copy(msg[:], data)


I think this copy is a bit risky. If the caller passes in data which is len(data) > sha256.Size then it will be truncated rather than included in the leaf in totality.

I think it would be best to just change data in the func sig to:

func NewNonSpendableScriptLeaf(version NonSpendLeafVersion, data [sha256.Size]byte) (txscript.TapLeaf, error)

That's all we need for now anyway.

Added a check to the last commit.

ffranr · 2025-01-16T14:40:01Z

asset/asset.go

@@ -1224,24 +1252,36 @@ func NewGKRCustomSubtreeRootRecord(root *chainhash.Hash) tlv.Record {
 //
 //	                       [tapscript_root]
 //	                         /          \
-//	[OP_RETURN <genesis asset ID>]   [tweaked_custom_branch]
+//	[non_spend(<genesis asset ID>)]   [tweaked_custom_branch]


references to [OP_RETURN] above also need to be updated:

// sibling is a non-spendable script leaf containing `[OP_RETURN]`. Its

// If `custom_root_hash` is not provided, it defaults to a bare `[OP_RETURN]` as

maybe others

Also is the tree struct just the genesis asset ID node now for HW signing without custom spend subtree?

Fixed the references to [OP_RETURN].

Also is the tree struct just the genesis asset ID node now for HW signing without custom spend subtree?

If there are no custom scripts, then yes, there's only a single leaf with the non_spend(<genesis asset ID>) script. Updated some references in the comments.

asset/group_key_reveal_test.go

asset/asset.go

ffranr · 2025-01-16T14:54:33Z

asset/asset.go

-	//
-	// NOTE: This field should not be serialized as part of the group key
-	// reveal. It is included here to ensure it is constructed concurrently
-	// with the tapscript root, maintaining consistency and minimizing
-	// errors.
-	customSubtreeInclusionProof []byte


Computing this on demand means that it could theoretically not correspond correctly to what's committed to in customSubtreeRoot. That's why customSubtreeInclusionProof was previously stored and was constructed using the same components which also constructed customSubtreeRoot.

But isn't this exactly what the if !g.root.IsEqual(&tapscript.root) { check is for?

asset/asset.go

In this commit, we add a new internal package that can be used to create and verify Pedersen commitments. We aim to make the API very simple to use, while providing configuration that's useful for tests and a general set of use cases. We also add property based tests to achieve 100% test coverage of all relevant invariants.

In this commit, we add a new version of the new more flexible GKR type. This version uses an `OP_CHECKSIG` with an un-spendable public key. We derive such a public key by using the Pedersen commitment of the asset ID, or blank message (for the dummy leaf case). As a Pedersen commitment uses an aux generator H, that no one knows the private key to, it's impossible to generate a signature using the commitment point. An informal sketch of a proof by contradiction goes something like: * Starting with a Pedersen commitment, P = m*G + r*H. * Let's assume that we can actually sign with P. This means that there's some x, where P = x*G that we know. * If we re-write x in terms of our commitment, we have: P = m*G + r*H. * Wlog, we'll simplify by removing `r`, the blinding factor, so: P = m*G + H. * In terms of private values we have x*G = m*G + H * If we solve for H, then we have H = (x-m)*G * If this is a Pedersen commitment, then the private key of H must be unknown to all parties. * If we can sign with P, then this means that we know the value (x-m). * However, since this is a Pedersen commitment, it's impossible to know the value `x`, which is a function of the private value H of the pedersen commitment (h*H = (x-m)*G. This is a contradiction, as we see the that if we know x-m, then we also know h.

This asserts that both variants of the leaf we generate are actually nonspendable.

To achieve hardware wallet signing support, we remove any siblings from the tree if there are no custom scripts present. This allows us to use the Pedersen commitment to create a public key miniscript policy which is compatible with certain hardware wallets.

This commit removes a field from the group key reveal V1 that wasn't serialized and calculated on demand if necessary anyway. Re-computing it when it is needed is a smaller cost than the added complexity of having a non-serialized field.

Hardware wallets that support miniscript policies only accept a pk() fragment that specifies an extended key, for example with: pk(@1/<0;1>/*). When signing the transaction the actual derivation path to use (for example 0/0) is defined in the PSBT itself. So the signing policy is generic and the user only has to accept it once. A child key derived from an extended NUMS key is still a NUMS key, as it's just tweaked again. The initial private key is still unknown.

gijswijs · 2025-01-16T21:26:38Z

Really cool stuff, especially the mask being 1.

I have no remarks on the code.

I do have some problems with the proof sketch.

If we can sign with P, then this means that we know the value (x-m).

However, since this is a Pedersen commitment, it's impossible to
know the value x, which is a function of the private value H of
the Pedersen commitment (P= (x-m)*G). This is a contradiction, as
we see the that if we know x-m, then we also know h.

I find this part of the sketch confusing. The assumption that knowing how to sign with P directly implies knowledge of x - m is problematic, iiuc. If you can sign with P, it means you know the private key x, not x-m.

The contradiction arises not from knowing x-m, but from the assumption that H can be expressed as a known scalar multiple of G. In a Pedersen commitment, H is chosen such that its discrete log with respect to G is unknown and cannot be derived from the commitment. P can never be expressed in terms of G. Assuming that you can sign with P implies that you can do that, and that's where the contradiction arises.

Maybe I'm being too academic. It does not have any implications for the code, which looks sound to me.

ffranr

Nice!

Roasbeef requested a review from ffranr January 10, 2025 21:40

dstadulis assigned Roasbeef Jan 10, 2025

dstadulis added this to the v0.6 milestone Jan 10, 2025

dstadulis added crypto signing group_key labels Jan 10, 2025

dstadulis reviewed Jan 10, 2025

View reviewed changes

ffranr requested changes Jan 13, 2025

View reviewed changes

asset/asset.go Outdated Show resolved Hide resolved

Roasbeef force-pushed the pederson-group-key branch 2 times, most recently from b00f5c5 to f7978c2 Compare January 14, 2025 01:22

guggero self-requested a review January 14, 2025 11:42

ffranr reviewed Jan 14, 2025

View reviewed changes

asset/asset.go Outdated Show resolved Hide resolved

asset/asset.go Outdated Show resolved Hide resolved

asset/asset.go Outdated Show resolved Hide resolved

asset/asset.go Outdated Show resolved Hide resolved

ffranr mentioned this pull request Jan 14, 2025

Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

Merged

Roasbeef force-pushed the pederson-group-key branch from f7978c2 to e1b919b Compare January 15, 2025 00:17

guggero force-pushed the pederson-group-key branch from e1b919b to 8f212f0 Compare January 15, 2025 13:48

guggero requested a review from ffranr January 15, 2025 13:50

guggero approved these changes Jan 15, 2025

View reviewed changes

guggero mentioned this pull request Jan 16, 2025

PoC: Use tweaked NUMS key for non-spendable Taproot script leaves #1289

Closed

guggero force-pushed the pederson-group-key branch from 8f212f0 to 2389629 Compare January 16, 2025 12:40

ffranr reviewed Jan 16, 2025

View reviewed changes

gijswijs self-requested a review January 16, 2025 16:37

ffranr mentioned this pull request Jan 16, 2025

[feature]: optionally generate asset group key without internal key tweak during minting #1226

Closed

Roasbeef added 2 commits January 16, 2025 18:15

Roasbeef and others added 4 commits January 16, 2025 18:15

asset: add test case for NewNonSpendableScriptLeaf

101516f

This asserts that both variants of the leaf we generate are actually nonspendable.

guggero force-pushed the pederson-group-key branch from 2389629 to 9733b3d Compare January 16, 2025 17:15

guggero requested a review from ffranr January 16, 2025 17:17

guggero mentioned this pull request Jan 16, 2025

Support minting on Ledger signing device #1301

Open

ffranr approved these changes Jan 16, 2025

View reviewed changes

ffranr enabled auto-merge January 16, 2025 22:05

ffranr added this pull request to the merge queue Jan 16, 2025

Merged via the queue into main with commit 375a5e2 Jan 16, 2025
18 checks passed

guggero deleted the pederson-group-key branch January 17, 2025 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: add new v2 version of GKR based on Pedersen commitments #1290

multi: add new v2 version of GKR based on Pedersen commitments #1290

Roasbeef commented Jan 10, 2025 •

edited by dstadulis

Loading

dstadulis commented Jan 10, 2025 •

edited

Loading

dstadulis Jan 10, 2025

ffranr Jan 16, 2025

ffranr left a comment •

edited

Loading

Roasbeef commented Jan 13, 2025

coveralls commented Jan 14, 2025 •

edited

Loading

ffranr commented Jan 14, 2025 •

edited

Loading

ffranr left a comment •

edited

Loading

Roasbeef commented Jan 14, 2025

guggero commented Jan 15, 2025

guggero left a comment

ffranr left a comment

ffranr Jan 16, 2025

ffranr Jan 16, 2025

guggero Jan 16, 2025

ffranr Jan 16, 2025

guggero Jan 16, 2025

ffranr Jan 16, 2025

ffranr Jan 16, 2025

guggero Jan 16, 2025

ffranr Jan 16, 2025

guggero Jan 16, 2025

gijswijs commented Jan 16, 2025

ffranr left a comment

		# 2025/01/10 12:23:16.878522 [TestPedersenCommitmentProperties] [rapid] draw bytes: []byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}
		# 2025/01/10 12:23:16.878526 [TestPedersenCommitmentProperties] [rapid] draw bytes: []byte{0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x2}

multi: add new v2 version of GKR based on Pedersen commitments #1290

multi: add new v2 version of GKR based on Pedersen commitments #1290

Conversation

Roasbeef commented Jan 10, 2025 • edited by dstadulis Loading

dstadulis commented Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr left a comment • edited Loading

Choose a reason for hiding this comment

Roasbeef commented Jan 13, 2025

coveralls commented Jan 14, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12814109365

Details

💛 - Coveralls

ffranr commented Jan 14, 2025 • edited Loading

ffranr left a comment • edited Loading

Choose a reason for hiding this comment

Roasbeef commented Jan 14, 2025

guggero commented Jan 15, 2025

guggero left a comment

Choose a reason for hiding this comment

ffranr 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

gijswijs commented Jan 16, 2025

ffranr left a comment

Choose a reason for hiding this comment

Roasbeef commented Jan 10, 2025 •

edited by dstadulis

Loading

dstadulis commented Jan 10, 2025 •

edited

Loading

ffranr left a comment •

edited

Loading

coveralls commented Jan 14, 2025 •

edited

Loading

ffranr commented Jan 14, 2025 •

edited

Loading

ffranr left a comment •

edited

Loading