server+contractcourt: create breachResolver to ensure htlc's are failed back #6158

Crypt-iQ · 2022-01-13T20:10:33Z

This pull request fails back HTLCs if we detect our remote party has broadcasted a breach transaction. This is accomplished by launching a breachResolver like any other resolver. A nice bonus is that since the breach-lifecycle is now tracked in the channel arbitrator, we can tack on an anchorResolver and sweep the anchor output. This would also allow the breacharbiter code to be moved into this breachResolver in a later changeset.

Fixes #3494

Roasbeef

Happy to finally resolve this cancel back issue, and also incrementally refactor things to let us bring this breach logic into a resolver, which was created only after the current breach arb was dreamed up!

Completed an initial pass, need to double check some assumptions here w.r.t restarts, and also handling cases where we didn't have a breach resolver for older nodes.

contractcourt/breach_resolver.go

contractcourt/channel_arbitrator.go

contractcourt/chain_watcher.go

yyforyongyu

Nice set of improvements! Will do a second round to review the tests and to understand the impact, meanwhile got a few questions.

contractcourt/breach_resolver.go

contractcourt/briefcase.go

yyforyongyu · 2022-01-17T18:47:53Z

contractcourt/chain_watcher.go

@@ -57,6 +57,11 @@ type RemoteUnilateralCloseInfo struct {
 	CommitSet CommitSet
 }

+// BreachResolution wraps the outpoint of the breached channel.
+type BreachResolution struct {
+	FundingOutPoint wire.OutPoint


Since the top bucket logScope already has the chanPoint info, seems like a bool can carry enough info here to make sure our db is compact?

That's true, this FundingOutPoint does not need to be here since in the breachResolver it will have access to the chanpoint through the resolver kit. I didn't know what else to put here. What would the bool store? I don't believe we can just store a nil value in bbolt because I would have done this

If we don't want to store this, but want to still store something useful, we could store the state number being breached.

I was thinking a version field in case the breachresolution needs to change when we upgrade it later?

I think we could achieve that simply with a new TLV type?

Either way we need to settle on what we want to store now. Funding outpoint here is fine, just that it doesn't give us any other unique information fwiw.

We also have the option to use TLV within the serialization as well which gives us a flexible upgrade path.

Don't have a better option here. Initially was thinking storing a 1 might be enough and save us some space. Since a breach rarely happens, I think it's fine to leave it as is.

I think that it's fine to store as-is, I would have liked to have caught this earlier in the process, but oh well. Wouldn't a TLV where the value is the version be the same as just storing the version but with slightly more bytes?

contractcourt/chain_watcher.go

yyforyongyu · 2022-01-17T19:09:31Z

contractcourt/chain_watcher.go

+	}
+
+	// We'll set the ConfCommitKey here as the remote htlc set. This is
+	// only used to ensure a nil-pointer-dereference doesn't occur and is


If I got this doc right, this can be dangerous if we are setting a value just to get around the nil pointer issue. Shouldn't we just check against nil?

We can't use nil without changing the way encodeHtlcSetKey works in InsertConfirmedCommitSet. InsertConfirmedCommitSet will need a ConfCommitKey when encoding the commit set. An alternative to this could be making a new ConfCommitKey since 3/4 are used we can "make" another one. IMO seems like more effort than is necessary as the callpath later on just uses RemoteHtlcSet in prepContractResolutions which calls constructChainActions which the breach case ignores. No codepath currently uses nil in the ConfCommitKey so it would only be dangerous if a code-path used nil.

This is correct though AFAICT: we're handling a breach, which means the remote party force closed, which means we only have one main commitment to play.

@Crypt-iQ thanks for the detailed explanation! My main concern is that we are setting a value where it shouldn't, as later on other callers may access it while it doesn't fulfill what it claims to be, thus increasing the cognitive load. Looks like it's the very ending of the code-path so I'd figure it should be fine, plus based on @Roasbeef 's comment I think the ConfCommitKey is indeed the RemoteHtlcSet?

The breach case is technically none of these ConfCommitKey - they track the current state of the commitment transactions (local, remote, remote pending) where the breach case is in the past. It is a remote commit tx, but some parts of the code in the channel arbitrator treats these commit keys as "current" to make decisions with so that's why I say the breach case is not really RemoteHtlcSet. After looking at the possible options I decided on the first one:

keep it as is in the pr

set nil and adjust all call-sites to handle nil, knowing it's the breach case

create a new confcommitkey that is the breach case

downside is that IsRemote would be false and IsPending would be true which is not the case and any code relying on IsRemote or IsPending may be messed up in the future

lnd/contractcourt/channel_arbitrator.go

Lines 280 to 290 in 234fdc6

var (

// LocalHtlcSet is the HtlcSetKey used for local commitments.

LocalHtlcSet = HtlcSetKey{IsRemote: false, IsPending: false}

// RemoteHtlcSet is the HtlcSetKey used for remote commitments.

RemoteHtlcSet = HtlcSetKey{IsRemote: true, IsPending: false}

// RemotePendingHtlcSet is the HtlcSetKey used for dangling remote

// commitment transactions.

RemotePendingHtlcSet = HtlcSetKey{IsRemote: true, IsPending: true}

)

go from the current scheme to something different

contractcourt/breach_resolver.go

Roasbeef · 2022-01-21T01:56:59Z

contractcourt/chain_watcher.go

@@ -57,6 +57,11 @@ type RemoteUnilateralCloseInfo struct {
 	CommitSet CommitSet
 }

+// BreachResolution wraps the outpoint of the breached channel.
+type BreachResolution struct {
+	FundingOutPoint wire.OutPoint


If we don't want to store this, but want to still store something useful, we could store the state number being breached.

Roasbeef · 2022-01-21T02:01:20Z

contractcourt/chain_watcher.go

+	}
+
+	// We'll set the ConfCommitKey here as the remote htlc set. This is
+	// only used to ensure a nil-pointer-dereference doesn't occur and is


This is correct though AFAICT: we're handling a breach, which means the remote party force closed, which means we only have one main commitment to play.

Roasbeef

LGTM 🪟

Working on a set up to be able to manually test some of the added backwards compatibility here.

Roasbeef · 2022-01-24T23:53:38Z

contractcourt/breach_resolver.go

