Wire shutdown by Vib-UX · Pull Request #185 · node-lightning/node-lightning

Vib-UX · 2021-07-31T08:10:05Z

wire: create shutdown message type #179

Parameters are defined according to shutdown in BOLT #2
Implementation is as per the standards given
UT is work in progress

codecov-commenter · 2021-07-31T08:13:36Z

Codecov Report

Merging #185 (f8c76cb) into main (a95e7d5) will increase coverage by 0.00%.
The diff coverage is 95.45%.

❗ Current head f8c76cb differs from pull request most recent head dd83c8a. Consider uploading reports for the commit dd83c8a to get more accurate results

@@           Coverage Diff           @@
##             main     #185   +/-   ##
=======================================
  Coverage   93.70%   93.71%           
=======================================
  Files         178      179    +1     
  Lines        4672     4693   +21     
  Branches      547      547           
=======================================
+ Hits         4378     4398   +20     
- Misses        162      163    +1     
  Partials      132      132

Impacted Files	Coverage Δ
packages/wire/lib/MessageFactory.ts	`68.57% <50.00%> (-1.13%)`	⬇️
packages/wire/lib/MessageType.ts	`100.00% <100.00%> (ø)`
packages/wire/lib/messages/ShutdownMessage.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a95e7d5...dd83c8a. Read the comment docs.

packages/wire/lib/messages/ShutdownMessage.ts

bmancini55

Looking good, couple comment changes and you'll need to fix the de/ser methods to properly handle the scriptPubKey len.

packages/wire/lib/messages/ShutdownMessage.ts

bmancini55 · 2021-08-03T11:51:19Z

packages/wire/lib/messages/ShutdownMessage.ts

+
+/**
+ * ShutdownMessage represents the `shutdown` message defined in
+ * BOLT #2 of the Lightning Specification. This message can be sent by the


s/sent by either/sent by the either/

bmancini55 · 2021-08-03T11:57:01Z

packages/wire/lib/messages/ShutdownMessage.ts

+/**
+ * ShutdownMessage represents the `shutdown` message defined in
+ * BOLT #2 of the Lightning Specification. This message can be sent by the
+ * either node. The scriptPubKey must follow the standards which are accepted


The scriptPubKey has stricter requirements than this. Must be a valid P2WPKH, P2WSH, P2SH-P2WPKH, P2SH-P2WSH, or any valid witness script if option_shutdown_anysegwit is negotiated.

Also may want to make a note that this scriptPubKey must be the same as the shutdown_scriptpubkey value if it was sent during channel open/accept.

1st remark comment added, For 2nd I have updated the scriptPubKey in test file!

bmancini55 · 2021-08-03T11:58:01Z

packages/wire/lib/messages/ShutdownMessage.ts

+ * by Bitcoin network that ensures the resulting transaction will propagate
+ * to miners. If shutdown is sent by either node , corresponding node should send
+ * commitment_signed to commit any outstanding changes before replying shutdown. Once
+ * shutdown is sent by both nodes no new HTLCs should be added or accepted by the channel.


Add one more note that after this, fee negotiation and signature sending can begin with closing_signed

packages/wire/lib/messages/ShutdownMessage.ts

bmancini55 · 2021-08-03T12:00:00Z

packages/wire/lib/messages/ShutdownMessage.ts

+        const writer = new BufferWriter();
+        writer.writeUInt16BE(this.type);
+        writer.writeBytes(this.channelId.toBuffer());
+        writer.writeBytes(this.len);


Length should be a u16be

bmancini55 · 2021-08-03T12:01:04Z

packages/wire/lib/messages/ShutdownMessage.ts

+
+        reader.readUInt16BE(); // read type
+        instance.channelId = new ChannelId(reader.readBytes(32));
+        instance.len = reader.readBytes(16);


Len should be a uint16be and used as input when reading the scriptPubKey bytes

bmancini55

Comments are looking good. Just need to add some info to the scriptPubKey property.

Biggest issue is we can get rid of the len property on the class. The length is implicit in the scriptPubKey buffer. During serialization you can just use the length property of the Buffer. For deserialization, you can read len into a temp variable and then call readBytes with len to read the scriptPubKey buffer.

bmancini55 · 2021-08-04T13:06:49Z

packages/wire/lib/messages/ShutdownMessage.ts

+
+        reader.readUInt16BE(); // read type
+        instance.channelId = new ChannelId(reader.readBytes(32));
+        instance.len = reader.readUInt16BE();


Just read this into a temp variable instead of a class property.

deser similar to this one - https://github.com/altangent/node-lightning/blob/main/packages/wire/lib/messages/ErrorMessage.ts ?

got it! Thanks for being patient with me!.🙇

No worries at all, you're doing great!

bmancini55 · 2021-08-04T13:07:28Z

packages/wire/lib/messages/ShutdownMessage.ts

+     */
+    public channelId: ChannelId;
+
+    public len: number;


Don't need a class property. The value is implicit in the scriptPubKey buffer.

bmancini55 · 2021-08-04T13:07:46Z

packages/wire/lib/messages/ShutdownMessage.ts

+        const writer = new BufferWriter();
+        writer.writeUInt16BE(this.type);
+        writer.writeBytes(this.channelId.toBuffer());
+        writer.writeUInt16BE(this.len);


Use the length of scriptPubKey

bmancini55 · 2021-08-04T13:09:37Z

packages/wire/lib/messages/ShutdownMessage.ts

+ * outstanding changes before replying shutdown. Once shutdown is sent by both nodes no new HTLCs should be added or accepted by the channel.
+ * After successful handshake of shutdown message, fee negotiation and signature sending can begin with `closing_signed` message.
+ */
+


nit: remove newline between comment block and class and watch comment line length.

bmancini55 · 2021-08-04T13:11:49Z

packages/wire/lib/messages/ShutdownMessage.ts

+
+    public len: number;
+    /**
+     * scriptPubKey is used by the sender to get paid.


Beef up the comment on what this value should or move comment from class to here. Either is fine.

bmancini55

Awesome, looks great! Thanks!

Vib-UX added 2 commits July 31, 2021 13:25

Shutdown Message

9a399d5

Message Factory update

9c4bf2f

Vib-UX commented Jul 31, 2021

View reviewed changes

packages/wire/lib/messages/ShutdownMessage.ts Outdated Show resolved Hide resolved

Vib-UX commented Jul 31, 2021

View reviewed changes

packages/wire/lib/messages/ShutdownMessage.ts Outdated Show resolved Hide resolved

Vib-UX added 3 commits August 3, 2021 02:48

Review V1

5831743

UT Update

11a88ec

Formatting Prettier Style

102a05e

bmancini55 suggested changes Aug 3, 2021

View reviewed changes

Vib-UX added 3 commits August 3, 2021 22:36

Review V2

b58506a

npm-errors_trialv1

328224a

Comments Update

dd83c8a

Vib-UX requested a review from bmancini55 August 3, 2021 18:19

bmancini55 suggested changes Aug 4, 2021

View reviewed changes

Review V3

47f73f2

Vib-UX requested a review from bmancini55 August 4, 2021 14:05

bmancini55 approved these changes Aug 4, 2021

View reviewed changes

bmancini55 marked this pull request as ready for review August 4, 2021 16:10

bmancini55 merged commit de8afec into node-lightning:main Aug 4, 2021

Conversation

Vib-UX commented Jul 31, 2021

wire: create shutdown message type #179

Uh oh!

codecov-commenter commented Jul 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmancini55 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Jul 31, 2021 •

edited

Loading