-
Notifications
You must be signed in to change notification settings - Fork 132
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
Enclave avoid duplicated Leaders #1760
Enclave avoid duplicated Leaders #1760
Conversation
It is not tested still. I wanted to add some considerations here first. |
jormungandr/src/secure/enclave.rs
Outdated
use std::sync::Arc; | ||
use tokio02::sync::RwLock; | ||
|
||
#[derive(Clone)] | ||
pub struct Enclave { | ||
leaders: Arc<RwLock<BTreeMap<LeaderId, Leader>>>, | ||
added_cache: Arc<RwLock<HashMap<String, LeaderId>>>, |
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 used String
but maybe we could use PublicKey
instead, I wanted to go ahead and used the easy way first but if PublicKey
is hashable should be fine also.
I added some tests to check that the method behaves as expected. |
I need to update the tests for the last commit |
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.
Only one lock max for this, the double locking will clearly rick having a deadlock on the operations here
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.
looking better, no risk of deadlock now
just a few sanity changes and it is good to go
jormungandr/Cargo.toml
Outdated
@@ -41,6 +41,7 @@ network-core = { path = "../chain-deps/network-core" } | |||
network-grpc = { path = "../chain-deps/network-grpc" } | |||
poldercast = "0.11.2" | |||
rand = "0.7" | |||
rand_core = "0.5" |
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.
this is only used for the test, move this to the dev-dependency
jormungandr/Cargo.toml
Outdated
[build-dependencies] | ||
versionisator = "1.0.2" | ||
|
||
|
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.
non necessary trailing line
jormungandr/src/secure/enclave.rs
Outdated
// leaders: Arc<RwLock<BTreeMap<LeaderId, Leader>>>, | ||
// added_leaders_cache: Arc<RwLock<HashMap<String, LeaderId>>>, |
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.
remove deadcode
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.
Yep, sorry for this.
Added add different leaders test
655c001
to
ec504fa
Compare
This should be ready now @NicolasDP , thanks! |
Should fix #1498