+func newBreachResolver(resCfg ResolverConfig) *breachResolver {
+	r := &breachResolver{
+		contractResolverKit: *newContractResolverKit(resCfg),
+		replyChan:           make(chan struct{}),


Could safely be buffered to a size of 1.

I opted to close it, do you think it should be passed a struct{}{} instead?

Roasbeef · 2022-01-24T23:56:32Z

contractcourt/chain_watcher.go

@@ -57,6 +57,11 @@ type RemoteUnilateralCloseInfo struct {
 	CommitSet CommitSet
 }

+// BreachResolution wraps the outpoint of the breached channel.
+type BreachResolution struct {
+	FundingOutPoint wire.OutPoint


I think we could achieve that simply with a new TLV type?

Either way we need to settle on what we want to store now. Funding outpoint here is fine, just that it doesn't give us any other unique information fwiw.

We also have the option to use TLV within the serialization as well which gives us a flexible upgrade path.

yyforyongyu

Looks pretty good, just a few nits, also the state transition doc needs an update, feel free to copy this commit.

Meanwhile I think the changes made here have already been covered by our itest right?

yyforyongyu · 2022-01-25T07:16:12Z

contractcourt/breacharbiter.go

+
+	// Otherwise since the channel point is not resolved, add a
+	// subscription. There can only be one subscription per channel point.
+	b.subscriptionsMtx.Lock()


Can just do b.Lock() here? Why do we need subscriptionsMtx since they are locking the same thing?

Good point, habit of mine to have a mutex with a map, will change

yyforyongyu · 2022-01-25T07:21:00Z

contractcourt/briefcase.go

@@ -45,7 +49,7 @@ func (c *ContractResolutions) IsEmpty() bool {
 	return c.CommitResolution == nil &&
 		len(c.HtlcResolutions.IncomingHTLCs) == 0 &&
 		len(c.HtlcResolutions.OutgoingHTLCs) == 0 &&
-		c.AnchorResolution == nil
+		c.AnchorResolution == nil && c.BreachResolution == nil


nit: the godoc needs an update

yyforyongyu · 2022-01-25T08:24:55Z

contractcourt/channel_arbitrator.go

@@ -1996,10 +2027,62 @@ func (c *ChannelArbitrator) prepContractResolutions(
 	commitHash := contractResolutions.CommitHash
 	failureMsg := &lnwire.FailPermanentChannelFailure{}

+	var htlcResolvers []ContractResolver


The order doesn't matter here right? Looks like we used to put anchor resolver at the end.

Order doesn't matter, caller just launches them

yyforyongyu · 2022-01-25T08:37:09Z

contractcourt/chain_watcher.go

+	}
+
+	// We'll set the ConfCommitKey here as the remote htlc set. This is
+	// only used to ensure a nil-pointer-dereference doesn't occur and is


@Crypt-iQ thanks for the detailed explanation! My main concern is that we are setting a value where it shouldn't, as later on other callers may access it while it doesn't fulfill what it claims to be, thus increasing the cognitive load. Looks like it's the very ending of the code-path so I'd figure it should be fine, plus based on @Roasbeef 's comment I think the ConfCommitKey is indeed the RemoteHtlcSet?

yyforyongyu · 2022-01-25T08:42:02Z

contractcourt/channel_arbitrator_test.go

@@ -686,13 +704,95 @@ func TestChannelArbitratorBreachClose(t *testing.T) {
 	// It should start out in the default state.
 	chanArbCtx.AssertState(StateDefault)

+	outgoingIdx := uint64(2)
+
+	rHash1 := [lntypes.PreimageSize]byte{1, 2, 3}


nit: can we add some docs here saying we've created one outgoing and one incoming HTLCs on the remote commit and test that only the outgoing one is resolved? Can be helpful for future developers.

…iter Introduces a breachResolver that subscribes to the breacharbiter to determine if the final justice transaction has confirmed and can clean itself up.

Also transitions to the proper state based on if this is a legacy breach in the channel arbitrator or a modern breach with a resolver.

This also changes the chain_watcher and breacharbiter handoff. The new logic ensures that the channel is only marked as pending closed when the channel arbitrator has persisted the resolutions and commit set.

yyforyongyu

Nice path on this one, LGTM👍

A total off-topic Q, looks like the commits are not signed...?

Crypt-iQ added contracts refactoring labels Jan 13, 2022

Crypt-iQ added this to the v0.14.2 milestone Jan 13, 2022

Crypt-iQ force-pushed the breach_resolver_cancel branch from 7ed52f1 to e3db3ab Compare January 13, 2022 20:52

Crypt-iQ requested review from yyforyongyu and Roasbeef January 13, 2022 20:57

Roasbeef requested changes Jan 14, 2022

View reviewed changes

yyforyongyu reviewed Jan 17, 2022

View reviewed changes

Roasbeef requested changes Jan 21, 2022

View reviewed changes

Crypt-iQ force-pushed the breach_resolver_cancel branch 2 times, most recently from 52a436f to e23fa57 Compare January 24, 2022 20:07

Roasbeef approved these changes Jan 24, 2022

View reviewed changes

yyforyongyu reviewed Jan 25, 2022

View reviewed changes

Crypt-iQ added 5 commits January 25, 2022 15:42

server+contractcourt: add breachResolver that subscribes to breacharb…

f99933f

…iter Introduces a breachResolver that subscribes to the breacharbiter to determine if the final justice transaction has confirmed and can clean itself up.

contractcourt: handle writing BreachResolution in arbitrator log

af03c8c

contractcourt: create breachResolver if BreachResolution present

b740788

Also transitions to the proper state based on if this is a legacy breach in the channel arbitrator or a modern breach with a resolver.

server+contractcourt: hand-off BreachCloseInfo to channel arbitrator

ebaffb9

This also changes the chain_watcher and breacharbiter handoff. The new logic ensures that the channel is only marked as pending closed when the channel arbitrator has persisted the resolutions and commit set.

docs/release-notes: update notes for 0.14.2

64a51c1

Crypt-iQ force-pushed the breach_resolver_cancel branch from e23fa57 to 64a51c1 Compare January 25, 2022 20:43

yyforyongyu approved these changes Jan 25, 2022

View reviewed changes

Roasbeef merged commit a57c650 into lightningnetwork:master Jan 26, 2022

Crypt-iQ deleted the breach_resolver_cancel branch January 26, 2022 15:05

guggero mentioned this pull request Jan 26, 2022

multi: create v0.14.2-beta-rc1 branch #6201

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server+contractcourt: create breachResolver to ensure htlc's are failed back #6158

server+contractcourt: create breachResolver to ensure htlc's are failed back #6158

Crypt-iQ commented Jan 13, 2022 •

edited by Roasbeef

Loading

Roasbeef left a comment

yyforyongyu left a comment

yyforyongyu Jan 17, 2022

Crypt-iQ Jan 18, 2022

Roasbeef Jan 21, 2022

Crypt-iQ Jan 21, 2022 •

edited

Loading

Roasbeef Jan 24, 2022

yyforyongyu Jan 25, 2022

Crypt-iQ Jan 25, 2022 •

edited

Loading

yyforyongyu Jan 17, 2022

Crypt-iQ Jan 18, 2022 •

edited

Loading

Roasbeef Jan 21, 2022

yyforyongyu Jan 25, 2022

Crypt-iQ Jan 25, 2022

Roasbeef Jan 21, 2022

Roasbeef Jan 21, 2022

Roasbeef left a comment

Roasbeef Jan 24, 2022

Crypt-iQ Jan 25, 2022

Roasbeef Jan 24, 2022

yyforyongyu left a comment

yyforyongyu Jan 25, 2022

Crypt-iQ Jan 25, 2022

Crypt-iQ Jan 25, 2022

yyforyongyu Jan 25, 2022

Crypt-iQ Jan 25, 2022

yyforyongyu Jan 25, 2022

Crypt-iQ Jan 25, 2022

yyforyongyu Jan 25, 2022

yyforyongyu Jan 25, 2022

Crypt-iQ Jan 25, 2022

yyforyongyu left a comment

	var (
	// LocalHtlcSet is the HtlcSetKey used for local commitments.
	LocalHtlcSet = HtlcSetKey{IsRemote: false, IsPending: false}

	// RemoteHtlcSet is the HtlcSetKey used for remote commitments.
	RemoteHtlcSet = HtlcSetKey{IsRemote: true, IsPending: false}

	// RemotePendingHtlcSet is the HtlcSetKey used for dangling remote
	// commitment transactions.
	RemotePendingHtlcSet = HtlcSetKey{IsRemote: true, IsPending: true}
	)

server+contractcourt: create breachResolver to ensure htlc's are failed back #6158

server+contractcourt: create breachResolver to ensure htlc's are failed back #6158

Conversation

Crypt-iQ commented Jan 13, 2022 • edited by Roasbeef Loading

Roasbeef left a comment

Choose a reason for hiding this comment

yyforyongyu 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

Crypt-iQ Jan 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Crypt-iQ Jan 18, 2022 • edited Loading

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

Choose a reason for hiding this comment

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

yyforyongyu 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

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

Choose a reason for hiding this comment

yyforyongyu left a comment

Choose a reason for hiding this comment

Crypt-iQ commented Jan 13, 2022 •

edited by Roasbeef

Loading

Crypt-iQ Jan 21, 2022 •

edited

Loading

Crypt-iQ Jan 25, 2022 •

edited

Loading

Crypt-iQ Jan 18, 2022 •

edited

Loading