akovalenko/lnd.xprv
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Merge pull request #4121 from cfromknecht/sort-htlc-sigs

Browse Source 
lnwallet: apply BIP69+CLTV tie-break to HTLC signature order
This commit is contained in:
Olaoluwa Osuntokun 2020-04-03 18:12:58 -07:00 committed by GitHub
commit 173f7d88e0
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 230 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

65
lnwallet/channel.go
View File

@ -67,6 +67,10 @@ var (
ErrBelowMinHTLC = fmt.Errorf("proposed HTLC value is below minimum " +
"allowed HTLC value")
// ErrInvalidHTLCAmt signals that a proposed HTLC has a value that is
// not positive.
ErrInvalidHTLCAmt = fmt.Errorf("proposed HTLC value must be positive")
// ErrCannotSyncCommitChains is returned if, upon receiving a ChanSync
// message, the state machine deems that is unable to properly
// synchronize states with the remote peer. In this case we should fail
@ -549,7 +553,7 @@ type commitment struct {
// transition. This ensures that we don't assign multiple HTLC's to the same
// index within the commitment transaction.
func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool,
dups map[PaymentHash][]int32) (int32, error) {
dups map[PaymentHash][]int32, cltvs []uint32) (int32, error) {
// Checks to see if element (e) exists in slice (s).
contains := func(s []int32, e int32) bool {
@ -571,8 +575,11 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool,
}
for i, txOut := range tx.TxOut {
cltv := cltvs[i]
if bytes.Equal(txOut.PkScript, pkScript) &&
txOut.Value == int64(p.Amount.ToSatoshis()) {
txOut.Value == int64(p.Amount.ToSatoshis()) &&
cltv == p.Timeout {
// If this payment hash and index has already been
// found, then we'll continue in order to avoid any
@ -587,8 +594,8 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool,
}
}
return 0, fmt.Errorf("unable to find htlc: script=%x, value=%v",
pkScript, p.Amount)
return 0, fmt.Errorf("unable to find htlc: script=%x, value=%v, "+
"cltv=%v", pkScript, p.Amount, p.Timeout)
}
// populateHtlcIndexes modifies the set of HTLC's locked-into the target view
@ -596,7 +603,9 @@ func locateOutputIndex(p *PaymentDescriptor, tx *wire.MsgTx, ourCommit bool,
// we need to keep track of the indexes of each HTLC in order to properly write
// the current state to disk, and also to locate the PaymentDescriptor
// corresponding to HTLC outputs in the commitment transaction.
func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType) error {
func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType,
cltvs []uint32) error {
// First, we'll set up some state to allow us to locate the output
// index of the all the HTLC's within the commitment transaction. We
// must keep this index so we can validate the HTLC signatures sent to
@ -632,7 +641,7 @@ func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType) error {
// signatures.
case c.isOurs:
htlc.localOutputIndex, err = locateOutputIndex(
htlc, c.txn, c.isOurs, dups,
htlc, c.txn, c.isOurs, dups, cltvs,
)
if err != nil {
return err
@ -653,7 +662,7 @@ func (c *commitment) populateHtlcIndexes(chanType channeldb.ChannelType) error {
// index within the HTLC index.
case !c.isOurs:
htlc.remoteOutputIndex, err = locateOutputIndex(
htlc, c.txn, c.isOurs, dups,
htlc, c.txn, c.isOurs, dups, cltvs,
)
if err != nil {
return err
@ -766,7 +775,8 @@ func (c *commitment) toDiskCommit(ourCommit bool) *channeldb.ChannelCommitment {
// restart a channel session.
func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
commitHeight uint64, htlc *channeldb.HTLC, localCommitKeys,
remoteCommitKeys *CommitmentKeyRing) (PaymentDescriptor, error) {
remoteCommitKeys *CommitmentKeyRing, isLocal bool) (PaymentDescriptor,
error) {
// The proper pkScripts for this PaymentDescriptor must be
// generated so we can easily locate them within the commitment
@ -811,6 +821,19 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
}
}
// Reconstruct the proper local/remote output indexes from the HTLC's
// persisted output index depending on whose commitment we are
// generating.
var (
localOutputIndex int32
remoteOutputIndex int32
)
if isLocal {
localOutputIndex = htlc.OutputIndex
} else {
remoteOutputIndex = htlc.OutputIndex
}
// With the scripts reconstructed (depending on if this is our commit
// vs theirs or a pending commit for the remote party), we can now
// re-create the original payment descriptor.
@ -822,6 +845,8 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
HtlcIndex: htlc.HtlcIndex,
LogIndex: htlc.LogIndex,
OnionBlob: htlc.OnionBlob,
localOutputIndex: localOutputIndex,
remoteOutputIndex: remoteOutputIndex,
ourPkScript: ourP2WSH,
ourWitnessScript: ourWitnessScript,
theirPkScript: theirP2WSH,
@ -837,7 +862,8 @@ func (lc *LightningChannel) diskHtlcToPayDesc(feeRate chainfee.SatPerKWeight,
// for each side.
func (lc *LightningChannel) extractPayDescs(commitHeight uint64,
feeRate chainfee.SatPerKWeight, htlcs []channeldb.HTLC, localCommitKeys,
remoteCommitKeys *CommitmentKeyRing) ([]PaymentDescriptor, []PaymentDescriptor, error) {
remoteCommitKeys *CommitmentKeyRing, isLocal bool) ([]PaymentDescriptor,
[]PaymentDescriptor, error) {
var (
incomingHtlcs []PaymentDescriptor
@ -855,6 +881,7 @@ func (lc *LightningChannel) extractPayDescs(commitHeight uint64,
payDesc, err := lc.diskHtlcToPayDesc(
feeRate, commitHeight, &htlc,
localCommitKeys, remoteCommitKeys,
isLocal,
)
if err != nil {
return incomingHtlcs, outgoingHtlcs, err
@ -905,6 +932,7 @@ func (lc *LightningChannel) diskCommitToMemCommit(isLocal bool,
diskCommit.CommitHeight,
chainfee.SatPerKWeight(diskCommit.FeePerKw),
diskCommit.Htlcs, localCommitKeys, remoteCommitKeys,
isLocal,
)
if err != nil {
return nil, err
@ -934,13 +962,6 @@ func (lc *LightningChannel) diskCommitToMemCommit(isLocal bool,
commit.dustLimit = lc.channelState.RemoteChanCfg.DustLimit
}
// Finally, we'll re-populate the HTLC index for this state so we can
// properly locate each HTLC within the commitment transaction.
err = commit.populateHtlcIndexes(lc.channelState.ChanType)
if err != nil {
return nil, err
}
return commit, nil
}
@ -2428,8 +2449,11 @@ func (lc *LightningChannel) fetchCommitmentView(remoteChain bool,
}
// Finally, we'll populate all the HTLC indexes so we can track the
// locations of each HTLC in the commitment state.
if err := c.populateHtlcIndexes(lc.channelState.ChanType); err != nil {
// locations of each HTLC in the commitment state. We pass in the sorted
// slice of CLTV deltas in order to properly locate HTLCs that otherwise
// have the same payment hash and amount.
err = c.populateHtlcIndexes(lc.channelState.ChanType, commitTx.cltvs)
if err != nil {
return nil, err
}
@ -3214,6 +3238,11 @@ func (lc *LightningChannel) validateCommitmentSanity(theirLogCounter,
amtInFlight += entry.Amount
numInFlight++
// Check that the HTLC amount is positive.
if entry.Amount == 0 {
return ErrInvalidHTLCAmt
}
// Check that the value of the HTLC they added
// is above our minimum.
if entry.Amount < constraints.MinHTLC {

168
lnwallet/channel_test.go
View File

@ -450,6 +450,139 @@ func TestCheckCommitTxSize(t *testing.T) {
}
}
// TestCommitHTLCSigTieBreak asserts that HTLC signatures are sent the proper
// BIP69+CLTV sorting expected by BOLT 3 when multiple HTLCs have identical
// payment hashes and amounts, but differing CLTVs. This is exercised by adding
// the HTLCs in the descending order of their CLTVs, and asserting that their
// order is reversed when signing.
func TestCommitHTLCSigTieBreak(t *testing.T) {
t.Run("no restart", func(t *testing.T) {
testCommitHTLCSigTieBreak(t, false)
})
t.Run("restart", func(t *testing.T) {
testCommitHTLCSigTieBreak(t, true)
})
}
func testCommitHTLCSigTieBreak(t *testing.T, restart bool) {
aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(
channeldb.SingleFunderTweaklessBit,
)
if err != nil {
t.Fatalf("unable to create test channels; %v", err)
}
defer cleanUp()
const (
htlcAmt = lnwire.MilliSatoshi(20000000)
numHtlcs = 2
)
// Add HTLCs with identical payment hashes and amounts, but descending
// CLTV values. We will expect the signatures to appear in the reverse
// order that the HTLCs are added due to the commitment sorting.
for i := 0; i < numHtlcs; i++ {
var (
preimage lntypes.Preimage
hash = preimage.Hash()
)
htlc := &lnwire.UpdateAddHTLC{
ID: uint64(i),
PaymentHash: hash,
Amount: htlcAmt,
Expiry: uint32(numHtlcs - i),
}
if _, err := aliceChannel.AddHTLC(htlc, nil); err != nil {
t.Fatalf("alice unable to add htlc: %v", err)
}
if _, err := bobChannel.ReceiveHTLC(htlc); err != nil {
t.Fatalf("bob unable to receive htlc: %v", err)
}
}
// Have Alice initiate the first half of the commitment dance. The
// tie-breaking for commitment sorting won't affect the commitment
// signed by Alice because received HTLC scripts commit to the CLTV
// directly, so the outputs will have different scriptPubkeys.
aliceSig, aliceHtlcSigs, _, err := aliceChannel.SignNextCommitment()
if err != nil {
t.Fatalf("unable to sign alice's commitment: %v", err)
}
err = bobChannel.ReceiveNewCommitment(aliceSig, aliceHtlcSigs)
if err != nil {
t.Fatalf("unable to receive alice's commitment: %v", err)
}
bobRevocation, _, err := bobChannel.RevokeCurrentCommitment()
if err != nil {
t.Fatalf("unable to revoke bob's commitment: %v", err)
}
_, _, _, _, err = aliceChannel.ReceiveRevocation(bobRevocation)
if err != nil {
t.Fatalf("unable to receive bob's revocation: %v", err)
}
// Now have Bob initiate the second half of the commitment dance. Here
// the offered HTLC scripts he adds for Alice will need to have the
// tie-breaking applied because the CLTV is not committed, but instead
// implicit via the construction of the second-level transactions.
bobSig, bobHtlcSigs, bobHtlcs, err := bobChannel.SignNextCommitment()
if err != nil {
t.Fatalf("unable to sign bob's commitment: %v", err)
}
if len(bobHtlcs) != numHtlcs {
t.Fatalf("expected %d htlcs, got: %v", numHtlcs, len(bobHtlcs))
}
// Ensure that our HTLCs appear in the reverse order from which they
// were added by inspecting each's outpoint index. We expect the output
// indexes to be in descending order, i.e. the first HTLC added had the
// highest CLTV and should end up last.
lastIndex := bobHtlcs[0].OutputIndex
for i, htlc := range bobHtlcs[1:] {
if htlc.OutputIndex >= lastIndex {
t.Fatalf("htlc %d output index %d is not descending",
i, htlc.OutputIndex)
}
lastIndex = htlc.OutputIndex
}
// If requsted, restart Alice so that we can test that the necessary
// indexes can be reconstructed before needing to validate the
// signatures from Bob.
if restart {
aliceState := aliceChannel.channelState
aliceChannels, err := aliceState.Db.FetchOpenChannels(
aliceState.IdentityPub,
)
if err != nil {
t.Fatalf("unable to fetch channel: %v", err)
}
aliceChannelNew, err := NewLightningChannel(
aliceChannel.Signer, aliceChannels[0],
aliceChannel.sigPool,
)
if err != nil {
t.Fatalf("unable to create new channel: %v", err)
}
aliceChannel = aliceChannelNew
}
// Finally, have Alice validate the signatures to ensure that she is
// expecting the signatures in the proper order.
err = aliceChannel.ReceiveNewCommitment(bobSig, bobHtlcSigs)
if err != nil {
t.Fatalf("unable to receive bob's commitment: %v", err)
}
}
func TestCooperativeChannelClosure(t *testing.T) {
t.Parallel()
@ -6503,6 +6636,41 @@ func TestMinHTLC(t *testing.T) {
}
}
// TestInvalidHTLCAmt tests that ErrInvalidHTLCAmt is returned when trying to
// add HTLCs that don't carry a positive value.
func TestInvalidHTLCAmt(t *testing.T) {
t.Parallel()
// We'll kick off the test by creating our channels which both are
// loaded with 5 BTC each.
aliceChannel, bobChannel, cleanUp, err := CreateTestChannels(
channeldb.SingleFunderTweaklessBit,
)
if err != nil {
t.Fatalf("unable to create test channels: %v", err)
}
defer cleanUp()
// We'll set the min HTLC values for each party to zero, which
// technically would permit zero-value HTLCs.
aliceChannel.channelState.LocalChanCfg.MinHTLC = 0
bobChannel.channelState.RemoteChanCfg.MinHTLC = 0
// Create a zero-value HTLC.
htlcAmt := lnwire.MilliSatoshi(0)
htlc, _ := createHTLC(0, htlcAmt)
// Sending or receiving the HTLC should fail with ErrInvalidHTLCAmt.
_, err = aliceChannel.AddHTLC(htlc, nil)
if err != ErrInvalidHTLCAmt {
t.Fatalf("expected ErrInvalidHTLCAmt, got: %v", err)
}
_, err = bobChannel.ReceiveHTLC(htlc)
if err != ErrInvalidHTLCAmt {
t.Fatalf("expected ErrInvalidHTLCAmt, got: %v", err)
}
}
// TestNewBreachRetributionSkipsDustHtlcs ensures that in the case of a
// contract breach, all dust HTLCs are ignored and not reflected in the
// produced BreachRetribution struct. We ignore these HTLCs as they aren't

20
lnwallet/commitment.go
View File

@ -371,12 +371,21 @@ type unsignedCommitmentTx struct {
// fee is the total fee of the commitment transaction.
fee btcutil.Amount
// ourBalance|theirBalance are the balances of this commitment *after*
// subtracting commitment fees and anchor outputs. This can be
// different than the balances before creating the commitment
// transaction as one party must pay the commitment fee.
ourBalance lnwire.MilliSatoshi
// ourBalance is our balance on this commitment *after* subtracting
// commitment fees and anchor outputs. This can be different than the
// balances before creating the commitment transaction as one party must
// pay the commitment fee.
ourBalance lnwire.MilliSatoshi
// theirBalance is their balance of this commitment *after* subtracting
// commitment fees and anchor outputs. This can be different than the
// balances before creating the commitment transaction as one party must
// pay the commitment fee.
theirBalance lnwire.MilliSatoshi
// cltvs is a sorted list of CLTV deltas for each HTLC on the commitment
// transaction. Any non-htlc outputs will have a CLTV delay of zero.
cltvs []uint32
}
// createUnsignedCommitmentTx generates the unsigned commitment transaction for
@ -557,6 +566,7 @@ func (cb *CommitmentBuilder) createUnsignedCommitmentTx(ourBalance,
fee: commitFee,
ourBalance: ourBalance,
theirBalance: theirBalance,
cltvs: cltvs,
}, nil
}