Add fee negotiation on channel cooperative shutdown. #225

halseth · 2017-06-22T14:23:38Z

This PR adds a simple fee negotiation flow on channel shutdown. Currently, the algorithm works as follows:

The node picks a desired fee and send it to the remote.
When the node receives a fee proposal from the remote, it accepts it if it is in the range [0.5*desired, 2*desired].
If the remote's fee is not in this range, the node will propose the average of its own last sent fee and the remote's, restricted to this range.

Both nodes in the channel will broadcast the closing transaction the moment they have the signatures for a transaction with the agreed fee.

TODO:

~~If we cannot agree on a fee, we should fail the channel.~~

bryanvu

Really nice work on this @halseth! I've added a few comments, questions and a bunch of formatting nitpicks. Let me know if you have any questions!

bryanvu · 2017-06-30T23:30:03Z

lnwallet/channel.go

@@ -2614,22 +2614,21 @@ func (lc *LightningChannel) ForceClose() (*ForceCloseSummary, error) {
 //
 // TODO(roasbeef): caller should initiate signal to reject all incoming HTLCs,
 // settle any inflight.
-func (lc *LightningChannel) CreateCloseProposal(feeRate uint64) ([]byte, uint64,
+func (lc *LightningChannel) CreateCloseProposal(proposedFee uint64) ([]byte, uint64,


This is something we should check with @Roasbeef about. At one point, I had CreateCloseProposal take the actual proposedFee rather than the feeRate, and @Roasbeef recommended that I use the feeRate. I can't remember the reasoning at the moment, though.

The reason I did this was because the closing_signed message contains a proposed fee (not fee-per-kw), and I would have to convert that back to a feeRate before creating a close proposal in response. This would lead to a slightly different fee in some cases due to rounding. I am happy to make it use feeRate if this can be resolved.

If the computation is performed correctly, both side should always arrive at the same fee rate.

Let 666 be an example fee rate. The old CreateCloseProposal would calculate a proposed fee of

666 * 724 / 1000 = 482,184 => 482 as uint64

If the receiver of this proposal agreed on this fee, he would need to create his own close proposal with the same fee rate, calculated as

482 * 1000 / 724 = 665,7458563536 => 665 as uint64

which leads to incompatible proposed fees.

We could make this work by using a more granular feeRate, but I found it easier to just use the fee directly. Any better way to do this, and what are the advantages of using the fee rate?

Upon reflection, the absolute fee value is the proper way to go here.

bryanvu · 2017-06-30T23:30:59Z

lnwallet/channel.go

 	// closed.
-	channelClosing
+	ChannelClosing


It seems a little strange to me that some of the possible states would be visible outside the package, but that others wouldn't. Is there a way to make them all one way or the other?

Yeah, this was me trying to do make as few visible as possible, but I agree that if one one is visible then they should all be. These are just constants in any case, so shouldn't hurt to be visible.

Why do these states need to be visible outside the package at all? FWIW, most of them aren't even being used within the package. Still getting through the PR, so there may be a reason that becomes apparent for me later on.

I initially had to make some of these public to move the "is closing" check to peer.go (see below comment). I deleted this check by instead using the state in channelManager to determine of a shutdown is already in progress, but the peer tests are still using these states to check that the channel is ChannelClosing/ChannelClosed when we expect it. Should these checks not be done by the peer tests (since the states really are internal to channel.go), or should I use another way of querying the state of the channel from the peer tests?

bryanvu · 2017-06-30T23:32:38Z

lnwallet/channel.go

+}
+
+// GetChannelStatus returns the channelState of this channel.
+func (lc *LightningChannel) GetChannelStatus() channelState {


I believe the use of "Get" on these methods is more Java-ish than Go-ish. Could this just be ChannelStatus()?

Yeah, sounds better :)

bryanvu · 2017-06-30T23:33:48Z

lnwallet/channel.go

@@ -2851,3 +2849,13 @@ func CreateCooperativeCloseTx(fundingTxIn *wire.TxIn,

 	return closeTx
 }
+
+// GetFee returns the commit fee to use for the given fee rate.
+func (lc *LightningChannel) GetFee(feeRate uint64) uint64 {


Related to the comment below about using "Get", maybe could call this "CalcFee"?

bryanvu · 2017-06-30T23:36:02Z

peer.go

-
-				delete(chanShutdowns, req.ChannelID)
+				initiatorSig := append(initiatorShutdownSigs[req.ChannelID],
+					byte(txscript.SigHashAll))


Due to all the nesting, there are a lot of lines here that are quite a bit longer than 80 columns. Would it make sense to refactor this to reduce all the long lines?

This is surprising. I have my editor (Atom) set to a line length of 80, and it looks good to me. How do you determine the length?

Ok, I think what causes the issue is that my current tab length is set to 2. Should we use 4, or something else?

You should be using gofmt, as it's used uniformly within the codebase.

As discussed, my editor was showing a 2 space tab length, which is not checked by gofmt (a tab is just a tab). I switched to using a 8 space tab length, so from now it should be showing the line lengths correctly, and I reduced the long lines.

bryanvu · 2017-06-30T23:52:08Z

peer_test.go

+
+	// Wait for closing tx to be broadcasted.
+	<-publTx
+}


Another test that could be added (may be out of scope for this PR) would be a negotiation failure test, where in the event that the two sides don't converge on a fee, the initiator force closes.

bryanvu · 2017-06-30T23:53:53Z

peer.go

+
+		// Our new proposed fee must be strictly between what we proposed before
+		// and what the peer proposed.
+		isStrictlyBetween := false


Maybe rename this to something like "isAcceptable"?

bryanvu · 2017-06-30T23:57:08Z

peer.go

+		}
+
+		if !isStrictlyBetween {
+			// TODO(halseth): fail channel


By "fail channel", do you mean initiate force close?

Also, I think we can actually take out the isStrictlyBetween variable altogether? Couldn't we just check the fee and fail the channel if the fee isn't in the acceptable range?

BOLT#2 only says "one side fails the channel", but I guess that means a force close.

We could definitely take out the variable, but couldn't find a very readable way:

if !((fee < peerFeeProposal && fee > ourFeeProp) || (fee < ourFeeProp && fee > peerFeeProposal)) { // fail channel }

Suggestions?

bryanvu · 2017-07-01T00:00:03Z

peer.go

+		// Our new proposed fee must be strictly between what we proposed before
+		// and what the peer proposed.
+		isStrictlyBetween := false
+		if fee < peerFeeProposal && fee > ourFeeProp {


Not sure if it's more or less readable, but couldn't these two conditionals be combined with an ||?

bryanvu · 2017-07-01T00:03:56Z

peer_test.go

+		return nil, nil, nil, nil, err
+	}
+	bobCommitTx, err := lnwallet.CreateCommitTx(fundingTxIn, bobKeyPub,
+		aliceKeyPub, bobRevokeKey, csvTimeoutBob, channelBal, channelBal, bobDustLimit)


Line is a bit long.

Roasbeef · 2017-07-04T03:30:06Z

lnwallet/channel.go

 	// closed.
-	channelClosing
+	ChannelClosing


Why do these states need to be visible outside the package at all? FWIW, most of them aren't even being used within the package. Still getting through the PR, so there may be a reason that becomes apparent for me later on.

Roasbeef · 2017-07-04T03:32:04Z

lnwallet/channel.go

@@ -2614,22 +2614,21 @@ func (lc *LightningChannel) ForceClose() (*ForceCloseSummary, error) {
 //
 // TODO(roasbeef): caller should initiate signal to reject all incoming HTLCs,
 // settle any inflight.
-func (lc *LightningChannel) CreateCloseProposal(feeRate uint64) ([]byte, uint64,
+func (lc *LightningChannel) CreateCloseProposal(proposedFee uint64) ([]byte, uint64,


If the computation is performed correctly, both side should always arrive at the same fee rate.

Roasbeef · 2017-07-04T03:32:23Z

lnwallet/channel.go

-	// If we're already closing the channel, then ignore this request.
-	if lc.status == channelClosing || lc.status == channelClosed {
+	// If we've already closed the channel, then ignore this request.
+	if lc.status == ChannelClosed {


Why was the second clause of the conditional removed?

This was done because in a negotiation situation, we potentially want to create several close proposals. Before CreateCloseProposal could only be called once, and would fail if called again later.

I moved the lc.status == channelClosed check to to peer.go such that this would be checked only the first time it was called (following a shutdown request), but as discussed elsewhere this state should probably be internal to channel.go. Maybe we should make CreateCloseProposal non-mutating? (since it's not obvious from the name that it actually changes the channelState)

Gotcha, make sense.

Roasbeef · 2017-07-04T03:39:26Z

lnwallet/channel.go

+
+// CalcFee returns the commit fee to use for the given fee rate.
+func (lc *LightningChannel) CalcFee(feeRate uint64) uint64 {
+	return uint64(btcutil.Amount(feeRate) * commitWeight / 1000)


Does this even compile? Where is the commitWeight local being pulled in from?

See above comment

Roasbeef · 2017-07-04T03:39:38Z

lnwallet/channel.go

@@ -2851,3 +2849,16 @@ func CreateCooperativeCloseTx(fundingTxIn *wire.TxIn,

 	return closeTx
 }
+
+// CalcFee returns the commit fee to use for the given fee rate.


commit fee -> commitment fee

Also it seems that this doesn't need to be a method of LightningChannel at all.

On the naming of the function: it isn't clear from the function itself what unit is being passed in.

commitWeight is a constant declared in channel.go (commitWeight = btcutil.Amount(724)). This is why CalcFee is a method on LightningChannel.

Changed the unit to btcutil.Amount

Roasbeef · 2017-07-04T04:04:02Z

peer_test.go

+	broadcastTxChan := make(chan *wire.MsgTx)
+
+	responder, responderChannel, initiatorChannel, cleanUp, err :=
+		createTestPeer(notifier, broadcastTxChan)


Odd line breakage here. The function should reside on the same line as the assignment operator.

responder, responderChannel, initiatorChannel, cleanUp, err := createTestPeer(notifier, broadcastTxChan)

does not work out of the box. Do you prefer something like

responder, responderChannel, initiatorChannel, cleanUp, err := createTestPeer(notifier, broadcastTxChan)

or another way of breaking it up?

How about

responder, responderChannel, initiatorChannel, cleanUp, err := createTestPeer( notifier, broadcastTxChan, )

?

There're three instances above here that would need to be modified as well.

Roasbeef · 2017-07-04T04:04:28Z

peer_test.go

+	"github.com/roasbeef/btcd/wire"
+)
+
+// Test the shutdown responder's behavior if we can agree on the fee


Should be prefixed with TestChannelClosureAcceptFeeResponder.

Roasbeef · 2017-07-04T04:04:44Z

peer_test.go

+
+// Test the shutdown initiator's behavior if we can agree on the fee
+// immediately.
+func TestChannelClosureAcceptFeeInitiator(t *testing.T) {


Should be prefixed with TestChannelClosureAcceptFeeInitiator.

Roasbeef · 2017-07-04T04:04:57Z

peer_test.go

+
+// Test the shutdown responder's behavior in the case where we must do several
+// rounds of fee negotiation before we agree on a fee.
+func TestChannelClosureFeeNegotiationsResponder(t *testing.T) {


Should be prefixed with TestChannelClosureFeeNegotiationsResponder.

Roasbeef · 2017-07-04T04:05:09Z

peer_test.go

+
+// Test the shutdown initiator's behavior in the case where we must do several
+// rounds of fee negotiation before we agree on a fee.
+func TestChannelClosureFeeNegotiationsInitiator(t *testing.T) {


Should be prefixed with TestChannelClosureFeeNegotiationsInitiator.

halseth

Another iteration pushed. Mostly fixes related to the long lines and style issues.

For the issues regarding visibility of channelState, feeRate vs fee, the return error of PublishTransaction and a few other, I left comments and questions for you. PTAL.

halseth · 2017-07-10T11:38:42Z

peer.go

-
-				delete(chanShutdowns, req.ChannelID)
+				initiatorSig := append(initiatorShutdownSigs[req.ChannelID],
+					byte(txscript.SigHashAll))


As discussed, my editor was showing a 2 space tab length, which is not checked by gofmt (a tab is just a tab). I switched to using a 8 space tab length, so from now it should be showing the line lengths correctly, and I reduced the long lines.

halseth · 2017-07-10T11:40:47Z

peer.go

+				// To finalize this shtudown, we'll now send a matching close signed
+				// message to the other party, and broadcast the closing transaction
+				// to the network. If the fees are still being negotiated,
+				// handleInitClosingSigned returns the signature and proposed fee we


halseth · 2017-07-10T12:12:20Z

peer.go

+		return nil, 0
+	}
+
+	// If we're already closing the channel, then ignore this request.


Good idea! I did this by checking if we already had a signature created for the shutdown response.

halseth · 2017-07-10T12:17:51Z

lnwallet/channel.go

 	// closed.
-	channelClosing
+	ChannelClosing


I initially had to make some of these public to move the "is closing" check to peer.go (see below comment). I deleted this check by instead using the state in channelManager to determine of a shutdown is already in progress, but the peer tests are still using these states to check that the channel is ChannelClosing/ChannelClosed when we expect it. Should these checks not be done by the peer tests (since the states really are internal to channel.go), or should I use another way of querying the state of the channel from the peer tests?

halseth · 2017-07-10T12:22:30Z

lnwallet/channel.go

@@ -2614,22 +2614,21 @@ func (lc *LightningChannel) ForceClose() (*ForceCloseSummary, error) {
 //
 // TODO(roasbeef): caller should initiate signal to reject all incoming HTLCs,
 // settle any inflight.
-func (lc *LightningChannel) CreateCloseProposal(feeRate uint64) ([]byte, uint64,
+func (lc *LightningChannel) CreateCloseProposal(proposedFee uint64) ([]byte, uint64,


Let 666 be an example fee rate. The old CreateCloseProposal would calculate a proposed fee of

666 * 724 / 1000 = 482,184 => 482 as uint64

If the receiver of this proposal agreed on this fee, he would need to create his own close proposal with the same fee rate, calculated as

482 * 1000 / 724 = 665,7458563536 => 665 as uint64

which leads to incompatible proposed fees.

We could make this work by using a more granular feeRate, but I found it easier to just use the fee directly. Any better way to do this, and what are the advantages of using the fee rate?

halseth · 2017-07-10T13:39:09Z

peer_test.go

+
+// Test the shutdown initiator's behavior if we can agree on the fee
+// immediately.
+func TestChannelClosureAcceptFeeInitiator(t *testing.T) {


halseth · 2017-07-10T13:39:13Z

peer_test.go

+
+// Test the shutdown responder's behavior in the case where we must do several
+// rounds of fee negotiation before we agree on a fee.
+func TestChannelClosureFeeNegotiationsResponder(t *testing.T) {


halseth · 2017-07-10T13:39:26Z

peer_test.go

+
+	chanID := lnwire.NewChanIDFromOutPoint(responderChannel.ChannelPoint())
+
+	// We send a shutdown request to Alice. She will now be the responding node


halseth · 2017-07-10T13:39:33Z

peer_test.go

+	avgFee := (preferredRespFee + increasedFee) / 2
+	peerFee := responderClosingSigned.FeeSatoshis
+	if peerFee != avgFee {
+		t.Fatalf("expected ClosingSigned with fee %v, got %v", proposedFee,


halseth · 2017-07-10T13:39:39Z

peer_test.go

+
+// Test the shutdown initiator's behavior in the case where we must do several
+// rounds of fee negotiation before we agree on a fee.
+func TestChannelClosureFeeNegotiationsInitiator(t *testing.T) {


Roasbeef · 2017-07-12T01:37:00Z

mock.go

@@ -0,0 +1,154 @@
+package main
+
+import (


Worth noting that this will need to be reconciled with the similar mock code added in the outstanding funding persistence PR.

Will do when funding persistance PR is merged, I think that one is closest.

Roasbeef

Nice work on the test coverage for this PR! I have some minor comments (main one is to fix a possible panic), but beyond that the PR is nearly complete.

After the comment are addressed, and the PR is rebased, I'll take a final look before merging. Also, when rebasing+squashing the review commits in, it would be appreciated if the commits were split up into a granular set of commits. Ideally one per sub-system, and if within a sub-system there're several changes, the should be broken up if possible.

Roasbeef · 2017-07-12T01:45:14Z

peer.go

+				// If such a signature exists, it means we
+				// already have sent a response to a shutdown
+				// message for this channel, so ignore this one.
+				_, exists := responderShutdownSigs[req.ChannelID]


Why is this explicitly needed? In order to handle re-transmitted shutdown messages over the same connection?

This is what replaces the if channel.ChannelStatus() == lnwallet.ChannelClosing check I removed from CreateCloseProposal (and initially moved to handleShutdownResponse). Similar to that check, this will reject any attempt to start the channel shutdown procedure twice.

I don't really know if it is needed, I just tried to keep the original behavior. Should I remove it?

Roasbeef · 2017-07-12T01:48:55Z

peer.go

+		// accept.
+		return maxFee
+	default:
+		// Cannot accept the average, as we consider it too low.


Can you put a TODO here to remind us to ensure that the fee proposed doesn't go over the current fee on the commitment transaction? Thanks!

Added a TODO, but this check should be fairly straightforward to just add in this PR I think. Where do I fetch the current fee? Create a getter for channel.channelState.CommitFee?

Roasbeef · 2017-07-12T01:54:46Z

peer.go

 	}

 	// Finally, launch a goroutine which will request to be notified by the
 	// ChainNotifier once the closure transaction obtains a single
 	// confirmation.
 	notifier := p.server.cc.chainNotifier
-	go waitForChanToClose(uint32(bestHeight), notifier, req.Err,
-		req.ChanPoint, &closingTxid, func() {
+	var errChan chan error


This will cause a panic if an error occurs and needs to be sent across this channel. In the case that the local request isn't nil, then this channel should be initialized with a buffer of one.

waitForChanToClose actually handles a nil error channel, and is currently called with the error chan set to nil in master. I see your point though, maybe we should rather start a goroutine to read the err chan in case of localReq == nil and print it?

Roasbeef · 2017-07-12T01:58:24Z

lnwallet/channel.go

@@ -2851,3 +2849,16 @@ func CreateCooperativeCloseTx(fundingTxIn *wire.TxIn,

 	return closeTx
 }
+
+// CalcFee returns the commitment fee to use for the given fee rate.
+func (lc *LightningChannel) CalcFee(feeRate btcutil.Amount) uint64 {


Can you add a comment here noting the units of the expected fee rate (fee-per-kw)? Thanks!

Roasbeef · 2017-07-12T02:11:20Z

peer_test.go

+	broadcastTxChan := make(chan *wire.MsgTx)
+
+	responder, responderChannel, initiatorChannel, cleanUp, err :=
+		createTestPeer(notifier, broadcastTxChan)


How about

responder, responderChannel, initiatorChannel, cleanUp, err := createTestPeer( notifier, broadcastTxChan, )

?

There're three instances above here that would need to be modified as well.

Roasbeef · 2017-07-12T02:17:50Z

test_utils.go

+		Port: 18555,
+	}
+
+	aliceChannelState.SyncPending(addr, 0)


The error is ignored here.

Roasbeef · 2017-07-12T02:17:59Z

test_utils.go

+		Port: 18556,
+	}
+
+	bobChannelState.SyncPending(addr, 0)


Error is also ignored here.

coveralls · 2017-07-13T12:34:37Z

Coverage decreased (-0.01%) to 74.089% when pulling ec6f48c on halseth:channel-close-negotiation2 into c47047c on lightningnetwork:master.

coveralls · 2017-07-13T12:34:37Z

Coverage decreased (-0.01%) to 74.089% when pulling ec6f48c on halseth:channel-close-negotiation2 into c47047c on lightningnetwork:master.

Roasbeef · 2017-07-14T18:23:56Z

lnwallet/channel.go

 	// detected within the channel.
 	channelDispute

-	// channelPendingPayment indicates that there a currently outstanding
+	// ChannelPendingPayment indicates that there a currently outstanding


The godoc comments were changed, yet the variables themselves are the same. I'm guessing this was left over before reverting a prior change to make all of these public.

Yep, fixed.

coveralls · 2017-07-14T19:19:40Z

Coverage increased (+0.07%) to 74.104% when pulling c38af18 on halseth:channel-close-negotiation2 into 9c16319 on lightningnetwork:master.

coveralls · 2017-07-17T19:22:23Z

Coverage decreased (-0.05%) to 73.824% when pulling 4e1686a on halseth:channel-close-negotiation2 into 78f6caf on lightningnetwork:master.

This commit adds the fee negotiation procedure performed on channel shutdown. The current algorithm picks an ideal a fee based on the FeeEstimator and commit weigth, then accepts the remote's fee if it is at most 50%-200% away from the ideal. The fee negotiation procedure is similar both as sender and receiver of the initial shutdown message, and this commit also make both sides use the same code path for handling these messages.

Roasbeef

LGTM 🏄🏾

Handed off review

bryanvu previously requested changes Jul 1, 2017

View reviewed changes

Roasbeef requested changes Jul 4, 2017

View reviewed changes

halseth commented Jul 10, 2017

View reviewed changes

Roasbeef reviewed Jul 12, 2017

View reviewed changes

Roasbeef requested changes Jul 12, 2017

View reviewed changes

halseth force-pushed the channel-close-negotiation2 branch from 5cdf04c to ec6f48c Compare July 13, 2017 12:26

Roasbeef reviewed Jul 14, 2017

View reviewed changes

halseth force-pushed the channel-close-negotiation2 branch 2 times, most recently from c38af18 to c32eb75 Compare July 14, 2017 19:19

halseth force-pushed the channel-close-negotiation2 branch from c32eb75 to 4e1686a Compare July 15, 2017 08:26

halseth force-pushed the channel-close-negotiation2 branch 6 times, most recently from 60bb5c3 to b8a30bd Compare August 8, 2017 06:22

halseth force-pushed the channel-close-negotiation2 branch 3 times, most recently from 0ef5c5e to 40fd123 Compare August 11, 2017 09:34

halseth added 2 commits August 11, 2017 12:09

lnwallet: make CreateCloseProposal take absolute fee instead of fee rate

b9516b7

halseth force-pushed the channel-close-negotiation2 branch from 40fd123 to e8e8732 Compare August 11, 2017 10:11

Roasbeef approved these changes Aug 15, 2017

View reviewed changes

Roasbeef merged commit 06782eb into lightningnetwork:master Aug 15, 2017

halseth deleted the channel-close-negotiation2 branch July 10, 2018 10:33


		chanID := lnwire.NewChanIDFromOutPoint(responderChannel.ChannelPoint())

		// We send a shutdown request to Alice. She will now be the responding node

Add fee negotiation on channel cooperative shutdown. #225

Add fee negotiation on channel cooperative shutdown. #225

Conversation

halseth commented Jun 22, 2017 • edited

bryanvu left a 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

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

halseth left a 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

Roasbeef left a 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

halseth Jul 13, 2017 • edited

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

coveralls commented Jul 13, 2017

halseth commented Jun 22, 2017 •

edited

halseth Jul 13, 2017 •

edited