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

Respond to a PING even if the source doesn't match the ENR #184

Merged
merged 19 commits into from
Jun 21, 2023
Merged

Respond to a PING even if the source doesn't match the ENR #184

Changes from 3 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f133400
Extract ping request from message
ackintosh May 17, 2023
0d56ee1
Respond PING request using one-time session
ackintosh May 19, 2023
7841d1d
cargo fmt
ackintosh May 24, 2023
e49dc65
Fix typo
ackintosh May 30, 2023
394f93e
Avoid macros for readability
ackintosh May 30, 2023
ed69f2c
Move the session failure up
ackintosh May 30, 2023
e6d95bc
Store one session per node
ackintosh Jun 3, 2023
fce0fce
Tweak comment
ackintosh Jun 3, 2023
4960465
Fix clippy warnings
ackintosh Jun 3, 2023
aacf2ca
Don't update the time when access one-time sessions
ackintosh Jun 3, 2023
d88590d
Add parameters for one-time session
ackintosh May 28, 2023
3a70c74
Merge branch 'master' into pong-even-if-src-not-match
ackintosh Jun 10, 2023
47b32c4
Merge remote-tracking branch 'upstream/master' into pong-even-if-src-…
ackintosh Jun 10, 2023
ad68d9b
Merge remote-tracking branch 'upstream/master' into pong-even-if-src-…
ackintosh Jun 13, 2023
d056db2
Add test for remove_one_time_session
ackintosh Jun 15, 2023
af4f804
Update src/handler/mod.rs
ackintosh Jun 19, 2023
dcba087
Move the one-time session related configs to be constants
ackintosh Jun 20, 2023
d6a43ce
Remove the one-time-session if failed to report the request
ackintosh Jun 20, 2023
11de4b2
Fix tests
ackintosh Jun 20, 2023
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
66 changes: 56 additions & 10 deletions src/handler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,8 @@ pub struct Handler {
active_challenges: HashMapDelay<NodeAddress, Challenge>,
/// Established sessions with peers.
sessions: LruTimeCache<NodeAddress, Session>,
/// Established sessions with peers for a specific request.
one_time_sessions: LruTimeCache<(NodeAddress, RequestId), Session>,
ackintosh marked this conversation as resolved.
Show resolved Hide resolved
/// The channel to receive messages from the application layer.
service_recv: mpsc::UnboundedReceiver<HandlerIn>,
/// The channel to send messages to the application layer.
Expand Down Expand Up @@ -281,6 +283,8 @@ impl Handler {
config.session_timeout,
Some(config.session_cache_capacity),
),
// TODO: config
one_time_sessions: LruTimeCache::new(Duration::from_secs(30), Some(50)),
active_challenges: HashMapDelay::new(config.request_timeout),
service_recv,
service_send,
Expand Down Expand Up @@ -515,17 +519,28 @@ impl Handler {
node_address: NodeAddress,
response: Response,
) {
macro_rules! send {
($session: ident) => {
// Encrypt the message and send
let packet = match $session.encrypt_message::<P>(self.node_id, &response.encode()) {
Ok(packet) => packet,
Err(e) => {
warn!("Could not encrypt response: {:?}", e);
return;
}
};
self.send(node_address, packet).await;
};
}

// Check for an established session
if let Some(session) = self.sessions.get_mut(&node_address) {
// Encrypt the message and send
let packet = match session.encrypt_message::<P>(self.node_id, &response.encode()) {
Ok(packet) => packet,
Err(e) => {
warn!("Could not encrypt response: {:?}", e);
return;
}
};
self.send(node_address, packet).await;
send!(session);
} else if let Some(mut session) = self
.one_time_sessions
.remove(&(node_address.clone(), response.id.clone()))
{
send!(session);
ackintosh marked this conversation as resolved.
Show resolved Hide resolved
} else {
// Either the session is being established or has expired. We simply drop the
// response in this case.
Expand Down Expand Up @@ -782,7 +797,7 @@ impl Handler {
ephem_pubkey,
enr_record,
) {
Ok((session, enr)) => {
Ok((mut session, enr)) => {
// Receiving an AuthResponse must give us an up-to-date view of the node ENR.
// Verify the ENR is valid
if self.verify_enr(&enr, &node_address) {
Expand Down Expand Up @@ -813,6 +828,37 @@ impl Handler {
// established. If so process them.
self.send_next_request::<P>(node_address).await;
} else {
// Respond to PING request even if the ENR or NodeAddress don't match
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here if we store just one one-time-session per peer, we would want to check first for one such session being already established before doing the decode

Copy link
Member Author

Choose a reason for hiding this comment

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

we would want to check first for one such session being already established

You mean we would want to move the second statement of below (else if ... self.remove_one_time_session() ...) to the top of the if statement?

discv5/src/handler/mod.rs

Lines 522 to 535 in fce0fce

// Check for an established session
let packet = if let Some(session) = self.sessions.get_mut(&node_address) {
session.encrypt_message::<P>(self.node_id, &response.encode())
} else if let Some(mut session) = self.remove_one_time_session(&node_address, &response.id)
{
session.encrypt_message::<P>(self.node_id, &response.encode())
} else {
// Either the session is being established or has expired. We simply drop the
// response in this case.
return warn!(
"Session is not established. Dropping response {} for node: {}",
response, node_address.node_id
);
};

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, this is what I mean

Copy link
Member Author

Choose a reason for hiding this comment

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

My slight concern about doing that is, the second statement (if ... self.remove_one_time_session() ...) is false in most cases. Establishing a one-time session is under limited conditions, so the result of the second statement should be false in most send_response() calls.

Do you have a good reason for moving the second statement to the first?

Copy link
Collaborator

Choose a reason for hiding this comment

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

My reasoning is that is not impossible to have a one-time session created for a peer, then a normal session created, and then hit this line. A bit of a race condition. Moving the check up would prevent this. It's a matter of correctness but I understand your pov as a matter of performance. What are your thoughts on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

I see your point. Your suggestion is right in terms of correctness.

a one-time session created for a peer, then a normal session created, and then hit this line

Yeah it looks a bit of a race condition but I think that if a normal session created, it's fine to use the normal session even if a one-time session (for the request) exists since the normal session is valid as well. The one-time session will be evicted over time without being used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AgeManning do you have any opinion here?

Copy link
Member

Choose a reason for hiding this comment

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

Not really.
Its a single hash lookup that we are worried about. I think if we have a session, we shouldn't establish a one-time session. But there is a race condition where a session gets created after. So I think its a relatively rare case that we have both sessions. In this case, we probably want to prioritise the normal session over the one-time session.

Seeing as we are probably running this a bit, I agree with @ackintosh in that it's nicer to avoid the hash lookup (i.e check and remove the one-time session) to handle this edge case. It's also no real loss because the one-time sessions only last 30 seconds (assuming they get pruned).

But imo we are debating over a single hash lookup vs correctness, so i'm really on the fence.

Maybe just leave it as @ackintosh has it, because we don't have to do another commit?
I don't feel strongly at all, if either one of you do, happy to go that way.

Copy link
Collaborator

Choose a reason for hiding this comment

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

also of no strong opinion so merged 🤷 seems we are all happy so

// so that the source node can notice its external IP address has been changed.
let maybe_ping_request = match session.decrypt_message(
message_nonce,
message,
authenticated_data,
) {
Ok(m) => match Message::decode(&m) {
Ok(Message::Request(request)) if request.msg_type() == 1 => {
Some(request)
}
_ => None,
},
_ => None,
};
if let Some(reqeust) = maybe_ping_request {
ackintosh marked this conversation as resolved.
Show resolved Hide resolved
debug!(
"Responding PING request using one-time session. node_address: {}",
ackintosh marked this conversation as resolved.
Show resolved Hide resolved
node_address
);
self.one_time_sessions
.insert((node_address.clone(), reqeust.id.clone()), session);
if let Err(e) = self
.service_send
.send(HandlerOut::Request(node_address.clone(), Box::new(reqeust)))
.await
{
warn!("Failed to report request to application {}", e)
}
}

ackintosh marked this conversation as resolved.
Show resolved Hide resolved
// IP's or NodeAddress don't match. Drop the session.
warn!(
"Session has invalid ENR. Enr sockets: {:?}, {:?}. Expected: {}",
Expand Down