Check Bolt 2 spec

[SWvheerden]

• Open_channel Message
• Accept_channel Message
• Funding_created Message
• Funding_signed Message
• Funding_locked Message
• Closing Initiation: shutdown
• Closing Negotiation: closing_signed
• Adding an HTLC: update_add_htlc
• Removing an HTLC: update_fulfill_htlc, update_fail_htlc, and update_fail_malformed_htlc
• Committing Updates So Far: commitment_signed
• Completing the Transition to the Updated State: revoke_and_ack
• Updating Fees: update_fee
• Message Retransmission

Audited

• Open_channel Message
• Accept_channel Message
• Funding_created Message
• Funding_signed Message
• Funding_locked Message
• Closing Initiation: shutdown
• Closing Negotiation: closing_signed
• Adding an HTLC: update_add_htlc
• Removing an HTLC: update_fulfill_htlc, update_fail_htlc, and update_fail_malformed_htlc
• Committing Updates So Far: commitment_signed
• Completing the Transition to the Updated State: revoke_and_ack
• Updating Fees: update_fee
• Message Retransmission

[SWvheerden]

#129

[SWvheerden]

Bolt 2 spec says this on open channel:
if the connection has been re-established after receiving a previous open_channel, BUT before receiving a funding_created message:
- accept a new open_channel message.
- discard the previous open_channel message.

Currently we store and accept channel message's on channel_id's and not on node_id's
This mean we conform to spec, but it also allows as to have more than one channel per node?
https://github.com/rust-bitcoin/rust-lightning/blob/d9d8ea3f65500c59e06f7f291c034d35bb08b502/src/ln/channelmanager.rs#L1692

[TheBlueMatt]

The spec is kinda inconsistent - originally it was written assuming one-channel-per-node but that was thrown out, so looks like a spec bug to me, but we do comply with that text as we'll drop the channel on disconnection if we never received the funding_created.

[TheBlueMatt]

Unchecked funding_signed - it still has some HandleError::action: None's in there. I've got a pending patchset to finish it off, though. Also as things get marked completed we should move from HandleError directly to ChannelError.

[SWvheerden]

Update fee is missing spec
#231

[SWvheerden]

message retransmission is missing
#238

[TheBlueMatt]

Pretty much all the message retransmission should work, though isn't sufficiently tested at this point.

…

On November 6, 2018 9:35:08 AM UTC, SW van Heerden ***@***.***> wrote: message retransmission is missing #239 and #238 -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #240 (comment)

[SWvheerden]

Pretty much all the message retransmission should work, though isn't sufficiently tested at this point.
…
On November 6, 2018 9:35:08 AM UTC, SW van Heerden @.***> wrote: message retransmission is missing #239 and #238 -- You are receiving this because you commented. Reply to this email directly or view it on GitHub: #240 (comment)

It looks fine, thats why I ticket it off. Those two are optional, although they are good ideas

[SWvheerden]

Removing an HTLC: update_fulfill_htlc, update_fail_htlc, and update_fail_malformed_htlc
Is missing: #215
#244

[neonknight64]

Audit of Bolt 2 spec for Closing Negotiation - closing_signed

• Message structure correct - ClosingSigned
• Check shutdown is complete before fee negotiation
• Check channel is empty of HTLCs before fee negotiation
• Funding node should send closing_signed message after receiving shutdown and no HTLCs remain
• Sending node must set fee less than or equal to the base fee of the final commitment transaction
• Sending node should set initial fee according to its estimate of cost of inclusion in a block
• Sending node must set signature to the signature of the close transaction
• Receiving node must fail connection when signature of close transactions invalid
• Receiving node should broadcast final closing transaction when current and previous fee are the same
• Receiving node must fail connection when fee is greater than the base fee of the final commitment transaction
  Comment: Implemented check with error but not failing connection
• Receiving node should fail connection when fee not between previous sent and received fee, except when reconnected
  Comment: Implemented check with error but not failing connection
• Receiving node should send closing_signed with same fee when agreement is reached
• Receiving node can propose new fee between previous received and sent fee

[TheBlueMatt]

Note that the closing_signed dropped-output stuff is pretty clearly broken, and we currently incorrectly fail channels when the remote node sends commitment_signeds with HTLC removes after shutdown messages. I saw a number of other bugs I dont recall anymore when looking at it last week but as noted in #250 I kinda gave up until that part of the spec gets rewritten. WIthout good test coverage of individual things I dont think we can consider anything properly "audited".

[philipr-za]

Tests have been written for the requirements for update_add_htlc message and was merged in as #291 / #296

[TheBlueMatt]

Closing as completed - most of the messages were audited as a part of this (thanks folks!) and the remaining ones have had plenty of eyeballs on them since (plus we'll be rewriting update_fee soon for update_fee v3 anyway!).