Skip to content

cnct: decode onion for incoming contest resolver#3594

Merged
Roasbeef merged 4 commits into
lightningnetwork:masterfrom
joostjager:resolver-onion-decode
Nov 13, 2019
Merged

cnct: decode onion for incoming contest resolver#3594
Roasbeef merged 4 commits into
lightningnetwork:masterfrom
joostjager:resolver-onion-decode

Conversation

@joostjager
Copy link
Copy Markdown
Contributor

@joostjager joostjager commented Oct 11, 2019

Also for on-chain resolution, we may require access to the onion payload. An example of this is the resolution of an mpp shard. We need the mpp fields in order to properly offer the htlc to the invoice registry.

@joostjager joostjager added incomplete WIP PR, not fully finalized, but light review possible blocked labels Oct 11, 2019
@joostjager joostjager changed the title cnct: decode onion for incoming contest resolver cnct: decode onion for incoming contest resolver [wip] Oct 11, 2019
@joostjager joostjager self-assigned this Oct 23, 2019
Comment thread contractcourt/channel_arbitrator.go Outdated
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
Comment thread contractcourt/channel_arbitrator.go Outdated
@lightningnetwork lightningnetwork deleted a comment from KarolStrzelec Nov 1, 2019
@joostjager joostjager force-pushed the resolver-onion-decode branch from 677cf2d to 9ef9187 Compare November 1, 2019 08:13
@joostjager joostjager requested a review from halseth as a code owner November 1, 2019 08:13
@joostjager joostjager force-pushed the resolver-onion-decode branch 8 times, most recently from b80d854 to 830acc4 Compare November 5, 2019 14:08
@joostjager
Copy link
Copy Markdown
Contributor Author

Changed the design slightly to make sure we always parse the onion blob. Also if we didn't just restart.

@joostjager joostjager force-pushed the resolver-onion-decode branch from 830acc4 to 5dc51b3 Compare November 5, 2019 14:32
@joostjager joostjager changed the title cnct: decode onion for incoming contest resolver [wip] cnct: decode onion for incoming contest resolver Nov 5, 2019
@joostjager joostjager force-pushed the resolver-onion-decode branch from 5dc51b3 to 4e5aac0 Compare November 6, 2019 10:43
Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

Latest version LGTM! Nice refactor on resolver supplements, only some minor nits

Comment thread contractcourt/contract_resolvers.go Outdated
Comment thread contractcourt/htlc_success_resolver.go Outdated
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
@cfromknecht
Copy link
Copy Markdown
Contributor

would be nice to get this in before #3644

@joostjager joostjager force-pushed the resolver-onion-decode branch 2 times, most recently from d7b9f79 to 4c8f0cf Compare November 7, 2019 09:07
Comment thread contractcourt/htlc_incoming_contest_resolver.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

how could we lock in such an HTLC?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

crash before we reached processRemoteAdds in the link

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So we will lock in the HTLC, crash, restart and forget about it? Shouldn't we either forward or cancel it in that case?

Just seems a bit dangerous to ignore a locked in HTLC without returning any error here...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can forget about it, because the onion payload is invalid. In the off-chain flow nothing would have happened either.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if the payload is invalid we would have canceled the htlc back offchain. now that we are on chain we can't cancel (or decode the payload and figure out what needs to be done) so we just let the incoming party timeout the htlc

@joostjager joostjager force-pushed the resolver-onion-decode branch from 4c8f0cf to 7bdcd0d Compare November 7, 2019 10:49
@joostjager joostjager requested a review from halseth November 7, 2019 10:49
@joostjager joostjager force-pushed the resolver-onion-decode branch 3 times, most recently from 9f536a1 to 710819a Compare November 7, 2019 21:41
@joostjager joostjager added this to the 0.9.0 milestone Nov 12, 2019
@joostjager joostjager force-pushed the resolver-onion-decode branch 2 times, most recently from 257e2cd to 13891fe Compare November 12, 2019 13:58
This change prepares for accessing the onion blob from a resolver.
With the introduction of additional payload fields for mpp, it becomes
a necessity to have their values available in the on-chain resolution
flow. The incoming contest resolver notifies the invoice registry of the
arrival of a payment and needs to supply all parameters for the registry
to validate the htlc.
@joostjager
Copy link
Copy Markdown
Contributor Author

@cfromknecht re-requesting review, as I got a rebase that was a bit more involved. I think it is better to be a bit more cautious with this sensitive part of the code.

Copy link
Copy Markdown
Contributor

@cfromknecht cfromknecht left a comment

Choose a reason for hiding this comment

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

LGTM


// decodePayload (re)decodes the hop payload of a received htlc.
func (h *htlcIncomingContestResolver) decodePayload() (*hop.Payload, error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: extra new line

Copy link
Copy Markdown
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 merged commit 9cc4125 into lightningnetwork:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

amp incomplete WIP PR, not fully finalized, but light review possible onion routing routing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants