htlcswitch: randomize link fee updates #1229

wpaulino · 2018-05-10T22:00:25Z

In this PR, we modify the behavior of links updating their commitment fees. Rather than attempting to update the commitment fee for each link every time a new block comes in, we'll use a timer with a random interval between 10 and 60 minutes for each link to determine when to update their corresponding commitment fee. This prevents us from oscillating the fee rate for our various commitment transactions.

We also move the block height dependency from the links in the switch to the switch itself. This is possible due to a recent change on the links no longer depending on the block height to update their commitment fees. We'll now only have the switch be alerted of new blocks coming in and
links will retrieve the height from it atomically.

Fixes #1129.

Roasbeef · 2018-05-17T01:27:16Z

htlcswitch/link.go

+	// UpdateFeeInterval is the interval at which the update fee timer will
+	// fire at in order to determine if the link should update its
+	// commitment fee.
+	UpdateFeeInterval func() time.Duration


Why does this need to be a method? It can simply be an attribute.

Actually, alternatively, it can be a min/max. Don't have a strong opinion in either direction.

Roasbeef · 2018-05-17T01:28:46Z

htlcswitch/link_test.go

-	}
+	// For the sake of this test, we'll reset the timer to fire in a second
+	// so that Alice's link queries for a new network fee.
+	n.aliceChannelLink.updateFeeTimer.Reset(time.Second)


Why not lower it even further to instantaneously trigger a fee update

Roasbeef · 2018-05-17T01:29:54Z

peer.go

@@ -438,6 +441,9 @@ func (p *peer) loadActiveChannels(chans []*channeldb.OpenChannel) error {
 				time.NewTicker(time.Minute)),
 			BatchSize:    10,
 			UnsafeReplay: cfg.UnsafeReplay,
+			UpdateFeeInterval: func() time.Duration {
+				return time.Duration(prand.Intn(60)+11) * time.Minute


Magic numbers should be lifted up into constants.

Roasbeef · 2018-05-17T01:30:14Z

peer.go

@@ -1416,6 +1422,9 @@ out:
 					time.NewTicker(time.Minute)),
 				BatchSize:    10,
 				UnsafeReplay: cfg.UnsafeReplay,
+				UpdateFeeInterval: func() time.Duration {
+					return time.Duration(prand.Intn(60)+11) * time.Minute


Same here w.r.t magic numbers.

Roasbeef · 2018-05-17T01:32:14Z

htlcswitch/link.go

@@ -2012,7 +1996,7 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
 			continue
 		}

-		heightNow := l.bestHeight
+		heightNow := atomic.LoadUint32(&l.cfg.Switch.bestHeight)


Rather than accessing a private member, the links should access a public method of the switch. This way, the details (the atomic load), don't need to be leaked to the lower layers.

Maybe make retrieve it using a new method BestHeight() set in the channel link config.

Fixed to use the a BestHeight method, although I'm not sure we need to set it in the config since we already have a reference to the switch itself in there.

Roasbeef · 2018-05-17T01:33:59Z

htlcswitch/switch.go

+
+	// Notifier is an instance of a chain notifier that we'll use to signal
+	// the switch when a new block has arrived.
+	Notifier chainntnfs.ChainNotifier


Why pass the full notifier, when we can pass an already instantiated BlockEpoch struct?

By the time the switch is declared in server.go, the ChainNotifier instance hasn't been initialized yet.

Roasbeef · 2018-05-17T01:34:22Z

htlcswitch/switch.go

+
+	// bestHeight is the best known height of the main chain. The links will
+	// use this information to govern decisions based on HTLC timeouts.
+	bestHeight uint32


Comment should note that this must be used atomically.

Also this should be moved to the top of the struct in order to not run into alignment issues with 32-bit systems.

Roasbeef · 2018-05-17T01:35:28Z

htlcswitch/switch.go

@@ -1712,6 +1744,8 @@ func (s *Switch) Stop() error {
 	// accessed and modified.
 	s.mailOrchestrator.Stop()

+	s.blockEpochs.Cancel()


This should be placed in the main goroutine as a defer instead. As otherwise, it's possible for the switch to exit, but the Stop method to not have been called.

Roasbeef · 2018-05-17T01:36:58Z

htlcswitch/switch.go

+				break out
+			}
+
+			atomic.StoreUint32(&s.bestHeight, uint32(blockEpoch.Height))


Carrying over a comment here to recommend a method is added so the link don't need to directly access this attribute.

halseth · 2018-05-30T10:59:20Z

htlcswitch/link.go

@@ -438,6 +449,7 @@ func (l *channelLink) Stop() {
 		l.cfg.ChainEvents.Cancel()
 	}

+	l.updateFeeTimer.Stop()


can this just be started and stopped within the htlcManager?

