Update their_keys on consensus about a section info #1716

fizyk20 · 2019-07-23T12:34:50Z

Closes #1671

jeanphilippeD

Looks mostly reasonable, but I'm not clear how the edge cases will behave.

src/chain/shared_state.rs

jeanphilippeD · 2019-07-23T13:16:57Z

src/chain/shared_state.rs

+        let mut state = SharedState::new(sec_0);
+
+        // add keys for neighbours 10 and 11
+        let pk_sec_10 = gen_pk(pfx_10);


What happen if we do not have pfx_10 or pfx_11?
What happen if we only have our side of the prefix when starting with pfx_0 (and we had a key when we were empty prefix).

What do you mean by "our side of the prefix"?
Also note that this starts with not having 10 or 11 in their_keys - they are simply inserted, then.

I meant something like.

Start empty prefix.
Split -> Prefix0 & Prefix1
We are Prefix0, we do not have key for Prefix1?

(not sure it is possible, I guess we would have both key at the time of the split and so we would also always add Prefix1?).

I think the plan was that we'll have both keys from the DKGs we were running when preparing for the split (we'll have the private key share only from one of them, but we'll be able to obtain PublicKeySets from both).

So I guess the initial state should be:

// our section is 0, so when we splited we have already added Prefix 1. let sec_0 = gen_section_info(pfx_0); let mut state = SharedState::new(sec_0); let pk_sec_1 = gen_pk(pfx_1); state.update_their_keys(pfx_1, pk_sec_1.clone());

Could be. But I think the one in the test is also valid - we might have just become an elder of 0, and 10 and 11 might have already existed. We would then initialise the chain in a manner similar to what is in the test.

i'm not 100% sure that is the case, that bring a new thing to be careful about:
Are we doing the right thing when a new node join? (i.e is all the information necessary in Parsec).
Or after a split when I guess we are re-voting for all these sections info so new joining nodes get the right thing. Do we have the section re-consensused in order?

i.e should

// Update their_keys in chain self.chain.update_their_keys( *sec_info.prefix(), BlsPublicKey::from_section_info(&sec_info), );

Take care of version number? Of parents consensused after children prefix?

Are we doing the right thing when a new node join? (i.e is all the information necessary in Parsec).

I'm not sure if it is there, but we certainly were planning for it to be there - the whole shared state is supposed to be included in the Genesis event eventually (maybe it already is, I'm not sure).

So a joining node should receive the initial shared state from the Genesis event, and then any updates would result from other consensused events, which it would receive as well.

src/chain/shared_state.rs

jeanphilippeD

Everything looks good, just a comment that will probably be relevant for the future.

jeanphilippeD · 2019-07-24T11:30:38Z

src/chain/shared_state.rs

+
+    #[test]
+    fn single_prefix_multiple_updates() {
+        update_keys_and_check("0", vec!["1", "1", "1", "1"], vec![("1", 3)]);


The fact that we keep the last that we see suggest that we have to make sure not to add a old one after the new one.
i think I mention this somewhere, but we will probably need to care about version number at some point.
(Nothing to do for this PR.)

maqi · 2019-07-24T14:21:17Z

src/chain/shared_state.rs

+        if let Some(&pfx) = self
+            .their_keys
+            .keys()
+            .find(|pfx| pfx.is_compatible(&prefix))


say the incoming prefix is 01, and we have 010 and 011, will this compatible check only make 010 to be updated?
or, we shall only keep one of the 010 or 011, due to the neighbouring rule ?

We should actually remove both 010 and 011 in such a case - but this situation can only happen if there is a merge, and we explicitly don't cater to merges for now. I could work on making this algorithm merge-safe, though, if we think it makes sense.

Not sure about the plan of merge though. maybe @jeanphilippeD will be better to decide whether shall be merge-safe or not.
Or maybe at least put a comment to remind?

I'm OK with having a very short comment here that this will not handle merging prefixes.

maqi · 2019-07-24T14:34:44Z

src/chain/shared_state.rs

+            let mut current_pfx = prefix.sibling();
+            while !self.their_keys.contains_key(&current_pfx) && current_pfx != old_pfx_sibling {
+                let _ = self.their_keys.insert(current_pfx, old_key.clone());
+                current_pfx = current_pfx.popped().sibling();


why a sibling after pop ?
is a pop enough?

Because we want to cover the whole address space that was covered by the old prefix.

Say we have a key for prefix 1 and receive one for prefix 1000. This means that 1 split at some point, but we don't know what the other sections are. We decide to just cover the space of prefix 1 with shortest possible prefixes, such that one of them is 1000 - this means we have to create entries for prefixes 1000, 1001, 101 and 11. So we have:

1000 → sibling → 1001

1001 → popped → 100 → sibling → 101

101 → popped → 10 → sibling → 11

hmm...
does the with shortest possible prefixes from 1000 to 1 is : 1000 -> 100 -> 10 -> 1 ?

But we don't want to just go from 1000 to 1. We want prefixes that cover the whole address space that 1 was covering. Basically, we want a set of potential prefixes that 1 could have split into, provided that one of them is 1000.

If prefix 1000 exists on the network, then neither 1, nor 10, nor 100 can exist. But 11 can exist, and so can 101 and 1001.

maqi · 2019-07-24T14:36:39Z

src/chain/shared_state.rs

+    }
+
+    #[cfg(test)]
+    /// Returns the relevant entry in their_keys if it exists


this function actually gives out their_keys if have ?
the comment of returns the relevant entry indicates it shall be something like get_their_keys(prefix) ?

Right, the old version of the function did that, but I forgot to change the comment. Good catch 👍

jeanphilippeD

Great

fizyk20 requested a review from jeanphilippeD July 23, 2019 12:37

jeanphilippeD reviewed Jul 23, 2019

View reviewed changes

fizyk20 force-pushed the issue-1671 branch from 593f98b to 7cddfa0 Compare July 23, 2019 16:45

jeanphilippeD reviewed Jul 23, 2019

View reviewed changes

src/chain/shared_state.rs Outdated Show resolved Hide resolved

jeanphilippeD reviewed Jul 23, 2019

View reviewed changes

src/chain/shared_state.rs Outdated Show resolved Hide resolved

fizyk20 force-pushed the issue-1671 branch from 8694761 to f1b6673 Compare July 24, 2019 11:26

jeanphilippeD previously approved these changes Jul 24, 2019

View reviewed changes

fizyk20 requested a review from maqi July 24, 2019 11:37

fizyk20 dismissed jeanphilippeD’s stale review via b3b77dd July 24, 2019 11:38

fizyk20 force-pushed the issue-1671 branch from f1b6673 to b3b77dd Compare July 24, 2019 11:38

jeanphilippeD previously approved these changes Jul 24, 2019

View reviewed changes

maqi reviewed Jul 24, 2019

View reviewed changes

fizyk20 dismissed jeanphilippeD’s stale review via 0048de9 July 24, 2019 15:16

fizyk20 force-pushed the issue-1671 branch from 0048de9 to 189256f Compare July 24, 2019 15:16

jeanphilippeD previously approved these changes Jul 24, 2019

View reviewed changes

maqi previously approved these changes Jul 25, 2019

View reviewed changes

fizyk20 added 2 commits July 25, 2019 10:49

feat/elder: update their_keys when handling a message

f43114c

test/chain: test updating their_keys

2e3f024

fizyk20 dismissed stale reviews from maqi and jeanphilippeD via 2e3f024 July 25, 2019 08:49

fizyk20 force-pushed the issue-1671 branch from 189256f to 2e3f024 Compare July 25, 2019 08:49

fix/clippy: temporarily disable two lints for mpmc::select macro

0b6996d

jeanphilippeD approved these changes Jul 25, 2019

View reviewed changes

jeanphilippeD merged commit 64abdd5 into maidsafe:fleming Jul 25, 2019

fizyk20 deleted the issue-1671 branch July 25, 2019 11:45

Update their_keys on consensus about a section info #1716

Update their_keys on consensus about a section info #1716

fizyk20 commented Jul 23, 2019

jeanphilippeD left a comment

jeanphilippeD Jul 23, 2019

fizyk20 Jul 23, 2019

jeanphilippeD Jul 23, 2019

fizyk20 Jul 23, 2019

jeanphilippeD Jul 23, 2019

fizyk20 Jul 23, 2019

jeanphilippeD Jul 23, 2019

fizyk20 Jul 23, 2019

jeanphilippeD left a comment

jeanphilippeD Jul 24, 2019

maqi Jul 24, 2019

fizyk20 Jul 24, 2019

maqi Jul 24, 2019 •

edited

jeanphilippeD Jul 24, 2019

maqi Jul 24, 2019

fizyk20 Jul 24, 2019

maqi Jul 24, 2019

fizyk20 Jul 24, 2019

maqi Jul 24, 2019

fizyk20 Jul 24, 2019

jeanphilippeD left a comment

Update their_keys on consensus about a section info #1716

Update their_keys on consensus about a section info #1716

Conversation

fizyk20 commented Jul 23, 2019

jeanphilippeD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanphilippeD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maqi Jul 24, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanphilippeD left a comment

Choose a reason for hiding this comment

maqi Jul 24, 2019 •

edited