From e677b1e9c4ad3325b461b0256fca426691e650e4 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 27 Apr 2018 04:11:40 -0700 Subject: [PATCH 01/18] htlcswitch/hodl/config: adds CLI bindings for hodl flags --- htlcswitch/hodl/config.go | 60 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 htlcswitch/hodl/config.go diff --git a/htlcswitch/hodl/config.go b/htlcswitch/hodl/config.go new file mode 100644 index 00000000..e14eb7ea --- /dev/null +++ b/htlcswitch/hodl/config.go @@ -0,0 +1,60 @@ +package hodl + +// Config is a struct enumerating the possible command line flags that are used +// to activate specific hodl modes. +// +// NOTE: THESE FLAGS ARE INTENDED FOR TESTING PURPOSES ONLY. ACTIVATING THESE +// FLAGS IN PRODUCTION WILL VIOLATE CRITICAL ASSUMPTIONS MADE BY THIS SOFTWARE. +type Config struct { + ExitSettle bool `long:"exit-settle" description:"Instructs the node to drop ADDs for which it is the exit node, and to not settle back to the sender"` + + AddIncoming bool `long:"add-incoming" description:"Instructs the node to drop incoming ADDs before processing them in the incoming link"` + + SettleIncoming bool `long:"settle-incoming" description:"Instructs the node to drop incoming SETTLEs before processing them in the incoming link"` + + FailIncoming bool `long:"fail-incoming" description:"Instructs the node to drop incoming FAILs before processing them in the incoming link"` + + AddOutgoing bool `long:"add-outgoing" description:"Instructs the node to drop outgoing ADDs before applying them to the channel state"` + + SettleOutgoing bool `long:"settle-outgoing" description:"Instructs the node to drop outgoing SETTLEs before applying them to the channel state"` + + FailOutgoing bool `long:"fail-outgoing" description:"Instructs the node to drop outgoing FAILs before applying them to the channel state"` + + Commit bool `long:"commit" description:"Instructs the node to add HTLCs to its local commitment state and to open circuits for any ADDs, but abort before committing the changes"` +} + +// Mask extracts the flags specified in the configuration, composing a Mask from +// the active flags. +func (c *Config) Mask() Mask { + var flags []Flag + + if c.ExitSettle { + flags = append(flags, ExitSettle) + } + if c.AddIncoming { + flags = append(flags, AddIncoming) + } + if c.SettleIncoming { + flags = append(flags, SettleIncoming) + } + if c.FailIncoming { + flags = append(flags, FailIncoming) + } + if c.AddOutgoing { + flags = append(flags, AddOutgoing) + } + if c.SettleOutgoing { + flags = append(flags, SettleOutgoing) + } + if c.FailOutgoing { + flags = append(flags, FailOutgoing) + } + if c.Commit { + flags = append(flags, Commit) + } + + // NOTE: The value returned here will only honor the configuration if + // the debug build flag is present. In production, this method always + // returns hodl.MaskNone and Active(*) always returns false. + return MaskFromFlags(flags...) +} From 941bdcafad2ed0f2e1c590f2c79acb3a4ddb1ebe Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 24 Apr 2018 18:09:51 -0700 Subject: [PATCH 02/18] htlcswitch/hodl/flags: adds flags signifying switch breakpoints --- htlcswitch/hodl/flags.go | 111 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 htlcswitch/hodl/flags.go diff --git a/htlcswitch/hodl/flags.go b/htlcswitch/hodl/flags.go new file mode 100644 index 00000000..688999f3 --- /dev/null +++ b/htlcswitch/hodl/flags.go @@ -0,0 +1,111 @@ +package hodl + +import "fmt" + +// MaskNone represents the empty Mask, in which no breakpoints are +// active. +const MaskNone = Mask(0) + +type ( + // Flag represents a single breakpoint where an HTLC should be dropped + // during forwarding. Flags can be composed into a Mask to express more + // complex combinations. + Flag uint32 + + // Mask is a bitvector combining multiple Flags that can be queried to + // see which breakpoints are active. + Mask uint32 +) + +const ( + // ExitSettle drops an incoming ADD for which we are the exit node, + // before processing in the link. + ExitSettle Flag = 1 << iota + + // AddIncoming drops an incoming ADD before processing if we are not + // the exit node. + AddIncoming + + // SettleIncoming drops an incoming SETTLE before processing if we + // are not the exit node. + SettleIncoming + + // FailIncoming drops an incoming FAIL before processing if we are + // not the exit node. + FailIncoming + + // TODO(conner): add modes for switch breakpoints + + // AddOutgoing drops an outgoing ADD before it is added to the + // in-memory commitment state of the link. + AddOutgoing + + // SettleOutgoing drops an SETTLE before it is added to the + // in-memory commitment state of the link. + SettleOutgoing + + // FailOutgoing drops an outgoing FAIL before is is added to the + // in-memory commitment state of the link. + FailOutgoing + + // Commit drops all HTLC after any outgoing circuits have been + // opened, but before the in-memory commitment state is persisted. + Commit +) + +// String returns a human-readable identifier for a given Flag. +func (f Flag) String() string { + switch f { + case ExitSettle: + return "ExitSettle" + case AddIncoming: + return "AddIncoming" + case SettleIncoming: + return "SettleIncoming" + case FailIncoming: + return "FailIncoming" + case AddOutgoing: + return "AddOutgoing" + case SettleOutgoing: + return "SettleOutgoing" + case FailOutgoing: + return "FailOutgoing" + case Commit: + return "Commit" + default: + return "UnknownHodlFlag" + } +} + +// Warning generates a warning message to log if a particular breakpoint is +// triggered during execution. +func (f Flag) Warning() string { + var msg string + switch f { + case ExitSettle: + msg = "will not attempt to settle ADD with sender" + case AddIncoming: + msg = "will not attempt to forward ADD to switch" + case SettleIncoming: + msg = "will not attempt to forward SETTLE to switch" + case FailIncoming: + msg = "will not attempt to forward FAIL to switch" + case AddOutgoing: + msg = "will not update channel state with downstream ADD" + case SettleOutgoing: + msg = "will not update channel state with downstream SETTLE" + case FailOutgoing: + msg = "will not update channel state with downstream FAIL" + case Commit: + msg = "will not commit pending channel updates" + default: + msg = "incorrect hodl flag usage" + } + + return fmt.Sprintf("%s mode enabled -- %s", f, msg) +} + +// Mask returns the Mask consisting solely of this Flag. +func (f Flag) Mask() Mask { + return Mask(f) +} From cd43285993280e78f5b8d34bb8f15f1e5cb59bbb Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Sun, 29 Apr 2018 04:57:34 -0700 Subject: [PATCH 03/18] htlcswitch/hodl/mask_production: disables Mask w/o debug flag --- htlcswitch/hodl/mask_production.go | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 htlcswitch/hodl/mask_production.go diff --git a/htlcswitch/hodl/mask_production.go b/htlcswitch/hodl/mask_production.go new file mode 100644 index 00000000..c2db04ef --- /dev/null +++ b/htlcswitch/hodl/mask_production.go @@ -0,0 +1,21 @@ +// +build !debug + +package hodl + +// DebugBuild signals that this is a production build. +const DebugBuild = false + +// MaskFromFlags in production always returns MaskNone. +func MaskFromFlags(_ ...Flag) Mask { + return MaskNone +} + +// Active in production always returns false for all Flags. +func (m Mask) Active(_ Flag) bool { + return false +} + +// String returns the human-readable identifier for MaskNone. +func (m Mask) String() string { + return "hodl.Mask(NONE)" +} From 8f786bb8601c3b449cd2031b181b5ce7b17b2da5 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Sun, 29 Apr 2018 04:58:36 -0700 Subject: [PATCH 04/18] htlcswitch/hodl/mask_debug: enable Mask w/ debug flag --- htlcswitch/hodl/mask_debug.go | 44 +++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 htlcswitch/hodl/mask_debug.go diff --git a/htlcswitch/hodl/mask_debug.go b/htlcswitch/hodl/mask_debug.go new file mode 100644 index 00000000..5d5fd065 --- /dev/null +++ b/htlcswitch/hodl/mask_debug.go @@ -0,0 +1,44 @@ +// +build debug + +package hodl + +import ( + "fmt" + "strings" +) + +// DebugBuild signals that this is a debug build. +const DebugBuild = true + +// MaskFromFlags merges a variadic set of Flags into a single Mask. +func MaskFromFlags(flags ...Flag) Mask { + var mask Mask + for _, flag := range flags { + mask |= Mask(flag) + } + + return mask +} + +// Active returns true if the bit corresponding to the flag is set within the +// mask. +func (m Mask) Active(flag Flag) bool { + return (Flag(m) & flag) > 0 +} + +// String returns a human-readable description of all active Flags. +func (m Mask) String() string { + if m == MaskNone { + return "hodl.Mask(NONE)" + } + + var activeFlags []string + for i := uint(0); i < 32; i++ { + flag := Flag(1 << i) + if m.Active(flag) { + activeFlags = append(activeFlags, flag.String()) + } + } + + return fmt.Sprintf("hodl.Mask(%s)", strings.Join(activeFlags, "|")) +} From 137ec37450bf7da7de69829b80cc6fc6df23084c Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 27 Apr 2018 02:40:04 -0700 Subject: [PATCH 05/18] htlcswitch/hodl_mask_test: tests HodlMask active flags --- htlcswitch/hodl/mask_test.go | 110 +++++++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) create mode 100644 htlcswitch/hodl/mask_test.go diff --git a/htlcswitch/hodl/mask_test.go b/htlcswitch/hodl/mask_test.go new file mode 100644 index 00000000..7becd457 --- /dev/null +++ b/htlcswitch/hodl/mask_test.go @@ -0,0 +1,110 @@ +package hodl_test + +import ( + "testing" + + "github.com/lightningnetwork/lnd/htlcswitch/hodl" +) + +var hodlMaskTests = []struct { + mask hodl.Mask + flags map[hodl.Flag]struct{} +}{ + { + // Check that the empty mask has no active flags. + mask: hodl.MaskNone, + flags: map[hodl.Flag]struct{}{}, + }, + { + // Check that passing no arguments to MaskFromFlags is + // equivalent to MaskNone. + mask: hodl.MaskFromFlags(), + flags: map[hodl.Flag]struct{}{}, + }, + + { + // Check using Mask to convert a single flag into a Mask only + // reports that flag active. + mask: hodl.ExitSettle.Mask(), + flags: map[hodl.Flag]struct{}{ + hodl.ExitSettle: {}, + }, + }, + { + // Check that using MaskFromFlags on a single flag only reports + // that flag active. + mask: hodl.MaskFromFlags(hodl.Commit), + flags: map[hodl.Flag]struct{}{ + hodl.Commit: {}, + }, + }, + + { + // Check that using MaskFromFlags on some-but-not-all flags + // reports the correct subset of flags as active. + mask: hodl.MaskFromFlags( + hodl.ExitSettle, + hodl.Commit, + hodl.AddIncoming, + hodl.SettleOutgoing, + ), + flags: map[hodl.Flag]struct{}{ + hodl.ExitSettle: {}, + hodl.Commit: {}, + hodl.AddIncoming: {}, + hodl.SettleOutgoing: {}, + }, + }, + { + // Check that using MaskFromFlags on all known flags reports + // those an no other flags. + mask: hodl.MaskFromFlags( + hodl.ExitSettle, + hodl.AddIncoming, + hodl.SettleIncoming, + hodl.FailIncoming, + hodl.AddOutgoing, + hodl.SettleOutgoing, + hodl.FailOutgoing, + hodl.Commit, + ), + flags: map[hodl.Flag]struct{}{ + hodl.ExitSettle: {}, + hodl.AddIncoming: {}, + hodl.SettleIncoming: {}, + hodl.FailIncoming: {}, + hodl.AddOutgoing: {}, + hodl.SettleOutgoing: {}, + hodl.FailOutgoing: {}, + hodl.Commit: {}, + }, + }, +} + +// TestMask iterates through all of the hodlMaskTests, checking that the mask +// correctly reports active for flags in the tests' expected flags, and inactive +// for all others. +func TestMask(t *testing.T) { + if !hodl.DebugBuild { + t.Fatalf("htlcswitch tests must be run with '-tags debug'") + } + + for i, test := range hodlMaskTests { + for j := uint32(0); i < 32; i++ { + flag := hodl.Flag(1 << j) + _, shouldBeActive := test.flags[flag] + + switch { + case shouldBeActive && !test.mask.Active(flag): + t.Fatalf("hodl mask test #%d -- "+ + "expected flag %s to be active", + i, flag) + + case !shouldBeActive && test.mask.Active(flag): + t.Fatalf("hodl mask test #%d -- "+ + "expected flag %s to be inactive", + i, flag) + } + } + } +} From ffd240e0eeb0ca3b30ca0d3cbda130e9a9f37bfe Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Sun, 29 Apr 2018 04:49:14 -0700 Subject: [PATCH 06/18] Makefile: build debug binaries with build directive --- Makefile | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index b2a18ae2..5297aed0 100644 --- a/Makefile +++ b/Makefile @@ -124,9 +124,9 @@ btcd: $(GLIDE_BIN) $(BTCD_DIR) # ============ build: - @$(call print, "Building lnd and lncli.") - $(GOBUILD) -o lnd $(LDFLAGS) $(PKG) - $(GOBUILD) -o lncli $(LDFLAGS) $(PKG)/cmd/lncli + @$(call print, "Building debug lnd and lncli.") + $(GOBUILD) -tags=$(TEST_TAGS) -o lnd-debug $(LDFLAGS) $(PKG) + $(GOBUILD) -tags=$(TEST_TAGS) -o lncli-debug $(LDFLAGS) $(PKG)/cmd/lncli install: @$(call print, "Installing lnd and lncli.") From da53b35c73073b2403d2286b101ec91457fa7880 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Sun, 29 Apr 2018 04:38:59 -0700 Subject: [PATCH 07/18] make/testing_flags: compiles tests with debug flag --- make/testing_flags.mk | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/make/testing_flags.mk b/make/testing_flags.mk index 3c3f926b..a9de0c88 100644 --- a/make/testing_flags.mk +++ b/make/testing_flags.mk @@ -1,3 +1,4 @@ +TEST_TAGS = debug TEST_FLAGS = # If specific package is being unit tested, construct the full name of the @@ -31,15 +32,16 @@ UNIT_TARGETED ?= no # If a specific package/test case was requested, run the unit test for the # targeted case. Otherwise, default to running all tests. ifeq ($(UNIT_TARGETED), yes) -UNIT := $(GOTEST) $(TEST_FLAGS) $(UNITPKG) -UNIT_RACE := $(GOTEST) $(TEST_FLAGS) -race $(UNITPKG) +UNIT := $(GOTEST) -tags="$(TEST_TAGS)" $(TEST_FLAGS) $(UNITPKG) +UNIT_RACE := $(GOTEST) -tags="$(TEST_TAGS)" $(TEST_FLAGS) -race $(UNITPKG) endif ifeq ($(UNIT_TARGETED), no) -UNIT := $(GOLIST) | $(XARGS) $(GOTEST) $(TEST_FLAGS) -UNIT_RACE := $(UNIT) -race +UNIT := $(GOLIST) | $(XARGS) $(GOTEST) -tags="$(TEST_TAGS)" $(TEST_FLAGS) +UNIT_RACE := $(UNIT) -race endif # Construct the integration test command with the added build flags. -ITEST := $(GOTEST) $(TEST_FLAGS) -tags rpctest -logoutput +ITEST_TAGS := $(TEST_TAGS) rpctest +ITEST := $(GOTEST) -tags="$(ITEST_TAGS)" $(TEST_FLAGS) -logoutput From a36e1e62783800b25e06fb15db2e1078a60b716b Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 27 Apr 2018 02:51:13 -0700 Subject: [PATCH 08/18] htlcswitch/link: adds HodlFlag breakpoints This commit inserts an initial set of HodlFlags into their correct places within the switch. In lieu of the existing HtlcHodl mode, it is been replaced with a configurable HodlMask, which is a bitvector representing the desired breakpoints. This will allow for fine grained testing of the switch's internals, since we can create arbitrary delays inside a otherwise asynchronous system. --- htlcswitch/link.go | 79 +++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 71 insertions(+), 8 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 01b5f2d5..33297ef3 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -14,6 +14,7 @@ import ( "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/contractcourt" + "github.com/lightningnetwork/lnd/htlcswitch/hodl" "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwire" "github.com/roasbeef/btcd/chaincfg/chainhash" @@ -201,12 +202,12 @@ type ChannelLinkConfig struct { // available state transition. DebugHTLC bool - // HodlHTLC should be active if you want this node to refrain from - // settling all incoming HTLCs with the sender if it finds itself to be - // the exit node. + // hodl.Mask is a bitvector composed of hodl.Flags, specifying breakpoints + // for HTLC forwarding internal to the switch. // - // NOTE: HodlHTLC should be active in conjunction with DebugHTLC. - HodlHTLC bool + // NOTE: This should only be used for testing, and should only be used + // simultaneously with DebugHTLC. + HodlMask hodl.Mask // SyncStates is used to indicate that we need send the channel // reestablishment message to the remote peer. It should be done if our @@ -933,6 +934,13 @@ func (l *channelLink) handleDownStreamPkt(pkt *htlcPacket, isReProcess bool) { var isSettle bool switch htlc := pkt.htlc.(type) { case *lnwire.UpdateAddHTLC: + // If hodl.AddOutgoing mode is active, we exit early to simulate + // arbitrary delays between the switch adding an ADD to the + // mailbox, and the HTLC being added to the commitment state. + if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.AddOutgoing) { + l.warnf(hodl.AddOutgoing.Warning()) + return + } // A new payment has been initiated via the downstream channel, // so we add the new HTLC to our local log, then update the @@ -1034,6 +1042,15 @@ func (l *channelLink) handleDownStreamPkt(pkt *htlcPacket, isReProcess bool) { l.cfg.Peer.SendMessage(htlc, false) case *lnwire.UpdateFulfillHTLC: + // If hodl.SettleOutgoing mode is active, we exit early to + // simulate arbitrary delays between the switch adding the + // SETTLE to the mailbox, and the HTLC being added to the + // commitment state. + if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.SettleOutgoing) { + l.warnf(hodl.SettleOutgoing.Warning()) + return + } + // An HTLC we forward to the switch has just settled somewhere // upstream. Therefore we settle the HTLC within the our local // state machine. @@ -1067,6 +1084,15 @@ func (l *channelLink) handleDownStreamPkt(pkt *htlcPacket, isReProcess bool) { isSettle = true case *lnwire.UpdateFailHTLC: + // If hodl.FailOutgoing mode is active, we exit early to + // simulate arbitrary delays between the switch adding a FAIL to + // the mailbox, and the HTLC being added to the commitment + // state. + if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.FailOutgoing) { + l.warnf(hodl.FailOutgoing.Warning()) + return + } + // An HTLC cancellation has been triggered somewhere upstream, // we'll remove then HTLC from our local state machine. closedCircuitRef := pkt.inKey() @@ -1421,6 +1447,15 @@ func (l *channelLink) updateCommitTx() error { // Reset the batch, but keep the backing buffer to avoid reallocating. l.keystoneBatch = l.keystoneBatch[:0] + // If hodl.Commit mode is active, we will refrain from attempting to + // commit any in-memory modifications to the channel state. Exiting here + // permits testing of either the switch or link's ability to trim + // circuits that have been opened, but unsuccessfully committed. + if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.Commit) { + l.warnf(hodl.Commit.Warning()) + return nil + } + theirCommitSig, htlcSigs, err := l.channel.SignNextCommitment() if err == lnwallet.ErrNoWindow { l.tracef("revocation window exhausted, unable to send: %v, "+ @@ -1761,6 +1796,14 @@ func (l *channelLink) processRemoteSettleFails(fwdPkg *channeldb.FwdPkg, // received. So we'll forward the HTLC to the switch which will // handle propagating the settle to the prior hop. case lnwallet.Settle: + // If hodl.SettleIncoming is requested, we will not + // forward the SETTLE to the switch and will not signal + // a free slot on the commitment transaction. + if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.SettleIncoming) { + l.warnf(hodl.SettleIncoming.Warning()) + continue + } + settlePacket := &htlcPacket{ outgoingChanID: l.ShortChanID(), outgoingHTLCID: pd.ParentIndex, @@ -1781,6 +1824,14 @@ func (l *channelLink) processRemoteSettleFails(fwdPkg *channeldb.FwdPkg, // our commitment state, so we'll forward this to the switch so // the backwards undo can continue. case lnwallet.Fail: + // If hodl.SettleIncoming is requested, we will not + // forward the FAIL to the switch and will not signal a + // free slot on the commitment transaction. + if l.cfg.DebugHTLC && l.cfg.HodlMask.Active(hodl.FailIncoming) { + l.warnf(hodl.FailIncoming.Warning()) + continue + } + // Fetch the reason the HTLC was cancelled so we can // continue to propagate it. failPacket := &htlcPacket{ @@ -1920,9 +1971,12 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg, fwdInfo := chanIterator.ForwardingInstructions() switch fwdInfo.NextHop { case exitHop: - if l.cfg.DebugHTLC && l.cfg.HodlHTLC { - log.Warnf("hodl HTLC mode enabled, will not " + - "attempt to settle HTLC with sender") + // If hodl.ExitSettle is requested, we will not validate + // the final hop's ADD, nor will we settle the + // corresponding invoice or respond with the preimage. + if l.cfg.DebugHTLC && + l.cfg.HodlMask.Active(hodl.ExitSettle) { + l.warnf(hodl.ExitSettle.Warning()) continue } @@ -2107,6 +2161,15 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg, // we'll verify that our forwarding constraints have been // properly met by this incoming HTLC. default: + // If hodl.AddIncoming is requested, we will not + // validate the forwarded ADD, nor will we send the + // packet to the htlc switch. + if l.cfg.DebugHTLC && + l.cfg.HodlMask.Active(hodl.AddIncoming) { + l.warnf(hodl.AddIncoming.Warning()) + continue + } + switch fwdPkg.State { case channeldb.FwdStateProcessed: // This add was not forwarded on the previous From ab607a7ec6910732749070383f7f4f9ef5708acf Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 26 Apr 2018 14:07:45 -0700 Subject: [PATCH 09/18] config: hodl config --- config.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/config.go b/config.go index 0c39c25a..0ac2f39f 100644 --- a/config.go +++ b/config.go @@ -20,6 +20,7 @@ import ( flags "github.com/jessevdk/go-flags" "github.com/lightningnetwork/lnd/brontide" + "github.com/lightningnetwork/lnd/htlcswitch/hodl" "github.com/lightningnetwork/lnd/lnwire" "github.com/lightningnetwork/lnd/torsvc" "github.com/roasbeef/btcd/btcec" @@ -174,7 +175,6 @@ type config struct { Profile string `long:"profile" description:"Enable HTTP profiling on given port -- NOTE port must be between 1024 and 65535"` DebugHTLC bool `long:"debughtlc" description:"Activate the debug htlc mode. With the debug HTLC mode, all payments sent use a pre-determined R-Hash. Additionally, all HTLCs sent to a node with the debug HTLC R-Hash are immediately settled in the next available state transition."` - HodlHTLC bool `long:"hodlhtlc" description:"Activate the hodl HTLC mode. With hodl HTLC mode, all incoming HTLCs will be accepted by the receiving node, but no attempt will be made to settle the payment with the sender."` UnsafeDisconnect bool `long:"unsafe-disconnect" description:"Allows the rpcserver to intentionally disconnect from peers with open channels. USED FOR TESTING ONLY."` UnsafeReplay bool `long:"unsafe-replay" description:"Causes a link to replay the adds on its commitment txn after starting up, this enables testing of the sphinx replay logic."` MaxPendingChannels int `long:"maxpendingchannels" description:"The maximum number of incoming pending channels permitted per peer."` @@ -192,6 +192,8 @@ type config struct { Tor *torConfig `group:"Tor" namespace:"tor"` + Hodl *hodl.Config `group:"hodl" namespace:"hodl"` + NoNetBootstrap bool `long:"nobootstrap" description:"If true, then automatic network bootstrapping will not be attempted."` NoEncryptWallet bool `long:"noencryptwallet" description:"If set, wallet will be encrypted using the default passphrase."` From 701d37725cff1f030a2f16a758f899d45d533517 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 26 Apr 2018 14:08:02 -0700 Subject: [PATCH 10/18] peer: extract hodl mask, remove htlchodl mode --- peer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/peer.go b/peer.go index 248e5830..c44697fc 100644 --- a/peer.go +++ b/peer.go @@ -417,7 +417,7 @@ func (p *peer) loadActiveChannels(chans []*channeldb.OpenChannel) error { p.server.chanRouter, p.PubKey(), ), DebugHTLC: cfg.DebugHTLC, - HodlHTLC: cfg.HodlHTLC, + HodlMask: cfg.Hodl.Mask(), Registry: p.server.invoices, Switch: p.server.htlcSwitch, Circuits: p.server.htlcSwitch.CircuitModifier(), @@ -1394,7 +1394,7 @@ out: p.server.chanRouter, p.PubKey(), ), DebugHTLC: cfg.DebugHTLC, - HodlHTLC: cfg.HodlHTLC, + HodlMask: cfg.Hodl.Mask(), Registry: p.server.invoices, Switch: p.server.htlcSwitch, Circuits: p.server.htlcSwitch.CircuitModifier(), From 6fa7b2f8f72d75dc155f9ef0ac0257796d273815 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 30 Apr 2018 04:23:52 -0700 Subject: [PATCH 11/18] lntest/node: execute lnd-debug binary --- lntest/node.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lntest/node.go b/lntest/node.go index aaf16b87..0d062c8b 100644 --- a/lntest/node.go +++ b/lntest/node.go @@ -260,7 +260,7 @@ func (hn *HarnessNode) start(lndError chan<- error) error { args := hn.cfg.genArgs() args = append(args, fmt.Sprintf("--profile=%d", 9000+hn.NodeID)) - hn.cmd = exec.Command("./lnd", args...) + hn.cmd = exec.Command("./lnd-debug", args...) // Redirect stderr output to buffer var errb bytes.Buffer From 57245b5784e109555d22024931db697548198e8e Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Thu, 26 Apr 2018 14:08:24 -0700 Subject: [PATCH 12/18] lnd_test: convert hodlhtlc -> hodl.exit_settle --- lnd_test.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index 7a5bf241..f1f38035 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -1665,7 +1665,7 @@ func testChannelForceClosure(net *lntest.NetworkHarness, t *harnessTest) { // Since we'd like to test failure scenarios with outstanding htlcs, // we'll introduce another node into our test network: Carol. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -2248,7 +2248,7 @@ func testSphinxReplayPersistence(net *lntest.NetworkHarness, t *harnessTest) { chanAmt := btcutil.Amount(100000) // First, we'll create Dave, the receiver, and start him in hodl mode. - dave, err := net.NewNode("Dave", []string{"--debughtlc", "--hodlhtlc"}) + dave, err := net.NewNode("Dave", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -4303,7 +4303,7 @@ func testRevokedCloseRetributionZeroValueRemoteOutput(net *lntest.NetworkHarness // Since we'd like to test some multi-hop failure scenarios, we'll // introduce another node into our test network: Carol. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -4537,7 +4537,7 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, // Since this test will result in the counterparty being left in a // weird state, we will introduce another node into our test network: // Carol. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -4545,7 +4545,7 @@ func testRevokedCloseRetributionRemoteHodl(net *lntest.NetworkHarness, // We'll also create a new node Dave, who will have a channel with // Carol, and also use similar settings so we can broadcast a commit // with active HTLCs. - dave, err := net.NewNode("Dave", []string{"--debughtlc", "--hodlhtlc"}) + dave, err := net.NewNode("Dave", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new dave node: %v", err) } @@ -6190,7 +6190,7 @@ func createThreeHopHodlNetwork(t *harnessTest, // Next, we'll create a new node "carol" and have Bob connect to her. // In this test, we'll make carol always hold onto the HTLC, this way // it'll force Bob to go to chain to resolve the HTLC. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new node: %v", err) } @@ -7601,7 +7601,7 @@ func testSwitchCircuitPersistence(net *lntest.NetworkHarness, t *harnessTest) { // Next, we'll create Carol and establish a channel to from her to // Dave. Carol is started in htlchodl mode so that we can disconnect the // intermediary hops before starting the settle. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -7922,7 +7922,7 @@ func testSwitchOfflineDelivery(net *lntest.NetworkHarness, t *harnessTest) { // Next, we'll create Carol and establish a channel to from her to // Dave. Carol is started in htlchodl mode so that we can disconnect the // intermediary hops before starting the settle. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -8248,7 +8248,7 @@ func testSwitchOfflineDeliveryPersistence(net *lntest.NetworkHarness, t *harness // Next, we'll create Carol and establish a channel to from her to // Dave. Carol is started in htlchodl mode so that we can disconnect the // intermediary hops before starting the settle. - carol, err := net.NewNode("Dave", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } @@ -8575,7 +8575,7 @@ func testSwitchOfflineDeliveryOutgoingOffline( // Next, we'll create Carol and establish a channel to from her to // Dave. Carol is started in htlchodl mode so that we can disconnect the // intermediary hops before starting the settle. - carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodlhtlc"}) + carol, err := net.NewNode("Carol", []string{"--debughtlc", "--hodl.exit-settle"}) if err != nil { t.Fatalf("unable to create new nodes: %v", err) } From 9c178f3d7f6db36b05a6a76cc5731b383fc561e0 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Sat, 28 Apr 2018 23:19:48 -0700 Subject: [PATCH 13/18] htlcswitch/link_test: use hodl ExitSettle instead of HodlHTLC --- htlcswitch/link_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 699773ed..1454847e 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -19,6 +19,7 @@ import ( "github.com/lightningnetwork/lnd/chainntnfs" "github.com/lightningnetwork/lnd/channeldb" "github.com/lightningnetwork/lnd/contractcourt" + "github.com/lightningnetwork/lnd/htlcswitch/hodl" "github.com/lightningnetwork/lnd/lnwallet" "github.com/lightningnetwork/lnd/lnwire" "github.com/roasbeef/btcd/btcec" @@ -1704,9 +1705,9 @@ func TestChannelLinkBandwidthConsistency(t *testing.T) { aliceMsgs = coreLink.cfg.Peer.(*mockPeer).sentMsgs ) - // We put Alice into HodlHTLC mode, such that she won't settle + // We put Alice into hodl.ExitSettle mode, such that she won't settle // incoming HTLCs automatically. - coreLink.cfg.HodlHTLC = true + coreLink.cfg.HodlMask = hodl.MaskFromFlags(hodl.ExitSettle) coreLink.cfg.DebugHTLC = true estimator := &lnwallet.StaticFeeEstimator{ From 308ad1caf67c3c7eb689b919b7d70503630cf74d Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Sat, 28 Apr 2018 23:21:43 -0700 Subject: [PATCH 14/18] htlcswitch/link_test: add link trimming tests --- htlcswitch/link_test.go | 844 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 822 insertions(+), 22 deletions(-) diff --git a/htlcswitch/link_test.go b/htlcswitch/link_test.go index 1454847e..892d5ec9 100644 --- a/htlcswitch/link_test.go +++ b/htlcswitch/link_test.go @@ -6,6 +6,7 @@ import ( "encoding/binary" "fmt" "io" + "reflect" "runtime" "strings" "sync" @@ -1413,30 +1414,31 @@ func (m *mockPeer) Disconnect(reason error) { var _ Peer = (*mockPeer)(nil) func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) ( - ChannelLink, *lnwallet.LightningChannel, chan time.Time, func(), error) { - globalEpoch := &chainntnfs.BlockEpochEvent{ - Epochs: make(chan *chainntnfs.BlockEpoch), - Cancel: func() { - }, - } + ChannelLink, *lnwallet.LightningChannel, chan time.Time, func(), + chanRestoreFunc, error) { var chanIDBytes [8]byte if _, err := io.ReadFull(rand.Reader, chanIDBytes[:]); err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, nil, err } chanID := lnwire.NewShortChanIDFromInt( binary.BigEndian.Uint64(chanIDBytes[:])) - aliceChannel, bobChannel, fCleanUp, _, err := createTestChannel( + aliceChannel, bobChannel, fCleanUp, restore, err := createTestChannel( alicePrivKey, bobPrivKey, chanAmt, chanAmt, chanReserve, chanReserve, chanID, ) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, nil, err } var ( + globalEpoch = &chainntnfs.BlockEpochEvent{ + Epochs: make(chan *chainntnfs.BlockEpoch), + Cancel: func() { + }, + } invoiceRegistry = newMockRegistry() decoder = newMockIteratorDecoder() obfuscator = NewMockObfuscator() @@ -1444,7 +1446,6 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) ( sentMsgs: make(chan lnwire.Message, 2000), quit: make(chan struct{}), } - globalPolicy = ForwardingPolicy{ MinHTLC: lnwire.NewMSatFromSatoshis(5), BaseFee: lnwire.NewMSatFromSatoshis(1), @@ -1461,7 +1462,7 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) ( aliceSwitch, err := New(Config{DB: aliceDb}) if err != nil { - return nil, nil, nil, nil, err + return nil, nil, nil, nil, nil, err } t := make(chan time.Time) @@ -1494,11 +1495,8 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) ( const startingHeight = 100 aliceLink := NewChannelLink(aliceCfg, aliceChannel, startingHeight) - mailbox := newMemoryMailBox() - mailbox.Start() - aliceLink.AttachMailBox(mailbox) - if err := aliceLink.Start(); err != nil { - return nil, nil, nil, nil, err + if err := aliceSwitch.AddLink(aliceLink); err != nil { + return nil, nil, nil, nil, nil, err } go func() { for { @@ -1517,7 +1515,7 @@ func newSingleLinkTestHarness(chanAmt, chanReserve btcutil.Amount) ( defer bobChannel.Stop() } - return aliceLink, bobChannel, t, cleanUp, nil + return aliceLink, bobChannel, t, cleanUp, restore, nil } func assertLinkBandwidth(t *testing.T, link ChannelLink, @@ -1689,7 +1687,8 @@ func TestChannelLinkBandwidthConsistency(t *testing.T) { // We'll start the test by creating a single instance of const chanAmt = btcutil.SatoshiPerBitcoin * 5 - aliceLink, bobChannel, tmr, cleanUp, err := newSingleLinkTestHarness(chanAmt, 0) + aliceLink, bobChannel, tmr, cleanUp, _, err := + newSingleLinkTestHarness(chanAmt, 0) if err != nil { t.Fatalf("unable to create link: %v", err) } @@ -2106,7 +2105,7 @@ func TestChannelLinkBandwidthConsistencyOverflow(t *testing.T) { var mockBlob [lnwire.OnionPacketSize]byte const chanAmt = btcutil.SatoshiPerBitcoin * 5 - aliceLink, bobChannel, batchTick, cleanUp, err := + aliceLink, bobChannel, batchTick, cleanUp, _, err := newSingleLinkTestHarness(chanAmt, 0) if err != nil { t.Fatalf("unable to create link: %v", err) @@ -2315,6 +2314,557 @@ func TestChannelLinkBandwidthConsistencyOverflow(t *testing.T) { } } +// genAddsAndCircuits creates `numHtlcs` sequential ADD packets and there +// corresponding circuits. The provided `htlc` is used in all test packets. +func genAddsAndCircuits(numHtlcs int, htlc *lnwire.UpdateAddHTLC) ( + []*htlcPacket, []*PaymentCircuit) { + + addPkts := make([]*htlcPacket, 0, numHtlcs) + circuits := make([]*PaymentCircuit, 0, numHtlcs) + for i := 0; i < numHtlcs; i++ { + addPkt := htlcPacket{ + htlc: htlc, + incomingChanID: sourceHop, + incomingHTLCID: uint64(i), + obfuscator: NewMockObfuscator(), + } + + circuit := makePaymentCircuit(&htlc.PaymentHash, &addPkt) + addPkt.circuit = &circuit + + addPkts = append(addPkts, &addPkt) + circuits = append(circuits, &circuit) + } + + return addPkts, circuits +} + +// TestChannelLinkTrimCircuitsPending checks that the switch and link properly +// trim circuits if there are open circuits corresponding to ADDs on a pending +// commmitment transaction. +func TestChannelLinkTrimCircuitsPending(t *testing.T) { + t.Parallel() + + const ( + chanAmt = btcutil.SatoshiPerBitcoin * 5 + numHtlcs = 4 + halfHtlcs = numHtlcs / 2 + ) + + // We'll start by creating a new link with our chanAmt (5 BTC). We will + // only be testing Alice's behavior, so the reference to Bob's channel + // state is unnecessary. + aliceLink, _, batchTicker, cleanUp, restore, err := + newSingleLinkTestHarness(chanAmt, 0) + if err != nil { + t.Fatalf("unable to create link: %v", err) + } + defer cleanUp() + + alice := newPersistentLinkHarness(t, aliceLink, batchTicker, restore) + + // Compute the static fees that will be used to determine the + // correctness of Alice's bandwidth when forwarding HTLCs. + estimator := &lnwallet.StaticFeeEstimator{ + FeeRate: 24, + } + feeRate, err := estimator.EstimateFeePerVSize(1) + if err != nil { + t.Fatalf("unable to query fee estimator: %v", err) + } + + defaultCommitFee := alice.channel.StateSnapshot().CommitFee + htlcFee := lnwire.NewMSatFromSatoshis( + feeRate.FeePerKWeight().FeeForWeight(lnwallet.HtlcWeight), + ) + + // The starting bandwidth of the channel should be exactly the amount + // that we created the channel between her and Bob, minus the commitment + // fee. + expectedBandwidth := lnwire.NewMSatFromSatoshis(chanAmt - defaultCommitFee) + assertLinkBandwidth(t, alice.link, expectedBandwidth) + + // Capture Alice's starting bandwidth to perform later, relative + // bandwidth assertions. + aliceStartingBandwidth := alice.link.Bandwidth() + + // Next, we'll create an HTLC worth 1 BTC that will be used as a dummy + // message for the test. + var mockBlob [lnwire.OnionPacketSize]byte + htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) + _, htlc, err := generatePayment(htlcAmt, htlcAmt, 5, mockBlob) + if err != nil { + t.Fatalf("unable to create payment: %v", err) + } + + // Create `numHtlc` htlcPackets and payment circuits that will be used + // to drive the test. All of the packets will use the same dummy HTLC. + addPkts, circuits := genAddsAndCircuits(numHtlcs, htlc) + + // To begin the test, start by committing the circuits belong to our + // first two HTLCs. + fwdActions := alice.commitCircuits(circuits[:halfHtlcs]) + + // Both of these circuits should have successfully added, as this is the + // first attempt to send them. + if len(fwdActions.Adds) != halfHtlcs { + t.Fatalf("expected %d circuits to be added", halfHtlcs) + } + alice.assertNumPendingNumOpenCircuits(2, 0) + + // Since both were committed successfully, we will now deliver them to + // Alice's link. + for _, addPkt := range addPkts[:halfHtlcs] { + if err := alice.link.HandleSwitchPacket(addPkt); err != nil { + t.Fatalf("unable to handle switch packet: %v", err) + } + } + + // Wait until Alice's link has sent both HTLCs via the peer. + alice.checkSent(addPkts[:halfHtlcs]) + + // The resulting bandwidth should reflect that Alice is paying both + // htlc amounts, in addition to both htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) + + // Now, initiate a state transition by Alice so that the pending HTLCs + // are locked in. This will *not* involve any participation by Bob, + // which ensures the commitment will remain in a pending state. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(2, 2) + + // Restart Alice's link, which simulates a disconnection with the remote + // peer. + cleanUp = alice.restart(false) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(2, 2) + + // Make a second attempt to commit the first two circuits. This can + // happen if the incoming link flaps, but also allows us to verify that + // the circuits were trimmed properly. + fwdActions = alice.commitCircuits(circuits[:halfHtlcs]) + + // Since Alice has a pending commitment with the first two HTLCs, the + // restart should not have trimmed them from the circuit map. + // Therefore, we expect both of these circuits to be dropped by the + // switch, as keystones should still be set. + if len(fwdActions.Drops) != halfHtlcs { + t.Fatalf("expected %d packets to be dropped", halfHtlcs) + } + + // The resulting bandwidth should remain unchanged from before, + // reflecting that Alice is paying both htlc amounts, in addition to + // both htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) + + // Now, restart Alice's link *and* the entire switch. This will ensure + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(2, 2) + + // Alice should not send out any messages. Even though Alice has a + // pending commitment transaction, channel reestablishment is not + // enabled in this test. + select { + case <-alice.msgs: + t.Fatalf("message should not have been sent by Alice") + case <-time.After(time.Second): + } + + // We will now try to commit the circuits for all of our HTLCs. The + // first two are already on the pending commitment transaction, the + // latter two are new HTLCs. + fwdActions = alice.commitCircuits(circuits) + + // The first two circuits should have been dropped, as they are still on + // the pending commitment transaction, and the restart should not have + // trimmed the circuits for these valid HTLCs. + if len(fwdActions.Drops) != halfHtlcs { + t.Fatalf("expected %d packets to be dropped", halfHtlcs) + } + // The latter two circuits are unknown the circuit map, and should + // report being added. + if len(fwdActions.Adds) != halfHtlcs { + t.Fatalf("expected %d packets to be added", halfHtlcs) + } + + // Deliver the latter two HTLCs to Alice's links so that they can be + // processed and added to the in-memory commitment state. + for _, addPkt := range addPkts[halfHtlcs:] { + if err := alice.link.HandleSwitchPacket(addPkt); err != nil { + t.Fatalf("unable to handle switch packet: %v", err) + } + } + + // Wait for Alice to send the two latter HTLCs via the peer. + alice.checkSent(addPkts[halfHtlcs:]) + + // With two HTLCs on the pending commit, and two added to the in-memory + // commitment state, the resulting bandwidth should reflect that Alice + // is paying the all htlc amounts in addition to all htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-numHtlcs*(htlcAmt+htlcFee), + ) + + // We will try to initiate a state transition for Alice, which will + // ensure the circuits for the two in-memory HTLCs are opened. However, + // since we have a pending commitment, these HTLCs will not actually be + // included in a commitment. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(4, 4) + + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(4, 4) + + // Again, try to recommit all of our circuits. + fwdActions = alice.commitCircuits(circuits) + + // It is expected that all of these will get dropped by the switch. + // The first two circuits are still open as a result of being on the + // commitment transaction. The latter two should have had their open + // circuits trimmed, *but* since the HTLCs are still in Alice's mailbox, + // the switch knows not to fail them as a result of the latter two + // circuits never having been loaded from disk. + if len(fwdActions.Drops) != numHtlcs { + t.Fatalf("expected %d packets to be dropped", numHtlcs) + } + + // Wait for the latter two htlcs to be pulled from the mailbox, added to + // the in-memory channel state, and sent out via the peer. + alice.checkSent(addPkts[halfHtlcs:]) + + // This should result in reconstructing the same bandwidth as our last + // assertion. There are two HTLCs on the pending commit, and two added + // to the in-memory commitment state, the resulting bandwidth should + // reflect that Alice is paying the all htlc amounts in addition to all + // htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-numHtlcs*(htlcAmt+htlcFee), + ) + + // Again, we will try to initiate a state transition for Alice, which + // will ensure the circuits for the two in-memory HTLCs are opened. + // As before, these HTLCs will not actually be included in a commitment + // since we have a pending commitment. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(4, 4) + + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(4, 2) + + // Alice's mailbox will be empty after the restart, and no channel + // reestablishment is configured, so no messages will be sent upon + // restart. + select { + case <-alice.msgs: + t.Fatalf("message should not have been sent by Alice") + case <-time.After(time.Second): + } + + // Finally, make one last attempt to commit all circuits. + fwdActions = alice.commitCircuits(circuits) + + // The first two HTLCs should still be dropped by the htlcswitch. Their + // existence on the pending commitment transaction should prevent their + // open circuits from being trimmed. + if len(fwdActions.Drops) != halfHtlcs { + t.Fatalf("expected %d packets to be dropped", halfHtlcs) + } + // The latter two HTLCs should now be failed by the switch. These will + // have been trimmed by the link or switch restarting, and since the + // HTLCs are known to be lost from memory (since their circuits were + // loaded from disk), it is safe fail them back as they won't ever be + // delivered to the outgoing link. + if len(fwdActions.Fails) != halfHtlcs { + t.Fatalf("expected %d packets to be dropped", halfHtlcs) + } + + // Since the latter two HTLCs have been completely dropped from memory, + // only the first two HTLCs we added should still be reflected in the + // channel bandwidth. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) +} + +// TestChannelLinkTrimCircuitsNoCommit checks that the switch and link properly trim +// circuits if the ADDs corresponding to open circuits are never committed. +func TestChannelLinkTrimCircuitsNoCommit(t *testing.T) { + t.Parallel() + + const ( + chanAmt = btcutil.SatoshiPerBitcoin * 5 + numHtlcs = 4 + halfHtlcs = numHtlcs / 2 + ) + + // We'll start by creating a new link with our chanAmt (5 BTC). We will + // only be testing Alice's behavior, so the reference to Bob's channel + // state is unnecessary. + aliceLink, _, batchTicker, cleanUp, restore, err := + newSingleLinkTestHarness(chanAmt, 0) + if err != nil { + t.Fatalf("unable to create link: %v", err) + } + defer cleanUp() + + alice := newPersistentLinkHarness(t, aliceLink, batchTicker, restore) + + // We'll put Alice into hodl.Commit mode, such that the circuits for any + // outgoing ADDs are opened, but the changes are not committed in the + // channel state. + alice.coreLink.cfg.HodlMask = hodl.Commit.Mask() + alice.coreLink.cfg.DebugHTLC = true + + // Compute the static fees that will be used to determine the + // correctness of Alice's bandwidth when forwarding HTLCs. + estimator := &lnwallet.StaticFeeEstimator{ + FeeRate: 24, + } + feeRate, err := estimator.EstimateFeePerVSize(1) + if err != nil { + t.Fatalf("unable to query fee estimator: %v", err) + } + + defaultCommitFee := alice.channel.StateSnapshot().CommitFee + htlcFee := lnwire.NewMSatFromSatoshis( + feeRate.FeePerKWeight().FeeForWeight(lnwallet.HtlcWeight), + ) + + // The starting bandwidth of the channel should be exactly the amount + // that we created the channel between her and Bob, minus the commitment + // fee. + expectedBandwidth := lnwire.NewMSatFromSatoshis(chanAmt - defaultCommitFee) + assertLinkBandwidth(t, alice.link, expectedBandwidth) + + // Capture Alice's starting bandwidth to perform later, relative + // bandwidth assertions. + aliceStartingBandwidth := alice.link.Bandwidth() + + // Next, we'll create an HTLC worth 1 BTC that will be used as a dummy + // message for the test. + var mockBlob [lnwire.OnionPacketSize]byte + htlcAmt := lnwire.NewMSatFromSatoshis(btcutil.SatoshiPerBitcoin) + _, htlc, err := generatePayment(htlcAmt, htlcAmt, 5, mockBlob) + if err != nil { + t.Fatalf("unable to create payment: %v", err) + } + + // Create `numHtlc` htlcPackets and payment circuits that will be used + // to drive the test. All of the packets will use the same dummy HTLC. + addPkts, circuits := genAddsAndCircuits(numHtlcs, htlc) + + // To begin the test, start by committing the circuits belong to our + // first two HTLCs. + fwdActions := alice.commitCircuits(circuits[:halfHtlcs]) + + // Both of these circuits should have successfully added, as this is the + // first attempt to send them. + if len(fwdActions.Adds) != halfHtlcs { + t.Fatalf("expected %d circuits to be added", halfHtlcs) + } + + // Since both were committed successfully, we will now deliver them to + // Alice's link. + for _, addPkt := range addPkts[:halfHtlcs] { + if err := alice.link.HandleSwitchPacket(addPkt); err != nil { + t.Fatalf("unable to handle switch packet: %v", err) + } + } + + // Wait until Alice's link has sent both HTLCs via the peer. + alice.checkSent(addPkts[:halfHtlcs]) + + // The resulting bandwidth should reflect that Alice is paying both + // htlc amounts, in addition to both htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) + + alice.assertNumPendingNumOpenCircuits(2, 0) + + // Now, init a state transition by Alice to try and commit the HTLCs. + // Since she is in hodl.Commit mode, this will fail, but the circuits + // will be opened persistently. + alice.trySignNextCommitment() + + alice.assertNumPendingNumOpenCircuits(2, 2) + + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(2, 2) + + // The first two HTLCs should have been reset in Alice's mailbox since + // the switch was not shutdown. Knowing this the switch should drop the + // two circuits, even if the circuits were trimmed. + fwdActions = alice.commitCircuits(circuits[:halfHtlcs]) + if len(fwdActions.Drops) != halfHtlcs { + t.Fatalf("expected %d packets to be dropped since "+ + "the switch has not been restarted", halfHtlcs) + } + + // Wait for alice to process the first two HTLCs resend them via the + // peer. + alice.checkSent(addPkts[:halfHtlcs]) + + // The resulting bandwidth should reflect that Alice is paying both htlc + // amounts, in addition to both htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) + // Again, initiate another state transition by Alice to try and commit + // the HTLCs. Since she is in hodl.Commit mode, this will fail, but the + // circuits will be opened persistently. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(2, 2) + + // Now, we we will do a full restart of the link and switch, configuring + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(2, 0) + + // Attempt another commit of our first two circuits. Both should fail, + // as the opened circuits should have been trimmed, and circuit map + // recognizes that these HTLCs were lost during the restart. + fwdActions = alice.commitCircuits(circuits[:halfHtlcs]) + if len(fwdActions.Fails) != halfHtlcs { + t.Fatalf("expected %d packets to be failed", halfHtlcs) + } + + // Bob should not receive any HTLCs from Alice, since Alice's mailbox is + // empty and there is no pending commitment. + select { + case <-alice.msgs: + t.Fatalf("received unexpected message from Alice") + case <-time.After(time.Second): + } + + // Alice's bandwidth should have reverted back to her starting value. + assertLinkBandwidth(t, alice.link, aliceStartingBandwidth) + + // Now, try to commit the last two payment circuits, which are unused + // thus far. These should succeed without hestiation. + fwdActions = alice.commitCircuits(circuits[halfHtlcs:]) + if len(fwdActions.Adds) != halfHtlcs { + t.Fatalf("expected %d packets to be added", halfHtlcs) + } + + // Deliver the last two HTLCs to the link via Alice's mailbox. + for _, addPkt := range addPkts[halfHtlcs:] { + if err := alice.link.HandleSwitchPacket(addPkt); err != nil { + t.Fatalf("unable to handle switch packet: %v", err) + } + } + + // Verify that Alice processed and sent out the ADD packets via the + // peer. + alice.checkSent(addPkts[halfHtlcs:]) + + // The resulting bandwidth should reflect that Alice is paying both htlc + // amounts, in addition to both htlc fees. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) + + // Now, initiate a state transition for Alice. Since we are hodl.Commit + // mode, this will only open the circuits that were added to the + // in-memory channel state. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(4, 2) + + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(4, 2) + + // Now, try to commit all of known circuits. + fwdActions = alice.commitCircuits(circuits) + + // The first two HTLCs will fail to commit for the same reason as + // before, the circuits have been trimmed. + if len(fwdActions.Fails) != halfHtlcs { + t.Fatalf("expected %d packet to be failed", halfHtlcs) + } + // The last two HTLCs will be dropped, as thought the circuits are + // trimmed, the switch is aware that the HTLCs are still in Alice's + // mailbox. + if len(fwdActions.Drops) != halfHtlcs { + t.Fatalf("expected %d packet to be dropped", halfHtlcs) + } + + // Wait until Alice reprocesses the last two HTLCs and sends them via + // the peer. + alice.checkSent(addPkts[halfHtlcs:]) + + // Her bandwidth should now reflect having sent only those two HTLCs. + assertLinkBandwidth(t, alice.link, + aliceStartingBandwidth-halfHtlcs*(htlcAmt+htlcFee), + ) + + // Now, initiate a state transition for Alice. Since we are hodl.Commit + // mode, this will only open the circuits that were added to the + // in-memory channel state. + alice.trySignNextCommitment() + alice.assertNumPendingNumOpenCircuits(4, 2) + + // 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) + defer cleanUp() + + alice.assertNumPendingNumOpenCircuits(4, 0) + + // Bob should not receive any HTLCs from Alice, as none of the HTLCs are + // in Alice's mailbox, and channel reestablishment is disabled. + select { + case <-alice.msgs: + t.Fatalf("received unexpected message from Alice") + case <-time.After(time.Second): + } + + // Attempt to commit the last two circuits, both should now fail since + // though they were opened before shutting down, the circuits have been + // properly trimmed. + fwdActions = alice.commitCircuits(circuits[halfHtlcs:]) + if len(fwdActions.Fails) != halfHtlcs { + t.Fatalf("expected %d packet to be failed", halfHtlcs) + } + + // Alice balance should not have changed since the start. + assertLinkBandwidth(t, alice.link, aliceStartingBandwidth) +} + // TestChannelLinkBandwidthChanReserve checks that the bandwidth available // on the channel link reflects the channel reserve that must be kept // at all times. @@ -2325,7 +2875,7 @@ func TestChannelLinkBandwidthChanReserve(t *testing.T) { // channel reserve. const chanAmt = btcutil.SatoshiPerBitcoin * 5 const chanReserve = btcutil.SatoshiPerBitcoin * 1 - aliceLink, bobChannel, batchTimer, cleanUp, err := + aliceLink, bobChannel, batchTimer, cleanUp, _, err := newSingleLinkTestHarness(chanAmt, chanReserve) if err != nil { t.Fatalf("unable to create link: %v", err) @@ -2440,8 +2990,8 @@ func TestChannelLinkBandwidthChanReserve(t *testing.T) { // should therefore be 0. const bobChanAmt = btcutil.SatoshiPerBitcoin * 1 const bobChanReserve = btcutil.SatoshiPerBitcoin * 1.5 - bobLink, _, _, bobCleanUp, err := newSingleLinkTestHarness(bobChanAmt, - bobChanReserve) + bobLink, _, _, bobCleanUp, _, err := + newSingleLinkTestHarness(bobChanAmt, bobChanReserve) if err != nil { t.Fatalf("unable to create link: %v", err) } @@ -3111,3 +3661,253 @@ func TestChannelLinkAcceptOverpay(t *testing.T) { expectedCarolBandwidth, n.carolChannelLink.Bandwidth()) } } + +// chanRestoreFunc is a method signature for functions that can reload both +// endpoints of a link from their persistent storage engines. +type chanRestoreFunc func() (*lnwallet.LightningChannel, *lnwallet.LightningChannel, error) + +// persistentLinkHarness is used to control the lifecylce of a link and the +// switch that operates it. It supports the ability to restart either the link +// or both the link and the switch. +type persistentLinkHarness struct { + t *testing.T + + link ChannelLink + coreLink *channelLink + channel *lnwallet.LightningChannel + + batchTicker chan time.Time + msgs chan lnwire.Message + + restoreChan chanRestoreFunc +} + +// newPersistentLinkHarness initializes a new persistentLinkHarness and derives +// the supporting references from the active link. +func newPersistentLinkHarness(t *testing.T, link ChannelLink, + batchTicker chan time.Time, + restore chanRestoreFunc) *persistentLinkHarness { + + coreLink := link.(*channelLink) + + return &persistentLinkHarness{ + t: t, + link: link, + coreLink: coreLink, + channel: coreLink.channel, + batchTicker: batchTicker, + msgs: coreLink.cfg.Peer.(*mockPeer).sentMsgs, + restoreChan: restore, + } +} + +// restart facilitates a shutdown and restart of the link maintained by the +// harness. The primary purpose of this method is to ensure the consistency of +// the supporting references is maintained across restarts. +// +// If `restartSwitch` is set, the entire switch will also be restarted, +// and will be reinitialized with the contents of the channeldb backing Alice's +// channel. +// +// 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, + hodlFlags ...hodl.Flag) func() { + + // First, remove the link from the switch. + h.coreLink.cfg.Switch.RemoveLink(h.link.ChanID()) + + var htlcSwitch *Switch + if restartSwitch { + // If a switch restart is requested, we will stop it and + // leave htlcSwitch nil, which will trigger the creation + // of a fresh instance in restartLink. + h.coreLink.cfg.Switch.Stop() + } else { + // Otherwise, we capture the switch's reference so that + // it can be carried over to the restarted link. + htlcSwitch = h.coreLink.cfg.Switch + } + + // Since our in-memory state may have diverged from our persistent + // state, we will restore the persisted state to ensure we always start + // the link in a consistent state. + var err error + h.channel, _, err = h.restoreChan() + if err != nil { + h.t.Fatalf("unable to restore channels: %v", err) + } + + // Now, restart the link using the channel state. This will take care of + // adding the link to an existing switch, or creating a new one using + // the database owned by the link. + var cleanUp func() + h.link, h.batchTicker, cleanUp, err = restartLink( + h.channel, htlcSwitch, hodlFlags, + ) + if err != nil { + h.t.Fatalf("unable to restart alicelink: %v", err) + } + + // Repopulate the remaining fields in the harness. + h.coreLink = h.link.(*channelLink) + h.msgs = h.coreLink.cfg.Peer.(*mockPeer).sentMsgs + + return cleanUp +} + +// checkSent reads the links message stream and verify that the messages are +// dequeued in the same order as provided by `pkts`. +func (h *persistentLinkHarness) checkSent(pkts []*htlcPacket) { + for _, pkt := range pkts { + var msg lnwire.Message + select { + case msg = <-h.msgs: + case <-time.After(15 * time.Second): + h.t.Fatalf("did not receive message") + } + + if !reflect.DeepEqual(msg, pkt.htlc) { + h.t.Fatalf("unexpected packet, want %v, got %v", + pkt.htlc, msg) + } + } +} + +// commitCircuits accepts a list of circuits and tries to commit them to the +// switch's circuit map. The forwarding actions are returned if there was no +// failure. +func (h *persistentLinkHarness) commitCircuits(circuits []*PaymentCircuit) *CircuitFwdActions { + fwdActions, err := h.coreLink.cfg.Switch.commitCircuits(circuits...) + if err != nil { + h.t.Fatalf("unable to commit circuit: %v", err) + } + + return fwdActions +} + +func (h *persistentLinkHarness) assertNumPendingNumOpenCircuits( + wantPending, wantOpen int) { + + _, _, line, _ := runtime.Caller(1) + + numPending := h.coreLink.cfg.Switch.circuits.NumPending() + if numPending != wantPending { + h.t.Fatalf("line: %d: wrong number of pending circuits: "+ + "want %d, got %d", line, wantPending, numPending) + } + numOpen := h.coreLink.cfg.Switch.circuits.NumOpen() + if numOpen != wantOpen { + h.t.Fatalf("line: %d: wrong number of open circuits: "+ + "want %d, got %d", line, wantOpen, numOpen) + } +} + +// trySignNextCommitment signals the batch ticker so that the link will try to +// update its commitment transaction. +func (h *persistentLinkHarness) trySignNextCommitment() { + select { + case h.batchTicker <- time.Now(): + // Give the link enough time to process the request. + time.Sleep(time.Millisecond * 500) + + case <-time.After(15 * time.Second): + h.t.Fatalf("did not initiate state transition") + } +} + +// restartLink creates a new channel link from the given channel state, and adds +// to an htlcswitch. If none is provided by the caller, a new one will be +// created using Alice's database. +func restartLink(aliceChannel *lnwallet.LightningChannel, aliceSwitch *Switch, + hodlFlags []hodl.Flag) (ChannelLink, chan time.Time, func(), error) { + + var ( + globalEpoch = &chainntnfs.BlockEpochEvent{ + Epochs: make(chan *chainntnfs.BlockEpoch), + Cancel: func() { + }, + } + invoiceRegistry = newMockRegistry() + decoder = newMockIteratorDecoder() + obfuscator = NewMockObfuscator() + alicePeer = &mockPeer{ + sentMsgs: make(chan lnwire.Message, 2000), + quit: make(chan struct{}), + } + + globalPolicy = ForwardingPolicy{ + MinHTLC: lnwire.NewMSatFromSatoshis(5), + BaseFee: lnwire.NewMSatFromSatoshis(1), + TimeLockDelta: 6, + } + + pCache = &mockPreimageCache{ + // hash -> preimage + preimageMap: make(map[[32]byte][]byte), + } + ) + + aliceDb := aliceChannel.State().Db + + if aliceSwitch == nil { + var err error + aliceSwitch, err = New(Config{DB: aliceDb}) + if err != nil { + return nil, nil, nil, err + } + } + + t := make(chan time.Time) + ticker := &mockTicker{t} + aliceCfg := ChannelLinkConfig{ + FwrdingPolicy: globalPolicy, + Peer: alicePeer, + Switch: aliceSwitch, + Circuits: aliceSwitch.CircuitModifier(), + ForwardPackets: aliceSwitch.ForwardPackets, + DecodeHopIterators: decoder.DecodeHopIterators, + ExtractErrorEncrypter: func(*btcec.PublicKey) ( + ErrorEncrypter, lnwire.FailCode) { + return obfuscator, lnwire.CodeNone + }, + FetchLastChannelUpdate: mockGetChanUpdateMessage, + PreimageCache: pCache, + UpdateContractSignals: func(*contractcourt.ContractSignals) error { + return nil + }, + Registry: invoiceRegistry, + ChainEvents: &contractcourt.ChainEventSubscription{}, + BlockEpochs: globalEpoch, + BatchTicker: ticker, + FwdPkgGCTicker: NewBatchTicker(time.NewTicker(5 * time.Second)), + // Make the BatchSize large enough to not + // trigger commit update automatically during tests. + BatchSize: 10000, + // Set any hodl flags requested for the new link. + HodlMask: hodl.MaskFromFlags(hodlFlags...), + DebugHTLC: len(hodlFlags) > 0, + } + + const startingHeight = 100 + aliceLink := NewChannelLink(aliceCfg, aliceChannel, startingHeight) + if err := aliceSwitch.AddLink(aliceLink); err != nil { + return nil, nil, nil, err + } + go func() { + for { + select { + case <-aliceLink.(*channelLink).htlcUpdates: + case <-aliceLink.(*channelLink).quit: + return + } + } + }() + + cleanUp := func() { + close(alicePeer.quit) + defer aliceLink.Stop() + } + + return aliceLink, t, cleanUp, nil +} From 1b6101b0c08d64cd1aa0c9aff04b78a921a9fc71 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 23 Apr 2018 13:20:39 -0700 Subject: [PATCH 15/18] channeldb/channel: add NextLocalHtlcIndex --- channeldb/channel.go | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/channeldb/channel.go b/channeldb/channel.go index b15943a2..9e13e5d3 100644 --- a/channeldb/channel.go +++ b/channeldb/channel.go @@ -1366,6 +1366,29 @@ func (c *OpenChannel) AdvanceCommitChainTail(fwdPkg *FwdPkg) error { return nil } +// NextLocalHtlcIndex returns the next unallocated local htlc index. To ensure +// this always returns the next index that has been not been allocated, this +// will first try to examine any pending commitments, before falling back to the +// last locked-in local commitment. +func (c *OpenChannel) NextLocalHtlcIndex() (uint64, error) { + // First, load the most recent commit diff that we initiated for the + // remote party. If no pending commit is found, this is not treated as + // a critical error, since we can always fall back. + pendingRemoteCommit, err := c.RemoteCommitChainTip() + if err != nil && err != ErrNoPendingCommit { + return 0, err + } + + // If a pending commit was found, its local htlc index will be at least + // as large as the one on our local commitment. + if pendingRemoteCommit != nil { + return pendingRemoteCommit.Commitment.LocalHtlcIndex, nil + } + + // Otherwise, fallback to using the local htlc index of our commitment. + return c.LocalCommitment.LocalHtlcIndex, nil +} + // LoadFwdPkgs scans the forwarding log for any packages that haven't been // processed, and returns their deserialized log updates in map indexed by the // remote commitment height at which the updates were locked in. From cae1d468e7179ddacbf6278a9d2cf6d7e8eb207f Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 23 Apr 2018 22:08:03 -0700 Subject: [PATCH 16/18] lnwallet/channel: expose NextLocalHtlcIndex --- lnwallet/channel.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index d5fd4bb8..34039db0 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -5885,12 +5885,15 @@ func (lc *LightningChannel) LocalChanReserve() btcutil.Amount { return lc.localChanCfg.ChanReserve } -// LocalHtlcIndex returns the next local htlc index to be allocated. -func (lc *LightningChannel) LocalHtlcIndex() uint64 { +// NextLocalHtlcIndex returns the next unallocated local htlc index. To ensure +// this always returns the next index that has been not been allocated, this +// will first try to examine any pending commitments, before falling back to the +// last locked-in local commitment. +func (lc *LightningChannel) NextLocalHtlcIndex() (uint64, error) { lc.RLock() defer lc.RUnlock() - return lc.channelState.LocalCommitment.LocalHtlcIndex + return lc.channelState.NextLocalHtlcIndex() } // RemoteCommitHeight returns the commitment height of the remote chain. From ed4f77871a42b626c95b4130b4cae6c70404eae9 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Mon, 23 Apr 2018 13:21:02 -0700 Subject: [PATCH 17/18] htlcswitch/circuit_map: trim using NextLocalHtlcIndex --- htlcswitch/circuit_map.go | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/htlcswitch/circuit_map.go b/htlcswitch/circuit_map.go index 844a9316..1fa0a042 100644 --- a/htlcswitch/circuit_map.go +++ b/htlcswitch/circuit_map.go @@ -343,8 +343,9 @@ func (cm *circuitMap) decodeCircuit(v []byte) (*PaymentCircuit, error) { } // trimAllOpenCircuits reads the set of active channels from disk and trims -// keystones for any non-pending channels. This method is intended to be called -// on startup. Each link will also trim it's own circuits upon startup. +// keystones for any non-pending channels using the next unallocated htlc index. +// This method is intended to be called on startup. Each link will also trim +// it's own circuits upon startup. // // NOTE: This operation will be applied to the persistent state of all active // channels. Therefore, it must be called before any links are created to avoid @@ -356,9 +357,31 @@ func (cm *circuitMap) trimAllOpenCircuits() error { } for _, activeChannel := range activeChannels { + if activeChannel.IsPending { + continue + } + + // First, skip any channels that have not been assigned their + // final channel identifier, otherwise we would try to trim + // htlcs belonging to the all-zero, sourceHop ID. chanID := activeChannel.ShortChanID - start := activeChannel.LocalCommitment.LocalHtlcIndex - if err := cm.TrimOpenCircuits(chanID, start); err != nil { + if chanID == sourceHop { + continue + } + + // Next, retrieve the next unallocated htlc index, which bounds + // the cutoff of confirmed htlc indexes. + start, err := activeChannel.NextLocalHtlcIndex() + if err != nil { + return err + } + + // Finally, remove all pending circuits above at or above the + // next unallocated local htlc indexes. This has the effect of + // reverting any circuits that have either not been locked in, + // or had not been included in a pending commitment. + err = cm.TrimOpenCircuits(chanID, start) + if err != nil { return err } } From 42a9a781802b5a08a053d17df63fc7830b19abdb Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Fri, 27 Apr 2018 02:50:53 -0700 Subject: [PATCH 18/18] htlcswitch/link: trim fix --- htlcswitch/link.go | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 33297ef3..2051561b 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -729,19 +729,31 @@ func (l *channelLink) htlcManager() { // Before handling any messages, revert any circuits that were marked // open in the switch's circuit map, but did not make it into a // commitment txn. We use the next local htlc index as the cut off - // point, since all indexes below that are committed. - // - // NOTE: This is automatically done by the switch when it starts up, - // but is necessary to prevent inconsistencies in the case that the - // link flaps. This is a result of a link's life-cycle being shorter - // than that of the switch. - localHtlcIndex := l.channel.LocalHtlcIndex() - err := l.cfg.Circuits.TrimOpenCircuits(l.ShortChanID(), localHtlcIndex) - if err != nil { - l.errorf("unable to trim circuits above local htlc index %d: %v", - localHtlcIndex, err) - l.fail(ErrInternalLinkFailure.Error()) - return + // point, since all indexes below that are committed. This action is + // only performed if the link's final short channel ID has been + // assigned, otherwise we would try to trim the htlcs belonging to the + // all-zero, sourceHop ID. + if l.ShortChanID() != sourceHop { + localHtlcIndex, err := l.channel.NextLocalHtlcIndex() + if err != nil { + l.errorf("unable to retrieve next local htlc index: %v", + err) + l.fail(ErrInternalLinkFailure.Error()) + return + } + + // NOTE: This is automatically done by the switch when it starts + // up, but is necessary to prevent inconsistencies in the case + // that the link flaps. This is a result of a link's life-cycle + // being shorter than that of the switch. + chanID := l.ShortChanID() + err = l.cfg.Circuits.TrimOpenCircuits(chanID, localHtlcIndex) + if err != nil { + l.errorf("unable to trim circuits above local htlc "+ + "index %d: %v", localHtlcIndex, err) + l.fail(ErrInternalLinkFailure.Error()) + return + } } // TODO(roasbeef): need to call wipe chan whenever D/C?