From 9b09895bde5f3084e969c90d938a876c64a507e6 Mon Sep 17 00:00:00 2001 From: eugene Date: Tue, 12 Jan 2021 15:26:47 -0500 Subject: [PATCH 1/2] channeldb+lnwallet: lastWasRevokeKey to store last sent rev/sig --- channeldb/channel.go | 61 ++++++++++++++++++++++++++++++++++++++------ lnwallet/channel.go | 26 +++++++++++++++++-- 2 files changed, 77 insertions(+), 10 deletions(-) diff --git a/channeldb/channel.go b/channeldb/channel.go index 14a0c62e..468847c7 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -36,7 +36,7 @@ var ( // previously open, but now closed channels. closedChannelBucket = []byte("closed-chan-bucket") - // openChanBucket stores all the currently open channels. This bucket + // openChannelBucket stores all the currently open channels. This bucket // has a second, nested bucket which is keyed by a node's ID. Within // that node ID bucket, all attributes required to track, update, and // close a channel are stored. @@ -128,6 +128,11 @@ var ( // active "frozen" channels. This key is present only in the leaf // bucket for a given channel. frozenChanKey = []byte("frozen-chans") + + // lastWasRevokeKey is a key that stores true when the last update we sent + // was a revocation and false when it was a commitment signature. This is + // nil in the case of new channels with no updates exchanged. + lastWasRevokeKey = []byte("last-was-revoke") ) var ( @@ -225,7 +230,7 @@ const ( // funded symmetrically or asymmetrically. DualFunderBit ChannelType = 1 << 0 - // SingleFunderTweakless is similar to the basic SingleFunder channel + // SingleFunderTweaklessBit is similar to the basic SingleFunder channel // type, but it omits the tweak for one's key in the commitment // transaction of the remote party. SingleFunderTweaklessBit ChannelType = 1 << 1 @@ -710,6 +715,10 @@ type OpenChannel struct { // interpreted as a relative height, or an absolute height otherwise. ThawHeight uint32 + // LastWasRevoke is a boolean that determines if the last update we sent + // was a revocation (true) or a commitment signature (false). + LastWasRevoke bool + // TODO(roasbeef): eww Db *DB @@ -1526,6 +1535,17 @@ func (c *OpenChannel) UpdateCommitment(newCommitment *ChannelCommitment, "updates: %v", err) } + // Since we have just sent the counterparty a revocation, store true + // under lastWasRevokeKey. + var b2 bytes.Buffer + if err := WriteElements(&b2, true); err != nil { + return err + } + + if err := chanBucket.Put(lastWasRevokeKey, b2.Bytes()); err != nil { + return err + } + // Persist the remote unsigned local updates that are not included // in our new commitment. updateBytes := chanBucket.Get(remoteUnsignedLocalUpdatesKey) @@ -1548,13 +1568,13 @@ func (c *OpenChannel) UpdateCommitment(newCommitment *ChannelCommitment, } } - var b2 bytes.Buffer - err = serializeLogUpdates(&b2, validUpdates) + var b3 bytes.Buffer + err = serializeLogUpdates(&b3, validUpdates) if err != nil { return fmt.Errorf("unable to serialize log updates: %v", err) } - err = chanBucket.Put(remoteUnsignedLocalUpdatesKey, b2.Bytes()) + err = chanBucket.Put(remoteUnsignedLocalUpdatesKey, b3.Bytes()) if err != nil { return fmt.Errorf("unable to restore chanbucket: %v", err) } @@ -2091,15 +2111,25 @@ func (c *OpenChannel) AppendRemoteCommitChain(diff *CommitDiff) error { return err } + // We are sending a commitment signature so lastWasRevokeKey should + // store false. + var b bytes.Buffer + if err := WriteElements(&b, false); err != nil { + return err + } + if err := chanBucket.Put(lastWasRevokeKey, b.Bytes()); err != nil { + return err + } + // TODO(roasbeef): use seqno to derive key for later LCP // With the bucket retrieved, we'll now serialize the commit // diff itself, and write it to disk. - var b bytes.Buffer - if err := serializeCommitDiff(&b, diff); err != nil { + var b2 bytes.Buffer + if err := serializeCommitDiff(&b2, diff); err != nil { return err } - return chanBucket.Put(commitDiffKey, b.Bytes()) + return chanBucket.Put(commitDiffKey, b2.Bytes()) }, func() {}) } @@ -3419,6 +3449,21 @@ func fetchChanInfo(chanBucket kvdb.RBucket, channel *OpenChannel) error { return err } + // Retrieve the boolean stored under lastWasRevokeKey. + lastWasRevokeBytes := chanBucket.Get(lastWasRevokeKey) + if lastWasRevokeBytes == nil { + // If nothing has been stored under this key, we store false in the + // OpenChannel struct. + channel.LastWasRevoke = false + } else { + // Otherwise, read the value into the LastWasRevoke field. + revokeReader := bytes.NewReader(lastWasRevokeBytes) + err := ReadElements(revokeReader, &channel.LastWasRevoke) + if err != nil { + return err + } + } + channel.Packager = NewChannelPackager(channel.ShortChannelID) // Finally, read the optional shutdown scripts. diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 6cd4389a..25f72a37 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -3892,16 +3892,38 @@ func (lc *LightningChannel) ProcessChanSyncMsg( return nil, nil, nil, err } + var commitUpdates []lnwire.Message + // Next, we'll need to send over any updates we sent as part of // this new proposed commitment state. for _, logUpdate := range commitDiff.LogUpdates { - updates = append(updates, logUpdate.UpdateMsg) + commitUpdates = append(commitUpdates, logUpdate.UpdateMsg) } // With the batch of updates accumulated, we'll now re-send the // original CommitSig message required to re-sync their remote // commitment chain with our local version of their chain. - updates = append(updates, commitDiff.CommitSig) + commitUpdates = append(commitUpdates, commitDiff.CommitSig) + + // NOTE: If a revocation is not owed, then updates is empty. + if lc.channelState.LastWasRevoke { + // If lastWasRevoke is set to true, a revocation was last and we + // need to reorder the updates so that the revocation stored in + // updates comes after the LogUpdates+CommitSig. + // + // ---logupdates---> + // ---commitsig----> + // ---revocation---> + updates = append(commitUpdates, updates...) + } else { + // Otherwise, the revocation should come before LogUpdates + // + CommitSig. + // + // ---revocation---> + // ---logupdates---> + // ---commitsig----> + updates = append(updates, commitUpdates...) + } openedCircuits = commitDiff.OpenedCircuitKeys closedCircuits = commitDiff.ClosedCircuitKeys From 1c407f402619975a656cab9a2a4311bfb61a3919 Mon Sep 17 00:00:00 2001 From: eugene Date: Tue, 26 Jan 2021 12:04:40 -0500 Subject: [PATCH 2/2] htlcswitch: reestablish unit tests --- htlcswitch/link_test.go | 295 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 281 insertions(+), 14 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 166b5340..e4ca32db 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -180,6 +180,272 @@ func createInterceptorFunc(prefix, receiver string, messages []expectedMessage, } } +// TestChannelLinkRevThenSig tests that if a link owes both a revocation and a +// signature to the counterparty (in this order), that they are sent as rev and +// then sig. +// +// Specifically, this tests the following scenario: +// +// A B +// <----add----- +// -----add----> +// <----sig----- +// -----rev----x +// -----sig----x +func TestChannelLinkRevThenSig(t *testing.T) { + t.Parallel() + + const chanAmt = btcutil.SatoshiPerBitcoin * 5 + const chanReserve = btcutil.SatoshiPerBitcoin * 1 + aliceLink, bobChannel, batchTicker, start, cleanUp, restore, err := + newSingleLinkTestHarness(chanAmt, chanReserve) + require.NoError(t, err) + defer cleanUp() + + err = start() + require.NoError(t, err) + defer aliceLink.Stop() + + alice := newPersistentLinkHarness( + t, aliceLink, batchTicker, restore, + ) + + var ( + coreLink = aliceLink.(*channelLink) + aliceMsgs = coreLink.cfg.Peer.(*mockPeer).sentMsgs + ) + + ctx := linkTestContext{ + t: t, + aliceLink: aliceLink, + aliceMsgs: aliceMsgs, + bobChannel: bobChannel, + } + + bobHtlc1 := generateHtlc(t, coreLink, 0) + + // <-----add----- + // Send an htlc from Bob to Alice. + ctx.sendHtlcBobToAlice(bobHtlc1) + + aliceHtlc1, _ := generateHtlcAndInvoice(t, 0) + + // ------add----> + ctx.sendHtlcAliceToBob(0, aliceHtlc1) + ctx.receiveHtlcAliceToBob() + + // <-----sig----- + ctx.sendCommitSigBobToAlice(1) + + // ------rev----x + var msg lnwire.Message + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + _, ok := msg.(*lnwire.RevokeAndAck) + require.True(t, ok) + + // ------sig----x + // Trigger a commitsig from Alice->Bob. + select { + case batchTicker <- time.Now(): + case <-time.After(5 * time.Second): + t.Fatalf("could not force commit sig") + } + + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + comSig, ok := msg.(*lnwire.CommitSig) + require.True(t, ok) + + if len(comSig.HtlcSigs) != 2 { + t.Fatalf("expected 2 htlc sigs, got %d", len(comSig.HtlcSigs)) + } + + // Restart Alice so she sends and accepts ChannelReestablish. + cleanUp = alice.restart(false, true) + defer cleanUp() + + ctx.aliceLink = alice.link + ctx.aliceMsgs = alice.msgs + + // Restart Bob as well by calling NewLightningChannel. + bobSigner := bobChannel.Signer + bobPool := lnwallet.NewSigPool(runtime.NumCPU(), bobSigner) + bobChannel, err = lnwallet.NewLightningChannel( + bobSigner, bobChannel.State(), bobPool, + ) + require.NoError(t, err) + err = bobPool.Start() + require.NoError(t, err) + + ctx.bobChannel = bobChannel + + // --reestablish-> + select { + case msg = <-ctx.aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + _, ok = msg.(*lnwire.ChannelReestablish) + require.True(t, ok) + + // <-reestablish-- + bobReest, err := bobChannel.State().ChanSyncMsg() + require.NoError(t, err) + ctx.aliceLink.HandleChannelUpdate(bobReest) + + // ------rev----> + ctx.receiveRevAndAckAliceToBob() + + // ------add----> + ctx.receiveHtlcAliceToBob() + + // ------sig----> + ctx.receiveCommitSigAliceToBob(2) +} + +// TestChannelLinkSigThenRev tests that if a link owes both a signature and a +// revocation to the counterparty (in this order), that they are sent as sig +// and then rev. +// +// Specifically, this tests the following scenario: +// +// A B +// <----add----- +// -----add----> +// -----sig----x +// <----sig----- +// -----rev----x +func TestChannelLinkSigThenRev(t *testing.T) { + t.Parallel() + + const chanAmt = btcutil.SatoshiPerBitcoin * 5 + const chanReserve = btcutil.SatoshiPerBitcoin * 1 + aliceLink, bobChannel, batchTicker, start, cleanUp, restore, err := + newSingleLinkTestHarness(chanAmt, chanReserve) + require.NoError(t, err) + defer cleanUp() + + err = start() + require.NoError(t, err) + defer aliceLink.Stop() + + alice := newPersistentLinkHarness( + t, aliceLink, batchTicker, restore, + ) + + var ( + coreLink = aliceLink.(*channelLink) + aliceMsgs = coreLink.cfg.Peer.(*mockPeer).sentMsgs + ) + + ctx := linkTestContext{ + t: t, + aliceLink: aliceLink, + aliceMsgs: aliceMsgs, + bobChannel: bobChannel, + } + + bobHtlc1 := generateHtlc(t, coreLink, 0) + + // <-----add----- + // Send an htlc from Bob to Alice. + ctx.sendHtlcBobToAlice(bobHtlc1) + + aliceHtlc1, _ := generateHtlcAndInvoice(t, 0) + + // ------add----> + ctx.sendHtlcAliceToBob(0, aliceHtlc1) + ctx.receiveHtlcAliceToBob() + + // ------sig----x + // Trigger a commitsig from Alice->Bob. + select { + case batchTicker <- time.Now(): + case <-time.After(5 * time.Second): + t.Fatalf("could not force commit sig") + } + + var msg lnwire.Message + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + comSig, ok := msg.(*lnwire.CommitSig) + require.True(t, ok) + + if len(comSig.HtlcSigs) != 1 { + t.Fatalf("expected 1 htlc sig, got %d", len(comSig.HtlcSigs)) + } + + // <-----sig----- + ctx.sendCommitSigBobToAlice(1) + + // ------rev----x + select { + case msg = <-aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + _, ok = msg.(*lnwire.RevokeAndAck) + require.True(t, ok) + + // Restart Alice so she sends and accepts ChannelReestablish. + cleanUp = alice.restart(false, true) + defer cleanUp() + + ctx.aliceLink = alice.link + ctx.aliceMsgs = alice.msgs + + // Restart Bob as well by calling NewLightningChannel. + bobSigner := bobChannel.Signer + bobPool := lnwallet.NewSigPool(runtime.NumCPU(), bobSigner) + bobChannel, err = lnwallet.NewLightningChannel( + bobSigner, bobChannel.State(), bobPool, + ) + require.NoError(t, err) + err = bobPool.Start() + require.NoError(t, err) + + ctx.bobChannel = bobChannel + + // --reestablish-> + select { + case msg = <-ctx.aliceMsgs: + case <-time.After(15 * time.Second): + t.Fatalf("did not receive message") + } + + _, ok = msg.(*lnwire.ChannelReestablish) + require.True(t, ok) + + // <-reestablish-- + bobReest, err := bobChannel.State().ChanSyncMsg() + require.NoError(t, err) + ctx.aliceLink.HandleChannelUpdate(bobReest) + + // ------add----> + ctx.receiveHtlcAliceToBob() + + // ------sig----> + ctx.receiveCommitSigAliceToBob(1) + + // ------rev----> + ctx.receiveRevAndAckAliceToBob() +} + // TestChannelLinkSingleHopPayment in this test we checks the interaction // between Alice and Bob within scope of one channel. func TestChannelLinkSingleHopPayment(t *testing.T) { @@ -2463,7 +2729,7 @@ func TestChannelLinkTrimCircuitsPending(t *testing.T) { // Restart Alice's link, which simulates a disconnection with the remote // peer. - cleanUp = alice.restart(false) + cleanUp = alice.restart(false, false) defer cleanUp() alice.assertNumPendingNumOpenCircuits(2, 2) @@ -2492,7 +2758,7 @@ func TestChannelLinkTrimCircuitsPending(t *testing.T) { // that entire circuit map is reloaded from disk, and we can now test // against the behavioral differences of committing circuits that // conflict with duplicate circuits after a restart. - cleanUp = alice.restart(true) + cleanUp = alice.restart(true, false) defer cleanUp() alice.assertNumPendingNumOpenCircuits(2, 2) @@ -2551,7 +2817,7 @@ func TestChannelLinkTrimCircuitsPending(t *testing.T) { // Restart Alice's link to simulate a disconnect. Since the switch // remains up throughout, the two latter HTLCs will remain in the link's // mailbox, and will reprocessed upon being reattached to the link. - cleanUp = alice.restart(false) + cleanUp = alice.restart(false, false) defer cleanUp() alice.assertNumPendingNumOpenCircuits(4, 2) @@ -2592,7 +2858,7 @@ func TestChannelLinkTrimCircuitsPending(t *testing.T) { // As a final persistence check, we will restart the link and switch, // wiping the latter two HTLCs from memory, and forcing their circuits // to be reloaded from disk. - cleanUp = alice.restart(true) + cleanUp = alice.restart(true, false) defer cleanUp() alice.assertNumPendingNumOpenCircuits(4, 2) @@ -2747,7 +3013,7 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { // Restart Alice's link, which simulates a disconnection with the remote // peer. Alice's link and switch should trim the circuits that were // opened but not committed. - cleanUp = alice.restart(false, hodl.Commit) + cleanUp = alice.restart(false, false, hodl.Commit) defer cleanUp() alice.assertNumPendingNumOpenCircuits(2, 0) @@ -2781,7 +3047,7 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { // Alice again in hodl.Commit mode. Since none of the HTLCs were // actually committed, the previously opened circuits should be trimmed // by both the link and switch. - cleanUp = alice.restart(true, hodl.Commit) + cleanUp = alice.restart(true, false, hodl.Commit) defer cleanUp() alice.assertNumPendingNumOpenCircuits(2, 0) @@ -2838,7 +3104,7 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { // Restart Alice's link, and place her back in hodl.Commit mode. On // restart, all previously opened circuits should be trimmed by both the // link and the switch. - cleanUp = alice.restart(false, hodl.Commit) + cleanUp = alice.restart(false, false, hodl.Commit) defer cleanUp() alice.assertNumPendingNumOpenCircuits(4, 0) @@ -2877,7 +3143,7 @@ func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { // Finally, do one last restart of both the link and switch. This will // flush the HTLCs from the mailbox. The circuits should now be trimmed // for all of the HTLCs. - cleanUp = alice.restart(true, hodl.Commit) + cleanUp = alice.restart(true, false, hodl.Commit) defer cleanUp() alice.assertNumPendingNumOpenCircuits(4, 0) @@ -3044,14 +3310,14 @@ func TestChannelLinkTrimCircuitsRemoteCommit(t *testing.T) { // Restart Alice's link, which simulates a disconnection with the remote // peer. - cleanUp = alice.restart(false) + cleanUp = alice.restart(false, false) defer cleanUp() alice.assertNumPendingNumOpenCircuits(2, 2) // Restart the link + switch and check that the number of open circuits // doesn't change. - cleanUp = alice.restart(true) + cleanUp = alice.restart(true, false) defer cleanUp() alice.assertNumPendingNumOpenCircuits(2, 2) @@ -4046,7 +4312,7 @@ func newPersistentLinkHarness(t *testing.T, link ChannelLink, // // Any number of hodl flags can be passed as additional arguments to this // method. If none are provided, the mask will be extracted as hodl.MaskNone. -func (h *persistentLinkHarness) restart(restartSwitch bool, +func (h *persistentLinkHarness) restart(restartSwitch, syncStates bool, hodlFlags ...hodl.Flag) func() { // First, remove the link from the switch. @@ -4072,7 +4338,7 @@ func (h *persistentLinkHarness) restart(restartSwitch bool, // the database owned by the link. var cleanUp func() h.link, h.batchTicker, cleanUp, err = h.restartLink( - h.channel, restartSwitch, hodlFlags, + h.channel, restartSwitch, syncStates, hodlFlags, ) if err != nil { h.t.Fatalf("unable to restart alicelink: %v", err) @@ -4149,7 +4415,7 @@ func (h *persistentLinkHarness) trySignNextCommitment() { // to an htlcswitch. If none is provided by the caller, a new one will be // created using Alice's database. func (h *persistentLinkHarness) restartLink( - aliceChannel *lnwallet.LightningChannel, restartSwitch bool, + aliceChannel *lnwallet.LightningChannel, restartSwitch, syncStates bool, hodlFlags []hodl.Flag) ( ChannelLink, chan time.Time, func(), error) { @@ -4220,6 +4486,7 @@ func (h *persistentLinkHarness) restartLink( NotifyActiveChannel: func(wire.OutPoint) {}, NotifyInactiveChannel: func(wire.OutPoint) {}, HtlcNotifier: aliceSwitch.cfg.HtlcNotifier, + SyncStates: syncStates, } aliceLink := NewChannelLink(aliceCfg, aliceChannel) @@ -5835,7 +6102,7 @@ func TestChannelLinkHoldInvoiceRestart(t *testing.T) { coreLink.cfg.Switch.bestHeight++ // Restart link. - alice.restart(false) + alice.restart(false, false) ctx.aliceLink = alice.link ctx.aliceMsgs = alice.msgs