This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

Improve CRDT convergence #438

Merged

garious merged 3 commits into solana-labs:master from carllin:CRDTConvergence

Jun 26, 2018

Contributor

carllin commented Jun 25, 2018 •

edited by garious

Loading

Instead of randomly choosing a peer to get gossip updates from, this PR adds an option to weight that choice by stake.

Fixes #302

carllin added 2 commits

June 25, 2018 03:56


          added remote table to update respones

d1093d3


          ran linter

2611a90

garious added the CI label

solana-grimes removed the CI label

aeyakovenko requested review from pgarg66 and aeyakovenko

June 25, 2018 14:38

Member

aeyakovenko commented Jun 25, 2018

@carllin @pgarg66 I think we need a testbed for this stuff. like running 1k+ crdt nodes and verifying convergence on 100 machine instances.

Contributor

pgarg66 commented Jun 25, 2018

Agree. I am trying to create a test bed on GCP for overall network testing.
@carllin, I'll keep you in loop. Any suggestions are welcome.

garious reviewed

View reviewed changes

Contributor

garious left a comment

This is great stuff! I offer lots of nits here, but please don't assume that implies I don't like the PR. I had an easy time following your code - thanks so much! Let's polish this one up and get it merged!

src/choose_gossip_peer_strategy.rs Outdated


		let result = weighted_strategy.calculate_weighted_remote_index(key1);

		// If nobody has seen a newer update then rever to default

Contributor

garious Jun 25, 2018

revert

src/choose_gossip_peer_strategy.rs

		@@ -0,0 +1,306 @@
		use crdt::ReplicatedData;

Contributor

garious Jun 25, 2018

Can you add module documentation?

src/choose_gossip_peer_strategy.rs Outdated

+              pub const DEFAULT_WEIGHT: u32 = 1;
+              pub trait ChooseGossipPeerStrategy {
+                  fn choose_peer(&self, options: Vec<&ReplicatedData>) -> Result<ReplicatedData>;

Contributor

garious Jun 25, 2018

Are you able to return a reference here to avoid the clone()?

Contributor

garious Jun 25, 2018

Also, @aeyakovenko, what do you think about the use of the term "peer" here? Should we consider replacing ReplicatedData with Peer?

Contributor Author

carllin Jun 26, 2018 •

edited

Loading

@garious, yeah I thought the reference would have been fine, but in the original gossip_request() code, there was a clone(), so I aligned with that. https://github.com/solana-labs/solana/blob/master/src/crdt.rs#L520. Looking at it more in depth now, it doesn't seem necessary.

src/choose_gossip_peer_strategy.rs Outdated

+                  fn choose_peer(&self, options: Vec<&ReplicatedData>) -> Result<ReplicatedData>;
+              }
+              pub struct ChooseRandomPeerStrategy<'a> {

Contributor

garious Jun 25, 2018

Can you add a comment that summarizes this strategy and its limitations?

src/choose_gossip_peer_strategy.rs Outdated

+                  }
+              }
+              pub struct ChooseWeightedPeerStrategy<'a> {

Contributor

garious Jun 25, 2018

Can you add a comment that summarizes this strategy and how it overcomes the limitations of ChooseRandomPeerStrategy?

tests/data_replicator.rs Outdated

+                  trace!("waiting to converge:");
+                  let mut done = false;
+                  for _ in 0..30 {
+                      done = c1.read().unwrap().table.len() == 3 && c2.read().unwrap().table.len() == 3

Contributor

garious Jun 25, 2018

Old version of cargo fmt - this will fail CI. If you rustup update stable, it'll automatically update rustfmt along with the compiler.

tests/data_replicator.rs Outdated

+                      // Make sure liveness table entry contains correct result for c2
+                      let c2_index_result_for_c4 = liveness_map.get(&c2_id);
+                      assert!(c2_index_result_for_c4.is_some());
+                      assert!(*(c2_index_result_for_c4.unwrap()) == c2_index_for_c4);

Contributor

garious Jun 25, 2018

assert_eq!

tests/data_replicator.rs Outdated

+                      // Make sure liveness table entry contains correct result for c3
+                      let c3_index_result_for_c4 = liveness_map.get(&c3_id);
+                      assert!(c3_index_result_for_c4.is_some());
+                      assert!(*(c3_index_result_for_c4.unwrap()) == c3_index_for_c4);

Contributor

garious Jun 25, 2018

assert_eq!

tests/data_replicator.rs Outdated

+                  threads.extend(dr2.thread_hdls.into_iter());
+                  threads.extend(dr3.thread_hdls.into_iter());
+                  for t in threads.into_iter() {

Contributor

garious Jun 25, 2018

No need for that into_iter()

tests/data_replicator.rs Outdated

+                  threads.extend(dr1.thread_hdls.into_iter());
+                  threads.extend(dr4.thread_hdls.into_iter());
+                  for t in threads.into_iter() {

Contributor

garious Jun 25, 2018

No need for that into_iter()

garious changed the title ~~Issue #302: Crdt convergence~~ Improve CRDT convergence

Contributor

garious commented Jun 25, 2018

I took a stab at updating the title and description. Feel free to update that again. A description that explains how the resulting system is somehow "better" would be helpful context for anyone else reviewing the PR. The ticket says "Improve convergence", but in what way? More secure? Faster? Perhaps this is a question for @aeyakovenko.

Member

aeyakovenko commented Jun 25, 2018 •

edited

Loading

@garious @carllin ideally we would have a test that can measure how fast information propagates through the network.

membership of a new node
old node leaving the network
update to a RD structure (NodeNetworkInfo maybe?). Lets rename after 0.7.0 is out
how stable 1 and 2 are during churn

Contributor

garious commented Jun 25, 2018

@aeyakovenko, in that list of metrics, which would you assume would change as a result of this PR?

Member

aeyakovenko commented Jun 25, 2018

in theory all 3, but we need some tests to experiment with this :). @carllin @pgarg66 how is your knowledge of http://man7.org/linux/man-pages/man8/tc-netem.8.html

I think we can sim gossip on a large set of nodes on just a single big machine with lots of cpu cores. since all the threads should only be doing network IO.

Contributor

pgarg66 commented Jun 25, 2018

I've used netem in past. I was planning to use it to induce network errors.

Contributor Author

carllin commented Jun 26, 2018

I haven't used netem before, but would sign up as a willing disciple/guinea pig of the wise one: @pgarg66


          Some pull request fixes(linting + documentation)

4c1f256

garious added the CI label

solana-grimes removed the CI label

garious merged commit 551f639 into solana-labs:master

Contributor

garious commented Jun 26, 2018

Thanks a bunch @carllin. Let's keep this going. Great code, great direction. Now the challenge is finding ways to measure all the value you're adding.

vkomenda pushed a commit to vkomenda/solana that referenced this pull request


          Allow source to be owner/close_authority in CloseAccount (solana-labs…

6c9e8ac

…#438)

* Add failing tests

* No need to hold source RefMut in process_close_account and process_toggle_freeze_account

vkomenda pushed a commit to vkomenda/solana that referenced this pull request


          backport solana-labs#438 to v2

36ec1b2

Kashuhackerone mentioned this pull request

[Snyk] Fix for 1 vulnerabilities Kashuhackerone/solana#60

Open

alessandrod pushed a commit to alessandrod/solana that referenced this pull request


          [keygen] Remove deprecated functions from keygen cli (minus grind…

b1919bd

… command) (solana-labs#438)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet