lnwire: adjusted coop close messages to comply with spec #159

bryanvu · 2017-03-08T23:33:24Z

Removed close_request and close_complete and replaced with shutdown and
closing_signed. Addresses the initial part of issue #49.

Roasbeef

This partially addresses the issue (the wire message portion), but both the channel closing workflow, and also the channel closure methods of the channel state machine should also be updated.

For now, we can at least send the Shutdown messages, but just immediately respond with a CloseSigned message upon receipt of the Shutdown message.

The CompleteCooperativeClose method should be modified to instead just verify the signature received, rather then generate a new signature.

Roasbeef · 2017-03-09T01:46:00Z

lnwire/closing_signed.go

+	"github.com/roasbeef/btcd/wire"
+)
+
+// ClosingSigned is sent by both parties to a channel once the channel is


both parties within a channel
or
both parties comprising of the channel
?

Roasbeef · 2017-03-09T01:47:14Z

lnwire/shutdown.go

+// the order of the inputs/outputs.
+type Shutdown struct {
+	// ChannelID serves to identify which channel is to be closed.
+	ChannelID ChannelID


This should also instead refer to the channel by it's outpoint.

Roasbeef · 2017-03-09T01:49:55Z

lnwire/shutdown.go

+	// ChannelID serves to identify which channel is to be closed.
+	ChannelID ChannelID
+
+	// Len describes the length of the key that will be used to receive


This is incorrect, the field of the shutdown struct (other than the target channel) should be either a btcutil.Address or a plain []byte. Personally, I lean more towards using a btcutil.Address within the field as it allow us to a number of related utilities.

coveralls · 2017-03-31T20:44:52Z

Coverage decreased (-0.3%) to 68.635% when pulling e3daceface1f4279d3aebe5aafdf74e2d3cadaa9 on bryanvu:channelclose into d2b4f14 on lightningnetwork:master.

bryanvu · 2017-03-31T20:48:54Z

Updated with support for shutdown messages and additional coop close workflow steps.

Roasbeef

Nice work! I think the structure of the changes is well inline with that of the existing codebase. However, I think the new closing workflow can be consolidated and simplified a bit as I'll point out within my in-line comments.

Roasbeef · 2017-04-11T02:35:47Z

lnwire/closing_signed.go

+	"github.com/roasbeef/btcd/wire"
+)
+
+// ClosingSigned is sent by both the initator and responder in a channel close


Minor nit: at a glance, this godoc comment doesn't appear to be wrapped to 80-characters.

Roasbeef · 2017-04-11T02:41:40Z

lnwire/closing_signed.go

+// workflow once the channel is clear of HTLCs. Each party provides a signature
+// for a transaction with a fee that they believe is fair. The process
+// terminates when both sides agree on the same fee, or when one side force
+// closes the channel.  NOTE: The responder is able to send a signature without


Minor nit: within the rest of the code we separate the "NOTE" portion from the rest of the comment by a line line.

Roasbeef · 2017-04-11T02:42:38Z

lnwire/closing_signed.go

