Log cases where an onion failure cannot be attributed or interpreted #3629
Conversation
| Ok(p) => p, | ||
| Err(_) => return, | ||
| }; | ||
| let decrypt_result = decrypt_onion_error_packet(&mut encrypted_packet, shared_secret); |
There was a problem hiding this comment.
Check hmac first to distinguish between unreadable failures and hmac mismatches.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3629 +/- ##
========================================
Coverage 89.16% 89.17%
========================================
Files 152 152
Lines 118791 118947 +156
Branches 118791 118947 +156
========================================
+ Hits 105921 106068 +147
- Misses 10312 10316 +4
- Partials 2558 2563 +5 ☔ View full report in Codecov by Sentry. |
|
Coverage reports suggests more test coverage needed... |
joostjager
force-pushed
the
log-attribution-failures
branch
5 times, most recently
from
56b0aa7 to
70c8e56
Compare
| is_permanent: true, | ||
| }); | ||
| let short_channel_id = Some(route_hop.short_channel_id); | ||
| res = Some(FailureLearnings { |
There was a problem hiding this comment.
This is a bug fix. Previously unreadable failures weren't attributable even though the hmac checked out.
joostjager
force-pushed
the
log-attribution-failures
branch
from
70c8e56 to
47caa1e
Compare
| fn build_test_path() -> Path { | ||
| Path { | ||
| hops: vec
| } | ||
|
|
||
| fn test_failure_attribution(packet: &[u8]) -> DecodedOnionFailure { | ||
| let logger: Arc<TestLogger> = Arc::new(TestLogger::new()); |
There was a problem hiding this comment.
I dunno if you wanna bother, but note that the TestLogger does allow you to inspect the things which were logged so you could check if the new logs were hit (though in general we try to avoid test-by-log-inspection where possible because they're just annoying to update when logs change and they tend to be more brittle...but for a unit test its more fine than a functional one).
|
|
||
| let network_update = Some(NetworkUpdate::NodeFailure { | ||
| node_id: route_hop.pubkey, | ||
| is_permanent: true, |
There was a problem hiding this comment.
IIRC is_permanent NodeFailures result in removing the node from the graph entirely, which seems a bit harsh, I think?
There was a problem hiding this comment.
The same was already done for a missing failure code, so it seemed reasonable to apply the same penalty for the more severe case where the message cannot even be decoded?
Create more visibility into these edge cases. The non-attributable failure in particular can be used to disrupt sender operation and it is therefore good to at least log these cases clearly.
Additionally a bug is fixed where an unreadable failure with valid hmac wasn't properly attributed to a node.