Partial integration of the Era Supervisor with the Reactor by fizyk20 · Pull Request #42 · casper-network/casper-node

fizyk20 · 2020-06-30T16:33:27Z

This PR wires up some events and effects of the Era Supervisor with the Reactor. There are also some changes to the implementation of the synchronizer for the Highway protocol.

https://casperlabs.atlassian.net/browse/HWY-74
https://casperlabs.atlassian.net/browse/HWY-87

…, Part 1

src/components/consensus/consensus_service/era_supervisor.rs

goral09 · 2020-07-01T06:31:54Z

src/reactor/validator.rs

        let storage = Storage::new(cfg.storage)?;
        let (api_server, api_server_effects) = ApiServer::new(cfg.http_server, effect_builder);
-        let (consensus, consensus_effects) = Consensus::new(effect_builder);
+        let consensus = EraSupervisor::new();


Nitpick: this is a bit leaking – reactor shouldn't have to know or care that there's something like eras or EraSupervisor.

Yes, we "merged" Consensus and EraSupervisor, but maybe it should be called Consensus.

goral09 · 2020-07-01T06:40:11Z

src/types/block.rs

+/// The piece of information that will become the content of a future block (isn't finalized or
+/// executed yet)
+#[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Serialize, Deserialize)]
+pub struct ProtoBlock {


It's missing more block information like seq number, panorama, signature etc. I guess that will come later? If so, can we create a ticket for it already?

I don't think it will contain the panorama or any signatures; that would be redundant, and an implementation detail of the consensus algorithm.

I must be missing something – is this data transferred over the wire to other nodes?

BTW, Isn't this ProtoBlock specific to Highway only? I thought that messages that will be transmitted over the wire will be generic in that.

The ProtoBlock is precisely meant to be the Highway/Casper-independent part of the block: the "consensus value".

So it is transferred over the wire, as part of a Highway block.

The ProtoBlock is precisely meant to be the Highway/Casper-independent part of the block: the "consensus value".

Ah, that isn't obvious. There's no comment that explains the intent and the name is also misleading. It even says proto but we won't be using protobuf most probably.

It's proto as in prototype, not as in protocol ;)

goral09 · 2020-07-01T06:40:47Z

src/types/block.rs

+#[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Serialize, Deserialize)]
+pub struct ExecutedBlock {
+    /// The executed proto-block
+    pub proto_block: ProtoBlock,


Why not just refer to the proto_block by its hash?

We could, but it might complicate things a bit: Then we'd need something like a proto block store as well…
Also: Once it's executed we don't really need to proto block alone anymore, do we?

We will need that anyway – in case of crashes, restarts, catching up?

But if it has already been added to the Highway protocol state, it would be part of it. (If we don't just store it as a hash.) So if we persist the protocol state, we automatically also persist the consensus values, i.e. the proto blocks.

If we don't want it in the protocol state, and use hashes instead, then we'd need a separate storage for them, yes.

ProtocolState is ephemeral and we need the finalized linear chain to be persistent.

goral09 · 2020-07-01T06:42:15Z

src/effect.rs

+    }
+
+    /// Checks whether the deploys included in the proto-block exist on the network
+    pub(crate) async fn validate_proto_block<I>(


Shouldn't this be done in the synchronizer of the consensus protocol?

We were thinking it makes more sense for another component (deploy fetcher?) to do this.
But it does happen after what the synchronizer does (downloading dependencies) and before adding the vertex to the DAG.

goral09 · 2020-07-01T06:45:04Z

src/components/consensus/consensus_protocol.rs

-    CreateNewBlock,
+    ScheduleTimer(u64, TimerId),
+    /// Request deploys for a new block, whose timestamp will be the given `u64`.
+    CreateNewBlock(u64),


It's not entirely clear to me how this message is going to be handled. Is it returned from the ConsensusProtocol to reactor? Or just EraSupervisor that will have access to a DeployBuffer (or some other deploy storage)? Shouldn't it also (besides the block timestamp) also carry an information about consensus value proposed between last finalized block and the current tip? I understand that it's not part of this task but we should leave a comment that is complete and clear.

Yes, the intention here is that another component (TBD) will select deploys.
I agree, a TODO makes sense. Although it's hard to "forget": We'll simply need that information once we try to handle deploy dependencies.

I don't understand why we would need yet another component for that. Only an active validator can ever produce new blocks so it would make sense to hide this logic there.

But it means that the consensus component — in addition to all the stuff it already deals with — needs to care about block gas limits, size limits, deploy dependencies, etc.

And it seems like it would be a relatively small interface between consensus and deploy buffer, if we make that separate.

goral09 · 2020-07-01T06:45:37Z

src/components/consensus/era_supervisor.rs

+}
+
+#[derive(Clone, Debug)]
+struct EraInstance<Id> {


I think it's not used anywhere? At least I can't find it.

goral09 · 2020-07-01T06:48:09Z

src/effect.rs

    }
+
+    /// Passes the timestamp of a future block for which deploys are to be proposed
+    pub(crate) async fn request_proto_block(self, instant: u64) -> ProtoBlock {


Is request a good verb here? Is it going to actually make any requests or just accept all the necessary information in the argument list?

Well, it's a "request" from the consensus component to some other component (deploy buffer?) to select a bunch of deploys. Not sure if it's a good word.

So it's related to another comment https://github.com/CasperLabs/casperlabs-node/pull/42/files#r448237398 . In any case, it should probably also carry an information about all the deploys that are already known to the current consensus instance.

Yes, let's add a TODO for that, too: It'll probably be a nontrivial issue in itself to add that.

src/components/consensus/era_supervisor.rs

goral09 · 2020-07-01T06:55:18Z

src/components/consensus/era_supervisor.rs

+                .ignore(),
+            ConsensusProtocolResult::ScheduleTimer(_timestamp, _timer_id) => {
+                // TODO: we need to get the current system time here somehow, in order to schedule
+                // a timer for the correct moment - and we don't want to use std::Instant


Isn't timestamp already a "real" moment in time in the future?

Why do we don't want to use std::Instant?

Instants are purely local (i.e. subjective), monotonically increasing timestamps: You can't communicate and agree on an Instant among different nodes. Highway assumes synchronized clocks and the timer needs to be scheduled at a particular "absolute" point in time (i.e. not relative to any particular local event).
See the discussion on Slack: https://casperlabs-team.slack.com/archives/C015VHEF532/p1593530821205400

goral09 · 2020-07-01T06:57:13Z

src/components/consensus/consensus_protocol.rs

+                            ));
+                        }
+                        Ok(SynchronizerEffect::InvalidVertex(v, sender, err)) => {
+                            warn!("Invalid vertex from {:?}: {:?}, {:?}", v, sender, err);


There's an InvalidIncomingMessage variant in the ConsensusProtocolResult enum.

goral09 · 2020-07-01T07:05:36Z

src/components/consensus/consensus_protocol.rs

+    /// Request deploys for a new block, whose timestamp will be the given `u64`.
+    CreateNewBlock(u64),
    FinalizedBlock(C),
+    ValidateConsensusValue(NodeId, C),


What does it mean to "validate a consensus value"? Can you leave a comment with a TODO what needs to be done? It's not clear to me whether "validating" means checking signatures (this should be done at the moment of receiving the deploy)? Downloading it from the peers (then it shouldn't be propagated to the reactor)? Or something different?

Maybe:

/// A consensus message we received from the specified node contains a consensus value. // TODO: Add a `ConsensusProtocol::consensus_value_validated` method to call back after validation.

Since C is generic here I wouldn't refer to ProtoBlocks, deploys and downloading then from the sender. That part should be explained in the era supervisor, where the consensus value is now ProtoBlock and not a generic argument anymore.

Anyway, concretely it means: "Make sure you have and validated all deploys mentioned in this ProtoBlock."

The only validations I can imagine performed on a deploy should be either done at the moment of receiving it or are consensus-instance specific. I can't think of anything that would require contact with an external component.

The idea here is that consensus values might have some intrinsic validity conditions (not related to signatures or the consensus protocol itself), so when you receive a proposal for a value from another node, you might want to check whether the value can actually be legally proposed. In our case, this intrinsic condition is the existence of all the deploys mentioned by hash within the value (proto-block), but the general idea could also be applied to other use cases.

OK, I wasn't aware that you changed the plan about where the syncing of consensus value will happen.

But the component that will usually be receiving and validating deploys (also gossiped pending ones) — do you think that should also be the consensus component?

afck

I'll remove myself from the reviewers, since I'm one of the authors.
(Edit: Looks like GitHub doesn't count me as a reviewer anyway. 👍 )

afck · 2020-07-01T08:42:09Z

src/components/consensus/consensus_protocol.rs

-    CreateNewBlock,
+    ScheduleTimer(u64, TimerId),
+    /// Request deploys for a new block, whose timestamp will be the given `u64`.
+    CreateNewBlock(u64),


Yes, the intention here is that another component (TBD) will select deploys.
I agree, a TODO makes sense. Although it's hard to "forget": We'll simply need that information once we try to handle deploy dependencies.

afck · 2020-07-01T08:51:20Z

src/components/consensus/consensus_protocol.rs

+    /// Request deploys for a new block, whose timestamp will be the given `u64`.
+    CreateNewBlock(u64),
    FinalizedBlock(C),
+    ValidateConsensusValue(NodeId, C),


Maybe:

/// A consensus message we received from the specified node contains a consensus value. // TODO: Add a `ConsensusProtocol::consensus_value_validated` method to call back after validation.

Since C is generic here I wouldn't refer to ProtoBlocks, deploys and downloading then from the sender. That part should be explained in the era supervisor, where the consensus value is now ProtoBlock and not a generic argument anymore.

Anyway, concretely it means: "Make sure you have and validated all deploys mentioned in this ProtoBlock."

afck · 2020-07-01T08:55:26Z

src/components/consensus/era_supervisor.rs

+                .ignore(),
+            ConsensusProtocolResult::ScheduleTimer(_timestamp, _timer_id) => {
+                // TODO: we need to get the current system time here somehow, in order to schedule
+                // a timer for the correct moment - and we don't want to use std::Instant


Instants are purely local (i.e. subjective), monotonically increasing timestamps: You can't communicate and agree on an Instant among different nodes. Highway assumes synchronized clocks and the timer needs to be scheduled at a particular "absolute" point in time (i.e. not relative to any particular local event).
See the discussion on Slack: https://casperlabs-team.slack.com/archives/C015VHEF532/p1593530821205400

afck · 2020-07-01T08:56:53Z

src/effect.rs

    }
+
+    /// Passes the timestamp of a future block for which deploys are to be proposed
+    pub(crate) async fn request_proto_block(self, instant: u64) -> ProtoBlock {


Well, it's a "request" from the consensus component to some other component (deploy buffer?) to select a bunch of deploys. Not sure if it's a good word.

afck · 2020-07-01T08:58:32Z

src/effect.rs

+    }
+
+    /// Checks whether the deploys included in the proto-block exist on the network
+    pub(crate) async fn validate_proto_block<I>(


We were thinking it makes more sense for another component (deploy fetcher?) to do this.
But it does happen after what the synchronizer does (downloading dependencies) and before adding the vertex to the DAG.

afck · 2020-07-01T08:59:07Z

src/reactor/validator.rs

        let storage = Storage::new(cfg.storage)?;
        let (api_server, api_server_effects) = ApiServer::new(cfg.http_server, effect_builder);
-        let (consensus, consensus_effects) = Consensus::new(effect_builder);
+        let consensus = EraSupervisor::new();


Yes, we "merged" Consensus and EraSupervisor, but maybe it should be called Consensus.

afck · 2020-07-01T09:00:32Z

src/types/block.rs

+/// The piece of information that will become the content of a future block (isn't finalized or
+/// executed yet)
+#[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Serialize, Deserialize)]
+pub struct ProtoBlock {


I don't think it will contain the panorama or any signatures; that would be redundant, and an implementation detail of the consensus algorithm.

afck · 2020-07-01T09:04:05Z

src/types/block.rs

+#[derive(Clone, Debug, PartialOrd, Ord, PartialEq, Eq, Hash, Serialize, Deserialize)]
+pub struct ExecutedBlock {
+    /// The executed proto-block
+    pub proto_block: ProtoBlock,


We could, but it might complicate things a bit: Then we'd need something like a proto block store as well…
Also: Once it's executed we don't really need to proto block alone anymore, do we?

goral09

No blockers, few requests for a TODO.

Current `Extend` trait of Rust doesn't support builder-like implementations. Hence, we need an explicit method for the `Hasher`

afck and others added 4 commits June 30, 2020 14:34

Initial Highway Core+Synchronizer integration.

30dbbee

Fix Clippy issues in gossip tests

58cbd6a

Merge the Consensus struct and the EraSupervisor

85c8db0

Wire up the events and effects of the Era Supervisor with the Reactor…

d028c53

…, Part 1

fizyk20 requested review from Fraser999, afck, goral09 and marc-casperlabs June 30, 2020 16:33

goral09 reviewed Jul 1, 2020

View reviewed changes

src/components/consensus/consensus_service/era_supervisor.rs Outdated Show resolved Hide resolved

goral09 reviewed Jul 1, 2020

View reviewed changes

src/components/consensus/era_supervisor.rs Show resolved Hide resolved

goral09 reviewed Jul 1, 2020

View reviewed changes

afck reviewed Jul 1, 2020

View reviewed changes

goral09 approved these changes Jul 1, 2020

View reviewed changes

afck merged commit f503cc6 into master Jul 1, 2020

afck deleted the highway_synchronizer branch July 1, 2020 12:45

afck mentioned this pull request Jul 1, 2020

Add comments, remove EraInstance, add resolve_validity method. #43

Merged

rafal-ch pushed a commit that referenced this pull request Sep 11, 2024

Add extend_chain method to Hasher (#42)

8de4eea

Current `Extend` trait of Rust doesn't support builder-like implementations. Hence, we need an explicit method for the `Hasher`

Conversation

fizyk20 commented Jun 30, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

goral09 Jul 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

afck left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

goral09 Jul 1, 2020 •

edited

Loading

afck left a comment •

edited

Loading