Validate the ChainConfig struct at construction time #1845

tyshko5 · 2022-08-22T09:21:50Z

Summary of changes
Changes introduced in this pull request:

Reference issue to close (if applicable)

Closes #1579

Other information and links

forest/src/daemon.rs

types/networks/src/lib.rs

lemmih · 2022-09-01T10:08:09Z

@tyshko5 You can write a From<Height> for NetworkVersion. It would be a total function and guaranteed to never fail.

tyshko5 · 2022-09-01T10:10:27Z

@tyshko5 You can write a From<Height> for NetworkVersion. It would be a total function and guaranteed to never fail.

I've already added a compile-time check

lemmih · 2022-09-01T10:13:24Z

@tyshko5 You can write a From<Height> for NetworkVersion. It would be a total function and guaranteed to never fail.

I've already added a compile-time check

Right but you can't use #![feature(variant_count)] (we want to avoid +unstable as soon as possible) and you can't assume Height is in the same order as NetworkVersion. A trait implementation that maps from Height to NetworkVersion would be stable rust and free of magic.

types/networks/src/lib.rs

lemmih · 2022-09-02T08:20:59Z

types/networks/src/lib.rs

+pub const HEIGHT_VEC: [Height; 17] = [
+    Height::Breeze,
+    Height::Smoke,
+    Height::Ignition,
+    Height::ActorsV2,
+    Height::Tape,
+    Height::Liftoff,
+    Height::Kumquat,
+    Height::Calico,
+    Height::Persian,
+    Height::Orange,
+    Height::Trust,
+    Height::Norwegian,
+    Height::Turbo,
+    Height::Hyperdrive,
+    Height::Chocolate,
+    Height::OhSnap,
+    Height::Skyr,
+];
+


This is unused.

types/networks/src/lib.rs

lemmih · 2022-09-02T08:22:05Z

types/networks/src/lib.rs

+            Height::Breeze => NetworkVersion::V0,
+            Height::Smoke => NetworkVersion::V1,
+            Height::Ignition => NetworkVersion::V2,
+            Height::ActorsV2 => NetworkVersion::V3,
+            Height::Tape => NetworkVersion::V4,


Bump everything such that Breeze is V1 and Tape is V5. See the deleted UPGRADE_INFOS for ground truth.

lemmih · 2022-09-02T08:24:17Z

types/networks/src/lib.rs

+pub fn sort_by_epoch(mut height_info_vec: Vec<HeightInfo>) -> Vec<HeightInfo> {
+    height_info_vec.sort_by(|a, b| a.epoch.cmp(&b.epoch));
+    height_info_vec
+}
+


If this function takes a slice of HeightInfo (and turns it into a Vec) then the rest of the code will become simpler.

lemmih · 2022-09-02T11:37:06Z

types/networks/src/lib.rs

+        let height_infos = sort_by_epoch(&self.height_infos);
+        let height = height_infos


Suggested change

let height_infos = sort_by_epoch(&self.height_infos);

let height = height_infos

let height = sort_by_epoch(&self.height_infos)

lemmih · 2022-09-02T11:37:36Z

types/networks/src/lib.rs

+        let height_infos = sort_by_epoch(&self.height_infos);
+        height_infos


Suggested change

let height_infos = sort_by_epoch(&self.height_infos);

height_infos

sort_by_epoch(&self.height_infos)

lemmih · 2022-09-02T11:38:24Z

types/networks/src/lib.rs

        self.name == other.name
            && self.bootstrap_peers == other.bootstrap_peers
            && self.block_delay_secs == other.block_delay_secs
-            && self.version_schedule == other.version_schedule
-            && self.height_infos == other.height_infos
+            && height_infos == other_height_infos


Suggested change

&& height_infos == other_height_infos

&& sort_by_epoch(&self.height_infos) == sort_by_epoch(&other.height_infos)

lemmih

Good. Only a few style suggestions.

elmattic · 2022-09-02T13:22:44Z

@tyshko5 Could you update the delegate consensus code that is still using Claus height in its toml settings file?

types/networks/src/lib.rs

tyshko5 · 2022-09-02T14:14:22Z

@tyshko5 Could you update the delegate consensus code that is still using Claus height in its toml settings file?

Done

* most changes * requested change * fix fmt * new ways: delete claus, version_schedule, sort * fix lint * requested changes * compile time check * from implementation * requested changes * requested changes * requested changes * requested changes * requested changes * delete Claus from deleg cns config

most changes

7a7959e

tyshko5 marked this pull request as ready for review August 22, 2022 10:48

tyshko5 requested review from connormullett, elmattic, lemmih, LesnyRumcajs and jdjaustin as code owners August 22, 2022 10:48

lemmih requested changes Aug 22, 2022

View reviewed changes

forest/src/daemon.rs Outdated Show resolved Hide resolved

