[VID Upgrade] add version number to VID interfaces #4062
Conversation
| @@ -262,6 +262,9 @@ impl Versions for TestVersions { | |||
| type Marketplace = StaticVersion<0, 3>; | |||
|
|
|||
| type Epochs = StaticVersion<0, 4>; | |||
|
|
|||
| // TODO(Chengyu): fill number here | |||
| type AVIDMUpgrade = StaticVersion<0, 0>; | |||
There was a problem hiding this comment.
this should probably be larger than all the other versions by default
There was a problem hiding this comment.
This is just a place holder. Will ask more about the version upgrade process.
There was a problem hiding this comment.
should we set it as <0,5>?
There was a problem hiding this comment.
Yeah, I think that's reasonable. I just wanted to make sure CI still passes with it set to something higher than the base/upgrade versions
| @@ -175,6 +181,9 @@ pub fn precompute_vid_commitment( | |||
| /// do dispersal for the genesis block. For simplicity and performance, we use 1. | |||
| pub const GENESIS_VID_NUM_STORAGE_NODES: usize = 1; | |||
|
|
|||
| /// The genesis block VID version number. | |||
| pub const GENESIS_VID_VERSION: Version = Version { major: 0, minor: 0 }; | |||
There was a problem hiding this comment.
I don't think this should be a global const; is there a reason we need this?
| @@ -145,28 +149,30 @@ pub trait TestableBlock<TYPES: NodeType>: BlockPayload<TYPES> + Debug { | |||
| /// If the VID computation fails. | |||
| #[must_use] | |||
| #[allow(clippy::panic)] | |||
| pub fn vid_commitment( | |||
| pub fn vid_commitment<V: Versions>( | |||
There was a problem hiding this comment.
I don't know if this is a strong preference, but I think typically we would implement a vid_commitment2 and switch on the version at use sites
There was a problem hiding this comment.
Then for future upgrades, we could have complicated codes at use sides. It might be better to hide the versions after a proxy fn.
crates/types/src/data.rs
Outdated
| payload: &TYPES::BlockPayload, | ||
| membership: &Arc<RwLock<TYPES::Membership>>, | ||
| view: TYPES::View, | ||
| target_epoch: Option<TYPES::Epoch>, | ||
| data_epoch: Option<TYPES::Epoch>, | ||
| version: Version, |
There was a problem hiding this comment.
minor comment: I usually prefer passing upgrade_lock over passing version
Closes #4061
This PR:
This PR does not:
Key places to review: