Remove version and prefix from SectionProofChain #2091
Conversation
The goal is to remove `prefix` from `SectionKeyInfo` and this is the first step towards it.
- ignore prefix and version, use only the keys themselves - simplify the logic - change `TrustStatus::ProofTooNew` to `TrustStatus::Unknown` as without version we can't tell whether the proof is too new or too old. - remove the key from `TrustStatus::Trusted` - if a proof chain is valid, it's always the latest key that should be trusted.
| /// The version acknowledged. | ||
| pub ack_version: u64, | ||
| /// The key acknowledged. | ||
| pub ack_key: bls::PublicKey, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord, Hash, Clone)] | ||
| pub struct SendAckMessagePayload { |
There was a problem hiding this comment.
I think this whole Ack approach can be deleted here actually. Perhaps when the pull mechanism in place would be more correct. Happy to waiton that PR
There was a problem hiding this comment.
Yes, that will be part of the pull model.
src/section/section_map.rs
Outdated
| } | ||
|
|
||
| pub fn latest_compatible_key(&self, name: &XorName) -> Option<&bls::PublicKey> { | ||
| // `keys` is already ordered newest to oldest. |
There was a problem hiding this comment.
keys is a BTreeMap keyed by prefix, why the comment says is already ordered newest to oldest ?
or you mean the keys function?
however, the function seems chain self.keys first then the self.recent_keys ?
There was a problem hiding this comment.
So keys contains only the latest (newest) keys for every section and it is then chained with recent_keys which has the previous keys in order newest to oldest. So the resulting iterator also yields the keys newest to oldest. The find function then returns the first matching entry which also happens to be the newest entry because of that. Maybe the comment should be clearer?
There was a problem hiding this comment.
Yes, I think the comment shall be clearer.
acutally, according what you explained, there are actually two set of keys chained within the result from fn keys(): one is keys of other section, and the other is previous keys (of ours ?).
which, to me, is bit weird/confusing to chain these two together. :(
There was a problem hiding this comment.
No, it contains only their keys. keys are the latest ones we know of and recent_keys is a few of previous ones, but still theirs. I'll try to make the comment clearer.
src/section/section_proof_chain.rs
Outdated
|
|
||
| /// Block of the section proof chain. Contains the section BLS public key and is signed by the |
There was a problem hiding this comment.
who will sign the first block?
or this block will only be in tail, not in head?
in that case, maybe the comment better to mention this explicitly?
There was a problem hiding this comment.
The first block is not signed. If this chain is the full section chain, then the first block is the genesis block created by the very first node in the network. If this is just a slice of a chain, the presumably the first block's key is already trusted (by being verified previously) so it doesn't need to be signed.
or this block will only be in tail, not in head?
That's right. The first block doesn't have signature so it's not stored as SectionProofBlock (which is key + signature) but only as the key alone.
There was a problem hiding this comment.
In this case, I'd prefer to make the comment to be more explicit, say:
Block of the section proof chain (tail part only)
src/section/section_proof_chain.rs
Outdated
| return; | ||
| } | ||
|
|
||
| /// Pushes a new block into the chain. No validation is performed. |
There was a problem hiding this comment.
wondering why the validation is removed?
There was a problem hiding this comment.
Mostly because I needed to be able to push invalid blocks for testing purposes. I could have instead created a separate #[feature = "mock"] function push_without_validation or so, but I decided to keep things simple instead. The validation itself weren't that useful anyway because even when you create an invalid chain, it's going to be rejected by the recipients anyway.
There was a problem hiding this comment.
hmm... I thought the validate function will be used (though maybe not that useful as you say) for the production code. So, if the removal is due to the testing purpose, I'd prefer make a separate function under the mock feature guard.
It's not a strong opinion here, so leave it to you to decide.
There was a problem hiding this comment.
What I meant is that we don't need to validate the block at the time we are pushing it, because we will validate the whole chain as part of message verification. But I'll add a separate function for it anyway.
madadam
force-pushed
the
remove-elders-version
branch
from
be57052
to
461408b
Compare
madadam
force-pushed
the
remove-elders-version
branch
from
461408b
to
958c348
Compare
This PR simplifies
SectionProofChainby storing only the keys in it, not versions or prefixes.Note: the concept of elders version isn't completely gone yet. It's still used in
EldersInfoto be able to determine causal relationship between them. We need that to prevent overwriting newer info with older info. After switching to the pull model (in a different PR), this will likely not be necessary anymore.