Add multiverse RPC proof courier #473

ffranr · 2023-08-29T17:42:34Z

This PR adds a new multiverse RPC proof service handle for use in the chain porter.

This PR should close these issues when merged:

Generalise proof query/insert RPC endpoints to support transfer proofs #439
Add test to ensure that a transfer proof can be inserted using RPC endpoint InsertProof and retrieved using RPC endpoint QueryProof. #467
Implement multiverse transfer proof distribution via RPC courier #344

This change is

proof/courier.go

ffranr · 2023-08-31T21:01:37Z

The integration test changes reveal that the proof insertion into the new universe RPC courier is failing. I think it would still be worth getting a first review on this PR given the team's schedule.

guggero

Looks very good! I dig how small the diff turned out to be in the end 🎉

itest/tapd_harness.go

itest/universerpc_harness.go

proof/courier.go

dstadulis · 2023-09-04T15:43:02Z

This PR will need to rebase to incorporate #483 (which may be subsumed into a PR @guggero opens)

ffranr · 2023-09-04T16:48:01Z

Should still be ready for review. Merge is waiting for rebase on #484 to double check that the itest still pass.

ffranr · 2023-09-04T22:08:33Z

I think this is failing during proof verification. The proof courier's VM is trying to verify that the asset associated with the proof was correctly split from its input asset. But I don't think that the input asset snapshot is available in the proof.

In a separate part of the codebase we perform proof verification on a complete proof file. And in this way we have the previous asset snapshot at any given transition proof. See https://github.com/ffranr/taproot-assets/blob/main/proof/verifier.go#L433-L450

When inserting a proof into the universe RPC courier we insert every proof in turn starting at the issuance proof. So we could reform the proof file before verification. But this strategy would entail reforming the proof file every time a transition proof is inserted into the courier.

@guggero any thoughts on this?

guggero

Took another look, looking really good. The issue with the empty PrevID is the split commitment as suspected. Should be very easy to fix.

taprpc/universerpc/marshal.go

proof/courier.go

universe/base.go

Roasbeef

Great work! Gotta love it when a design plan comes together neatly 😎

Main thing that popped out is to ensure that we're handling cases of merged inputs for an asset properly.

taprpc/marshal.go

taprpc/universerpc/marshal.go

Roasbeef · 2023-09-13T01:57:17Z

proof/proof.go

-	// PrevOut is the previous on-chain outpoint of the asset.
+	// PrevOut is the previous on-chain outpoint of the asset. This outpoint
+	// is that of the first on-chain input. Outpoints which correspond to
+	// the other inputs can be found in AdditionalInputs.


Not sure this is a clarification: this is the outpoint where the asset currently resides (or previously).

taproot-assets/proof/verifier.go

Lines 310 to 313 in f37f45f

}

if !txSpendsPrevOut(&p.AnchorTx, &p.PrevOut) {

return nil, commitment.ErrInvalidTaprootProof // TODO

}

The existence of PrevOut as a field was/is confusing to me. Because in the context of a multi input spend (merge) we seem to make an arbitrary choice of "the asset" that we're spending, is that correct?

this is the outpoint where the asset currently resides (or previously).

In the context of a multi input spend, what do you mean by "the asset" here?

It's this confusion/uncertainty that motivated me to add more to the comment to make sure I understand what's going on. I think by "the asset" we mean the first input, and that's what I've tried to capture in the comment.

asset/asset.go

proof/courier.go

Roasbeef · 2023-09-13T02:05:29Z

proof/courier.go

+			return nil, err
+		}
+
+		revProofs = append(revProofs, transitionProof)


Re this and above, what about the additional inputs? So the case where you have an asset, but it was created by merging 3 change UTXOs.

Each individual (transition) Proof contains a field called AdditionalInputs []File. The entire proof file for each additional inputs is included in that field. At least, that's my current understanding after speaking with Oli.

We noticed that this is a memory inefficiency within the universe tree (on the courier server side). Each leaf of the universe tree contains a proof and each one of those proofs may contain entire proof files for additional inputs. Those proof files may already exist in the universe as other leaves.

However, from a tapd peer sending/receiving perspective, this setup makes things simpler. I'll add an issue to talk about the potential optimisation here. It will help me get a clear picture of what's going on.

Issue here: #503

proof/courier.go

universe/base.go

