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

(1/3) Minor error code fixes #3699

Merged
merged 4 commits into from
Apr 4, 2025

Conversation

carlaKC
Copy link
Contributor

@carlaKC carlaKC commented Apr 2, 2025

This PR pulls a few trivial commits out of #3601 which deal with error code handling unrelated to the larger refactor.

Opening this up in a separate PR because it makes a change to one of the trampoline error codes that is a little spec-ambiguous, originally pointed out here.

This failure code isn't used anywhere in the codebase and is not defined
in BOLT 04.
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Apr 2, 2025

I've assigned @valentinewallace as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

@carlaKC carlaKC changed the title Minor error code fixes (1/3) Minor error code fixes Apr 2, 2025
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

Attention: Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 89.15%. Comparing base (dd4b580) to head (87a5756).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
lightning/src/ln/onion_utils.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3699      +/-   ##
==========================================
- Coverage   89.17%   89.15%   -0.02%     
==========================================
  Files         155      155              
  Lines      121023   121023              
  Branches   121023   121023              
==========================================
- Hits       107926   107904      -22     
- Misses      10447    10461      +14     
- Partials     2650     2658       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

Great clean up :) I left a comment, otherwise LGTM

@carlaKC
Copy link
Contributor Author

carlaKC commented Apr 3, 2025

Pushed fixup for incorrect code for invalid_onion_version that @valentinewallace caught 🙏

Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Feel free to squash!

@@ -1837,7 +1837,7 @@ where
if hop_data.intro_node_blinding_point.is_some() {
return Err(OnionDecodeErr::Relay {
err_msg: "Non-final intro node Trampoline onion data provided to us as last hop",
err_code: INVALID_ONION_BLINDING,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch here!

carlaKC added 3 commits April 3, 2025 13:59
Realm is no longer specified in BOLT04, use the specified version
error instead.
As is, we're returning an `invalid_onion_blinding` error with no
associated data, because the calling code does not attach any data
when we have a `OnionDecodeErr::Relay` error. This is arguably not
the correct error code anyway, as described below, so this commit
updates it to `invalid_onion_payload` which does not require additional
error data.

In this case, we have received a trampoline payload which we believe
should contain details of the next trampoline hop. This information
is missing, so we've received an invalid payload. When a blinding point
point is present in the payload, we're also the introduction node.

BOLT04 indicates that if this `current_path_key` is present and we are
the final hop we should return a regular error (rather than using
`invalid_onion_blinding` when you're the the final hop). Before this
commit, this code was considering our node to be non-final and returning
an error accordingly.

Our role as the final hop is ambiguous here - we are the final hop in
the sphinx packet, but the trampoline packet implies that there should
be more hops to come. Given that the trampoline packet is invalid, we
defer to our position in the sphinx packet and send back an invalid
payload error instead.
Although the specification allows an all-zero sha256_of_onion for
invalid_onion_blinding errors, it still requires that the value is set.
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Thanks

@TheBlueMatt TheBlueMatt merged commit 8ee85bf into lightningdevkit:main Apr 4, 2025
24 of 27 checks passed
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.

5 participants