-
Notifications
You must be signed in to change notification settings - Fork 225
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
Rework PeerList, improve its API, and fix a bug in swap_primary
#397
Conversation
Thanks @OStevan for prompting this little refactor and reviewing it. |
@romac thanks for following up on this. |
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.
As far as I can tell this looks correct. Module level tests would help asserting the right behaviour.
🕶 😼 🖍
Yep, working on those :) Thanks for the review! |
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.
👏 LGTM
Will revisit after tests were added.
This commit changes the `PeerList` type to be parametrized by the type of values associated with a `PeerId`. This enables easier testing of the peer list without having to construct light client`Instance`s. This commit additionally introduces a safer API by taking into account the invariant associated with a `PeerList`, which lets us return unconditionally the value associated with the primary peer. In turn, this simplifies the verification loop in the supervisor, which is now expressed more simply as a recursive function whose correctness is more obvious than the imperative one, at least to me.
swap_primary
Adding tests prompted me to refactor the PeerList definition and API in cbf9753. |
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 a great refactoring and I agree it makes the core of the verification indeed easier to follow and comprehend. My main concern is the very constrained T
on the PeerList
. Could it not give stronger guarantees if Instance
would be a trait and PeerList
expects that as its type parameter?
witnesses: HashSet<PeerId>, | ||
full_nodes: HashSet<PeerId>, | ||
faulty_nodes: HashSet<PeerId>, | ||
witnesses: BTreeSet<PeerId>, |
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.
Maybe I missed some context in a commit, but what's the motivation to move to a BTreeSet
? Asking purely out of curioisity.
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 to provide a stable ordering on the peer ids, to make the whole process (more) deterministic.
Good question by the way, I failed to mention that in the commit.
} | ||
|
||
impl PeerList { | ||
impl<T> PeerList<T> { |
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.
Is there any constraints we want to have on T
that would help make the construction of the PeerList
even more safe?
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 don't see any constraints which would make it safer, but perhaps I am not fully understanding/seeing what you mean by safer in this context?
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 think what I'm getting at is partially what I said in #397 (review) - given this is the PeerList
of the light client crate, it might not be all that beneficial to generalise entirely over T
and rather constraint it to a trait (capabilities) that indicate that whatever is held as a peer value can be used as an Instance
, or whatever other abstraction has the right surface for other components to rely/integrate on. It is in the same direction as making the Handle
a trait, I suspect these changes to make it easier to build up the object graph in tests and safer to construct. It's mostly food for thought and can be iterated on over time.
pub struct PeerList { | ||
instances: HashMap<PeerId, Instance>, | ||
pub struct PeerList<T> { | ||
values: HashMap<PeerId, T>, |
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.
Why the quite generic rename 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.
Since the peer list can now hold to any datatype rather than just light client instances, I thought values
was more appropriate than instances
, but I am open to suggestions.
faulty_nodes: HashSet<PeerId>, | ||
witnesses: BTreeSet<PeerId>, | ||
full_nodes: BTreeSet<PeerId>, | ||
faulty_nodes: BTreeSet<PeerId>, |
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.
It seems wasteful to see the exact same fields as in PeerList
, why does the builder not hold on to a fresh PeerList
and returning it on build
instead of constructing it.
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 am more or less following the internal representation used by the derive_builder
crate, and didn't give it much more thought. But I agree that it's a bit wasteful and that what you suggest would work as well. I don't feel strongly either way.
} | ||
} | ||
|
||
fn a() -> PeerId { |
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.
You could name these according to their use case further down.
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.
Previous comment was not posted in the right place
@xla Thanks for the review! :) See my answers inline.
I don't really see the value of having Instance defined as a trait and constraining T over that, since the behavior of PeerList does not depend in any way on the choice of T. My general approach to building data structures is to keep them as generic as possible, and only constrain on types if absolutely required by the definition of the struct, or on a per-method basis. Admittedly, this is a mindset I inherited from working in Haskell, where it really pays off to have very generic data structures with fully unconstrained type parameters, but it seems to be mostly shared by the Rust community as well: rust-lang/rust-clippy#1689. Of course, if there is actual (type) safety to be gained by having a bound on T (that I am at the moment not seeing, but feel free to expand on that), I'd be happy to revisit that decision in that case. |
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.
Thought you might approach it like that and it's a common way esp. for people coming from Haskell. It's a sound way of building datastructures and I'm merely challenging from the perspective of domain coherence. As we not building general container datastructures but rather specific ones, i.e. the peer list of what the light client understands as a peer.
In any case my comments are just food for thought. All my review comments are sufficiently addressed.
➿ 🐬 🎈 🗝
I agree, and that's why the peer list was initially specialized to I still wonder what you had in mind w.r.t. to safety, because I don't see any constraints on |
let _ = peer_list.replace_faulty_witness(d()); | ||
unreachable!(); | ||
} | ||
} |
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.
Thanks for adding tests 👍
My understanding is that this isn't necessarily about safety per se, only that a completely generic |
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.
👏 LGTM 👍
Spot on, sorry @romac - what I was going at is not so much safety but communication of intent. |
Removes the newly assigned
primary
from thewitnesses
.Ensures the primary is not part of any of the peer lists (
witnesses
,full_nodes
,faulty_nodes
).Fixes a bug in
swap_primary
(nowreplace_faulty_primary
) where we would iterate the first element ofwitnesses
over and over.Rework and generalize the PeerList for easier testing and safer API
This commit changes the
PeerList
type to be parametrized by the type of values associated with aPeerId
. This enables easier testing of the peer list without having to construct light clientInstance
s.This commit additionally introduces a safer API by taking into account the invariant associated with a
PeerList
, which lets us return unconditionally the value associated with the primary peer.In turn, this simplifies the verification loop in the supervisor, which is now expressed more simply as a recursive function whose correctness is more obvious than the imperative one, at least to me.