lnwallet: fix bug that retrieves incorrect pkScript in toChannelDelta()

Description of bug:
When calling ReceiveNewCommitment() we will progress through methods
fetchCommitmentView and addHTLC which will add HTLC outputs to the
commitment transaction in the local commitment chain and save the
pkScript to the relevant PaymentDescriptor which resides in the
corresponding updateLog. Finally the local commitment will be added
to the local commitment chain.

When the same user next calls SignNextCommitment we will again
progress through fetchCommitmentView and addHTLC. In addHTLC we will
now overwrite the pkScripts in the PaymentDescriptors with the
pkScript from the context of the remote commitment. When we later
call RevokeCurrentCommitment and proceed into toChannelDelta, we
will not be able to find the correct pkScript in the PaymentDescriptor
to match it against the outputs in the commitment transaction.
This will lead to the nested function locateOutputIndex returning
incorrect values.

Fixing the bug:
We introduce three new fields in PaymentDescriptor:
* ourPkScript
* theirPkScript
* theirPrevPkScript

ourPkScript will include the pkScript for the HTLC from the context
of the local commitment.

theirPkScript will take the value of the latest pkScript for the HTLC
from the context of the remote commitment.

theirPrevPkScript will take the second-latest pkScript for the HTLC
from the context of the remote commitment. This is the value we use
in toChannelDelta when we save a revoked commitment from our peer.

The appropriate value of these fields are set in the addHTLC method.

Additionally we pass a boolean value to toChannelDelta so we know
whether we are operating on a local or remote commitment and grab
the correct pkScript in locateUpdateIndex.
This commit is contained in:
Christopher Jämthagen 2017-01-20 12:37:30 +01:00 committed by Olaoluwa Osuntokun
parent 4d03f60e40
commit 9083007ece

@ -176,10 +176,15 @@ type PaymentDescriptor struct {
// possible upstream peers in the route. // possible upstream peers in the route.
isForwarded bool isForwarded bool
// pkScript is the raw public key script that encodes the redemption // [our|their|theirPrev]PkScript are the raw public key scripts that
// rules for this particular HTLC. This field will only be populated // encodes the redemption rules for this particular HTLC. These fields
// iff the EntryType of this PaymentDescriptor is Add. // will only be populated iff the EntryType of this PaymentDescriptor
pkScript []byte // is Add. ourPkScript is the ourPkScript from the context of our local
// commitment chain. [their|theirPrev]PkScript are the two latest
// pkScripts from the context of the remote commitment chain.
ourPkScript []byte
theirPkScript []byte
theirPrevPkScript []byte
} }
// commitment represents a commitment to a new state within an active channel. // commitment represents a commitment to a new state within an active channel.
@ -228,7 +233,7 @@ type commitment struct {
// toChannelDelta converts the target commitment into a format suitable to be // toChannelDelta converts the target commitment into a format suitable to be
// written to disk after an accepted state transition. // written to disk after an accepted state transition.
// TODO(roasbeef): properly fill in refund timeouts // TODO(roasbeef): properly fill in refund timeouts
func (c *commitment) toChannelDelta() (*channeldb.ChannelDelta, error) { func (c *commitment) toChannelDelta(ourCommit bool) (*channeldb.ChannelDelta, error) {
numHtlcs := len(c.outgoingHTLCs) + len(c.incomingHTLCs) numHtlcs := len(c.outgoingHTLCs) + len(c.incomingHTLCs)
// Save output indexes for RHash values found, so we don't return the // Save output indexes for RHash values found, so we don't return the
@ -257,8 +262,13 @@ func (c *commitment) toChannelDelta() (*channeldb.ChannelDelta, error) {
// transaction. // transaction.
locateOutputIndex := func(p *PaymentDescriptor) uint16 { locateOutputIndex := func(p *PaymentDescriptor) uint16 {
var idx uint16 var idx uint16
pkScript := p.theirPrevPkScript
if ourCommit {
pkScript = p.ourPkScript
}
for i, txOut := range c.txn.TxOut { for i, txOut := range c.txn.TxOut {
if bytes.Equal(txOut.PkScript, p.pkScript) { if bytes.Equal(txOut.PkScript, pkScript) {
if contains(dups[p.RHash], uint16(i)) { if contains(dups[p.RHash], uint16(i)) {
continue continue
} }
@ -1636,7 +1646,7 @@ func (lc *LightningChannel) RevokeCurrentCommitment() (*lnwire.RevokeAndAck, err
// Additionally, generate a channel delta for this state transition for // Additionally, generate a channel delta for this state transition for
// persistent storage. // persistent storage.
tail := lc.localCommitChain.tail() tail := lc.localCommitChain.tail()
delta, err := tail.toChannelDelta() delta, err := tail.toChannelDelta(true)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -1731,7 +1741,7 @@ func (lc *LightningChannel) ReceiveRevocation(revMsg *lnwire.RevokeAndAck) ([]*P
// sync now to ensure the revocation producer state is consistent with // sync now to ensure the revocation producer state is consistent with
// the current commitment height. // the current commitment height.
tail := lc.remoteCommitChain.tail() tail := lc.remoteCommitChain.tail()
delta, err := tail.toChannelDelta() delta, err := tail.toChannelDelta(false)
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -2093,7 +2103,12 @@ func (lc *LightningChannel) addHTLC(commitTx *wire.MsgTx, ourCommit bool,
// Store the pkScript of this particular PaymentDescriptor so we can // Store the pkScript of this particular PaymentDescriptor so we can
// quickly locate it within the commitment transaction later. // quickly locate it within the commitment transaction later.
paymentDesc.pkScript = htlcP2WSH if ourCommit {
paymentDesc.ourPkScript = htlcP2WSH
} else {
paymentDesc.theirPrevPkScript = paymentDesc.theirPkScript
paymentDesc.theirPkScript = htlcP2WSH
}
return nil return nil
} }