[1/2] custodian: look up proofs in local universe as well #726

guggero · 2023-12-06T16:22:46Z

~~Depends on #712.~~

Fixes #578.

This PR updates the proof notification publish/subscribe mechanism in a way that allows us to pass in multiple proof sources to the custodian so it can detect incoming proofs from multiple sources.

Roasbeef

Nice work! Main comment is if we really want to overload Blob (tho it already kinda is) vs using distinct types, and then an Either to allow them both be used in instances where either of them will work.

Reading over this change, it struck me (once again), that we should probably try to consolidate all the different proof storage interfaces we have. At times it isn't super easy to follow where a proof if coming from or why, but the multi wrappers do help a bit.

proof/courier.go

tapdb/assets_store.go

tapdb/multiverse.go

Roasbeef · 2023-12-09T01:31:00Z

proof/mint.go

-// Blob represents a serialized proof file, including the checksum.
+// Blob either represents a serialized proof file, including the checksum or a
+// single serialized issuance/transition proof. Which one it is can be found out
+// from the leading magic bytes (or the helper methods that inspect those).


Hmm, I wonder if we would be better of making these into different types instead. We added the magic prefix bytes partially to avoid confusing them in the first place. With the type route, we'd break compilation if one was attempted to be passed in where the other was accepted. As is, we reply on runtime checks to avoid such classes of bugs.

This could use the Either type added in the multiverse root PR as an example. Then you can still use Blob everywhere, but rely on type safety re protection of trying to use one vs the other.

Yes, I think distinguishing between a single proof or a proof file would help a lot in the clarity of the code. But I think that would be a bigger refactor and should probably be done in a separate PR.
Do you think this is a reasonable temporary step for this PR?

SGTM, down to make an issue for it so we can move on here. Main thing is we can supplement the byte level prefix with a type, and then accept that type contextually to cut down on possible bugs in this area.

tapgarden/custodian.go

itest/send_test.go

jharveyb

A wonderful PR and itest 🎉

The callback for proof fetching is pretty neat also. Small Q on how we're fetching proofs overall + some nits.

fn/iter.go

proof/archive.go

tapdb/multiverse.go

tapdb/assets_store.go

jharveyb · 2023-12-14T00:41:39Z

Actually, given that some parts of this only handle transfer proofs, do these changes enable 'sideloaded' issuance proofs to get added to our local universe state?

I think for transfers it's implicit as the transfer proof includes the issuance proof, but for those who want to manually sync issuance i'm not sure if we test that rn.

ffranr · 2023-12-20T14:06:20Z

This PR has two reviewers. I've had a review request for this. @guggero would you like me to review also?

guggero · 2023-12-20T14:21:35Z

This PR has two reviewers. I've had a review request for this. @guggero would you like me to review also?

No, it's fine, thank you. You basically already reviewed this PR since the commits of this one used to be included in #730 which you did look at.

guggero · 2023-12-20T15:04:52Z

Actually, given that some parts of this only handle transfer proofs, do these changes enable 'sideloaded' issuance proofs to get added to our local universe state?

Yes, since the only way to side load a proof currently (without using dev mode), is the universe RPC's InsertProof method which does allow you to insert an issuance proof directly into the universe.
And that does insert the meta data and genesis information into the asset DB as well.
But it does sound like a good integration test case, so I'll add it.

jharveyb · 2023-12-21T18:41:35Z

Yes, since the only way to side load a proof currently (without using dev mode), is the universe RPC's InsertProof method which does allow you to insert an issuance proof directly into the universe. And that does insert the meta data and genesis information into the asset DB as well. But it does sound like a good integration test case, so I'll add it.

With the itest change, I think we still don't exercise import on the genesis proof?

The second node has the default universe server set, so IIUC they sync the genesis proof at that point. Then they receive only the transfer proof, and import that successfully.

What I meant earlier was to actually have the universe server unset for the second node and import the issuance proof manually via ImportProof. Then, perform the transfer and import that proof manually as well (which the test does do right now).

Latest changes LGTM otherwise.

lightninglabs-deploy · 2023-12-28T19:14:21Z

@Roasbeef: review reminder
@jharveyb: review reminder

guggero · 2024-01-03T12:39:03Z