Roasbeef · 2023-09-13T02:11:40Z

universe/base.go

+	// previous (input) asset. We know that it was already verified
+	// as it was present in the multiverse/universe archive.
+	return &proof.AssetSnapshot{
+		Asset:    &prevProof.Asset,


What about the other set of assets in the case of inputs merged to create this new asset?

I think this relies on the additional inputs field, see https://github.com/ffranr/taproot-assets/blob/main/proof/verifier.go#L206-L213

And my answer here: #473 (comment)

Roasbeef · 2023-09-13T02:16:16Z

Also I think we can use the HasSplitCommitmentWitness method in most places here?

ffranr · 2023-09-13T18:33:54Z

Also I think we can use the HasSplitCommitmentWitness method in most places here?

Where are you referring to please @Roasbeef ?

This refactor allows us to avoid an import cycle when the RPC proof courier marshals an asset.

This commit adds a method to the Asset structure which returns the asset's primary PrevID and takes account of the fact that the asset may correspond to a split output.

Roasbeef

LGTM 🐊

Reviewed 6 of 10 files at r7, 7 of 7 files at r8, all commit messages.
Reviewable status: 13 of 17 files reviewed, 34 unresolved discussions (waiting on @ffranr and @guggero)

tapgarden/custodian.go line 391 at r8 (raw file):

			// Sleep to give the sender an opportunity to transfer
			// the proof to the proof courier service.
			time.Sleep(defaultProofRetrievalDelay)

I think let's endeavor to remove this in a follow up PR. IMO the backoff should just handle this, and it can speed up our itests with it gone as well.

ffranr changed the base branch from main to hashmail-courier-session August 29, 2023 17:42

ffranr self-assigned this Aug 29, 2023

ffranr force-pushed the rpc-courier branch from 5e2fce0 to 8e75eb8 Compare August 29, 2023 17:45

ffranr force-pushed the hashmail-courier-session branch 3 times, most recently from 9d2cd98 to e5ef9bd Compare August 30, 2023 13:50

ffranr commented Aug 30, 2023

View reviewed changes

proof/courier.go Outdated Show resolved Hide resolved

Base automatically changed from hashmail-courier-session to main August 31, 2023 07:29

ffranr force-pushed the rpc-courier branch 4 times, most recently from d4462b5 to 6590830 Compare August 31, 2023 21:00

ffranr marked this pull request as ready for review August 31, 2023 21:02

ffranr requested review from Roasbeef and guggero August 31, 2023 21:03

guggero reviewed Sep 1, 2023

View reviewed changes

itest/tapd_harness.go Outdated Show resolved Hide resolved

itest/universerpc_harness.go Show resolved Hide resolved

proof/courier.go Outdated Show resolved Hide resolved

proof/courier.go Outdated Show resolved Hide resolved

proof/courier.go Show resolved Hide resolved

ffranr force-pushed the rpc-courier branch 2 times, most recently from 15d2cfe to ab506bb Compare September 4, 2023 11:48

ffranr requested a review from guggero September 4, 2023 11:59

ffranr force-pushed the rpc-courier branch from acf2c94 to ab506bb Compare September 4, 2023 14:12

ffranr force-pushed the rpc-courier branch from ab506bb to c9a6b45 Compare September 4, 2023 21:43

ffranr changed the base branch from main to proof-courier-ntfn September 4, 2023 21:43

Base automatically changed from proof-courier-ntfn to main September 5, 2023 17:33

guggero reviewed Sep 6, 2023

View reviewed changes

taprpc/universerpc/marshal.go Outdated Show resolved Hide resolved

proof/courier.go Show resolved Hide resolved

universe/base.go Outdated Show resolved Hide resolved

universe/base.go Show resolved Hide resolved

universe/base.go Outdated Show resolved Hide resolved

ffranr force-pushed the rpc-courier branch 2 times, most recently from fc55f2c to ee04e76 Compare September 6, 2023 15:48

ffranr mentioned this pull request Sep 11, 2023

Generalise multiverse method names to support all proofs. #496

Merged

ffranr linked an issue Sep 11, 2023 that may be closed by this pull request

Add test to ensure that a transfer proof can be inserted using RPC endpoint InsertProof and retrieved using RPC endpoint QueryProof. #467

Closed

ffranr mentioned this pull request Sep 11, 2023

multi: add CLI flag to enable public access to uni proof courier RPCs #499

Merged

Roasbeef reviewed Sep 13, 2023

View reviewed changes

ffranr force-pushed the rpc-courier branch from 4250666 to 0fc1b94 Compare September 13, 2023 18:48

ffranr requested a review from Roasbeef September 13, 2023 18:49

ffranr added 13 commits September 19, 2023 13:39

tapfreighter: fix proof transition state doc

f90fa56

rpc: move MarshalAsset function to new file taprpc/marshal.go

9899ee4

This refactor allows us to avoid an import cycle when the RPC proof courier marshals an asset.

taprpc: add marshal functions for universe RPC types

9184c17

proof: clarify doc for Proof.PrevOut

d97665f

asset: add asset method for retrieving PrevID which handles split commit

fa90ec9

This commit adds a method to the Asset structure which returns the asset's primary PrevID and takes account of the fact that the asset may correspond to a split output.

multi: add universe RPC proof courier

3d768f7

tapgarden: add proof retrieval delay to custodian

1a180af

proof: remove roasbeef's TODO now that we have two different couriers

a3ada70

rpc: add debug messages for InsertProof and QueryProof endpoints

8904c8d

universe: allow registering non-issuance proofs in universe/multiverse

6b6b749

universe: use previous asset snapshot when verifying transition proof

0417d8d

itest: fix nits

f39dd1a

itest: add universe RPC harness and utilise for basic send

147af79

ffranr force-pushed the rpc-courier branch 3 times, most recently from ab506bb to 147af79 Compare September 19, 2023 13:39

Roasbeef requested a review from guggero September 19, 2023 18:30

Roasbeef approved these changes Sep 19, 2023

View reviewed changes

Roasbeef merged commit 90b0ac3 into main Sep 19, 2023
14 checks passed

This was referenced Sep 19, 2023

Generalise proof query/insert RPC endpoints to support transfer proofs #439

Closed

Implement multiverse transfer proof distribution via RPC courier #344

Closed

guggero deleted the rpc-courier branch September 19, 2023 18:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multiverse RPC proof courier #473

Add multiverse RPC proof courier #473

ffranr commented Aug 29, 2023 •

edited by Roasbeef

Loading

ffranr commented Aug 31, 2023 •

edited

Loading

guggero left a comment

dstadulis commented Sep 4, 2023

ffranr commented Sep 4, 2023 •

edited

Loading

ffranr commented Sep 4, 2023

guggero left a comment

Roasbeef left a comment

Roasbeef Sep 13, 2023

ffranr Sep 13, 2023

Roasbeef Sep 13, 2023

ffranr Sep 13, 2023

ffranr Sep 13, 2023

Roasbeef Sep 13, 2023

ffranr Sep 13, 2023

Roasbeef commented Sep 13, 2023

ffranr commented Sep 13, 2023

Roasbeef left a comment

	}
	if !txSpendsPrevOut(&p.AnchorTx, &p.PrevOut) {
	return nil, commitment.ErrInvalidTaprootProof // TODO
	}

