Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent storing non-contactable ENRs #246

Merged
merged 2 commits into from
Apr 9, 2024
Merged

Prevent storing non-contactable ENRs #246

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff25c4d
Prevent storing non-contactable ENRs
AgeManning Apr 2, 2024
5a863be
fmt
AgeManning Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 31 additions & 7 deletions src/service.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -886,7 +886,13 @@ impl Service {
};
self.send_rpc_request(active_request);
}
self.connection_updated(node_id, ConnectionStatus::PongReceived(enr));
// Only update the routing table if the new ENR is contactable
if self.ip_mode.get_contactable_addr(&enr).is_some() {
self.connection_updated(
node_id,
ConnectionStatus::PongReceived(enr),
);
}
}
}
}
Expand Down Expand Up @@ -1188,16 +1194,19 @@ impl Service {
return false;
}

// If any of the discovered nodes are in the routing table, and there contains an older ENR, update it.
// If there is an event stream send the Discovered event
if self.config.report_discovered_peers {
self.send_event(Event::Discovered(enr.clone()));
}

// ignore peers that don't pass the table filter
if (self.config.table_filter)(enr) {
let key = kbucket::Key::from(enr.node_id());

// Check that peers are compatible to be included into the routing table. They must:
// - Pass the table filter
// - Be contactable
//
// Failing this, they are not added, and if there is an older version of them in our
// table, we remove them.
let key = kbucket::Key::from(enr.node_id());
if (self.config.table_filter)(enr) && self.ip_mode.get_contactable_addr(enr).is_some() {
// If the ENR exists in the routing table and the discovered ENR has a greater
// sequence number, perform some filter checks before updating the enr.

Expand All @@ -1221,7 +1230,22 @@ impl Service {
}
}
} else {
return false; // Didn't pass the table filter remove the peer
// Is either non-contactable or didn't pass the table filter. If it exists in the
// routing table, remove it.
match self.kbuckets.write().entry(&key) {
kbucket::Entry::Present(entry, _) if entry.value().seq() < enr.seq() => {
entry.remove()
}
kbucket::Entry::Pending(mut entry, _) => {
if entry.value().seq() < enr.seq() {
entry.remove()
}
}
_ => {}
}
Comment on lines +1235 to +1245
Copy link
Member

@jxs jxs Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:
is there be a situation for legit storing a non contactable ENR?
If not can we (instead) panic if it exists inkbuckets` while on debug, as we do in

discv5/src/service.rs

Line 598 in 6707bcf

debug_unreachable!("Stored ENR is not contactable. {}", enr);

something like:

#[cfg(debug_assertions)]
{
    match self.kbuckets.write().entry(&key) {
        kbucket::Entry::Present(entry, _) if entry.value().seq() < enr.seq() => {
            panic!("ENR shouldn't have been stored");
        }
        kbucket::Entry::Pending(mut entry, _) => {
            if entry.value().seq() < enr.seq() {
                panic!("ENR shouldn't have been stored");
            }
        }
        _ => {}
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entry here is about finding a new ENR.

So we could have an ENR that is contactable and we store it in the routing table. Then later, we find they have changed their ENR to something non-contactable and we see an updated version. Because the key in the table is the node-id, it can be that the earlier version was contactable but the new one is not. So we remove the old one here and don't store the new one.

I can't wrap this in a debug only mode, because this is a valid case.

For the sake of speed, I"ll merge this PR and if my analysis here is wrong we can make a future PR to add this in.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarifying age, makes sense


// Didn't pass the requirements remove the ENR
return false;
}

// The remaining ENRs are used if this request was part of a query. If we are
Expand Down
Loading