chore: simplify fn prune_messages_to_be_saved #414
Conversation
quake
force-pushed
the
quake/refactor-prune_messages_to_be_saved
branch
from
ab9c88a to
1a2ff8f
Compare
| } | ||
| } | ||
| let mut sorted_messages = complete_messages.into_iter().collect::<Vec<_>>(); | ||
| sorted_messages.sort_unstable(); |
There was a problem hiding this comment.
This seems to use the Ord implementation of BroadcastMessageWithTimestamp, which will sort messages according to their timestamp. It is possible that the timestamp of ChannelUpdate is smaller than that of Channel announcement (we didn't have code to specifically make the timestamp of ChannelUpdate later than the timestamp of the funding transaction block, we just use the system time).
There was a problem hiding this comment.
will order by message_id first:
Lines 2435 to 2441 in 1a2ff8f
There was a problem hiding this comment.
Sorry. I didn't look carefully. In that case, this PR LGTM. I created a few tests for the dependency of channel gossip messages in #418 . Can you take a look and check if the code change here does not break the tests (I believe it doesn't. Still, it is better to
have some tests)?
quake
force-pushed
the
quake/refactor-prune_messages_to_be_saved
branch
from
1a2ff8f to
c950ba1
Compare
| let messages_to_be_saved = take(&mut self.messages_to_be_saved); | ||
| let (complete_messages, uncomplete_messages) = messages_to_be_saved | ||
| .into_iter() | ||
| .partition(|m| self.has_dependencies_available(m)); |
There was a problem hiding this comment.
There is a subtle bug here. Because we replaces messages_to_be_saved in line 1021, the line 1024 will always return false for ChannelUpdate messages that do not have corresponding ChannelAnnouncement in the store. Previously, the behavior is to also check corresponding ChannelAnnouncement in messages_to_be_saved. This bug is fixed in contrun@8ca34e0 . I will create a PR later.
| BroadcastMessageID::NodeAnnouncement(pubkey2), | ||
| ) => pubkey1.cmp(pubkey2), | ||
| (BroadcastMessageID::ChannelUpdate(_), _) => Ordering::Less, | ||
| (BroadcastMessageID::NodeAnnouncement(_), _) => Ordering::Greater, |
There was a problem hiding this comment.
Did you intend to order messages by making NodeAnnouncement in the first place, ChannelAnnouncement the second place and ChannelUpdate the last place (this way the sorted messages in the list messages_to_be_saved can be processed sequentially)? If that is the case, I think the correct Ordering should be contrun@ea5c1fe. This problem is not discovered in the test because another bug above always excluding ChannelUpdate from being included in the complete messages when there is a corresponding ChannelAnnouncement in the messages_to_be_saved.
The current implementation of prune_messages_to_be_saved is complex and unintuitive, it can be implemented in a much simpler way.