[group key addrs 5/6]: Add Pedersen unique script key type #1621

guggero · 2025-06-26T09:16:37Z

More commits extracted from #1587 that contain the last (mostly) non-functional changes that can be merged independently before we're starting to change the address behavior.

coveralls · 2025-06-26T10:41:48Z

Pull Request Test Coverage Report for Build 16075189182

Details

372 of 564 (65.96%) changed or added relevant lines in 16 files are covered.
13 unchanged lines in 6 files lost coverage.
Overall coverage increased (+0.1%) to 39.358%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
proof/records.go	34	35	97.14%
rpcserver.go	0	1	0.0%
address/book.go	1	5	20.0%
rpcutils/marshal.go	0	4	0.0%
tapdb/sqlc/assets.sql.go	46	52	88.46%
authmailbox/receive_subscription.go	8	15	53.33%
authmailbox/multi_subscription.go	71	86	82.56%
tapdb/addrs.go	42	58	72.41%
proof/send.go	35	65	53.85%
proof/encoding.go	66	103	64.08%

Files with Coverage Reduction	New Missed Lines	%
asset/asset.go	2	44.36%
asset/group_key.go	2	62.64%
commitment/tap.go	2	72.05%
tapdb/addrs.go	2	63.36%
tappsbt/create.go	2	26.74%
asset/mock.go	3	65.89%

Totals
Change from base Build 16059643780:	0.1%
Covered Lines:	30914
Relevant Lines:	78546

💛 - Coveralls

guggero · 2025-06-26T16:45:18Z

Going to look into the test failures (I think the mock just needs some mutexes).
But requesting review on this anyway so folks can load up on context.

tapdb/addrs.go

tapdb/sqlc/queries/addrs.sql

asset/asset.go

ffranr · 2025-07-01T18:34:34Z

asset/asset.go

+	// to avoid proof collisions in the universe, where the script keys
+	// should be spendable by a hardware wallet that only supports
+	// miniscript policies for signing P2TR outputs.
+	ScriptKeyDerivationUniquePedersen ScriptKeyDerivationMethod = 0


why not use OP_RETURN method here rather than pedersen? Are we leaving the door open for hardware wallets, is that the idea here still?

Yes, mainly for the slightly easier hardware wallet support of the Pedersen construction.

ffranr · 2025-07-01T18:50:49Z

authmailbox/multi_subscription.go

