Add SignMessage and VerifyMessage rpc commands #192

phlip9 · 2017-04-20T04:41:03Z

See Issue: #187

mention-bot · 2017-04-20T04:41:05Z

@phlip9, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Roasbeef, @bryanvu and @AndrewSamokhvalov to be potential reviewers.

coveralls · 2017-04-23T02:13:46Z

Coverage increased (+5.1%) to 73.942% when pulling 84c551246681439122fb9d820b019d6e5be6c9a5 on phlip9:master into 4939443 on lightningnetwork:master.

Roasbeef

Hi @phlip9, thanks for this PR!

I've completed an initial round of review. Most of my comments are pretty minor and are concerned with consistent code style throughput the project. After this first round of review is addressed, I'll move onto a bit of local testing before the PR is ultimately approved.

Roasbeef · 2017-04-27T04:43:51Z

cmd/lncli/commands.go

+			Usage: "the message to verify",
+		},
+		cli.StringFlag{
+			Name:  "signature",


Minor nit: I recommend going with simply sig here as it's quicker to type on the command line.

Roasbeef · 2017-04-27T04:47:12Z

lnd_test.go

+func testNodeSignVerify(net *networkHarness, t *harnessTest) {
+	ctxb := context.Background()
+
+	// bob should be able to verify alice's signature


In order to be consistent with the coding style of the project: all comments should be complete sentences and should also be flush with a coding fragment below it (meaning eliminate the extra new-line).

Roasbeef · 2017-04-27T04:47:56Z

lnd_test.go

+	if verifyResp.Valid {
+		t.Fatalf("carol's signature should not be valid")
+	}
+}


The carol node should be shutdown at the end of this test as it's no longer of any use:

if err := carol.shutdown()l err != nil { ... handle err... }

Roasbeef · 2017-04-27T04:49:54Z

lnrpc/rpc.proto

+    string signature = 2 [ json_name = "signature" ];
+}
+message VerifyMessageResponse {
+    bool valid = 1 [ json_name = "valid" ];


In-line with @AndrewSamokhvalov's comment on the original issue, this should also return the compressed public key of the signer that was extracted from the signature. This will allow higher-level to enforce any sort of channel capitalization constraint themselves as additional checks after the signature validates.

Roasbeef · 2017-04-27T04:51:12Z

nodesigner.go

+		return nil, fmt.Errorf("unknown public key")
+	}
+
+	// Otherwise, we'll sign the dsha256 of the target message.


In line with the contribution guidelines of this project, the code fragment under this line should be modified to have some breathing room.

Roasbeef · 2017-04-27T04:51:52Z

nodesigner.go

+	// Otherwise, we'll sign the dsha256 of the target message.
+	digest := chainhash.DoubleHashB(msg)
+	// Should the signature reference a compressed public key or not.
+	compressedPubKey := false


Keys should always be interpreted as being compressed. All public keys circulated within the network are always serialized in compressed format.

Roasbeef · 2017-04-27T04:53:24Z

nodesigner.go

