From 9083007ece3e8b38fc21dc307f4493dcaf422a19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christopher=20J=C3=A4mthagen?= Date: Fri, 20 Jan 2017 12:37:30 +0100 Subject: [PATCH] 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. --- lnwallet/channel.go | 33 ++++++++++++++++++++++++--------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index ed72ee0c..7c0930e1 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -176,10 +176,15 @@ type PaymentDescriptor struct { // possible upstream peers in the route. isForwarded bool - // pkScript is the raw public key script that encodes the redemption - // rules for this particular HTLC. This field will only be populated - // iff the EntryType of this PaymentDescriptor is Add. - pkScript []byte + // [our|their|theirPrev]PkScript are the raw public key scripts that + // encodes the redemption rules for this particular HTLC. These fields + // will only be populated iff the EntryType of this PaymentDescriptor + // 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. @@ -228,7 +233,7 @@ type commitment struct { // toChannelDelta converts the target commitment into a format suitable to be // written to disk after an accepted state transition. // 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) // 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. locateOutputIndex := func(p *PaymentDescriptor) uint16 { var idx uint16 + + pkScript := p.theirPrevPkScript + if ourCommit { + pkScript = p.ourPkScript + } 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)) { continue } @@ -1636,7 +1646,7 @@ func (lc *LightningChannel) RevokeCurrentCommitment() (*lnwire.RevokeAndAck, err // Additionally, generate a channel delta for this state transition for // persistent storage. tail := lc.localCommitChain.tail() - delta, err := tail.toChannelDelta() + delta, err := tail.toChannelDelta(true) if err != nil { 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 // the current commitment height. tail := lc.remoteCommitChain.tail() - delta, err := tail.toChannelDelta() + delta, err := tail.toChannelDelta(false) if err != nil { 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 // 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 }