+//
+// This is part of the lnwire.Message interface.
+func (c *ClosingSigned) Decode(r io.Reader, pver uint32) error {
+	err := readElements(r,


This'll probably show up in the linter, but this bit of code can be simplified to:

return readElements(r, &c.ChannelID, &c.FeeSatoshis, &c.Signature)

Roasbeef · 2017-04-11T02:43:09Z

lnwire/closing_signed.go

+//
+// This is part of the lnwire.Message interface.
+func (c *ClosingSigned) Encode(w io.Writer, pver uint32) error {
+	err := writeElements(w,


Similar comment here about simply returning the function directly.

Roasbeef · 2017-04-11T02:46:00Z

lnwire/shutdown.go

+// funds of both parties to the final delivery addresses negotiated during the
+// funding workflow.
+//
+// NOTE: The requester is able to only send a signature to initiate the


I think this NOTE: section can safely be eliminated as it was only meant to be included with the message that piggybacks the closing signature itself.

Roasbeef · 2017-04-11T18:47:21Z

peer.go

+	channel := p.activeChannels[*req.chanPoint]
+	p.activeChanMtx.RUnlock()
+
+	err := p.sendClosingSigned(channel)


Hmm, I think we can eliminate a need for this method call by adding the closing signature as a return parameter to CompleteCooperativeClose.

Atm work is duplicated a bit since on each channel closing workflow, each side will always generate their signature twice. In order to fix this, we can expand the newly added local scope map to channelManager to also hold one half of the closing signature spending on context and execution order. Next, we can modify the cooperative closure methods on the channel to have the CompleteCoopertiveClose method accept both signatures.

Roasbeef · 2017-04-11T18:48:14Z

peer.go

-			closingTxid)
+		err := p.sendShutdown(channel)
+		if err != nil {
+			req.err <- err


It's not strictly necessary, but in order to possibly eliminate some future bugs that arise by inserting further statements below this if, a return should be inserted here.

Roasbeef · 2017-04-11T18:49:28Z

peer.go

-			return
-		}
+		doneChan := make(chan struct{})
+		go p.waitForCloseConfirmation(channel, closingTxid, doneChan)


While you're at, this method can also be used within the rpcserver when we're handling the force closure of a channel.

Roasbeef · 2017-04-11T18:49:56Z

peer.go

-			return spew.Sdump(closeTx)
-		}))
+	closingTxid := closeTx.TxHash()
+	// Finally, launch a goroutine which will request to be notified by the


A new line should be inserted above this line.

Roasbeef · 2017-04-11T18:53:30Z

peer.go

+		return err
+	}
+	channelPoint := channel.ChannelPoint()
+	closingSigned := lnwire.NewClosingSigned(*channelPoint, 5000, closeSig)


The 5000 should be pulled out into a constant (and then later be made dynamic by your follow up work).

bryanvu · 2017-05-11T00:55:05Z

Updated with requested changes. Previous version can be found at: https://github.com/bryanvu/lnd/tree/channelclose1

Roasbeef · 2017-05-12T21:58:26Z

lnwallet/channel.go

-// transaction is returned. It is the duty of the responding node to broadcast
-// a signed+valid closure transaction to the network.
+// active lightning channel. A fully signed closure transaction as well as the
+// signature itself are returned.


This godoc comment should be expanded to detail the behavior of the nil-ness of either the localSig and the or remoteSig.

Refactored so that nil sigs shouldn't be passed in.

Roasbeef · 2017-05-12T22:00:53Z

lnwallet/channel.go

+	// in as the localSig. If we're the initiator, we'll generate our
+	// half of the 2-of-2 multi-sig needed to redeem the funding output.
+	var ourSig []byte
+	var ourFlaggedSig []byte


Namning suggestion: ourFlaggedSig -> sigWithSighash.

Also this comment doesn't seem quite correct. Based on my interpretation, the responder will be the one that calls this method w/o a signature in the case that they immediately accept the initiators proposal w.r.t the fee amount. The initiator will be the one that passes in a non-nil value for both signatures as they currently hold the current pending signature in volatile memory.

Roasbeef · 2017-05-12T22:02:31Z

lnwire/closing_signed.go

