Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lnrpc: re-enable custom records #3744

Merged

Conversation

joostjager
Copy link
Collaborator

@joostjager joostjager commented Nov 20, 2019

This PR re-enables custom record sending that was disabled in #3575.

It leaves the database serialization format as is, because there currently is no functional need to change it.

@joostjager joostjager self-assigned this Nov 20, 2019
@joostjager joostjager added payments routing rpc v0.9.0 labels Nov 20, 2019
@joostjager joostjager added this to WIP in v0.9.0-beta via automation Nov 20, 2019
@joostjager joostjager added this to the 0.9.0 milestone Nov 20, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Nov 20, 2019

@cfromknecht, we spoke offline about the risk that we wouldn't be able to deserialize even custom records from our own database as long as odd/even checking was part of the tlv package. That has been removed in #3701, but looking at the hop serialization code again it also seems that we weren't using tlv there in the first place?

@joostjager joostjager requested a review from cfromknecht Nov 21, 2019
@joostjager joostjager force-pushed the enable-custom-records branch 3 times, most recently from f31c80a to 3b5c934 Compare Nov 22, 2019
@joostjager joostjager requested a review from halseth as a code owner Nov 22, 2019
@joostjager joostjager requested review from guggero and removed request for halseth Nov 22, 2019
@joostjager
Copy link
Collaborator Author

@joostjager joostjager commented Nov 22, 2019

Added the custom range check. Hopefully this PR didn't miss any entry/exit point for custom records. Because of the duplicated send interface, it is quite a few places.

@joostjager joostjager moved this from WIP to Needs Review in v0.9.0-beta Nov 22, 2019
Copy link
Collaborator

@guggero guggero left a comment

Nice to see those custom TLVs enabled again!
My only question is about the documentation comments in the proto file.

lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
lnrpc/routerrpc/router_backend.go Outdated Show resolved Hide resolved
lnrpc/rpc.proto Outdated
application specific data during the payment attempt. If the destination
does not support the specified recrods, and error will be returned.
*/
map<uint64, bytes> dest_tlv = 13;
Copy link
Collaborator

@guggero guggero Nov 25, 2019

Choose a reason for hiding this comment

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

On every other field that has the type bytes involved, we have the following comment:

When using REST, this field must be encoded as base64.

Do we want to add this here too?

Copy link
Collaborator Author

@joostjager joostjager Nov 26, 2019

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@cfromknecht cfromknecht Dec 4, 2019

Choose a reason for hiding this comment

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

To be consistent with the hop message, I would say this should be renamed to something like dest_custom_records. Comment should also mention custom record range

@joostjager joostjager force-pushed the enable-custom-records branch 2 times, most recently from 360b3b7 to 0c6dd92 Compare Nov 26, 2019
@joostjager joostjager requested a review from guggero Nov 26, 2019
@joostjager joostjager force-pushed the enable-custom-records branch 2 times, most recently from 77d1038 to fce2d0d Compare Nov 26, 2019
Copy link
Collaborator

@guggero guggero left a comment

LGTM 🚀

@joostjager joostjager requested a review from cfromknecht Dec 3, 2019
Copy link
Collaborator

@cfromknecht cfromknecht left a comment

LGTM, just one small comment about aligning the rpc field names

lnrpc/rpc.proto Outdated
application specific data during the payment attempt. If the destination
does not support the specified recrods, and error will be returned.
*/
map<uint64, bytes> dest_tlv = 13;
Copy link
Collaborator

@cfromknecht cfromknecht Dec 4, 2019

Choose a reason for hiding this comment

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

To be consistent with the hop message, I would say this should be renamed to something like dest_custom_records. Comment should also mention custom record range

v0.9.0-beta automation moved this from Needs Review to Approved Dec 4, 2019
joostjager added 3 commits Dec 4, 2019
This commit also modifies a previous migration. Because the change is so
limited, we don't consider this a significant risk.
Remove code duplication in preparation for additional unmarshalling.
This commit also renames the rpc field for consistency. As it wasn't
functional and the id doesn't change, this isn't considered a breaking
change.
@joostjager joostjager merged commit 1371e5a into lightningnetwork:master Dec 4, 2019
1 of 2 checks passed
v0.9.0-beta automation moved this from Approved to Done Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
payments routing rpc v0.9.0
Projects
No open projects
v0.9.0-beta
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants