Trust Quorum: Start implementing Node
#7929
Conversation
This code builds upon #7859, which itself builds upon #7891. Those need to be merged in order first. A `Node` is the sans-io entity driving the trust quorum protocol. This PR starts the work of creating the `Node` type and coordinating an initial configuration. There's a property based test that generates an initial `ReconfigureMsg` that is then used to call `Node::coordinate_reconfiguration`, which will internally setup a `CoordinatorState` and create a bunch of messages to be sent. We verify that a new `PersistentState` is returned and that messages are sent. This needs a bit more documentation and testing, so it's still WIP.
andrewjstone
changed the title
NodeNode
| }; | ||
|
|
||
| let state = CoordinatorState::new(my_platform_id, now, msg, config, op); | ||
| Ok((state, my_prepare_msg.unwrap())) |
There was a problem hiding this comment.
Under what circumstances would we expect my_prepare_msg to be None?
There was a problem hiding this comment.
Never. This is the Precondition discussed in the function doc comment. As part of creating the ValidatedReconfigureMsg we ensure that this node is part of the configuration and will have a Prepare message.
That being said, I really dislike the way I implemented this. In order to extract my_prepare_msg from the loop above I made my_prepare_msg a mutable option initialized to None. This is the root of the issue. I could also just always include it in prepares and then pull the value out, but that would also return an Option.
I'm really not sure of a better way forward here, although I could certainly add a safety comment above the unwrap. I'm open to other suggestions as well.
There was a problem hiding this comment.
Hmm. Actually, I could have Configuration::new separate out this node's share and return it separately. That would prevent the need to filter here.
There was a problem hiding this comment.
I think either a refactor of a comment explaining the precondition would be helpful
There was a problem hiding this comment.
I did refactor a bit, and also added some comments. Now the shares are always packaged up with the digests so that they don't get out of order by accident. However, there's still a need for an Option.
I also made all fields of ValidatedReconfigureMsg private so that it cannot be constructed without running through validation.
There was a problem hiding this comment.
I actually think I want to add the coordinator platform_id into the ValidatedReconfigureMsg so that it is indeed guaranteed known to be part of the membership. Some code could always call new_uninitialized with a different platform_id from itself, which would obviously be wrong, but isn't statically guaranteed not to happen.
This would also remove the extra parameter to the method.
There was a problem hiding this comment.
I actually think I want to add the coordinator platform_id into the ValidatedReconfigureMsg
I went ahead and did this in a5323a5.
It had nice knock-on effects in that now Configuration and CoordinatorState have one fewer constructor parameter.
This code builds upon #7859, which itself builds upon #7891. Those need to be merged in order first.
A
Nodeis the sans-io entity driving the trust quorum protocol. This PR starts the work of creating theNodetype and coordinating an initial configuration.There's a property based test that generates an initial
ReconfigureMsgthat is then used to callNode::coordinate_reconfiguration, which will internally setup aCoordinatorStateand create a bunch of messages to be sent. We verify that a newPersistentStateis returned and that messages are sent.Some of validation code is also tested. We test a subset of error conditions, but ensure all the error check methods themselves actually get called among the checks we choose.