Attributable errors 2025 #3611
Attributable errors 2025 #3611
Conversation
joostjager
force-pushed
the
attr-errors-2025
branch
from
97480b6 to
72a8397
Compare
|
Okay, you got totally bitten by some really old serialization logic that was written when upgrade/downgrade support was...not super well thought-through (and which was not upgraded when we made the transition, sadly). Luckily there's fairly little of this code left, but you jumped right into an area where there's a good bit of it :(. I went ahead and implemented the serialization logic at https://git.bitcoin.ninja/?p=rust-lightning;a=log;h=refs/heads/2025-02-3611-ser-impl note that the |
joostjager
force-pushed
the
attr-errors-2025
branch
from
d2a3b6a to
936a2b9
Compare
| 4 => InboundHTLCState::LocalRemoved(Readable::read(reader)?), | ||
| 4 => { | ||
| let reason = match <u8 as Readable>::read(reader)? { | ||
| 0 => InboundHTLCRemovalReason::FailRelay(msgs::OnionErrorPacket { |
There was a problem hiding this comment.
Update in holding cell, still waiting for next round of commitment dance. Should be tests that test holding cell persistence.
Strategy: break main, see if tests fail.
There was a problem hiding this comment.
expect_pending_htlcs_forwardable!(nodes[1]); - in here let node 0 do something else like add another htlc, so that the failure gets into the holding cell
lightning/src/ln/channelmanager.rs
Outdated
| 0u8.write(writer)?; | ||
| channel_id.write(writer)?; | ||
| htlc_id.write(writer)?; | ||
| reason.write(writer)?; | ||
| attribution_data.write(writer)?; // TODO: Make TLV? |
There was a problem hiding this comment.
Try remove serialization logic, see where it used.
Look at call site, see what context is, and find how to add more data at a higher level (tlv?)
lightning/src/ln/channelmanager.rs
Outdated
| @@ -12881,6 +12887,7 @@ impl Readable for HTLCFailureMsg { | |||
| channel_id: Readable::read(reader)?, | |||
| htlc_id: Readable::read(reader)?, | |||
| reason: Readable::read(reader)?, | |||
| attribution_data: Readable::read(reader)?, | |||
There was a problem hiding this comment.
change write side to use version 2/3 and it will have the length descriptor. can add additional content at the end.
maybe intro tlv stream here?
joostjager
force-pushed
the
attr-errors-2025
branch
2 times, most recently
from
33c4368 to
dc4b841
Compare
joostjager
force-pushed
the
attr-errors-2025
branch
15 times, most recently
from
fa88d2f to
fbd8e91
Compare
joostjager
force-pushed
the
attr-errors-2025
branch
from
fbd8e91 to
2c07c7b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3611 +/- ##
==========================================
+ Coverage 89.17% 90.24% +1.06%
==========================================
Files 155 156 +1
Lines 119379 126680 +7301
Branches 119379 126680 +7301
==========================================
+ Hits 106462 114326 +7864
+ Misses 10312 9818 -494
+ Partials 2605 2536 -69 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
joostjager
force-pushed
the
attr-errors-2025
branch
3 times, most recently
from
a8316f1 to
06b8979
Compare
| .copy_from_slice(&payloads[..payloads.len() - PAYLOAD_LEN]); | ||
| } | ||
|
|
||
| // Shift hmacs right. |
There was a problem hiding this comment.
Hmac pruning happening here
Signal to senders that the node will return attributable failures. When the sender makes sure all path nodes supports this, they will be able to attribute every failure that may occur.
joostjager
force-pushed
the
attr-errors-2025
branch
from
06b8979 to
e7923e9
Compare
|
Closing this work PR, pushed updated and reworked code to original PR #2256 |
Work PR for reviving #2256