diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 83a146be0..97b7ac44d 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -108,6 +108,10 @@ var ( // ErrOutputIndexOutOfRange is returned when an output index is greater // than or equal to the length of a given transaction's outputs. ErrOutputIndexOutOfRange = errors.New("output index is out of range") + + // ErrRevLogDataMissing is returned when a certain wanted optional field + // in a revocation log entry is missing. + ErrRevLogDataMissing = errors.New("revocation log data missing") ) // ErrCommitSyncLocalDataLoss is returned in the case that we receive a valid @@ -2287,8 +2291,12 @@ type BreachRetribution struct { } // NewBreachRetribution creates a new fully populated BreachRetribution for the -// passed channel, at a particular revoked state number, and one which targets -// the passed commitment transaction. +// passed channel, at a particular revoked state number. If the spend +// transaction that the breach retribution should target is known, then it can +// be provided via the spendTx parameter. Otherwise, if the spendTx parameter is +// nil, then the revocation log will be checked to see if it contains the info +// required to construct the BreachRetribution. If the revocation log is missing +// the required fields then ErrRevLogDataMissing will be returned. func NewBreachRetribution(chanState *channeldb.OpenChannel, stateNum uint64, breachHeight uint32, spendTx *wire.MsgTx) (*BreachRetribution, error) { @@ -2493,7 +2501,11 @@ func createHtlcRetribution(chanState *channeldb.OpenChannel, // createBreachRetribution creates a partially initiated BreachRetribution // using a RevocationLog. Returns the constructed retribution, our amount, -// their amount, and a possible non-nil error. +// their amount, and a possible non-nil error. If the spendTx parameter is +// non-nil, then it will be used to glean the breach transaction's to-local and +// to-remote output amounts. Otherwise, the RevocationLog will be checked to +// see if these fields are present there. If they are not, then +// ErrRevLogDataMissing is returned. func createBreachRetribution(revokedLog *channeldb.RevocationLog, spendTx *wire.MsgTx, chanState *channeldb.OpenChannel, keyRing *CommitmentKeyRing, commitmentSecret *btcec.PrivateKey, @@ -2523,18 +2535,33 @@ func createBreachRetribution(revokedLog *channeldb.RevocationLog, if revokedLog.OurOutputIndex != channeldb.OutputIndexEmpty { ourOutpoint.Index = uint32(revokedLog.OurOutputIndex) - // Sanity check that OurOutputIndex is within range. - if int(ourOutpoint.Index) >= len(spendTx.TxOut) { - return nil, 0, 0, fmt.Errorf("%w: ours=%v, "+ - "len(TxOut)=%v", ErrOutputIndexOutOfRange, - ourOutpoint.Index, len(spendTx.TxOut), - ) + // If the spend transaction is provided, then we use it to get + // the value of our output. + if spendTx != nil { + // Sanity check that OurOutputIndex is within range. + if int(ourOutpoint.Index) >= len(spendTx.TxOut) { + return nil, 0, 0, fmt.Errorf("%w: ours=%v, "+ + "len(TxOut)=%v", + ErrOutputIndexOutOfRange, + ourOutpoint.Index, len(spendTx.TxOut), + ) + } + // Read the amounts from the breach transaction. + // + // NOTE: ourAmt here includes commit fee and anchor + // amount (if enabled). + ourAmt = spendTx.TxOut[ourOutpoint.Index].Value + } else { + // Otherwise, we check to see if the revocation log + // contains our output amount. Due to a previous + // migration, this field may be empty in which case an + // error will be returned. + if revokedLog.OurBalance == nil { + return nil, 0, 0, ErrRevLogDataMissing + } + + ourAmt = int64(revokedLog.OurBalance.ToSatoshis()) } - // Read the amounts from the breach transaction. - // - // NOTE: ourAmt here includes commit fee and anchor amount(if - // enabled). - ourAmt = spendTx.TxOut[ourOutpoint.Index].Value } // Construct the their outpoint. @@ -2544,16 +2571,35 @@ func createBreachRetribution(revokedLog *channeldb.RevocationLog, if revokedLog.TheirOutputIndex != channeldb.OutputIndexEmpty { theirOutpoint.Index = uint32(revokedLog.TheirOutputIndex) - // Sanity check that TheirOutputIndex is within range. - if int(revokedLog.TheirOutputIndex) >= len(spendTx.TxOut) { - return nil, 0, 0, fmt.Errorf("%w: theirs=%v, "+ - "len(TxOut)=%v", ErrOutputIndexOutOfRange, - revokedLog.TheirOutputIndex, len(spendTx.TxOut), - ) - } + // If the spend transaction is provided, then we use it to get + // the value of the remote parties' output. + if spendTx != nil { + // Sanity check that TheirOutputIndex is within range. + if int(revokedLog.TheirOutputIndex) >= + len(spendTx.TxOut) { - // Read the amounts from the breach transaction. - theirAmt = spendTx.TxOut[theirOutpoint.Index].Value + return nil, 0, 0, fmt.Errorf("%w: theirs=%v, "+ + "len(TxOut)=%v", + ErrOutputIndexOutOfRange, + revokedLog.TheirOutputIndex, + len(spendTx.TxOut), + ) + } + + // Read the amounts from the breach transaction. + theirAmt = spendTx.TxOut[theirOutpoint.Index].Value + + } else { + // Otherwise, we check to see if the revocation log + // contains remote parties' output amount. Due to a + // previous migration, this field may be empty in which + // case an error will be returned. + if revokedLog.TheirBalance == nil { + return nil, 0, 0, ErrRevLogDataMissing + } + + theirAmt = int64(revokedLog.TheirBalance.ToSatoshis()) + } } return &BreachRetribution{ diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index db72e5db5..fc8a7bef7 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -9482,7 +9482,7 @@ func TestCreateHtlcRetribution(t *testing.T) { } // TestCreateBreachRetribution checks that `createBreachRetribution` behaves as -// epxected. +// expected. func TestCreateBreachRetribution(t *testing.T) { t.Parallel() @@ -9524,11 +9524,15 @@ func TestCreateBreachRetribution(t *testing.T) { } // Create a dummy revocation log. + ourAmtMsat := lnwire.MilliSatoshi(ourAmt * 1000) + theirAmtMsat := lnwire.MilliSatoshi(theirAmt * 1000) revokedLog := channeldb.RevocationLog{ CommitTxHash: commitHash, OurOutputIndex: uint16(localIndex), TheirOutputIndex: uint16(remoteIndex), HTLCEntries: []*channeldb.HTLCEntry{htlc}, + TheirBalance: &theirAmtMsat, + OurBalance: &ourAmtMsat, } // Create a log with an empty local output index. @@ -9545,14 +9549,25 @@ func TestCreateBreachRetribution(t *testing.T) { expectedErr error expectedOurAmt int64 expectedTheirAmt int64 + noSpendTx bool }{ { - name: "create retribution successfully", + name: "create retribution successfully " + + "with spend tx", revocationLog: &revokedLog, expectedErr: nil, expectedOurAmt: ourAmt, expectedTheirAmt: theirAmt, }, + { + name: "create retribution successfully " + + "without spend tx", + revocationLog: &revokedLog, + expectedErr: nil, + expectedOurAmt: ourAmt, + expectedTheirAmt: theirAmt, + noSpendTx: true, + }, { name: "fail due to our index too big", revocationLog: &channeldb.RevocationLog{ @@ -9568,19 +9583,39 @@ func TestCreateBreachRetribution(t *testing.T) { expectedErr: ErrOutputIndexOutOfRange, }, { - name: "empty local output index", + name: "empty local output index with spend " + + "tx", revocationLog: &revokedLogNoLocal, expectedErr: nil, expectedOurAmt: 0, expectedTheirAmt: theirAmt, }, { - name: "empty remote output index", + name: "empty local output index without spend " + + "tx", + revocationLog: &revokedLogNoLocal, + expectedErr: nil, + expectedOurAmt: 0, + expectedTheirAmt: theirAmt, + noSpendTx: true, + }, + { + name: "empty remote output index with spend " + + "tx", revocationLog: &revokedLogNoRemote, expectedErr: nil, expectedOurAmt: ourAmt, expectedTheirAmt: 0, }, + { + name: "empty remote output index without spend " + + "tx", + revocationLog: &revokedLogNoRemote, + expectedErr: nil, + expectedOurAmt: ourAmt, + expectedTheirAmt: 0, + noSpendTx: true, + }, } // assertRetribution is a helper closure that checks a given breach @@ -9623,8 +9658,13 @@ func TestCreateBreachRetribution(t *testing.T) { for _, tc := range testCases { tc := tc t.Run(tc.name, func(t *testing.T) { + tx := spendTx + if tc.noSpendTx { + tx = nil + } + br, our, their, err := createBreachRetribution( - tc.revocationLog, spendTx, + tc.revocationLog, tx, aliceChannel.channelState, keyRing, dummyPrivate, leaseExpiry, ) @@ -9748,6 +9788,13 @@ func testNewBreachRetribution(t *testing.T, chanType channeldb.ChannelType) { ) require.ErrorIs(t, err, channeldb.ErrNoPastDeltas) + // We also check that the same error is returned if no breach tx is + // provided. + _, err = NewBreachRetribution( + aliceChannel.channelState, stateNum, breachHeight, nil, + ) + require.ErrorIs(t, err, channeldb.ErrNoPastDeltas) + // We now force a state transition which will give us a revocation log // at height 0. txid := aliceChannel.channelState.RemoteCommitment.CommitTx.TxHash() @@ -9797,10 +9844,25 @@ func testNewBreachRetribution(t *testing.T, chanType channeldb.ChannelType) { t.Log(spew.Sdump(breachTx)) assertRetribution(br, 1, 0) + // Repeat the check but with the breach tx set to nil. This should work + // since the necessary info should now be found in the revocation log. + br, err = NewBreachRetribution( + aliceChannel.channelState, stateNum, breachHeight, nil, + ) + require.NoError(t, err) + assertRetribution(br, 1, 0) + // Create the retribution using a stateNum+1 and we should expect an // error. _, err = NewBreachRetribution( aliceChannel.channelState, stateNum+1, breachHeight, breachTx, ) require.ErrorIs(t, err, channeldb.ErrLogEntryNotFound) + + // Once again, repeat the check for the case when no breach tx is + // provided. + _, err = NewBreachRetribution( + aliceChannel.channelState, stateNum+1, breachHeight, nil, + ) + require.ErrorIs(t, err, channeldb.ErrLogEntryNotFound) }