-
Notifications
You must be signed in to change notification settings - Fork 94
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
feat(p2p): ensure time synchronization in the network #2255
base: dev
Are you sure you want to change the base?
Changes from 20 commits
4343c0e
3b69689
5acfe8d
82ae6d2
5771790
f42e706
b693d8c
92af285
86eb068
c1a6eaf
e5c284d
130af67
422626c
3859afc
1c7229c
0691aba
3692673
8487ab1
faeee43
1f2950e
9ad57a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,7 +7,7 @@ use futures::lock::Mutex as AsyncMutex; | |
use http::StatusCode; | ||
use mm2_core::mm_ctx::{from_ctx, MmArc}; | ||
use mm2_err_handle::prelude::*; | ||
use mm2_libp2p::application::request_response::network_info::NetworkInfoRequest; | ||
use mm2_libp2p::application::request_response::peer_info::PeerInfoRequest; | ||
use mm2_libp2p::{encode_message, NetworkInfo, PeerId, RelayAddress, RelayAddressError}; | ||
use serde_json::{self as json, Value as Json}; | ||
use std::collections::{HashMap, HashSet}; | ||
|
@@ -170,16 +170,20 @@ struct Mm2VersionRes { | |
nodes: HashMap<String, String>, | ||
} | ||
|
||
fn process_get_version_request(ctx: MmArc) -> Result<Option<Vec<u8>>, String> { | ||
fn process_get_version_request(ctx: MmArc) -> Result<Vec<u8>, String> { | ||
let response = ctx.mm_version().to_string(); | ||
let encoded = try_s!(encode_message(&response)); | ||
Ok(Some(encoded)) | ||
encode_message(&response).map_err(|e| e.to_string()) | ||
} | ||
|
||
pub fn process_info_request(ctx: MmArc, request: NetworkInfoRequest) -> Result<Option<Vec<u8>>, String> { | ||
log::debug!("Got stats request {:?}", request); | ||
fn process_get_peer_utc_timestamp_request() -> Result<Vec<u8>, String> { | ||
let timestamp = common::get_utc_timestamp(); | ||
encode_message(×tamp).map_err(|e| e.to_string()) | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ...encodes it as a but curious, is the encoding of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why do you want to cast the value again when you can simply encode it?
They are There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
didn't know the encoding for both is compatible, still not sure how the negative side is encoded (probably as rust's then, will checkout). what i am suggesting is, we can encode it as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Theoretically, it should be fine on encoding but should panic on decoding, which is the main purpose on the decoding side (as negative ones are invalid). |
||
|
||
pub fn process_info_request(ctx: MmArc, request: PeerInfoRequest) -> Result<Vec<u8>, String> { | ||
match request { | ||
NetworkInfoRequest::GetMm2Version => process_get_version_request(ctx), | ||
PeerInfoRequest::GetMm2Version => process_get_version_request(ctx), | ||
PeerInfoRequest::GetPeerUtcTimestamp => process_get_peer_utc_timestamp_request(), | ||
} | ||
} | ||
|
||
|
@@ -298,20 +302,17 @@ async fn stat_collection_loop(ctx: MmArc, interval: f64) { | |
let peers: Vec<String> = peers_names.keys().cloned().collect(); | ||
|
||
let timestamp = now_sec(); | ||
let get_versions_res = match request_peers::<String>( | ||
ctx.clone(), | ||
P2PRequest::NetworkInfo(NetworkInfoRequest::GetMm2Version), | ||
peers, | ||
) | ||
.await | ||
{ | ||
Ok(res) => res, | ||
Err(e) => { | ||
log::error!("Error getting nodes versions from peers: {}", e); | ||
Timer::sleep(10.).await; | ||
continue; | ||
}, | ||
}; | ||
let get_versions_res = | ||
match request_peers::<String>(ctx.clone(), P2PRequest::PeerInfo(PeerInfoRequest::GetMm2Version), peers) | ||
.await | ||
{ | ||
Ok(res) => res, | ||
Err(e) => { | ||
log::error!("Error getting nodes versions from peers: {}", e); | ||
Timer::sleep(10.).await; | ||
continue; | ||
}, | ||
}; | ||
|
||
for (peer_id, response) in get_versions_res { | ||
let name = match peers_names.get(&peer_id.to_string()) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,8 @@ | |
//! which are separate from other request types such as RPC requests or Gossipsub | ||
//! messages. | ||
|
||
pub mod network_info; | ||
pub mod ordermatch; | ||
pub mod peer_info; | ||
|
||
use serde::{Deserialize, Serialize}; | ||
|
||
|
@@ -12,10 +12,6 @@ use serde::{Deserialize, Serialize}; | |
pub enum P2PRequest { | ||
/// Request for order matching. | ||
Ordermatch(ordermatch::OrdermatchRequest), | ||
/// Request for network information from the target peer. | ||
/// | ||
/// TODO: This should be called `PeerInfoRequest` instead. However, renaming it | ||
/// will introduce a breaking change in the network and is not worth it. Do this | ||
/// renaming when there is already a breaking change in the release. | ||
NetworkInfo(network_info::NetworkInfoRequest), | ||
Comment on lines
-17
to
-20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This PR already leads to breaking change. It's perfect time to rename this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One way to avoid breaking changes is to have a timeout on the request for peer timestamp and allow them to connect if there is no response from the peer. We later remove this when most peers update. Another way is to get the mm2 version and do the timestamp request depending on the version. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This sounds like a good plan!
I think the first idea was better, but then I need to revert the peer-info renaming part. What about bundling these changes in some branch, and then releasing multiple breaking changes at once (like removing mm2 binaries and some other stuff) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I am fine with this or the first approach. |
||
/// Request various information from the target peer. | ||
PeerInfo(peer_info::PeerInfoRequest), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't wanna be explicit with casting, let's at least leave a comment that i64 will encode just as u64.
it's so confusing seeing one type sent from this end and another one received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that explanation alone makes it more complicated than it should be. I didn't think it would be this confusing 🤔. I would rather cast it to
u64
before sending it instead of explaining it like "we do this because of encoding numbers..."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
me too tbh, i favor explicit casting all the way :/