We can't due to needing to reset the timer withinTestChannelLinkUpdateCommitFee. Otherwise, we'll run into a race where we might update the timer without it being initialized first.

halseth · 2018-05-30T11:02:07Z

htlcswitch/link_test.go

+		// Make the BatchSize and UpdateFeeInterval large enough to not
+		// trigger commit updates automatically during tests.
+		BatchSize:         10000,
+		UpdateFeeInterval: func() time.Duration { return time.Hour },


Can you use the BatchTicker pattern to control the ticks instead?

The BatchTicker seems to be of better use when wanting continuous ticks within the same interval, rather than a different interval after every tick.

halseth · 2018-05-30T11:06:45Z

htlcswitch/link.go

@@ -2012,7 +1996,7 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
 			continue
 		}

-		heightNow := l.bestHeight
+		heightNow := atomic.LoadUint32(&l.cfg.Switch.bestHeight)


Maybe make retrieve it using a new method BestHeight() set in the channel link config.

halseth · 2018-06-01T12:07:39Z

htlcswitch/link.go

@@ -2074,7 +2064,7 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
 			continue
 		}

-		heightNow := l.bestHeight
+		heightNow := l.cfg.Switch.BestHeight()


the l.bestHeight can be removed after this change.

halseth · 2018-06-01T12:09:24Z

htlcswitch/switch.go

 	for {
 		select {
+		case blockEpoch, ok := <-s.blockEpochStream.Epochs:


Is the reason we are not usingGetBestBlock() performance?

I believe so. In the Neutrino case, we end up going to disk. This also notifies us as soon as we know about the new block, rather than having to do some kind of long-polling with GetBestBlock.

Roasbeef · 2018-06-14T00:11:00Z

htlcswitch/link.go

+func (l *channelLink) randomFeeUpdateTimeout() time.Duration {
+	lower := int64(l.cfg.MinFeeUpdateTimeout)
+	upper := int64(l.cfg.MaxFeeUpdateTimeout)
+	rand := prand.Int63n(upper) + lower + 1


This can cause is to accept (in the worst case), a value of lower+upper+1, rather than [lower, upper). Instead we can do something like the following:

lower := int64(l.cfg.MinFeeUpdateTimeout) upper := int64(l.cfg.MaxFeeUpdateTimeout) rand := prand.Int63n(upper) if rand < lower { rand = lower }

In this commit, we modify the behavior of links updating their commitment fees. Rather than attempting to update the commitment fee for each link every time a new block comes in, we'll use a timer with a random interval between 10 and 60 minutes for each link to determine when to update their corresponding commitment fee. This prevents us from oscillating the fee rate for our various commitment transactions.

In this commit, we move the block height dependency from the links in the switch to the switch itself. This is possible due to a recent change on the links no longer depending on the block height to update their commitment fees. We'll now only have the switch be alerted of new blocks coming in and links will retrieve the height from it atomically.

wpaulino added this to the 0.5 milestone May 10, 2018

Roasbeef requested changes May 17, 2018

View reviewed changes

halseth suggested changes May 30, 2018

View reviewed changes

wpaulino force-pushed the randomize-link-fee-updates branch from 53e7824 to c9a84ae Compare June 1, 2018 04:28

halseth reviewed Jun 1, 2018

View reviewed changes

wpaulino force-pushed the randomize-link-fee-updates branch from c9a84ae to 66b2d80 Compare June 5, 2018 01:35

wpaulino force-pushed the randomize-link-fee-updates branch from 66b2d80 to 58b358c Compare June 13, 2018 19:02

Roasbeef requested changes Jun 14, 2018

View reviewed changes

wpaulino added 2 commits June 13, 2018 17:41

wpaulino force-pushed the randomize-link-fee-updates branch from 58b358c to 8198466 Compare June 14, 2018 00:42

Roasbeef merged commit 39f2739 into lightningnetwork:master Jun 14, 2018

wpaulino deleted the randomize-link-fee-updates branch June 14, 2018 02:12

cfromknecht mentioned this pull request Jun 30, 2018

htlcswitch/link: correct bias in fee update backoff #1482

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

htlcswitch: randomize link fee updates #1229

htlcswitch: randomize link fee updates #1229

wpaulino commented May 10, 2018

Roasbeef May 17, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

halseth May 30, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

Roasbeef May 17, 2018

wpaulino Jun 1, 2018

halseth May 30, 2018

wpaulino Jun 1, 2018

halseth May 30, 2018

wpaulino Jun 1, 2018

halseth May 30, 2018

halseth Jun 1, 2018

wpaulino Jun 5, 2018

halseth Jun 1, 2018

wpaulino Jun 5, 2018

Roasbeef Jun 14, 2018

wpaulino Jun 14, 2018

htlcswitch: randomize link fee updates #1229

htlcswitch: randomize link fee updates #1229

Conversation

wpaulino commented May 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment