Skip to content

Add htlcMaximumMsat field to ChannelUpdate message#679

Closed
akumaigorodski wants to merge 4 commits into
ACINQ:masterfrom
akumaigorodski:update-htlc-maximum-msat
Closed

Add htlcMaximumMsat field to ChannelUpdate message#679
akumaigorodski wants to merge 4 commits into
ACINQ:masterfrom
akumaigorodski:update-htlc-maximum-msat

Conversation

@akumaigorodski
Copy link
Copy Markdown
Contributor

The only test failure is at failureMessageCodec which can't encode new ChannelUpdate format with the following message:

java.lang.IllegalArgumentException: channelUpdate/size: failed to encode size of [ChannelUpdate(3045022100c451cd65c88f55b1767941a247e849e12f5f4d4a93a07316659e22f5267d2088022009042a595c6bc8942cd9d729317b82b306edc259fb6b3a3cecb3dd1bd446e90601,06226e46111a0b59caaf126043eb5bbf28c34f3a5e332a1fc7b2b73cf188910f,3039,1234567,00,01,100,1000,12,76,None)]: 1025 is not evenly divisible by 8

don't yet understand why this is happening.

.typecase(UPDATE | 13, (("expiry" | uint32) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[IncorrectCltvExpiry])
.typecase(UPDATE | 14, (("channelUpdate" | channelUpdateWithLengthCodec)).as[ExpiryTooSoon])
.typecase(UPDATE | 20, (("flags" | binarydata(2)) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[ChannelDisabled])
.typecase(UPDATE | 20, (("flags" | binarydata(1)) :: ("channelUpdate" | channelUpdateWithLengthCodec)).as[ChannelDisabled])
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not compliant, the UPDATE|20 message still includes the 2-bytes flags.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do I have to change value back to 2 there?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's what BOLT 4 says

("feeBaseMsat" | uint32) ::
("feeProportionalMillionths" | uint32))
("feeProportionalMillionths" | uint32) ::
("htlcMaximumMsat" | optional(bool, uint64)))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not compliant: this encoding will prefix the htlc_maximum_msat field by a bit telling if there is data or not. Instead the bit should be set in the message_flags field.

One way to do it is to add a require in the ChannelUpdate constructor that makes sure that the flag is set when the optional field is defined, and use a conditional codec with a >>:~ operator, à la:

https://github.com/scodec/scodec-protocols/blob/6b33ffcea6da43d499ee455f201db9389476d51e/src/main/scala/scodec/protocols/ip/tcp/TcpHeader.scala#L37-L49

Let me know if you need help with this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw this bit is also what causes the test error in FailureMessageCodecsSpec.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've decided to use flatAppend because >>:~ produces an hlist of different structure.

@pm47 pm47 self-assigned this Sep 12, 2018
@pm47
Copy link
Copy Markdown
Member

pm47 commented Oct 17, 2018

Looks like parent branch/repo was deleted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants