- 
                Notifications
    You must be signed in to change notification settings 
- Fork 421
Fix some onion errors and assert their length is correct #1895
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
Fix some onion errors and assert their length is correct #1895
Conversation
`impl_writeable_tlv_based_enum` shouldn't be assuming that `DecodeError` is in scope, which we address here.
7e29b27    to
    db345a5      
    Compare
  
    | I think this was it, so  | 
| Codecov ReportBase: 90.69% // Head: 90.63% // Decreases project coverage by  
 
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #1895      +/-   ##
==========================================
- Coverage   90.69%   90.63%   -0.06%     
==========================================
  Files          91       91              
  Lines       48404    51171    +2767     
  Branches    48404    51171    +2767     
==========================================
+ Hits        43898    46380    +2482     
- Misses       4506     4791     +285     
 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. | 
db345a5    to
    99c91bd      
    Compare
  
    | Ah, thanks, fixed. | 
99c91bd    to
    22d94a8      
    Compare
  
    | use crate::ln::onion_utils; | ||
| use crate::ln::onion_utils::HTLCFailReason; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| use crate::ln::onion_utils; | |
| use crate::ln::onion_utils::HTLCFailReason; | |
| use crate::ln::onion_utils::{self, HTLCFailReason}; | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the original more readable 🤷
        
          
                lightning/src/ln/onion_utils.rs
              
                Outdated
          
        
      | // we get a fail_malformed_htlc from the first hop | ||
| // TODO: We'd like to generate a NetworkUpdate for temporary | ||
| // failures here, but that would be insufficient as find_route | ||
| // generally ignores its view of our own channels as we provide them via | ||
| // ChannelDetails. | ||
| // TODO: For non-temporary failures, we really should be closing the | ||
| // channel here as we apparently can't relay through them anyway. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these comments still make sense here, or should they stay in fail_backwards_internal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first TODO belongs here I think - it talks about building a NetworkUpdate which this fn is responsible for. The second we could debate but there's already a TODO that is equivalent at the callsite talking about if we blame our own channel.
22d94a8    to
    af89d18      
    Compare
  
    | This test hits one of the new debug panics in  I think the other phantom  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice cleanup, basically LGTM
| // ChannelDetails. | ||
| if let &HTLCSource::OutboundRoute { ref path, .. } = htlc_source { | ||
| (None, Some(path.first().unwrap().short_channel_id), true, Some(*failure_code), Some(data.clone())) | ||
| } else { unreachable!(); } | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd remove some unreachables to make HTLCSource::OutboundRoute have an inner struct, but I think that'd be a future consideration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, we should, agree it doesnt need to happen here.
a663ab2    to
    35d8eb3      
    Compare
  
    35d8eb3    to
    878a41f      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after squash
Now that it's entirely abstracted, there's no reason for `HTLCFailReason` to be in `channelmanager`, it's really an onion-level abstraction.
Now that `HTLCFailReason` is opaque and in `onion_utils`, we should encapsulate it so that `ChannelManager` can no longer directly access its inner fields.
This replaces `final_expiry_too_soon` with `incorrect_or_unknown_payment` as was done in lightning/bolts#608. Note that the rationale for this (that it may expose whether you are the final recipient for the payment or not) does not currently apply to us - we don't apply different final CLTV values to different payments. However, we might in the future, and this will make us slightly more consistent with other nodes.
The spec mandates that we copy the `sha256_hash_of_onion` field from the `UpdateFailMalformedHTLC` message into the error message we send back to the sender, however we simply ignored it. Here we copy it into the message correctly.
When we're constructing an HTLCFailReason, we should check that we set the data to at least the correct length for the given failure code, which we do here.
When we receive a phantom HTLC with a bogus/modified CLTV, we should fail back with `incorrect_cltv_expiry`, but that requires a `channel_update`, which we cannot generate for a phantom HTLC which has no corresponding channel. Thus, instead, we have to fall back to `incorrect_cltv_expiry`. Fixes lightningdevkit#1879
This ensures we always hit our new debug assertions while building failure packets in the immediately-fail pipeline while processing an inbound HTLC.
If we try to send any onion error with the `UPDATE` flag in response to a phantom receipt, we should always swap it for something generic that doesn't require a `channel_update` in it. Here we use `temporary_node_failure`. Test provided by Valentine Wallace <[email protected]>
878a41f    to
    c9fe69f      
    Compare
  
    | Squashed without further changes. | 
When we're constructing an HTLCFailReason, we should check that we
set the data to at least the correct length for the given failure
code, which we do here.
First we move HTLCFailReason into
onion_utilsand encapsulate it a bit better, making channelmanager just a bit smaller. Then we can assert the length, but only after fixing a few issues.Fixes #1879 but I'm frankly a bit confused - I don't see the
14@valentinewallace claimed was being used incorrectly in phantom failures - maybe it was already fixed?