+// Subscribe adds a new subscription for the specified client URL and receiver
+// key. It starts a new mailbox client if one does not already exist for the
+// given URL. The subscription will receive messages that match the provided
+// filter and will send them to the shared message queue.
+func (m *MultiSubscription) Subscribe(serverURL url.URL,
+	receiverKey keychain.KeyDescriptor, filter MessageFilter) error {


Maybe this should be thread safe? I'm not sure how this might be used. But if it's going to be called from an RPC endpoint then it probably needs to be thread safe.

Currently we only access this from a single Goroutine in the asset custodian. But doesn't hurt to make it thread safe, so I did that.

ffranr · 2025-07-01T18:54:56Z

proof/send.go

+const (
+	// SendFragmentV0 is the first version of the send fragment.
+	SendFragmentV0 SendFragmentVersion = 0
+)


Might be useful to reserve 0 for "unspecified version". Useful for returning errors on version parsing.

Might also be useful to have a "latest version" var here.

tapdb/sqlc/queries/addrs.sql

scripts/gen_sqlc_docker.sh

asset/asset.go

Roasbeef · 2025-07-02T01:51:38Z

scripts/gen_sqlc_docker.sh

+
+  # First, we replace the `$X/*SLICE:<field_name>*/` placeholders with
+  # the actual placeholder that sqlc will use: `/*SLICE:<field_name>*/?`.
+  sed -i.bak -E 's/\$([0-9]+)\/\*SLICE:([a-zA-Z_][a-zA-Z0-9_]*)\*\//\/\*SLICE:\2\*\/\?/g' "$file"


Yeah, regular expressions (combined with the required escaping needed for the shell script and sed syntax) aren't easy to read. But I guess we can always ask Gemini to explain it to us 😝 I did provide before and after examples to hopefully make them a bit easier to understand.

Roasbeef · 2025-07-02T02:02:26Z

asset/asset.go

+// depending on the `filterChannelRelated` parameter. Unless the user specifies
+// a specific script key type, in which case the returned slice will only
+// contain that specific script key type.
+func ScriptKeyTypeForDatabaseQuery(filterChannelRelated bool,


Perhaps this should instead live in the DB? As it's only used there for now, and makes some assumptions w.r.t how the types are stored in the db.

Eg: IIUC, if someone re-orders the script key types in the definition above (eg: by accident when adding a new type), queries will break.

This is generally why I lean towards making these db enum types into their own table, as then we lean on the database engine to help us enforce referential integrity.

I moved this specifically here to be close to the different script key types. The idea is that we don't forget to add new script keys to the two lists (AllScriptKeyTypes and ScriptKeyTypesNoChannel).
I also felt it was weird having this business logic in the database itself.

assumptions w.r.t how the types are stored in the db

No, we don't make any assumptions, the type is a simple numeric type. Sure, the database doesn't know what possible types there are, but other than that there are no assumptions that I can think of.

IIUC, if someone re-orders the script key types in the definition above (eg: by accident when adding a new type), queries will break.

That luckily isn't the case. Because we use WHERE script_key_type IN (?, ?, ?, ...), the order of the enum-like values doesn't matter.
Perhaps you have the event status in mind where we still do this status >= @status_from AND status <= @status_to in
https://github.com/lightninglabs/taproot-assets/blob/main/tapdb/sqlc/queries/addrs.sql#L170

We can turn that into a WHERE status IN (?, ?, ?, ...) query now too to make the query order-independent.

Roasbeef · 2025-07-02T02:07:37Z

proof/records.go

@@ -54,6 +54,12 @@ const (
 	MetaRevealUniverseCommitments    tlv.Type = 7
 	MetaRevealCanonicalUniversesType tlv.Type = 9
 	MetaRevealDelegationKeyType      tlv.Type = 11
+
+	SendFragmentVersionType     tlv.Type = 0


Arguably could live in the address package, as IIUC we're not placing this information into proofs yet.

Ah yes, makes sense.

Because we'll use the address' script key as the bare/raw public key that will be tweaked for each individual output of an address v2 send, we'll need to be able to find addresses by that script key. We'll also make the script key unique for v2 addresses to avoid multiple records being returned here.

This introduces a workaround that gets us WHERE xxx IN (...) queries working with a little trick. See the comment in scripts/gen_sqlc_docker.sh for more information on how this works and why the workaround is needed.

This MultiSubscription helper struct allows us to subscribe to receive messages for multiple keys held by a receiving wallet but all consolidated into a single message channel.

We need to place these types in the proof package because the proof courier will need access to them. And because we already use the proof package from the address package, we can't place the new types there, as that would create a circular package dependency.

guggero · 2025-07-04T14:38:33Z

I addressed all comments, should be ready for another round of review.

guggero added this to the v0.7 milestone Jun 26, 2025

guggero self-assigned this Jun 26, 2025

guggero added this to Taproot-Assets Project Board Jun 26, 2025

guggero added scripts addrs group_key labels Jun 26, 2025

github-project-automation bot moved this to 🆕 New in Taproot-Assets Project Board Jun 26, 2025

guggero mentioned this pull request Jun 26, 2025

[group key addrs 6/6]: send and receive support for V2 addresses #1587

Open

guggero force-pushed the pedersen-keys-refactor branch from 5687720 to 46d9159 Compare June 26, 2025 10:18

levmi moved this from 🆕 New to 🏗 In progress in Taproot-Assets Project Board Jun 26, 2025

guggero force-pushed the pedersen-keys-refactor branch from 46d9159 to e5fdf7e Compare June 26, 2025 16:06

guggero requested review from Roasbeef and ffranr June 26, 2025 16:45

guggero force-pushed the pedersen-keys-refactor branch 3 times, most recently from 555c711 to 6a85168 Compare June 30, 2025 15:41

ffranr reviewed Jul 1, 2025

View reviewed changes

Roasbeef requested changes Jul 2, 2025

View reviewed changes

github-project-automation bot moved this from 🏗 In progress to 👀 In review in Taproot-Assets Project Board Jul 2, 2025

guggero force-pushed the pedersen-keys-refactor branch from 6a85168 to 3e10e34 Compare July 2, 2025 17:08

guggero force-pushed the pedersen-keys-refactor branch from 3e10e34 to d050a45 Compare July 4, 2025 13:42

guggero added 5 commits July 4, 2025 15:44

scripts+tapdb: update sqlc to v1.29.0

ffd2929

asset+scripts+tapd: allow querying multiple script key types

f69c9a9

This introduces a workaround that gets us WHERE xxx IN (...) queries working with a little trick. See the comment in scripts/gen_sqlc_docker.sh for more information on how this works and why the workaround is needed.

asset: add new script key type for pedersen unique keys

5eee33a

authmailbox: add multi subscriber helper type

a72ed75

This MultiSubscription helper struct allows us to subscribe to receive messages for multiple keys held by a receiving wallet but all consolidated into a single message channel.

guggero force-pushed the pedersen-keys-refactor branch from d050a45 to 770baa6 Compare July 4, 2025 13:44

guggero requested review from ffranr and Roasbeef July 4, 2025 14:38

[group key addrs 5/6]: Add Pedersen unique script key type #1621

Are you sure you want to change the base?

[group key addrs 5/6]: Add Pedersen unique script key type #1621

Uh oh!

Conversation

guggero commented Jun 26, 2025

Uh oh!

coveralls commented Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 16075189182

Details

💛 - Coveralls

Uh oh!

guggero commented Jun 26, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guggero commented Jul 4, 2025

Uh oh!

Uh oh!

coveralls commented Jun 26, 2025 •

edited

Loading