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.

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

P2P use round trip of time_message to calculate latency #1243

Merged
merged 24 commits into from
Jun 12, 2023

Conversation

heifner
Copy link
Member

@heifner heifner commented Jun 7, 2023

Address peer review comment: #1215 (comment)

Use round trip time for time_message which was already implemented. Since this already did a round trip for previous versions of P2P, no new P2P protocol version is needed. All that was needed was to add the calculation and normalize epoch time. Previous versions of leap did not specify duration resolution of time_since_epoch. Some clients returned microsecond values and others returned nanosecond values. Included in this PR is a fix so that all nodeos going forward will return nanosecond values even if the resolution is only microseconds.

The new latency calculation aggravated some issues introduced by #1215 and that have been around since 4.0, see #1240 which this PR should also fix.

Resolves #1072

@greg7mdp greg7mdp self-requested a review June 7, 2023 17:03
@heifner heifner added the OCI Work exclusive to OCI team label Jun 7, 2023
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
void connection::handle_message( const time_message& msg ) {
peer_ilog( this, "received time_message" );
peer_dlog( this, "received time_message: ${t}", ("t", msg) );
Copy link
Member

Choose a reason for hiding this comment

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

Like the change. I was wondering why time_message deserved info log while looking at log files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too!

plugins/net_plugin/net_plugin.cpp Outdated Show resolved Hide resolved
@greg7mdp

This comment was marked as resolved.

@heifner

This comment was marked as resolved.

@greg7mdp

This comment was marked as resolved.

@greg7mdp

This comment was marked as resolved.

@heifner

This comment was marked as resolved.

@heifner

This comment was marked as resolved.

@greg7mdp

This comment was marked as resolved.

heifner added 2 commits June 9, 2023 11:37
… not noticed before because handshake would provide it eventually.
@heifner heifner merged commit c2026ce into main Jun 12, 2023
@heifner heifner deleted the GH-1072-send-time-latency branch June 12, 2023 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCI Work exclusive to OCI team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

P2P Improve sync behavior
3 participants