Make min_round_exp, forgiveness and observer status configurable in Highway. by afck · Pull Request #140 · casper-network/casper-node

afck · 2020-08-06T13:32:06Z

https://casperlabs.atlassian.net/browse/HWY-131
https://casperlabs.atlassian.net/browse/HWY-101

afck · 2020-08-06T13:32:32Z

I'll add it to the chain spec in a separate PR.

goral09 · 2020-08-06T13:33:33Z

I'll add it to the chain spec in a separate PR.

Why chainspec and not node configuration? It's not required to be the same for all nodes?

afck · 2020-08-06T13:34:42Z

min_round_exp and forgiveness_factor is required to be the same in all nodes.
The local round exponent can differ, of course. That's yet another issue to make that adaptive.

goral09 · 2020-08-06T13:34:43Z

node/src/components/consensus/highway_core/state.rs

    /// summit does not exceed half the total weight.
    forgiveness_factor: u64,
+    /// The minimum round exponent. `1 << min_round_exp` milliseconds is the minimum round length.
+    min_round_exp: u8,


Why is it part of the State and not HighwayParams? Similarly forgiveness_factor.

I removed HighwayParams.
Moving these to Highway would require using Highway instead State in a lot of tests, which will be a major change.

Previously State was encapsulating one thing, now it seems like it's a bag for everything single-era-related.

I agree: State and Highway should just be merged. But that's a task for several days in itself.
(Highway is also single-era-related.)

I liked the fact that State was exactly just that – state, and the logic for accessing/modifying it was somewhere else.

But was that ever really cleanly separated like that? Most of the logic was always in state.rs. 😕

I'd really love to have some discussion about that in general: How should the highway_core logic be separated into modules? I'm pretty sure there's a much better way to do it that what we have now, but I don't know.

node/src/components/consensus/highway_core/active_validator.rs

node/src/components/consensus/highway_core/state.rs

goral09 · 2020-08-06T13:48:26Z

node/src/components/consensus/highway_core/highway.rs

+    /// regular reward.
+    pub(crate) fn forgiveness_factor(mut self, numerator: u16, denominator: u16) -> Self {
+        if numerator > denominator {
+            warn!("forgiveness factor should be at most 100%");


If it should be at most 100% why not panic?

Hmm… I guess so. I'll change it.
Running with more than 100% is perfectly possible, of course. I just can't imagine how it could ever make any economic sense.

goral09 · 2020-08-06T13:49:57Z

node/src/components/consensus/highway_core/highway.rs

+    /// Creates and returns a new `Highway` instance with an empty protocol state.
+    pub(crate) fn build(self) -> Highway<C> {
+        let state = State::new(
+            self.validators.enumerate().map(|(_, val)| val.weight()),


Why not just into_iter() if you're ignoring index anyway?

goral09 · 2020-08-06T13:52:00Z

node/src/components/consensus/highway_core/highway.rs

+            instance_id,
+            validators,
+            seed,
+            forgiveness_factor: (1, 5),


Shouldn't forgiveness_factor and min_round_exp be carried over from the previous State instance rather than using magic numbers as defaults?

But highway_core doesn't know about eras.
Also, I don't think it should be copied over from the previous one. It should rather be read from the chain spec or (probably) the most recent protocol upgrade.

But maybe default numbers don't make sense, that's true. Maybe I should just remove the whole HighwayBuilder again, because in the end, none of those values are really optional.

goral09 · 2020-08-06T13:53:18Z

node/src/components/consensus/highway_core/highway.rs

+        let idx = self.validators.get_index(&id);
+        let (av, effects) = ActiveValidator::new(idx, secret, round_exp, start_time, &self.state);
+        self.active_validator = Some(av);
+        effects


😬 I don't like mutating it like that, I would prefer if it returned new instance of Highway.

Wouldn't it make more sense actually? One cannot become an active validator in the middle of an era, right? you can only become one if you're bonded.

I thought it might be useful for syncing up, e.g. if you had an outage for a few minutes: You'd run as an "observer" until you've caught up and downloaded the current protocol state, and only then "activate" it again, so you don't create vertices for old rounds.

This makes sense but I would prefer if we didn't have to worry about stuff like that – that our code might produce messages where it shouldn't have and it's not doing that only b/c we have the fuse here :)

Not sure… in the tests we do activate the validators right away, to the problem would still show up there, wouldn't it? I think at least conceptually it makes sense to have such a mutating activate_validator method, and it even makes the code a bit simpler because nothing has to return a tuple (Highway, Effects).

goral09

Just nitpicks, nothing blocking from me.

afck · 2020-08-06T14:34:49Z

But enumerate was just something like a five-line method? We can just recreate it whenever we need it again. It turned out we never used the index, so it's just unused at the moment.

140: Add an expiry to requests for block to propose r=fizyk20 a=fizyk20 This adds a `request_expiry` field on `DeployBufferRequest::GetAppendableBlock`, which is the timestamp by which a response must be received to be still relevant. Co-authored-by: Bartłomiej Kamiński <bart@casperlabs.io> Co-authored-by: Ed Hastings <ed@casperlabs.io> Co-authored-by: Bartłomiej Kamiński <fizyk20@gmail.com>

The VM memory section for the receipts root was updated only after the script execution. It is desirable to have it updated on the fly so the scripts can consume the generated receipts root. Closes #125

afck added 3 commits August 6, 2020 15:04

Add State::min_round_exp

faf5d9c

Make forgiveness & min round exp an option of State.

81e543e

Add a HighwayBuilder.

2b3f1b7

afck requested review from goral09 and marc-casperlabs August 6, 2020 13:32

afck assigned goral09 Aug 6, 2020

goral09 reviewed Aug 6, 2020

View reviewed changes

node/src/components/consensus/highway_core/active_validator.rs Show resolved Hide resolved

goral09 reviewed Aug 6, 2020

View reviewed changes

node/src/components/consensus/highway_core/state.rs Show resolved Hide resolved

goral09 reviewed Aug 6, 2020

View reviewed changes

goral09 approved these changes Aug 6, 2020

View reviewed changes

Address review comments.

70a617b

afck merged commit 46606fd into casper-network:master Aug 6, 2020

afck deleted the min_round_exp branch August 6, 2020 15:13

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

Bump to v0.12.2 (#140)

ef4b62e

Conversation

afck commented Aug 6, 2020

Uh oh!

afck commented Aug 6, 2020

Uh oh!

goral09 commented Aug 6, 2020

Uh oh!

afck commented Aug 6, 2020

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!

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!

goral09 left a comment

Choose a reason for hiding this comment

Uh oh!

afck commented Aug 6, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants