Drop address ordering enforcement in NodeAnnouncement deser #797
Conversation
|
Agreed. I had to comment these exact lines out to get the |
Codecov Report
@@ Coverage Diff @@
## main #797 +/- ##
==========================================
- Coverage 90.84% 90.79% -0.05%
==========================================
Files 44 44
Lines 24073 24064 -9
==========================================
- Hits 21868 21848 -20
- Misses 2205 2216 +11
Continue to review full report at Codecov.
|
It seems many other nodes never bothered to enforce these requirements, so there's little reason that we should either. cc lightning/bolts#842
TheBlueMatt
force-pushed
the
2021-02-no-addr-order
branch
from
b3676c6 to
8dd08bd
Compare
|
It was further pointed out upstream that we really shouldn't be following the old-spec-recommendation to not relay if a message has any excess data, but instead allow some, so I pushed a commit to do so as well. |
TheBlueMatt
force-pushed
the
2021-02-no-addr-order
branch
from
1b33a3e to
d682718
Compare
TheBlueMatt
force-pushed
the
2021-02-no-addr-order
branch
from
d682718 to
d873e72
Compare
| Ok(msg.contents.excess_data.is_empty() && msg.contents.excess_address_data.is_empty()) | ||
| Ok(msg.contents.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY && | ||
| msg.contents.excess_address_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY && | ||
| msg.contents.excess_data.len() + msg.contents.excess_address_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY) |
There was a problem hiding this comment.
Any reason for the above two checks when the additive branch exists?
There was a problem hiding this comment.
Its probably not possible to hit just because of the deserialization limits, but I like to make sure code is obviously overflow-safe :)
| let should_relay = | ||
| msg.excess_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY && | ||
| msg.excess_address_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY && | ||
| msg.excess_data.len() + msg.excess_address_data.len() <= MAX_EXCESS_BYTES_FOR_RELAY; |
|
Note that the spec PR got some pushback (the ordering requirement, in the future, as we add more address types, is important to allow us to read all known versions before we hit unknown ones, though it doesnt really impact us today). I'd still prefer to just merge this and we can revisit when the spec PR gets some more responses as we need to do something and I don't really want to create a whole getter/setter API for this when other implementations dont enforce this on the read side. |
In lightningdevkit#797, we stopped enforcing that read/sent node_announcements had their addresses sorted. While this is fine in practice, we should still make a best-effort to sort them to comply with the spec's forward-compatibility requirements, which we do here in the ChannelManager.
In lightningdevkit#797, we stopped enforcing that read/sent node_announcements had their addresses sorted. While this is fine in practice, we should still make a best-effort to sort them to comply with the spec's forward-compatibility requirements, which we do here in the ChannelManager.
It seems many other nodes never bothered to enforce these
requirements, so there's little reason that we should either.
cc lightning/bolts#842
Its honstly long-since time we drop this check. As noted upstream we should maybe add a check to not relay node announcements which have more than 10 addresses, but this on its own doesn't change the malicious-DDoS attack surface.