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

Preserve artifact cache unless stale #1918

Merged
merged 41 commits into from
Nov 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
a563e25
make pruning explicit
eagr Oct 17, 2023
f31d354
preserve cache unless stale
eagr Oct 17, 2023
195bbce
barely working
eagr Oct 17, 2023
99aa012
use ArtifactId::from_file_name()
eagr Oct 17, 2023
17973ef
ignore non-unicode file names
eagr Oct 17, 2023
e81b456
generalize concat_const!()
eagr Oct 18, 2023
851a77a
per advices
eagr Oct 18, 2023
a3b0fcf
break on IO error
eagr Oct 18, 2023
b323c28
make pruning sound
eagr Oct 18, 2023
a2808f8
log more events
eagr Oct 18, 2023
0657d41
refactor
eagr Oct 20, 2023
0384ae7
doc
eagr Oct 20, 2023
4b1bb0a
Refactor indentation
mrcnski Oct 21, 2023
a8bcce4
refactor
eagr Oct 21, 2023
50b7ccc
checksum poc
eagr Oct 22, 2023
b6c1a07
Revert "checksum poc"
eagr Oct 30, 2023
2ad5262
redo checksum p1
eagr Oct 30, 2023
3723806
p2
eagr Oct 30, 2023
e44c451
remove corrupted cache
eagr Oct 31, 2023
6c19164
diversify results
eagr Nov 1, 2023
dad5285
fix tests
eagr Nov 1, 2023
1ede201
fix pruning
eagr Nov 1, 2023
2d51b52
fix message serialization
eagr Nov 1, 2023
36a33e8
clean up
eagr Nov 1, 2023
69f6a44
retire path_prefix()
eagr Nov 2, 2023
d3254f5
improve test
eagr Nov 3, 2023
f4d22fa
Merge branch 'master' into preserve-art
mrcnski Nov 12, 2023
39448ff
Fix test
mrcnski Nov 12, 2023
3a2c1cd
cargo fmt
mrcnski Nov 12, 2023
a02cb06
as per advices
eagr Nov 13, 2023
53e4557
tag artifact with runtime version
eagr Nov 13, 2023
d4f3083
fix tests
eagr Nov 13, 2023
9d2142e
Merge branch 'master' into preserve-art
mrcnski Nov 14, 2023
931aae1
upstream build fn to substrate
eagr Nov 15, 2023
5de4e8e
glitch
eagr Nov 15, 2023
a0b71c5
wrong attribution
eagr Nov 15, 2023
c85adfe
as per suggestions
eagr Nov 17, 2023
b897f1c
glitch
eagr Nov 17, 2023
89ada31
prevent `cargo tree` from accessing network
eagr Nov 17, 2023
868426a
glitch
eagr Nov 17, 2023
652bec7
Merge branch 'master' into preserve-art
eagr Nov 19, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 29 additions & 61 deletions polkadot/node/core/pvf/src/artifacts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,47 +71,12 @@ use std::{
time::{Duration, SystemTime},
};

// A workaround for defining a `const` that is a concatenation of other constants.
macro_rules! concat_const {
($($arg:expr),*) => {{
// ensure inputs to be strings
$(const _: &str = $arg;)*

const LEN: usize = 0 $(+ $arg.len())*;

// concatenate strings as byte slices
const CAT: [u8; LEN] = {
let mut cat = [0u8; LEN];
let mut _offset = 0;

$({
const BYTES: &[u8] = $arg.as_bytes();

let mut i = 0;
let len = BYTES.len();
while i < len {
cat[_offset + i] = BYTES[i];
i += 1;
}
_offset += len;
})*

cat
};

// The concatenation of two string slices is guaranteed to be valid UTF8,
// so are the byte slices as they have the same memory layout.
match std::str::from_utf8(&CAT) {
Ok(s) => s,
Err(_) => panic!("Error converting bytes to str"),
}
}}
}

const RUNTIME_PREFIX: &str = "wasmtime_v";
const NODE_PREFIX: &str = "polkadot_v";
const ARTIFACT_PREFIX: &str =
concat_const!(RUNTIME_PREFIX, RUNTIME_VERSION, "_", NODE_PREFIX, NODE_VERSION);

fn artifact_prefix() -> String {
format!("{}{}_{}{}", RUNTIME_PREFIX, RUNTIME_VERSION, NODE_PREFIX, NODE_VERSION)
}

/// Identifier of an artifact. Encodes a code hash of the PVF and a hash of executor parameter set.
#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
Expand All @@ -135,7 +100,10 @@ impl ArtifactId {
pub(crate) fn path(&self, cache_path: &Path, checksum: &str) -> PathBuf {
let file_name = format!(
"{}_{:#x}_{:#x}_0x{}",
ARTIFACT_PREFIX, self.code_hash, self.executor_params_hash, checksum
artifact_prefix(),
self.code_hash,
self.executor_params_hash,
checksum
);
cache_path.join(file_name)
}
Expand All @@ -144,7 +112,7 @@ impl ArtifactId {
/// Return `None` if the given file name is invalid.
/// VALID_NAME := <PREFIX> _ <CODE_HASH> _ <PARAM_HASH> _ <CHECKSUM>
fn from_file_name(file_name: &str) -> Option<Self> {
let file_name = file_name.strip_prefix(ARTIFACT_PREFIX)?.strip_prefix('_')?;
let file_name = file_name.strip_prefix(&artifact_prefix())?.strip_prefix('_')?;
let parts: Vec<&str> = file_name.split('_').collect();

if let [code_hash, param_hash, _checksum] = parts[..] {
Expand Down Expand Up @@ -240,11 +208,6 @@ impl Artifacts {

async fn insert_and_prune(&mut self, cache_path: &Path) {
async fn is_corrupted(path: &Path) -> bool {
let file_name = path
.file_name()
.expect("path should never terminate in '..'")
.to_str()
.expect("file name must be valid UTF-8");
let checksum = match tokio::fs::read(path).await {
Ok(bytes) => blake3::hash(&bytes),
Err(err) => {
Expand All @@ -258,7 +221,13 @@ impl Artifacts {
return true
},
};
!file_name.ends_with(checksum.to_hex().as_str())

if let Some(file_name) = path.file_name {
if let Some(file_name) = file_name.to_str() {
return !file_name.ends_with(checksum.to_hex().as_str())
}
}
false
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be true right? Could use a test for this case if we don't have one yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops! That's right. But this line of can only be reached when

  • the path end in "/..", or
  • the path contain non-Unicode

These are all screened out at the call site, so I don't even know how to write a test that could fail because of this line lol.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess is_corrupt can be extracted and unit-tested, but whatever, looks good already!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably stick to testing the interfaces, this kind of tests often become maintenance liability (prone to change). 😉

}

// Insert the entry into the artifacts table if it is valid.
Expand Down Expand Up @@ -375,6 +344,9 @@ impl Artifacts {
}

/// Insert an artifact with the given ID as "prepared".
///
/// This function should only be used to build the artifact table at startup with valid
/// artifact caches.
pub(crate) fn insert_prepared(
&mut self,
artifact_id: ArtifactId,
Expand Down Expand Up @@ -416,7 +388,7 @@ impl Artifacts {

#[cfg(test)]
mod tests {
use super::{ArtifactId, Artifacts, ARTIFACT_PREFIX, NODE_VERSION, RUNTIME_VERSION};
use super::{artifact_prefix as prefix, ArtifactId, Artifacts, NODE_VERSION, RUNTIME_VERSION};
use polkadot_primitives::ExecutorParamsHash;
use rand::Rng;
use sp_core::H256;
Expand All @@ -434,7 +406,7 @@ mod tests {
}

fn file_name(code_hash: &str, param_hash: &str, checksum: &str) -> String {
format!("{}_0x{}_0x{}_0x{}", ARTIFACT_PREFIX, code_hash, param_hash, checksum)
format!("{}_0x{}_0x{}_0x{}", prefix(), code_hash, param_hash, checksum)
}

fn create_artifact(
Expand Down Expand Up @@ -481,10 +453,7 @@ mod tests {

#[test]
fn artifact_prefix() {
assert_eq!(
ARTIFACT_PREFIX,
format!("wasmtime_v{}_polkadot_v{}", RUNTIME_VERSION, NODE_VERSION)
);
assert_eq!(prefix(), format!("wasmtime_v{}_polkadot_v{}", RUNTIME_VERSION, NODE_VERSION));
}

#[test]
Expand Down Expand Up @@ -533,29 +502,28 @@ mod tests {
let cache_dir = crate::worker_intf::tmppath("test-cache").await.unwrap();
fs::create_dir_all(&cache_dir).unwrap();

// 6 invalid

// invalid prefix
create_rand_artifact(&cache_dir, "");
create_rand_artifact(&cache_dir, "wasmtime_polkadot_v");
create_rand_artifact(&cache_dir, "wasmtime_v8.0.0_polkadot_v1.0.0");

let prefix = prefix();

// no checksum
create_rand_artifact(&cache_dir, ARTIFACT_PREFIX);
create_rand_artifact(&cache_dir, &prefix);

// invalid hashes
let (path, checksum) = create_artifact(&cache_dir, ARTIFACT_PREFIX, "000", "000001");
let (path, checksum) = create_artifact(&cache_dir, &prefix, "000", "000001");
let new_path = concluded_path(&path, &checksum);
fs::rename(&path, &new_path).unwrap();

// checksum reversed
let (path, checksum) = create_rand_artifact(&cache_dir, ARTIFACT_PREFIX);
let (path, checksum) = create_rand_artifact(&cache_dir, &prefix);
let new_path = concluded_path(&path, checksum.chars().rev().collect::<String>().as_str());
fs::rename(&path, &new_path).unwrap();

// 1 valid

let (path, checksum) = create_rand_artifact(&cache_dir, ARTIFACT_PREFIX);
// valid
let (path, checksum) = create_rand_artifact(&cache_dir, &prefix);
let new_path = concluded_path(&path, &checksum);
fs::rename(&path, &new_path).unwrap();

Expand Down
3 changes: 1 addition & 2 deletions polkadot/node/core/pvf/tests/it/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,7 @@ use assert_matches::assert_matches;
use parity_scale_codec::Encode as _;
use polkadot_node_core_pvf::{
start, testing::build_workers_and_get_paths, Config, InvalidCandidate, Metrics, PrepareError,
PrepareJobKind, PvfPrepData, ValidationError, ValidationHost,
JOB_TIMEOUT_WALL_CLOCK_FACTOR,
PrepareJobKind, PvfPrepData, ValidationError, ValidationHost, JOB_TIMEOUT_WALL_CLOCK_FACTOR,
};
use polkadot_parachain_primitives::primitives::{BlockData, ValidationParams, ValidationResult};
use polkadot_primitives::{ExecutorParam, ExecutorParams};
Expand Down
2 changes: 1 addition & 1 deletion polkadot/xcm/procedural/tests/ui.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
fn ui() {
// Only run the ui tests when `RUN_UI_TESTS` is set.
if std::env::var("RUN_UI_TESTS").is_err() {
return
return;
}

// As trybuild is using `cargo check`, we don't need the real WASM binaries.
Expand Down
4 changes: 3 additions & 1 deletion substrate/client/chain-spec/src/chain_spec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,9 @@ fn json_eval_value_at_key(
path: &mut VecDeque<&str>,
fun: &dyn Fn(&json::Value) -> bool,
) -> bool {
let Some(key) = path.pop_front() else { return false };
let Some(key) = path.pop_front() else {
return false;
};

if path.is_empty() {
doc.as_object().map_or(false, |o| o.get(key).map_or(false, |v| fun(v)))
Expand Down