types/networks/src/lib.rs Outdated Show resolved Hide resolved

tyshko5 added 2 commits August 22, 2022 17:17

requested change

9e9360b

fix fmt

bab321e

LesnyRumcajs requested changes Aug 29, 2022

View reviewed changes

types/networks/src/lib.rs Outdated Show resolved Hide resolved

types/networks/src/lib.rs Outdated Show resolved Hide resolved

types/networks/src/lib.rs Outdated Show resolved Hide resolved

types/networks/src/lib.rs Outdated Show resolved Hide resolved

tyshko5 added 2 commits August 30, 2022 16:25

new ways: delete claus, version_schedule, sort

9cf2af0

fix lint

ec53f94

tyshko5 requested review from lemmih and LesnyRumcajs August 31, 2022 08:15

LesnyRumcajs reviewed Aug 31, 2022

View reviewed changes

types/networks/src/lib.rs Outdated Show resolved Hide resolved

types/networks/src/lib.rs Outdated Show resolved Hide resolved

types/networks/src/lib.rs Outdated Show resolved Hide resolved

tyshko5 added 2 commits August 31, 2022 14:45

requested changes

a9bcf06

compile time check

e40f143

tyshko5 requested a review from LesnyRumcajs September 1, 2022 10:07

from implementation

45dd497

lemmih reviewed Sep 1, 2022

View reviewed changes

types/networks/src/lib.rs Outdated Show resolved Hide resolved

requested changes

00af883

tyshko5 requested a review from lemmih September 1, 2022 11:57

lemmih reviewed Sep 1, 2022

View reviewed changes

types/networks/src/lib.rs Show resolved Hide resolved

lemmih requested changes Sep 1, 2022

View reviewed changes

types/networks/src/lib.rs Show resolved Hide resolved

tyshko5 added 2 commits September 1, 2022 16:53

requested changes

70f458e

requested changes

c2c0883

tyshko5 requested a review from lemmih September 1, 2022 15:36

LesnyRumcajs approved these changes Sep 2, 2022

View reviewed changes

lemmih requested changes Sep 2, 2022

View reviewed changes

requested changes

fa26316

tyshko5 requested a review from lemmih September 2, 2022 10:49

lemmih reviewed Sep 2, 2022

View reviewed changes

lemmih approved these changes Sep 2, 2022

View reviewed changes

elmattic reviewed Sep 2, 2022

View reviewed changes

types/networks/src/lib.rs Outdated Show resolved Hide resolved

tyshko5 added 2 commits September 2, 2022 16:04

requested changes

5c259bb

delete Claus from deleg cns config

2de70b6

tyshko5 merged commit 0f78aa4 into main Sep 2, 2022

tyshko5 deleted the tyshko5/issue-1579 branch September 2, 2022 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Validate the ChainConfig struct at construction time #1845

Validate the ChainConfig struct at construction time #1845

tyshko5 commented Aug 22, 2022

lemmih commented Sep 1, 2022

tyshko5 commented Sep 1, 2022

lemmih commented Sep 1, 2022

lemmih Sep 2, 2022

lemmih Sep 2, 2022

lemmih Sep 2, 2022

lemmih Sep 2, 2022

lemmih Sep 2, 2022

lemmih Sep 2, 2022

lemmih left a comment

elmattic commented Sep 2, 2022

tyshko5 commented Sep 2, 2022

		let height_infos = sort_by_epoch(&self.height_infos);
		let height = height_infos

	let height_infos = sort_by_epoch(&self.height_infos);
	let height = height_infos
	let height = sort_by_epoch(&self.height_infos)

		let height_infos = sort_by_epoch(&self.height_infos);
		height_infos

	let height_infos = sort_by_epoch(&self.height_infos);
	height_infos
	sort_by_epoch(&self.height_infos)

	&& height_infos == other_height_infos
	&& sort_by_epoch(&self.height_infos) == sort_by_epoch(&other.height_infos)

Validate the ChainConfig struct at construction time #1845

Validate the ChainConfig struct at construction time #1845

Conversation

tyshko5 commented Aug 22, 2022

Summary of changes Changes introduced in this pull request:

lemmih commented Sep 1, 2022

tyshko5 commented Sep 1, 2022

lemmih commented Sep 1, 2022

lemmih Sep 2, 2022

Choose a reason for hiding this comment

lemmih Sep 2, 2022

Choose a reason for hiding this comment

lemmih Sep 2, 2022

Choose a reason for hiding this comment

lemmih Sep 2, 2022

Choose a reason for hiding this comment

lemmih Sep 2, 2022

Choose a reason for hiding this comment

lemmih Sep 2, 2022

Choose a reason for hiding this comment

lemmih left a comment

Choose a reason for hiding this comment

elmattic commented Sep 2, 2022

tyshko5 commented Sep 2, 2022

Summary of changes
Changes introduced in this pull request: