-
Notifications
You must be signed in to change notification settings - Fork 7
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
Rewrite Task of listening Stellar Messages #545
Changes from 55 commits
cc7c9cc
5ea8b71
9a4b1c4
8b75f56
519b7eb
878470d
67e540c
12779c7
806568d
b110ad1
fe68519
4d3183b
53c88de
8c345e1
2ad0d7f
0cb2927
7e891d4
deba3da
970fac4
0004c3a
a4f2477
7b45cc5
bcc5903
2d664c4
d5a5adf
b8b4387
76fa194
cdb10a4
59aca69
ae50727
4777855
413fda1
6b2905e
2cf9075
1335c98
5b4307c
01046af
be7ba9e
ee156ee
2eecdae
37bc319
5f833d3
c44bac8
3f03be8
6cc05d7
b1800e8
9cc31b5
9d6fe30
23a1f9b
524342e
9b9dfd3
7b9ddda
6e3f7c3
71222e1
458ba4f
3bd0100
e557b44
9cad8e4
e7f3e26
0aa969e
e2b3f02
c88e317
8c07a3d
4d40c33
636b608
ebb96c4
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 |
---|---|---|
|
@@ -35,13 +35,16 @@ impl StellarOverlayConfig { | |
|
||
#[allow(dead_code)] | ||
pub(crate) fn node_info(&self) -> NodeInfo { | ||
self.node_info.clone().into() | ||
NodeInfo::new(&self.node_info) | ||
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. reduce clones |
||
} | ||
|
||
#[allow(dead_code)] | ||
pub(crate) fn connection_info(&self, secret_key: &str) -> Result<ConnectionInfo, Error> { | ||
pub(crate) fn connection_info( | ||
&self, | ||
secret_key_as_string: String, | ||
) -> Result<ConnectionInfo, Error> { | ||
let cfg = &self.connection_info; | ||
let secret_key = SecretKey::from_encoding(secret_key)?; | ||
let secret_key = SecretKey::from_encoding(secret_key_as_string)?; | ||
|
||
let public_key = secret_key.get_public().to_encoding(); | ||
let public_key = std::str::from_utf8(&public_key).unwrap(); | ||
|
@@ -128,9 +131,9 @@ impl ConnectionInfoCfg { | |
/// Returns the `StellarOverlayConnection` if connection is a success, otherwise an Error | ||
pub async fn connect_to_stellar_overlay_network( | ||
cfg: StellarOverlayConfig, | ||
secret_key: &str, | ||
secret_key_as_string: String, | ||
) -> Result<StellarOverlayConnection, Error> { | ||
let conn_info = cfg.connection_info(secret_key)?; | ||
let conn_info = cfg.connection_info(secret_key_as_string)?; | ||
let local_node = cfg.node_info; | ||
|
||
StellarOverlayConnection::connect(local_node.into(), conn_info).await | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -282,7 +282,7 @@ mod test { | |
let cfg = | ||
StellarOverlayConfig::try_from_path(cfg_file_path).expect("should create a config"); | ||
let node_info = cfg.node_info(); | ||
let conn_info = cfg.connection_info(&secret_key).expect("should create a connection info"); | ||
let conn_info = cfg.connection_info(secret_key).expect("should create a connection info"); | ||
// this is a channel to communicate with the connection/config (this needs renaming) | ||
|
||
let connector = Connector::start(node_info.clone(), conn_info.clone()) | ||
|
@@ -333,7 +333,7 @@ mod test { | |
cert: new_auth_cert, | ||
nonce: [0; 32], | ||
}; | ||
connector.set_remote(RemoteInfo::new(&hello)); | ||
connector.set_remote(RemoteInfo::new(hello)); | ||
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. Consume |
||
|
||
assert!(connector.remote().is_some()); | ||
} | ||
|
@@ -357,7 +357,7 @@ mod test { | |
cert: new_auth_cert, | ||
nonce: [0; 32], | ||
}; | ||
connector.set_remote(RemoteInfo::new(&hello)); | ||
connector.set_remote(RemoteInfo::new(hello)); | ||
assert_eq!(connector.remote().unwrap().sequence(), 0); | ||
|
||
connector.increment_remote_sequence().unwrap(); | ||
|
@@ -385,7 +385,7 @@ mod test { | |
cert: new_auth_cert, | ||
nonce: [0; 32], | ||
}; | ||
let remote = RemoteInfo::new(&hello); | ||
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.
|
||
let remote = RemoteInfo::new(hello); | ||
let remote_nonce = remote.nonce(); | ||
connector.set_remote(remote.clone()); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,7 +3,10 @@ use crate::connection::{ | |
Connector, Error, | ||
}; | ||
use substrate_stellar_sdk::{ | ||
types::{AuthenticatedMessage, AuthenticatedMessageV0, HmacSha256Mac, StellarMessage}, | ||
compound_types::LimitedString, | ||
types::{ | ||
AuthenticatedMessage, AuthenticatedMessageV0, ErrorCode, HmacSha256Mac, StellarMessage, | ||
}, | ||
XdrCodec, | ||
}; | ||
|
||
|
@@ -75,3 +78,14 @@ impl Connector { | |
) | ||
} | ||
} | ||
|
||
/// Create our own error to send over to the user/outsider. | ||
pub(crate) fn crate_specific_error() -> StellarMessage { | ||
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 is mostly for lib-related errors, not necessarily caused by Stellar itself. But to communicate this error back to the user/caller, we have to wrap it with |
||
let error = "Stellar Relay Error".as_bytes().to_vec(); | ||
let error = substrate_stellar_sdk::types::Error { | ||
code: ErrorCode::ErrMisc, | ||
msg: LimitedString::new(error).expect("should return a valid LimitedString"), | ||
}; | ||
|
||
StellarMessage::ErrorMsg(error) | ||
} |
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 this is problematic because we are again creating tasks that never yield, potentially causing issues for the scheduling of other tasks.
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.
Or we can use
precheck_signal.recv().await
? this one is async.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.
await
will not yield; it will potentially wait forever. I've yielded instead of the sleep.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 if the receiver
precheck_signal
has already consumed the message then next time callingrecv().await
, it will yield until a new one arrives. Or do you think there will always be a message in the broadcast channel?This implementation is equivalent though. But with
try_recv
we do need to yield that's true.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.
It will only send 1
()
message. See lines 335-337 in clients/vault/src/requests/execution.rs.The first try was to use
recv().await
but the possibility of it waiting forever is there.However.. we also cannot move forward with the task anyway, if the precheck (or the
execute_open_requests
) is not finished.I also would not like to wrap the
recv()
in atimeout()
because open requests might really take longer than the timeout we set. Also, atimeout
is another thread; That's 8 tasks requiring prechecks, meaning 8 additional threads. (See Marcel's comment for the image of these tasks: Issue Cancel Listener, Issue Request Listener, etc.)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.
Readability, reverting to
recv().await
would be better. I would add that back, but we have to risk it awaiting.I think
execute_open_requests()
is reliable enough to be able to send the message fast.