feat/elder: update shared state on consensus of AckMessage #1720
Conversation
maqi
force-pushed
the
issue_1670_vote_on_ack
branch
from
6e7bf09
to
b2f8cf4
Compare
maqi
force-pushed
the
issue_1670_vote_on_ack
branch
from
8aee950
to
bbcfa2a
Compare
|
Looks good, just the issue of the test code. |
maqi
force-pushed
the
issue_1670_vote_on_ack
branch
from
2796289
to
f652378
Compare
| .count(); | ||
| assert!(num_of_receives >= min_section_size); | ||
| assert!(num_of_receives < 2 * min_section_size); | ||
| assert!(num_of_consensus >= min_section_size); |
There was a problem hiding this comment.
I am ok with this assert in this PR, but I am wondering if this reflects a problem that we will have to solve in a follow up PR (i.e shouldn't both side know the other their_knowledge)?
There was a problem hiding this comment.
Yeah, I agree.
btw. as the function used to setup is adding nodes until split. So at the tip of split, only the section that not containing the last node (which causes the split), will have their_knowledge (i.e. the last added node cause that the peer section changed.
So, maybe that's why there is only one section having their_knowledge?
There was a problem hiding this comment.
Strange, when the last node is added, it change both section since they are both the result of the split (But maybe a message is not sent, or consensus not reached somewhere).
maqi
force-pushed
the
issue_1670_vote_on_ack
branch
from
f652378
to
4a8ad63
Compare
The AckMessage shall do the job, hence removing the flow of NeighbourConfirm totally.
maqi
force-pushed
the
issue_1670_vote_on_ack
branch
from
4a8ad63
to
aaefab1
Compare
maqi
changed the title
| @@ -2087,6 +2059,12 @@ impl Approved for Elder { | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn handle_ack_message_event(&mut self, sec_info: SectionInfo) -> Result<(), RoutingError> { | |||
| self.chain | |||
| .update_their_knowledge(*sec_info.prefix(), *sec_info.version()); | |||
There was a problem hiding this comment.
Is sec_info.version() correct here? We want the their_knowledge to point not to a SectionInfo, but to a SectionProofBlock within our_history. The index in our_history and the version in SectionInfo might differ.
What we should be doing, in my opinion, would be to put a BlsPublicKey in the AckMessage (which would ideally be taken from security_metadata of the original message, but that's what I'm working on at the moment), then on receiving AckMessage find the block that contains this key and update their_knowledge to the index of this block.
Since security_metadata is not yet done, we could temporarily do what's in this PR (and maybe add a TODO), or alternatively we'd have to postpone this until we have security_metadata and a proper source of the public key to vote on.
There was a problem hiding this comment.
The index in our_history and the version in SectionInfo might differ.
True, but that does not seem to be the their_knowledge job to make that adjustment, otherwise we need to update that map every time we prune our_history.
What we should be doing, in my opinion...
Seem to be functionaly equivalent until the part were we use the block index which may change, and that does not seem nice.
Partial fix for #1670