Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce increasing of the Executor::VERSION on each release #1898

Merged
merged 9 commits into from
May 27, 2024
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

### Added
- [#1898](https://github.com/FuelLabs/fuel-core/pull/1898): Enforce increasing of the `Executor::VERSION` on each release.

### Changed

- [#1906](https://github.com/FuelLabs/fuel-core/pull/1906): Makes `cli::snapshot::Command` members public such that clients can create and execute snapshot commands programmatically. This enables snapshot execution in external programs, such as the regenesis test suite.
Expand Down
35 changes: 35 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ This is a rough outline of what a contributor's workflow looks like:
- Write code, add test cases, and commit your work.
- Run tests and make sure all tests pass.
- If the PR contains any breaking changes, add the breaking label to your PR.
- Update `CHANGELOG.md` with a proper description of your changes.
- If the change is breaking, please include a migration guide.
- If you are part of the FuelLabs Github org, please open a PR from the repository itself.
- Otherwise, push your changes to a branch in your fork of the repository and submit a pull request.
- Make sure mention the issue, which is created at step 1, in the commit message.
Expand Down Expand Up @@ -150,3 +152,36 @@ Multiple issues should use full syntax for each issue and separate by a comma, l
```md
close #123, ref #456
```

### Releasing

Each release should have its own new version of the `fuel_core_upgradable_executor::Executor` regardless of minor or major release. The version of the executor should grow without gaps.
If publishing the release fails or the release is invalid and
we don't plan to upgrade the network to use this release,
we still need to increase the version.
The network can easily skip releases by upgrading to the old state transition function.

The `fuel_core_upgradable_executor` crate also contains the mapping of the crate
version to the executor's version(`CRATE_VERSIONS`). For each release,
we need to add a new entry there. The version of the crate uses `-` instead
of the `.` to avoid overriding old entries accidentally.
If you forgot to upgrade the version or add a new entry, the `version_check` test will fail.

```rust
/// This constant is used along with the `version_check` test.
/// To avoid automatic bumping during release, the constant uses `-` instead of `.`.
/// Each release should have its own new version of the executor.
/// The version of the executor should grow without gaps.
/// If publishing the release fails or the release is invalid, and
/// we don't plan to upgrade the network to use this release,
/// we still need to increase the version. The network can
/// easily skip releases by upgrading to the old state transition function.
pub const CRATE_VERSIONS: &'static [(
&'static str,
StateTransitionBytecodeVersion,
)] = &[
("0-26-0", StateTransitionBytecodeVersion::MIN),
("0-27-0", 1),
...
];
```
86 changes: 74 additions & 12 deletions crates/services/upgradable-executor/src/executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,22 @@ impl<S, R> Executor<S, R> {
/// on the block, native execution is used. If the version is not the same
/// as in the block, then the WASM executor is used.
pub const VERSION: u32 = StateTransitionBytecodeVersion::MIN;

/// This constant is used along with the `version_check` test.
/// To avoid automatic bumping during release, the constant uses `-` instead of `.`.
#[cfg(test)]
pub const CRATE_VERSION: &'static str = "0-26-0";
/// Each release should have its own new version of the executor.
Copy link
Member

@Voxelot Voxelot May 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should document this in the CONTRIBUTING.md file to explain that this needs to be manually updated every time the state transition function changes.

/// The version of the executor should grow without gaps.
/// If publishing the release fails or the release is invalid, and
/// we don't plan to upgrade the network to use this release,
/// we still need to increase the version. The network can
/// easily skip releases by upgrading to the old state transition function.
pub const CRATE_VERSIONS: &'static [(
&'static str,
StateTransitionBytecodeVersion,
)] = &[
("0-26-0", StateTransitionBytecodeVersion::MIN),
// ("0-27-0", 1),
];

pub fn new(
storage_view_provider: S,
Expand Down Expand Up @@ -587,6 +599,7 @@ where
}
}

#[allow(clippy::cast_possible_truncation)]
#[cfg(test)]
mod test {
use super::*;
Expand Down Expand Up @@ -620,6 +633,10 @@ mod test {
services::relayer::Event,
tai64::Tai64,
};
use std::collections::{
BTreeMap,
BTreeSet,
};

#[derive(Clone, Debug)]
struct Storage(InMemoryStorage<Column>);
Expand Down Expand Up @@ -685,18 +702,62 @@ mod test {
}
}

// When this test fails, it is a sign that we need to increase the `Executor::VERSION`.
#[test]
fn version_check() {
let crate_version = env!("CARGO_PKG_VERSION");
let executor_cate_version = Executor::<Storage, DisabledRelayer>::CRATE_VERSION
.to_string()
.replace('-', ".");
let dashed_crate_version = crate_version.to_string().replace('.', "-");
let mut seen_executor_versions =
BTreeSet::<StateTransitionBytecodeVersion>::new();
let seen_crate_versions = Executor::<Storage, DisabledRelayer>::CRATE_VERSIONS
.iter()
.map(|(crate_version, version)| {
let executor_crate_version = crate_version.to_string().replace('-', ".");
seen_executor_versions.insert(*version);
(executor_crate_version, *version)
})
.collect::<BTreeMap<_, _>>();

if let Some(expected_version) =
seen_crate_versions.get(&crate_version.to_string())
{
assert_eq!(
*expected_version,
Executor::<Storage, DisabledRelayer>::VERSION,
"The version of the executor should be the same as in the `CRATE_VERSIONS` constant"
);
} else {
let next_version = Executor::<Storage, DisabledRelayer>::VERSION + 1;
panic!(
"New release {crate_version} is not mentioned in the `CRATE_VERSIONS` constant.\
Please add the new entry `(\"{dashed_crate_version}\", {next_version})` \
to the `CRATE_VERSIONS` constant."
);
}

let last_crate_version = seen_crate_versions.last_key_value().unwrap().0.clone();
assert_eq!(
crate_version, last_crate_version,
"The last version in the `CRATE_VERSIONS` constant \
should be the same as the current crate version."
);

assert_eq!(
executor_cate_version, crate_version,
"When this test fails, \
it is a sign that maybe we need to increase the `Executor::VERSION`. \
If there are no breaking changes that affect the execution, \
then you can only increase `Executor::CRATE_VERSION` to pass this test."
seen_executor_versions.len(),
seen_crate_versions.len(),
"Each executor version should be unique"
);

assert_eq!(
seen_executor_versions.len() as u32 - 1,
Executor::<Storage, DisabledRelayer>::VERSION,
"The version of the executor should monotonically grow without gaps"
);

assert_eq!(
seen_executor_versions.last().cloned().unwrap(),
Executor::<Storage, DisabledRelayer>::VERSION,
"The latest version of the executor should be the last version in the `CRATE_VERSIONS` constant"
);
}

Expand Down Expand Up @@ -749,6 +810,7 @@ mod test {
mod native {
use super::*;
use crate::executor::Executor;
use ntest as _;

#[test]
fn can_validate_block() {
Expand All @@ -759,7 +821,7 @@ mod test {
let block = valid_block(Executor::<Storage, DisabledRelayer>::VERSION);

// When
let result = executor.validate_without_commit(block).map(|_| ());
let result = executor.validate(&block).map(|_| ());

// Then
assert_eq!(Ok(()), result);
Expand All @@ -775,7 +837,7 @@ mod test {
let block = valid_block(wrong_version);

// When
let result = executor.validate_without_commit(block).map(|_| ());
let result = executor.validate(&block).map(|_| ());

// Then
result.expect_err("The validation should fail because of versions mismatch");
Expand Down
Loading