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

breacharbiter: watch for spends on all inputs until reaching terminal states #2765

Merged
merged 4 commits into from Mar 22, 2019

Conversation

Projects
None yet
4 participants
@cfromknecht
Copy link
Collaborator

cfromknecht commented Mar 13, 2019

See second commit message for details

@cfromknecht cfromknecht changed the title breacharbiter: watch for spends on all inputs until reach terminal states breacharbiter: watch for spends on all inputs until reaching terminal states Mar 13, 2019

@cfromknecht cfromknecht added this to the 0.6 milestone Mar 13, 2019

@cfromknecht cfromknecht force-pushed the cfromknecht:brar-handle-tower-sweep branch from 5f848bc to 4d5466d Mar 13, 2019

@Roasbeef Roasbeef requested a review from wpaulino Mar 13, 2019

@cfromknecht cfromknecht force-pushed the cfromknecht:brar-handle-tower-sweep branch from 4d5466d to 3672aca Mar 14, 2019

Show resolved Hide resolved breacharbiter.go Outdated
Show resolved Hide resolved breacharbiter.go Outdated
Show resolved Hide resolved breacharbiter_test.go Outdated

@cfromknecht cfromknecht force-pushed the cfromknecht:brar-handle-tower-sweep branch from 3672aca to 1bf7b25 Mar 15, 2019

@wpaulino
Copy link
Collaborator

wpaulino left a comment

LGTM ⚠️

@halseth
Copy link
Collaborator

halseth left a comment

LGTM 🥖

SpendingTx: txn,
SpenderTxHash: &txnHash,
SpenderInputIndex: outpoint.Index,
SpentOutPoint: details.SpentOutPoint,

This comment has been minimized.

Copy link
@halseth

halseth Mar 18, 2019

Collaborator

Just send details instead of making a copy?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Mar 20, 2019

Author Collaborator

yeah just wanted to make sure the spend details cached inside the mock were somewhat isolated from the ones we give back to clients. should probably even make a deep copy, but didn't feel necessary atm :P

cfromknecht added some commits Mar 20, 2019

breacharbiter: detect all spends, add terminal states
This commit modifies the breach arbiter to monitor
all breached inputs for spends and remove them from
the set of inputs to be swept if they are spent to
a terminal state. Prior, we would only watch for
spends on htlcs that may need to transition and
sweep the corresponding second-level htlc.

With these changes, we will no monitor commitment
outputs for spends, as well as spends from the
second level htlcs themselves. If either of these
is detected, we remove them from the set of inputs
to sweep via the justice transaction because there
is nothing the breach arbiter can do.

This functionality will be needed when adding
watchtower support, as the breach arbiter must
detect the case when the tower sweeps on behalf of
the user and stop pursuing the sweep itself. In
addition, this now properly handles the potential
case where somehow the remote party is able to sweep
the their commitment or second-level htlc to their
wallet, and prevent the breach arbiter from trying
to sweep the outputs as it would now.

Note that in the latter event, the internal
accounting may still be incorrect, as it is assumed
that all breached funds return to the victim.
However, these issues will deferred and fixed at a
later date, as the more crucial aspect is that the
breach arbiter doesn't blow up as a result of towers
sweeping channels.

@cfromknecht cfromknecht force-pushed the cfromknecht:brar-handle-tower-sweep branch from 1bf7b25 to 9523b42 Mar 20, 2019

@Roasbeef
Copy link
Member

Roasbeef left a comment

Small comment style nit, but LGTM other than that (can be cleaned up with future changes to this area: possible sweeper integration?).

breachedOutput := &inputs[s.index]
delete(spendNtfns, breachedOutput.outpoint)

switch breachedOutput.witnessType {

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Mar 22, 2019

Member

Could use a comment here saying something along the lines of:

If the 1st level HTLCs are spent, that that means the remote party has went ahead and attempted to redeem them. For the other commitment outputs, they can only be spent (before the CSV timeout) by ourselves or the tower, so we can mark those as terminated.

@Roasbeef Roasbeef merged commit 5ee40df into lightningnetwork:master Mar 22, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-1.3%) to 59.742%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@cfromknecht cfromknecht deleted the cfromknecht:brar-handle-tower-sweep branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.