+	// Should the signature reference a compressed public key or not.
+	compressedPubKey := false
+	// btcec.SignCompact returns a pubkey-recoverable signature
+	sign, err := btcec.SignCompact(


Minor nit, modify the line-breaking here to be consistent with the rest of the codebase:

sign, err := btcec.SignCompact(btcec.S256(), n.privKey, digest, compressedPubKey)

Roasbeef · 2017-04-27T04:53:39Z

nodesigner.go

+	// Should the signature reference a compressed public key or not.
+	compressedPubKey := false
+	// btcec.SignCompact returns a pubkey-recoverable signature
+	sign, err := btcec.SignCompact(


sign -> sig

Roasbeef · 2017-04-27T04:55:01Z

nodesigner.go

+// resident node's private key. If the target public key is _not_ the node's
+// private key, then an error will be returned. The returned signature is a
+// pubkey-recoverable signature.
+func (n *nodeSigner) SignCompact(pubKey *btcec.PublicKey,


As we don't need to adhere to a particular interface (yet) here (which is why the SignMessage method is how it is atm), we can omit the public key argument all together. Implicitly, SignCompact will always use the resident node's private key.

Roasbeef · 2017-04-27T04:55:43Z

rpcserver.go

+	}
+
+	graph := r.server.chanDB.ChannelGraph()
+	// TODO(phlip9): Require valid nodes to have capital in active channels.


Similar comment here about code spacing.

coveralls · 2017-04-29T08:42:16Z

Coverage remained the same at 73.938% when pulling 635a869d43c66bb661b2b6382576969b0b13c7a2 on phlip9:master into 337a4a4 on lightningnetwork:master.

coveralls · 2017-04-29T11:51:20Z

Coverage remained the same at 73.938% when pulling 42c79d5a7f0a87d21bf24acf1c67338f6ac0f1e8 on phlip9:master into 337a4a4 on lightningnetwork:master.

phlip9 · 2017-04-29T11:54:05Z

Fixed the review requests. Let me know if you spot anything else!

The only notable, recent addition was openChannel / closeChannel between Alice and Bob in the sign + verify test, since it was implicitly relying on previous tests executing and leaving Alice and Bob in a particular state. I noticed this issue when I commented out the other integration tests and saw that the sign + verify test was failing because Alice and Bob didn't have a channel open.

Roasbeef

LGTM ⚡️

Once the last comment is addressed (public keys should always be interpreted as being encoded in compressed format), and the PR is rebased, I'll get this merged!

Roasbeef · 2017-05-05T23:11:39Z

nodesigner.go

+	digest := chainhash.DoubleHashB(msg)
+
+	// Should the signature reference a compressed public key or not.
+	isCompressedKey := false


Public keys within the protocol are always meant to be handled or transferred in a compressed format.

coveralls · 2017-05-07T12:08:35Z

Coverage remained the same at 73.437% when pulling d13c36613217a04e87498c384ae9fa61ff0f526f on phlip9:master into 8e4199e on lightningnetwork:master.

coveralls · 2017-05-07T12:12:09Z

Coverage decreased (-0.004%) to 73.433% when pulling d13c36613217a04e87498c384ae9fa61ff0f526f on phlip9:master into 8e4199e on lightningnetwork:master.

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

Hi @phlip9, think you could rebase this again? Another PR which touched similar files (adding a RPC new command) was merged before this one. Thanks!

This commit allows users to sign messages with their node's private key with the SignMessage interface. The signatures are zbase32 encoded for human readability/paste-ability. Others users can verify that a message was signed by another node in their channel database with the VerifyMessage interface.

phlip9 · 2017-05-12T01:02:00Z

@Roasbeef Ok, just rebased on master; it should be good to merge.

coveralls · 2017-05-12T21:11:54Z

Coverage remained the same at 73.542% when pulling 86218b3 on phlip9:master into 2486097 on lightningnetwork:master.

Roasbeef requested changes Apr 27, 2017

View reviewed changes

phlip9 force-pushed the master branch from 84c5512 to 635a869 Compare April 29, 2017 08:34

Roasbeef approved these changes May 5, 2017

View reviewed changes

phlip9 force-pushed the master branch from 42c79d5 to eb1a942 Compare May 7, 2017 12:00

phlip9 added 4 commits May 11, 2017 17:54

cmd/lncli: add signmessage and verifymessage commands

087c380

rpc: add signature to VerifyMessage response

18c4144

nodesigner: compact sigs should reference compact pubkeys

86218b3

phlip9 force-pushed the master branch from d13c366 to 86218b3 Compare May 12, 2017 01:00

Roasbeef merged commit 14f49d4 into lightningnetwork:master May 12, 2017

Roasbeef mentioned this pull request May 15, 2017

Add a SignMessage/VerifyMessage RPC command #187

Closed

4 tasks

jamaljsr mentioned this pull request Jan 17, 2019

Sign and Verify messages joule-labs/joule-extension#146

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add SignMessage and VerifyMessage rpc commands #192

Add SignMessage and VerifyMessage rpc commands #192

phlip9 commented Apr 20, 2017

mention-bot commented Apr 20, 2017

coveralls commented Apr 23, 2017 •

edited

Loading

Roasbeef left a comment

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

Roasbeef Apr 27, 2017

coveralls commented Apr 29, 2017

coveralls commented Apr 29, 2017 •

edited

Loading

phlip9 commented Apr 29, 2017

Roasbeef left a comment

Roasbeef May 5, 2017

coveralls commented May 7, 2017

coveralls commented May 7, 2017

Roasbeef commented May 11, 2017

phlip9 commented May 12, 2017

coveralls commented May 12, 2017 •

edited

Loading

Add SignMessage and VerifyMessage rpc commands #192

Add SignMessage and VerifyMessage rpc commands #192

Conversation

phlip9 commented Apr 20, 2017

mention-bot commented Apr 20, 2017

coveralls commented Apr 23, 2017 • edited Loading

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

coveralls commented Apr 29, 2017

coveralls commented Apr 29, 2017 • edited Loading

phlip9 commented Apr 29, 2017

Roasbeef left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 7, 2017

coveralls commented May 7, 2017

Roasbeef commented May 11, 2017

phlip9 commented May 12, 2017

coveralls commented May 12, 2017 • edited Loading

coveralls commented Apr 23, 2017 •

edited

Loading

coveralls commented Apr 29, 2017 •

edited

Loading

coveralls commented May 12, 2017 •

edited

Loading