You mean because t.universeServer is still passed in?
We only discussed changing the behavior of this, but currently that variable is required. We do set params.noDefaultUniverseSync = true though, which does disable syncing of any proofs with the universe.

So I think the test does exactly what you propose?
With sendProofUniRPC(t, t.tapd, bob, firstAsset.ScriptKey, firstAssetGen) manually syncing the issuance proof.

jharveyb

Good point re: testing issuance insertion as-is. LGTM!

With this commit we change the NotifyArchiver to the bare minimum that we actually need in the custodian.

With this commit we extract the logic for iteratively retrieving the full provenance for an asset by starting at the last proof then querying each previous proof until we arrive at the genesis. We will want to re-use this logic outside of the proof courier itself, so it's useful to extract into a general-purpose function where the actual retrieval of an individual proof is done in a callback and can be adjusted to the current proof storage (local or remote).

This is an optimization that allows us to only extract the asset from the proof when fetching the full proof provenance. That saves us a whole decode/encode round trip per proof.

In some situations we want a quick way to find out if we have a proof (or not) without actually fetching it, so we add a HasProof method to the proof.Archive interface. The MultiArchiver will return false if one of the backends does not have the proof, so we can use that method to find out if we have a discrepancy between the backends and should import a proof file.

With this commit we prepare the MultiverseStore to also act as a NotifyArchiver that can notify about new incoming proofs.

Because the universe only deals with individual proof leaves, we want the custodian to be able to deal with both proof files and single proofs. To make it easy to distinguish and convert between the two, we add some helper methods to the proof.Blob type.

Since we now might get either a full proof file or just a transition proof, the custodian needs to be updated to be able to deal with both.

This commit creates a new notifier that can relay registrations for new proofs to multiple notifier/archiver backends. We then use that to pass in both the local assets store as well as the multiverse store to the custodian to detect incoming proofs.

guggero · 2024-01-09T07:45:11Z

Rebased, ready for final review @Roasbeef.

Roasbeef

LGTM 🍉

Roasbeef · 2024-01-12T02:33:32Z

fn/func.go

@@ -130,6 +130,18 @@ func All[T any](xs []T, pred func(T) bool) bool {
 	return true
 }

