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

lnwallet: Remove breach txn from NewBreachRetribution #2430

Conversation

Projects
None yet
3 participants
@cfromknecht
Copy link
Collaborator

cfromknecht commented Jan 8, 2019

In this PR, we remove the passed breachTxn argument from NewBreachRetribution. We already store the transaction on disk, which we retrieve via the state number. Currently, we pull values from both the argument and the transaction loaded from disk, which is unnecessary since they should both be the same.

We modify this method to only inspect the on-disk transaction, as this allows NewBreachRetribution to be queried for a specific state number w/o needing to have the breach transaction itself.

This is a preliminary step to adding the watchtower client, which may need to be able to retroactively sign justice transactions for any particular revoked state. This also implies that the client/links don't have to do any bookkeeping/recording of breach transactions outside the wallet.

@Roasbeef Roasbeef requested a review from wpaulino Jan 9, 2019

@wpaulino
Copy link
Collaborator

wpaulino left a comment

Looks like the commits can just be squashed into one? LGTM 🐧

multi: remove breach tx arg from NewBreachRetribution args
This commit removes the breach transaction from the
arguments passed to NewBreachRetribution. We already
keep all prior remote commitments on disk in the
commitment log, and load that transaction from disk
inside the method. In practice, the one loaded from
disk will be the same one that is passed in by the
caller, so there should be no change in behavior
as we've already derived the appropriate state number.

This changes makes integration with the watchtower
client simpler, since we no longer need to acquire
the breaching commitment transaction to be able to
construct the BreachRetribution. This simplifies
not only the logic surrounding transient backsups,
but also on startup (and later, retroactively
backing up historic updates).

@cfromknecht cfromknecht force-pushed the cfromknecht:remove-breach-txn-from-retribution branch from f1770c3 to eb2f5ce Jan 10, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

cfromknecht commented Jan 10, 2019

rebased and squashed

@@ -1952,7 +1951,7 @@ func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64,
remoteOutpoint := wire.OutPoint{
Hash: commitHash,
}
for i, txOut := range broadcastCommitment.TxOut {
for i, txOut := range revokedSnapshot.CommitTx.TxOut {

This comment has been minimized.

Copy link
@halseth

halseth Jan 11, 2019

Collaborator

It is surprising to me that we store the whole remote commit tx for each state num! Has this always been the case?

Seems a bit wasteful and not necessary?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Jan 11, 2019

Author Collaborator

We considered removing them at one point and then didn't iirc. Turns out they're useful!

Seems a bit wasteful and not necessary?

Never hurts to keep channel data, deleting is forever. if it becomes an issue we should reconsider it, but i don't think that'll be any time soon :)

This comment has been minimized.

Copy link
@halseth

halseth Jan 11, 2019

Collaborator

True, must leave some low-hanging optimizations around in case we run out of work 🤣

@cfromknecht cfromknecht merged commit 043631e into lightningnetwork:master Jan 11, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 56.283%
Details

@cfromknecht cfromknecht deleted the cfromknecht:remove-breach-txn-from-retribution branch Jan 11, 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.