Add multiverse RPC proof courier #473

Add multiverse RPC proof courier #473

Conversation

ffranr commented Aug 29, 2023 • edited by Roasbeef Loading

ffranr commented Aug 31, 2023 • edited Loading

guggero left a comment

Choose a reason for hiding this comment

dstadulis commented Sep 4, 2023

ffranr commented Sep 4, 2023 • edited Loading

ffranr commented Sep 4, 2023

guggero left a comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef Sep 13, 2023

Choose a reason for hiding this comment

ffranr Sep 13, 2023

Choose a reason for hiding this comment

Roasbeef Sep 13, 2023

Choose a reason for hiding this comment

ffranr Sep 13, 2023

Choose a reason for hiding this comment

ffranr Sep 13, 2023

Choose a reason for hiding this comment

Roasbeef Sep 13, 2023

Choose a reason for hiding this comment

ffranr Sep 13, 2023

Choose a reason for hiding this comment

Roasbeef commented Sep 13, 2023

ffranr commented Sep 13, 2023

Roasbeef left a comment

Choose a reason for hiding this comment

ffranr commented Aug 29, 2023 •

edited by Roasbeef

Loading

ffranr commented Aug 31, 2023 •

edited

Loading

ffranr commented Sep 4, 2023 •

edited

Loading