+// AllMapItems returns true if the passed predicate returns true for all items
+// in the map.
+func AllMapItems[T any, K comparable](xs map[K]T, pred func(T) bool) bool {


Cool! Looking forward to when/if they add more direct iterator stuff to the Go stdlib, then we'd only need one version of this.

Roasbeef · 2024-01-12T02:42:57Z

proof/archive.go

+// RegisterSubscriber adds a new subscriber for receiving events. The
+// registration request is forwarded to all registered archives.
+func (m *MultiArchiveNotifier) RegisterSubscriber(
+	receiver *fn.EventReceiver[Blob], deliverExisting bool,


Leaving review note to self: does the subscriber need to be ready to handle multiple subscriptions notifications from all the archives for the same proof file?

Roasbeef · 2024-01-12T02:44:36Z

itest/test_list_on_test.go

@@ -76,6 +76,10 @@ var testCases = []*testCase{
 		test:             testOfflineReceiverEventuallyReceives,
 		proofCourierType: proof.HashmailCourierType,
 	},
+	{
+		name: "addr send no proof courier with local universe import",
+		test: testSendNoCourierUniverseImport,


Not gofmt'd?

Linter says: it actually is!

guggero force-pushed the custodian-local-universe branch from 3db2a58 to 0b182fc Compare December 8, 2023 14:19

guggero requested review from Roasbeef and ffranr December 8, 2023 14:20

guggero force-pushed the custodian-local-universe branch from 0b182fc to 663c01f Compare December 8, 2023 15:21

guggero mentioned this pull request Dec 8, 2023

[2/2] proof: allow proof courier to short cut with local archive #730

Merged

Roasbeef requested changes Dec 9, 2023

View reviewed changes

guggero force-pushed the custodian-local-universe branch 2 times, most recently from c534ce1 to 994837f Compare December 13, 2023 14:22

guggero requested a review from Roasbeef December 13, 2023 14:23

guggero force-pushed the custodian-local-universe branch from 994837f to 54bc8e7 Compare December 13, 2023 18:10

jharveyb self-requested a review December 14, 2023 00:17

jharveyb reviewed Dec 14, 2023

View reviewed changes

fn/iter.go Show resolved Hide resolved

proof/archive.go Show resolved Hide resolved

tapdb/multiverse.go Show resolved Hide resolved

tapdb/assets_store.go Show resolved Hide resolved

guggero changed the title ~~custodian: look up proofs in local universe as well~~ [1/2] custodian: look up proofs in local universe as well Dec 14, 2023

guggero removed the request for review from ffranr December 20, 2023 14:21

guggero force-pushed the custodian-local-universe branch from 54bc8e7 to 32fe75c Compare December 20, 2023 15:28

guggero requested a review from jharveyb December 20, 2023 15:28

guggero force-pushed the custodian-local-universe branch from 32fe75c to dc08080 Compare December 20, 2023 15:31

guggero force-pushed the custodian-local-universe branch from dc08080 to 6cc7b2a Compare January 3, 2024 12:39

guggero mentioned this pull request Jan 5, 2024

Refactor and cleanup custodian events cache handling #756

Merged

jharveyb approved these changes Jan 8, 2024

View reviewed changes

guggero added 3 commits January 9, 2024 08:38

proof+tapgarden: shrink NotifyArchiver interface

c315ba5

With this commit we change the NotifyArchiver to the bare minimum that we actually need in the custodian.

proof: sparse decode proof in FetchProofProvenance

03d40e6

This is an optimization that allows us to only extract the asset from the proof when fetching the full proof provenance. That saves us a whole decode/encode round trip per proof.

guggero added 8 commits January 9, 2024 08:38

tapdb: turn MultiverseStore into NotifyArchiver

45da9d6

With this commit we prepare the MultiverseStore to also act as a NotifyArchiver that can notify about new incoming proofs.

fn: add generic functions for maps

fc035bb

tapgarden: switch custodian over to updated interface

ff5f9cb

Since we now might get either a full proof file or just a transition proof, the custodian needs to be updated to be able to deal with both.

itest: add test for local universe proof delivery

7b0541a

itest: test side loading of issuance proof

864b554

guggero force-pushed the custodian-local-universe branch from 6cc7b2a to 864b554 Compare January 9, 2024 07:44

Roasbeef approved these changes Jan 12, 2024

View reviewed changes

Roasbeef merged commit 1741b14 into main Jan 12, 2024
14 checks passed

guggero deleted the custodian-local-universe branch January 12, 2024 08:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/2] custodian: look up proofs in local universe as well #726

[1/2] custodian: look up proofs in local universe as well #726

guggero commented Dec 6, 2023 •

edited

Loading

Roasbeef left a comment

Roasbeef Dec 9, 2023

Roasbeef Dec 9, 2023

guggero Dec 13, 2023

Roasbeef Jan 12, 2024

jharveyb left a comment

jharveyb commented Dec 14, 2023

ffranr commented Dec 20, 2023 •

edited

Loading

guggero commented Dec 20, 2023

guggero commented Dec 20, 2023

jharveyb commented Dec 21, 2023 •

edited

Loading

lightninglabs-deploy commented Dec 28, 2023

guggero commented Jan 3, 2024

jharveyb left a comment

guggero commented Jan 9, 2024

Roasbeef left a comment

Roasbeef Jan 12, 2024

Roasbeef Jan 12, 2024

Roasbeef Jan 12, 2024

Roasbeef Jan 12, 2024

[1/2] custodian: look up proofs in local universe as well #726

[1/2] custodian: look up proofs in local universe as well #726

Conversation

guggero commented Dec 6, 2023 • edited Loading

Roasbeef 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

jharveyb left a comment

Choose a reason for hiding this comment

jharveyb commented Dec 14, 2023

ffranr commented Dec 20, 2023 • edited Loading

guggero commented Dec 20, 2023

guggero commented Dec 20, 2023

jharveyb commented Dec 21, 2023 • edited Loading

lightninglabs-deploy commented Dec 28, 2023

guggero commented Jan 3, 2024

jharveyb left a comment

Choose a reason for hiding this comment

guggero commented Jan 9, 2024

Roasbeef 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

guggero commented Dec 6, 2023 •

edited

Loading

ffranr commented Dec 20, 2023 •

edited

Loading

jharveyb commented Dec 21, 2023 •

edited

Loading