Improve type safety for message failures by D4nte · Pull Request #600 · lightningdevkit/rust-lightning

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte · 2020-04-20T10:50:01Z

Follow-up of #596

Edit: Ready for review.

This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.

I see several ways to follow-up with this change:

Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

jkczyz · 2020-04-20T14:59:18Z

Concept ACK

Would love to see the code move in this direction, too. Thanks for jumping on this!

TheBlueMatt · 2020-04-21T00:11:02Z

Yep, what Jeff said!

D4nte · 2020-04-21T11:03:05Z

Need to sort build on old Rust toolchain 😟

jkczyz · 2020-04-21T17:04:49Z

Need to sort build on old Rust toolchain 😟

Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:

cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning

You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

All message failure are constructed on the fly in the channel manager. Instead, we add the `MessageFailure` enum. The enum will list the possible message failures and type the data to be included. The code would also be defined once in an impl block and this enum can directly provide the data byte array and code using `Writeable` trait. This commit adds the skeleton.

These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte · 2020-04-23T04:44:46Z

Need to sort build on old Rust toolchain 😟
Looks like you're running into issues on 1.22.0 with matching enum variants. The syntax is a bit more verbose in this version. You can reproduce the errors using:
cargo +1.22.0 check -p lightning
cargo +1.22.0 test -p lightning
You might find the wire module useful as an example of the syntax if the compiler messages aren't helpful.

Yep, thanks, no worries. I was working in Rust already when they did those improvements on the compiler where it is able to infer referencing. It's now ready for review/merge.

jkczyz · 2020-04-23T14:47:58Z

Follow-up of #596
Edit: Ready for review.
This a step towards a safer handling of failure code and associated data by using a new MessageFailure enum instead of serialising the code and data on call site.
I see several ways to follow-up with this change:
Deserialise all possible message failures to MessageFailure, allowing the removal of the HTLCFailReason::Reason variant.
Continue to replace call site assignment of code value with instantiation of MessageFailure variables.

I have a bit of a preference to make all these changes in one PR rather than leaving the code in an intermediary state. Or at very least for (2). @TheBlueMatt What do you think?

One downside of this solution is that I had to clone ChannelUpdate. Let me know if you'd prefer I optimise this before merging.

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

TheBlueMatt · 2020-04-23T21:26:42Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

D4nte · 2020-04-23T23:56:00Z

Looks like we were already cloning ChannelUpdate inside peer_disconnected. Or were you referring to somewhere else?

Ah yes indeed, all good.

D4nte · 2020-04-23T23:56:16Z

Yea, at least personally I really don't mind a larger PR that removes HTLCFailReason::Reason and replaces it with types, obviously with smaller intermediary commits, though.

Ok cool, will do that then.

TheBlueMatt · 2020-09-27T01:28:06Z

@D4nte any plans on updating this?

D4nte · 2020-10-06T02:33:24Z

@D4nte any plans on updating this?

Hi @TheBlueMatt, I don't see an opportunity to follow-up within the next two months. I'll close for now.

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

D4nte force-pushed the type-safety-message-failures branch from 9851d05 to e98b0c3Compare April 21, 2020 09:58

D4nte changed the title ~~[draft]Type message failures~~Improve type safety for message failuresApr 21, 2020

D4nte marked this pull request as ready for review April 21, 2020 10:04

D4nte added 6 commits April 22, 2020 09:35

Use TypedReason for incorrect_or_unknown_payment_details
61d45d4

Use TypedReason for permanent_channel_failure
e5645a6

Use TypedReason for unknown_next_peer
fb82ff5

Use TypedReason for temporary_channel_failure
930a325

Set top bytes of failure_code in constants
b608426
These values are used at several places of the code. Constants usage is preferred to avoid typos and make code more readable.

D4nte force-pushed the type-safety-message-failures branch from e98b0c3 to b608426Compare April 21, 2020 23:38

jkczyz self-requested a review April 23, 2020 14:48

D4nte closed this Oct 6, 2020

Uh oh!

Conversation

D4nte commented Apr 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz commented Apr 20, 2020

Uh oh!

TheBlueMatt commented Apr 21, 2020

Uh oh!

D4nte commented Apr 21, 2020

Uh oh!

jkczyz commented Apr 21, 2020

Uh oh!

D4nte commented Apr 23, 2020

Uh oh!

jkczyz commented Apr 23, 2020

Uh oh!

TheBlueMatt commented Apr 23, 2020

Uh oh!

D4nte commented Apr 23, 2020

Uh oh!

D4nte commented Apr 23, 2020

Uh oh!

TheBlueMatt commented Sep 27, 2020

Uh oh!

D4nte commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

D4nte commented Apr 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz commented Apr 20, 2020

Uh oh!

TheBlueMatt commented Apr 21, 2020

Uh oh!

D4nte commented Apr 21, 2020

Uh oh!

jkczyz commented Apr 21, 2020

Uh oh!

D4nte commented Apr 23, 2020

Uh oh!

jkczyz commented Apr 23, 2020

Uh oh!

TheBlueMatt commented Apr 23, 2020

Uh oh!

D4nte commented Apr 23, 2020

Uh oh!

D4nte commented Apr 23, 2020

Uh oh!

TheBlueMatt commented Sep 27, 2020

Uh oh!

D4nte commented Oct 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

D4nte commented Apr 20, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkczyz commented Apr 20, 2020

Uh oh!

TheBlueMatt commented Apr 21, 2020

Uh oh!

D4nte commented Apr 21, 2020

D4nte commented Apr 20, 2020•
edited
Loading

D4nte commented Apr 20, 2020•
edited
Loading

D4nte commented Apr 20, 2020•
edited
Loading

D4nte commented Apr 20, 2020•
edited
Loading