+// inputs/outputs.
+type ClosingSigned struct {
+	// ChannelID serves to identify which channel is to be closed.
+	// TODO(bvu): this should be switched to a ChannelID when the


This TODO comment can be removed. We know properly use the 32-byte channel ID's everywhere.

Roasbeef · 2017-05-12T22:03:11Z

lnwire/closing_signed.go

+
+	// ChannelID (outpoint) - 36 bytes
+	// TODO(bvu): Adjust when new ChannelID is added.
+	length += 36


Should be 32-bytes as we now use the 32-byte channel ID's uniformly when de-multiplexing channel modification messages.

Roasbeef · 2017-05-12T22:03:31Z

lnwire/closing_signed.go

+// on the ClosingSigned are valid.
+//
+// This is part of the lnwire.Message interface.
+func (c *ClosingSigned) Validate() error {


Validate is no longer a part of the lnwire.Message interface.

Roasbeef · 2017-05-12T22:05:25Z

lnwire/lnwire.go

+		}
+		length := binary.BigEndian.Uint16(addrLen[:])
+
+		var addrBytes [32]byte


The max value here is actually 34 bytes. 34 bytes are needed to encode the script for a p2wsh script: OP_0 OP_PUSH_DATA_32 <32 bytes>.

Roasbeef · 2017-05-12T22:05:56Z

lnwire/shutdown.go

+// sanity. For example, signature fields must not be nil.
+//
+// This is part of the lnwire.Message interface.
+func (s *Shutdown) Validate() error {


Validate is no longer a part of the lnwire.Message interface.

Roasbeef · 2017-05-12T22:10:40Z

peer.go

+// HTLCs for the specified channel.
+func (p *peer) sendShutdown(channel *lnwallet.LightningChannel) error {
+	_, addrs, _, err :=
+		txscript.ExtractPkScriptAddrs(channel.LocalDeliveryScript,


Should be lifted into the line above.

coveralls · 2017-05-13T02:04:28Z

Coverage decreased (-0.09%) to 73.457% when pulling bfe539d3a82b1372a61f9661e2f160ca36ee1424 on bryanvu:channelclose into 1124b45 on lightningnetwork:master.

bryanvu · 2017-05-13T02:05:35Z

Updated. Previous version at: https://github.com/bryanvu/lnd/tree/channelclose2

coveralls · 2017-05-13T02:18:54Z

Coverage decreased (-0.1%) to 73.438% when pulling 131f3ad on bryanvu:channelclose into 1124b45 on lightningnetwork:master.

bryanvu · 2017-05-17T23:57:00Z

Rebased and added a commit. Previous version at: https://github.com/bryanvu/lnd/tree/channelclose3

coveralls · 2017-05-18T19:22:17Z

Coverage decreased (-0.09%) to 73.526% when pulling 9f39bac22307125472f57d40b338ec1ed13bd51e on bryanvu:channelclose into a5616a3 on lightningnetwork:master.

Roasbeef · 2017-05-22T22:09:10Z

lnwallet/channel.go

+// fee that will be used for a cooperative channel close before generating the
+// proposed channel close transaction. This is called by the responder in a
+// cooperative channel close workflow.
+func (lc *LightningChannel) CreateRespCloseProposal() ([]byte, uint64, error) {


Why's this needed? In the case of a responder to a cooperative closure workflow, they'd still need to be aware of the proposed fee, as using the prior (latest) fee-per-kw may be inaccurate in the case that the initiator chose a diff fee than was set initially within the channel.

Roasbeef · 2017-05-22T22:10:02Z

lnwallet/channel.go

@@ -2532,7 +2549,7 @@ 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, error) {
+func (lc *LightningChannel) CreateCloseProposal(proposedFee uint64) ([]byte, error) {


Rather than the fee entirely, IMO the fee rate instead should be passed. This lets us keep the internal details of calculating the proper fee (based on the base weight, etc), within the state machine entirely.

Roasbeef · 2017-05-22T22:12:19Z

lnwallet/channel.go

@@ -2590,8 +2605,7 @@ func (lc *LightningChannel) CreateCloseProposal(feeRate uint64) ([]byte, error)
 //
 // NOTE: The passed local and remote sigs are expected to be fully complete
 // signatures including the proper sighash byte.
-func (lc *LightningChannel) CompleteCooperativeClose(localSig,
-	remoteSig []byte, proposedFee uint64) (*wire.MsgTx, error) {
+func (lc *LightningChannel) CompleteCooperativeClose(localSig, remoteSig []byte) (*wire.MsgTx, error) {


Fee rate should still be passed in, otherwise this will sign a commitment transaction with the prior fee of the latest commitment state.

Removed close_request and close_complete and replaced with shutdown and closing_signed.

This commit changes the cooperative channel close workflow to comply with the latest spec. This adds steps to handle and send shutdown messages as well as moving responsibility for sending the channel close message from the initiator to the responder.

coveralls · 2017-05-23T01:56:13Z

Coverage decreased (-0.09%) to 73.529% when pulling edbf0b8999338de178537cac56d83cc1c7f76939 on bryanvu:channelclose into 570c0e5 on lightningnetwork:master.

This commit switches the channel close workflow to use the lnwallet fee estimation interface rather than the hardcoded proposedFee.

coveralls · 2017-05-23T04:26:14Z

Coverage decreased (-0.08%) to 73.539% when pulling 4660ddf on bryanvu:channelclose into 570c0e5 on lightningnetwork:master.

Roasbeef

LGTM ⛷

Roasbeef requested changes Mar 9, 2017

View reviewed changes

bryanvu force-pushed the channelclose branch 5 times, most recently from 94cea59 to e3dacef Compare March 31, 2017 20:39

Roasbeef requested changes Apr 11, 2017

View reviewed changes

halseth mentioned this pull request Apr 13, 2017

Keeping track of pending channel states #179

Closed

bryanvu force-pushed the channelclose branch from e3dacef to 0fdeb09 Compare May 11, 2017 00:42

Roasbeef requested changes May 12, 2017

View reviewed changes

bryanvu force-pushed the channelclose branch 4 times, most recently from f4b4de1 to bfe539d Compare May 13, 2017 01:58

bryanvu force-pushed the channelclose branch from bfe539d to 131f3ad Compare May 13, 2017 02:09

bryanvu force-pushed the channelclose branch from 131f3ad to 0027114 Compare May 17, 2017 23:54

bryanvu force-pushed the channelclose branch 2 times, most recently from 44b18d6 to 9f39bac Compare May 18, 2017 19:15

Roasbeef requested changes May 22, 2017

View reviewed changes

bryanvu added 2 commits May 22, 2017 17:55

lnwire: adjusted coop close messages to comply with spec

20247d7

Removed close_request and close_complete and replaced with shutdown and closing_signed.

bryanvu force-pushed the channelclose branch from 9f39bac to c9fe463 Compare May 23, 2017 01:03

bryanvu force-pushed the channelclose branch from c9fe463 to edbf0b8 Compare May 23, 2017 01:49

lnwallet: update channel close to use fee estimation interface

4660ddf

This commit switches the channel close workflow to use the lnwallet fee estimation interface rather than the hardcoded proposedFee.

bryanvu force-pushed the channelclose branch from edbf0b8 to 4660ddf Compare May 23, 2017 04:17

Roasbeef approved these changes May 23, 2017

View reviewed changes

Roasbeef merged commit 408be35 into lightningnetwork:master May 23, 2017

lnwire: adjusted coop close messages to comply with spec #159

lnwire: adjusted coop close messages to comply with spec #159

Conversation

bryanvu commented Mar 8, 2017

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

coveralls commented Mar 31, 2017 • edited Loading

bryanvu commented Mar 31, 2017

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

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

bryanvu commented May 11, 2017

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

coveralls commented May 13, 2017

bryanvu commented May 13, 2017

coveralls commented May 13, 2017

bryanvu commented May 17, 2017

coveralls commented May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 23, 2017

coveralls commented May 23, 2017 • edited Loading

Roasbeef left a comment

Choose a reason for hiding this comment

coveralls commented Mar 31, 2017 •

edited

Loading

coveralls commented May 18, 2017 •

edited

Loading

coveralls commented May 23, 2017 •

edited

Loading