Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Chain porter proof courier service initialised using configurable address #459

Merged
merged 7 commits into from
Aug 30, 2023

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Aug 21, 2023

This PR includes changes which allow us to persist a proof courier address for use in resuming a pending package's send process.

Fixes #446

@ffranr ffranr changed the title Chain porter proof courier service initialised using new address Chain porter proof courier service initialised using configurable address Aug 21, 2023
@ffranr
Copy link
Contributor Author

ffranr commented Aug 21, 2023

Having spoken with @guggero , I'm going to modify this PR as follows:

The goal is for the proof courier address to survive the chain porter send package log/resume cycle so that it is available when constructing the proof courier service handle.

To accomplish that, instead of adding the proof courier address as a new field in the asset transfer table, I will add a new addr field to vPSBT outputs and corresponding db table.

@guggero
Copy link
Member

guggero commented Aug 21, 2023

Just to clarify: my intention was to store the full, serialized tap-address in the vPSBT and transfer_output_table, not just the proof courier address. that way we get ourselves some future proofing if we ever add another attribute to a tap-address, then don't need yet another field in vPSBT/transfer-output.
I think that outweighs the downside of making the vPSBT slightly bigger with the somewhat duplicate info. Or do you strongly disagree on this, @Roasbeef?

Base automatically changed from add-proof-courier-to-addr to main August 22, 2023 07:42
@Roasbeef
Copy link
Member

To accomplish that, instead of adding the proof courier address as a new field in the asset transfer table,

Or we just also store the full address as well which may be useful for record keeping purposes?

serialized tap-address in the vPSBT and transfer_output_table, not just the proof courier address. that way we get ourselves some future proofing if we ever add another attribute to a tap-address, then don't need yet another field in vPSBT/transfer-output.

Why's it need to be the vPBST vs just a new column in the row?

@ffranr ffranr force-pushed the hashmail-courier-session branch from 3c26ba3 to ee66887 Compare August 25, 2023 15:54
@ffranr
Copy link
Contributor Author

ffranr commented Aug 26, 2023

Following a call with @Roasbeef , this is the latest direction for this PR:

Where the chain porter is concerned, we no longer add the Tap address to VPacket.VOutput. Instead, we now carry the addresses in sendPackage using a map from output index to Tap address. Then, we carry the proof courier address only in transfer output and it's SQL table.

This PR also includes changes to the custodian, which now instantiates a proof courier service using the address that we lookup in our address book.

This PR also changes the proof.Courier interface: proof.Courier[Addr any] -> proof.Courier. However, the recipient arg still remains in the DeliverProof and ReceiveProof methods. My idea there is that we might, in the future, want to connect to a service once and then reuse that service for multiple different recipient lookups. So I figured I wouldn't make a courier service handle specific to a recipient.

I think the following test breaks between 00:00 and 01:00 https://github.com/lightninglabs/taproot-assets/blob/fad3d845f68ec7fa09eb0c05f5f9c7455358928e/tapdb/universe_stats_test.go#L201C42-L201C42

This PR doesn't include a new RpcCourier, I'll add that in another PR. I would prefer not to extend this PR if possible.

This PR should now also close #462

@ffranr ffranr marked this pull request as ready for review August 26, 2023 00:29
@ffranr ffranr requested review from Roasbeef and guggero August 26, 2023 00:29
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice progress on this! Everything around the proof courier interfaces and delivery looks great. Though I don't think we can get away without storing the address in the vPSBT as well (see inline comment) for the full vPSBT interactive cases.

tapfreighter/parcel.go Outdated Show resolved Hide resolved
tapfreighter/chain_porter.go Show resolved Hide resolved
tapfreighter/chain_porter.go Show resolved Hide resolved
@ffranr ffranr force-pushed the hashmail-courier-session branch from 99763e6 to 7a6f08c Compare August 29, 2023 12:13
@ffranr ffranr requested a review from guggero August 29, 2023 12:14
tapfreighter/parcel.go Show resolved Hide resolved
proof/courier.go Outdated 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
tapcfg/server.go Outdated Show resolved Hide resolved
tapgarden/custodian.go Outdated Show resolved Hide resolved
tapgarden/custodian.go Show resolved Hide resolved
tapcfg/server.go Outdated Show resolved Hide resolved
tapfreighter/chain_porter.go Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending @Roasbeef's comments.

tappsbt/address.go Outdated Show resolved Hide resolved
proof/courier.go Outdated Show resolved Hide resolved
@ffranr ffranr force-pushed the hashmail-courier-session branch from 7a6f08c to 0be6f9d Compare August 30, 2023 11:44
@ffranr ffranr force-pushed the hashmail-courier-session branch from 0be6f9d to 9d2cd98 Compare August 30, 2023 13:13
@ffranr ffranr requested a review from Roasbeef August 30, 2023 13:23
Instead of specifying a TLS certificate file path, we now skip TLS
certificate verification whilst dialing into the proof courier service.
@ffranr ffranr force-pushed the hashmail-courier-session branch from 9d2cd98 to e5ef9bd Compare August 30, 2023 13:50
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🏄🏾

@Roasbeef Roasbeef added this pull request to the merge queue Aug 30, 2023
@Roasbeef Roasbeef removed this pull request from the merge queue due to a manual request Aug 30, 2023
@Roasbeef Roasbeef merged commit 4a0a16c into main Aug 30, 2023
@guggero guggero deleted the hashmail-courier-session branch August 31, 2023 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

Asset send/receive procedure makes use of Tap address hashmail proof courier addr
3 participants