Revert attribution of failures #3817
Conversation
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
joostjager
force-pushed
the
attr-failure-decode-fix
branch
from
9d082a4 to
a75adb3
Compare
joostjager
force-pushed
the
attr-failure-decode-fix
branch
from
a75adb3 to
914ec1a
Compare
Prepares for a DropAttributionData mutation.
joostjager
force-pushed
the
attr-failure-decode-fix
branch
from
914ec1a to
7c7f7bc
Compare
joostjager
force-pushed
the
attr-failure-decode-fix
branch
from
7c7f7bc to
567ac64
Compare
|
Added coverage for dropping attribution data. |
|
🔔 1st Reminder Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review. |
| // Only check attribution when an attribution data failure has not yet occurred. | ||
| if attribution_failed_channel.is_none() { | ||
| // Check attr error HMACs if present. | ||
| if let Some(ref mut attribution_data) = encrypted_packet.attribution_data { |
There was a problem hiding this comment.
Pre-existing, but don't we need to check whether the node supports attributable failures here?
As-is we would:
- Set
attribution_failed_channelfor a node that doesn't support attributable failures - Not set
attribution_failed_channelfor a node that does support attributable failures but didn't include them
There was a problem hiding this comment.
The code is currently written as if there is universal support for it, but then - and this is what this PR changes - we don't act on it (yet).
Not set attribution_failed_channel for a node that does support attributable failures but didn't include them
I think we do now, because when we don't receive attribution_data from the first hop, we blame that hop?
I am not completely sure how to weave the feature bit into this when we have this deployed. If by that time the large majority supports it, maybe the logic as is is fine? If the original failure field is readable, we don't even need to look at attribution_data because the failure is already attributable. So we'd still only make a decision for that random data vulnerability that currently doesn't seem to be exploited.
But interested to hear how you see this.
|
🔔 1st Reminder Hey @TheBlueMatt @carlaKC! This PR has been waiting for your review. |
|
🔔 1st Reminder Hey @carlaKC! This PR has been waiting for your review. |
Until a sufficient number of nodes on the network has upgraded, it is not possible to use attribution data to assign blame. Otherwise nodes that have not upgraded yet would already receive penalties.
Pointed out by @GeorgeTsagk