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

Incorrect validation on keysend route hints #6143

Closed
Evanfeenstra opened this issue Apr 3, 2023 · 3 comments · Fixed by #6154
Closed

Incorrect validation on keysend route hints #6143

Evanfeenstra opened this issue Apr 3, 2023 · 3 comments · Fixed by #6154
Assignees
Milestone

Comments

@Evanfeenstra
Copy link

Issue and Steps to Reproduce

The param_routehint_array function here https://github.com/ElementsProject/lightning/blob/master/common/json_param.c#L953 checks if the RoutehintList is an array (JSMN_ARRAY). However the RoutehintList is actually an object that has the array of hints inside a hints field https://github.com/ElementsProject/lightning/blob/master/cln-grpc/proto/primitives.proto#L73. The param_routehint function looks like it has the same problem.

You can try it using a cln-grpc client, like in this example https://github.com/Evanfeenstra/cln-grpc-routehints-test/blob/master/src/cln.rs#L50. The exact error message is:

Routehint array routehints (\\"{\\"hints\\":[{\\"hops\\":[{\\"expirydelta\\":40,\\"feebase\\":\\"1000msat\\",\\"feeprop\\":1,\\"id\\":\\"02c7046d20f62012362ccf835fe5b4d4a1708e518592f216afeefabeadfc20154b\\",\\"scid\\":\\"1x5x1\\"}]}]}\\") is not an array
@cdecker
Copy link
Member

cdecker commented Apr 5, 2023

Hm, that's definitely wrong on my side. Seems like this is the only case of a 2D array, which is treated a bit differently in the code generation tool. I will:

  • Add a custom conversion for routehints if not already present
  • Amend the msggen to either not handle 2D arrays or fix its behavior
  • Publish a new version to crates.io

However I am going to be offline till end of month, hopefully that is not too late :-)

@ShahanaFarooqui ShahanaFarooqui modified the milestones: v23.05, v23.08 Apr 5, 2023
cdecker added a commit to cdecker/lightning that referenced this issue Apr 6, 2023
cdecker added a commit to cdecker/lightning that referenced this issue Apr 6, 2023
Fixes ElementsProject#6143
Changelog-Fixed: clnrs: Fixed an issue converting routehints in keysend
@cdecker
Copy link
Member

cdecker commented Apr 6, 2023

Hi @Evanfeenstra, I pushed a preliminary fix as #6154. It converts the custom types correctly to the 2d array now. For cln-grpc the inverse conversion is not required but we may want it at some time in the future, hence why there are the todo!() markers. I'll push the new version to crate.io as soon as the PR is merged ^^

@cdecker cdecker modified the milestones: v23.08, v23.05 Apr 6, 2023
@Evanfeenstra
Copy link
Author

thanks for working on it @cdecker !

rustyrussell pushed a commit that referenced this issue Apr 10, 2023
rustyrussell pushed a commit that referenced this issue Apr 10, 2023
Fixes #6143
Changelog-Fixed: clnrs: Fixed an issue converting routehints in keysend
ksedgwic pushed a commit to lightning-signer/c-lightning that referenced this issue Apr 11, 2023
Fixes ElementsProject#6143
Changelog-Fixed: clnrs: Fixed an issue converting routehints in keysend
gkrizek pushed a commit to voltagecloud/lightning that referenced this issue Apr 26, 2023
gkrizek pushed a commit to voltagecloud/lightning that referenced this issue Apr 26, 2023
Fixes ElementsProject#6143
Changelog-Fixed: clnrs: Fixed an issue converting routehints in keysend
ddustin pushed a commit to ddustin/lightning that referenced this issue May 12, 2023
ddustin pushed a commit to ddustin/lightning that referenced this issue May 12, 2023
Fixes ElementsProject#6143
Changelog-Fixed: clnrs: Fixed an issue converting routehints in keysend
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants