-
Notifications
You must be signed in to change notification settings - Fork 372
fix: ensure reproducibility of the ledger_suite_orchestrator_canister #5347
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| load("@rules_rust//cargo:defs.bzl", "cargo_build_script") | ||
| load("@rules_rust//rust:defs.bzl", "rust_doc", "rust_library", "rust_test") | ||
| load("//bazel:canisters.bzl", "rust_canister") | ||
| load("//bazel:defs.bzl", "rust_ic_test") | ||
|
|
@@ -100,6 +101,15 @@ rust_test( | |
| ], | ||
| ) | ||
|
|
||
| cargo_build_script( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @basvandijk : If I understand that failure correctly, this means that it doesn't solve the problem or there is another source of non-determinism?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you mean? That this PR is insufficient?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mraszyk The description of the PR states
However, that PR seems to suffer from the same problem
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes I think I was too soon turning this from draft into a PR. There seems to be another source of non-determinism.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the logs it seems that there is a non-determinism with the archive. This would definitely produce non-determinism in the ledger (because it embeds its wasm) and in the ledger suite orchestrator (which embeds among other also the archive wasm)
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gregorydemay we're embedding other canisters into the ledger_suite_orchestrator_canister right? Are those other canisters also using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
yes that's correct, they are defined here
I don't think so, none of the canisters in the ledger suite have a dependency on
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @basvandijk To me the root cause of the non-determinism seen here could be explained by the archive since this would trigger non-determinism for ledger_u256 and the ledger suite orchestrator.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you're right the archive canister is the source. I think I jumped to conclusions too soon since we have been suffering from non-determinism from a very similar use of |
||
| name = "build_script", | ||
| srcs = ["build.rs"], | ||
| build_script_env = { | ||
| "IN_BAZEL": "1", | ||
| }, | ||
| data = ["templates/dashboard.html"], | ||
| ) | ||
|
|
||
| [ | ||
| rust_canister( | ||
| name = "ledger_suite_orchestrator_canister" + name_suffix, | ||
|
|
@@ -109,9 +119,6 @@ rust_test( | |
| "src/dashboard/tests.rs", | ||
| "src/main.rs", | ||
| ], | ||
| compile_data = [ | ||
| "templates/dashboard.html", | ||
| ], | ||
| crate_name = "ic_ledger_suite_orchestrator_canister" + name_suffix, | ||
| opt = "z", | ||
| proc_macro_deps = [ | ||
|
|
@@ -120,6 +127,7 @@ rust_test( | |
| service_file = "ledger_suite_orchestrator.did", | ||
| deps = [ | ||
| # Keep sorted. | ||
| ":build_script", | ||
| ":ledger_suite_orchestrator" + name_suffix, | ||
| "//packages/ic-http-types", | ||
| "@crate_index//:askama", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,29 +1,68 @@ | ||
| use std::env::{self}; | ||
| use std::fs::File; | ||
| use std::io::Write; | ||
| use std::path::PathBuf; | ||
|
|
||
| fn main() { | ||
| let cargo_manifest_dir = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()); | ||
| let compile_time_env_variables = [ | ||
| "LEDGER_CANISTER_WASM_PATH", | ||
| "INDEX_CANISTER_WASM_PATH", | ||
| "LEDGER_ARCHIVE_NODE_CANISTER_WASM_PATH", | ||
| ]; | ||
| for env_var in compile_time_env_variables { | ||
| let archive_path = match env::var_os(env_var) { | ||
| Some(wasm_path) => PathBuf::from(wasm_path), | ||
| None => cargo_manifest_dir | ||
| // This is a hack. | ||
| // Cargo is called on CI via ci/src/rust_lint/lint.sh. | ||
| // The included WASMS binary for ledger, index and archive canisters are built by BAZEL tasks | ||
| // which would need here to be somehow spawned by Cargo. To avoid this, we just use a wasm binary that | ||
| // happens to be already checked-in in the repo. | ||
| .join("../../ledger_suite/icrc1/wasm/ic-icrc1-archive.wasm.gz") | ||
| .canonicalize() | ||
| .expect("failed to canonicalize a path"), | ||
| }; | ||
| if env::var_os("IN_BAZEL").is_none() { | ||
| let cargo_manifest_dir = PathBuf::from(env::var_os("CARGO_MANIFEST_DIR").unwrap()); | ||
| let compile_time_env_variables = [ | ||
| "LEDGER_CANISTER_WASM_PATH", | ||
| "INDEX_CANISTER_WASM_PATH", | ||
| "LEDGER_ARCHIVE_NODE_CANISTER_WASM_PATH", | ||
| ]; | ||
| for env_var in compile_time_env_variables { | ||
| let archive_path = match env::var_os(env_var) { | ||
| Some(wasm_path) => PathBuf::from(wasm_path), | ||
| None => cargo_manifest_dir | ||
| // This is a hack. | ||
| // Cargo is called on CI via ci/src/rust_lint/lint.sh. | ||
| // The included WASMS binary for ledger, index and archive canisters are built by BAZEL tasks | ||
| // which would need here to be somehow spawned by Cargo. To avoid this, we just use a wasm binary that | ||
| // happens to be already checked-in in the repo. | ||
| .join("../../ledger_suite/icrc1/wasm/ic-icrc1-archive.wasm.gz") | ||
| .canonicalize() | ||
| .expect("failed to canonicalize a path"), | ||
| }; | ||
|
|
||
| println!("cargo:rerun-if-changed={}", archive_path.display()); | ||
| println!("cargo:rerun-if-env-changed={}", env_var); | ||
| println!("cargo:rustc-env={}={}", env_var, archive_path.display()); | ||
| println!("cargo:rerun-if-changed={}", archive_path.display()); | ||
| println!("cargo:rerun-if-env-changed={}", env_var); | ||
| println!("cargo:rustc-env={}={}", env_var, archive_path.display()); | ||
| } | ||
| } | ||
|
|
||
| // Build reproducibility. askama adds a include_bytes! call when it's generating | ||
| // a template impl so that rustc will recompile the module when the file changes | ||
| // on disk. See https://github.com/djc/askama/blob/180696053833147a61b3348646a953e7d92ae582/askama_shared/src/generator.rs#L141 | ||
| // The stringified output of every proc-macro is added to the metadata hash for | ||
| // a crate. That output includes the full filepath to include_bytes!. It may be | ||
| // different on two machines, if they use different tempdir paths for the build. | ||
| // The metadata hash is an input to generated symbol names. | ||
| // So using the askama proc-macro could result in slightly different symbols. | ||
| // However, if we include the html source directly in the output, no | ||
| // inconsistency is introduced. | ||
| // | ||
| // This should really be fixed in askama. See: | ||
| // https://github.com/askama-rs/askama/issues/461 | ||
| println!("cargo:rerun-if-changed=templates/dashboard.html"); | ||
| let mut f = File::create( | ||
| PathBuf::from(std::env::var("OUT_DIR").unwrap()).join("dashboard_template.rs"), | ||
| ) | ||
| .unwrap(); | ||
| f.write_all( | ||
| format!( | ||
| r#" | ||
| #[derive(Template)] | ||
| #[template(escape = "html", source = {:?}, ext = "html")] | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. understanding question: why was this line added? |
||
| pub struct DashboardTemplate {{ | ||
| managed_canisters: BTreeMap<Erc20Token, CanistersDashboardData>, | ||
| other_canisters: BTreeMap<String, Vec<CanisterDashboardData>>, | ||
| wasm_store: Vec<DashboardStoredWasm>, | ||
| }} | ||
| "#, | ||
| std::fs::read_to_string("templates/dashboard.html").unwrap() | ||
| ) | ||
| .as_bytes(), | ||
| ) | ||
| .unwrap(); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understanding question: we use
askamafor other canisters, are they also not a problem for the reproducibility issue?