Issue Verifying Proposal#130106

EnzoPlayer0ne · May 28, 2024, 6:48pm

We tried verifying proposal #130106 and ran into a couple of issues.

TLDR;

To successfully verify the proposal, we had to run the following commands which were not fully specified or specified with given assumptions :

For argument verification:

ubuntu@devenv-container:/ic$ /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/didc encode -d rs/rosetta-api/icp_ledger/ledger.did -t '(LedgerCanisterPayload)' '(variant { Upgrade = opt record { feature_flags = opt record {icrc2 = true} } })' | xxd -r -p > ~/ledger_arg.bin
ubuntu@devenv-container:/ic$ sha256sum ~/ledger_arg.bin # provided correctly in the proposal
9eb41ff666a127c661ed04551fbae12481cfe88a5e85143ef8ed504083f4a19e  /home/ubuntu/ledger_arg.bin

For new wasm hash verification:

ubuntu@devenv-container:/ic$ gunzip ./artifacts/canisters/ledger-canister_notify-method.wasm.gz
ubuntu@devenv-container:/ic$ sha256sum ./artifacts/canisters/ledger-canister_notify-method.wasm
f4bbb3b2fe29819f3317c6d108b4452c94c937d46d40f953f9367be869615bcc  ./artifacts/canisters/ledger-canister_notify-method.wasm

We also verified the hex representation of the upgrade hash indeed matches the one given with the didc command. Effectively establishing the link between the payload arg_hex field and the argument verification snippet:

ubuntu@devenv-container:/ic$ /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/didc encode -d rs/rosetta-api/icp_ledger/ledger.did -t '(LedgerCanisterPayload)' '(variant { Upgrade = opt record { feature_flags = opt record {icrc2 = true} } })'
4449444c166b02fcb88b840301b0ced184030a6e026c0397aabdbb0603f5e7d98a0904bea3d1c30f086e786e056c02b3b0dac30368ad86ca8305066e076d7b6e096c01c7bfe7b60b7e6c0d90c5a3a9010b9efeb9a4030cf2c794ae030daecbeb88047197aabdbb06038484d5c00703f1b6f985080fe0ab86ef0803f5e7d98a0904a1e5f7a10a11fdbacfcc0d1491c9aafe0d0cbea3d1c30f086d686e716e0e6c01e0a9b302786e106c02c287c2e20478bfb595b409796e126c089ea581d20178b2a7c2d20303a495a5e90678ffb08aab0813e0ab86ef0803e4d8cce80b0393c8e6c70c03dec5d8ae0e686e0b6d156c020071010e0100000100000101

Findings

Here is a list of our findings in partial order:

The argument verification part needs to be run inside the container with container-run.sh, which is never mentioned.
Still, in the argument verification part, didc is not in the PATH of the container. Rather it needs to be run via Bazel. However, no command is given, and the exercise is left to the reader. Instead of didc, it should mention bazel run @candid//:didc

ubuntu@devenv-container:/ic$ didc
bash: didc: command not found
ubuntu@devenv-container:/ic$ bazel run @candid//:didc help
INFO: Analyzed target @candid//:didc (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
WARNING: /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/BUILD.bazel:3:14: @candid//:didc is a source file, nothing will be built for it. If you want to build a target that consumes this file, try --compile_one_dependency
INFO: Elapsed time: 0.042s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/didc help
Usage: didc <COMMAND>

Commands:
  check    Type check Candid file
  bind     Generate binding for different languages
  test     Generate test suites for different languages
  hash     Compute the hash of a field name
  encode   Encode Candid value
  decode   Decode Candid binary data
  random   Generate random Candid values
  subtype  Check for subtyping
  help     Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help
  -V, --version  Print version

Arguments containing dashe(s) are not passed properly to the didc binary via the above Bazel command. See the examples below:

here we try a command that does contain dashes didc help, and it executes successfully

ubuntu@devenv-container:/ic$ bazel run @candid//:didc help
INFO: Analyzed target @candid//:didc (0 packages loaded, 0 targets configured).
INFO: Found 1 target...
WARNING: /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/BUILD.bazel:3:14: @candid//:didc is a source file, nothing will be built for it. If you want to build a target that consumes this file, try --compile_one_dependency
INFO: Elapsed time: 0.049s, Critical Path: 0.01s
INFO: 1 process: 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/didc help
Usage: didc <COMMAND>

Commands:
  check    Type check Candid file
  bind     Generate binding for different languages
  test     Generate test suites for different languages
  hash     Compute the hash of a field name
  encode   Encode Candid value
  decode   Decode Candid binary data
  random   Generate random Candid values
  subtype  Check for subtyping
  help     Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help
  -V, --version  Print version

Here, we run the same command that should be available but does not work:

ubuntu@devenv-container:/ic$ bazel run @candid//:didc --help
ERROR: --help :: Unrecognized option: --help

we try the same again but with the actual binary rather than with the bazel command and it does work

ubuntu@devenv-container:/ic$ /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/didc --help
Usage: didc <COMMAND>

Commands:
  check    Type check Candid file
  bind     Generate binding for different languages
  test     Generate test suites for different languages
  hash     Compute the hash of a field name
  encode   Encode Candid value
  decode   Decode Candid binary data
  random   Generate random Candid values
  subtype  Check for subtyping
  help     Print this message or the help of the given subcommand(s)

Options:
  -h, --help     Print help
  -V, --version  Print version

The latest candid release does not contain the SHASUMS, thus we cannot check if what we have downloaded is indeed what we should have downloaded, see the screenshot below:

image2634×1134 194 KB
didc in the container is not at latest version of 0.4.0 but rather at 0.3.5

ubuntu@devenv-container:/ic$ /home/ubuntu/.cache/bazel/_bazel_ubuntu/6d065581cce7ad9076e3b8db2b3afaf0/external/candid/didc
--version
didc 0.3.5

There is no verification step to check the new wasm hash part (f4bbb3b2fe29819f3317c6d108b4452c94c937d46d40f953f9367be869615bcc) is correct. Normally, if the gzip file has the correct hash, so should the wasm; however, given the new hash is mentioned, we believe it should also have a verification step.
As a result, we ran the commands below:

ubuntu@devenv-container:/ic$ gunzip ./artifacts/canisters/ledger-canister_notify-method.wasm.gz
ubuntu@devenv-container:/ic$ sha256sum ./artifacts/canisters/ledger-canister_notify-method.wasm
f4bbb3b2fe29819f3317c6d108b4452c94c937d46d40f953f9367be869615bcc  ./artifacts/canisters/ledger-canister_notify-method.wasm

Bazel actively tries to connect to some internal servers

ubuntu@devenv-container:/ic$ sha256sum ./artifacts/canisters/ledger-canister_notify-method.wasm
f4bbb3b2fe29819f3317c6d108b4452c94c937d46d40f953f9367be869615bcc  ./artifacts/canisters/ledger-canister_notify-method.wasm
ubuntu@devenv-container:/ic$ bazel run @candid//:didc encode -d rs/rosetta-api/icp_ledger/ledger.did -t '(LedgerCanisterPayload)' '(variant { Upgrade = opt record { feature_flags = opt record {icrc2 = true} } })' | xxd -r -p > ~/ledger_arg.bin
ERROR: -d :: Unrecognized option: -d
INFO: Invocation ID: 4020d313-a32f-4925-be42-af54aaeebb50
WARNING: Failed to query remote execution capabilities: UNAVAILABLE: io exception
INFO: Streaming build results to: https://dash.idx.dfinity.network/invocation/4020d313-a32f-4925-be42-af54aaeebb50

by looking at the .bazelrc file we can see the following configurations options

build --bes_results_url=https://dash.idx.dfinity.network/invocation/
build --bes_backend=bes.idx.dfinity.network
build --bes_timeout=60s # Default is no timeout.
build --bes_upload_mode=wait_for_upload_complete
build:ci --bes_timeout=180s # Default is no timeout.
build:ci --bes_upload_mode=fully_async
build --experimental_remote_build_event_upload=minimal

However, a few lines below, we also see

build --remote_local_fallback

Which should per SourceGraph comments:

# Fall back to standalone local execution strategy if remote execution fails. If the grpc remote
# cache connection fails, it will fail the build, add this so it falls back to the local cache.
# Docs: https://bazel.build/reference/command-line-reference#flag--remote_local_fallback

Hence, the above error should not have happened in the .bazelrc configuration file.

We tried adding the following configuration options and --config=local to our command per the following commit:

run:local --remote_cache=
run:local --bes_backend=

This did not work as well!

To make it work we had to modify the configuration file to look like so:

ubuntu@devenv-container:/ic$ git diff
diff --git a/.bazelrc b/.bazelrc
index 8896f59b7f..e5d01c3e21 100644
--- a/.bazelrc
+++ b/.bazelrc
@@ -37,33 +37,10 @@ build --strip=never

 build --strategy_regexp=ic-os/.*=local

-build --remote_cache=bazel-remote.idx.dfinity.network
-build --experimental_remote_cache_async
-build --experimental_remote_cache_compression # If enabled, compress/decompress cache blobs with zstd.
-build --remote_timeout=60s # Default is also 60s but we set it explicitly to remind ourselves of this timeout.
-build:ci --remote_timeout=5m # Default is 60s.
-# TODO: re-enable after fixing the error like this:
-# `Failed to fetch file with hash 'xxx' because it does not exist remotely. --remote_download_outputs=minimal does not work if your remote cache evicts files during builds.`
-# Probably disabling `--experimental_remote_cache_async` will help
-#build --remote_download_minimal # https://bazel.build/reference/command-line-reference#flag--remote_download_minimal
-#build --remote_download_outputs=toplevel # Still download outputs from top level targets.
-
-build --experimental_remote_downloader=bazel-remote.idx.dfinity.network --experimental_remote_downloader_local_fallback
-build:local --experimental_remote_downloader=
-
 # Does not produce valid JSON. See https://github.com/bazelbuild/bazel/issues/14209
 build --execution_log_json_file=bazel-build-log.json
 build:ci --build_event_binary_file=bazel-bep.pb

-build --bes_results_url=https://dash.idx.dfinity.network/invocation/
-build --bes_backend=bes.idx.dfinity.network
-build --bes_timeout=60s # Default is no timeout.
-build --bes_upload_mode=wait_for_upload_complete
-build:ci --bes_timeout=180s # Default is no timeout.
-build:ci --bes_upload_mode=fully_async
-build --experimental_remote_build_event_upload=minimal
-
-build --remote_local_fallback
 build --workspace_status_command=$(pwd)/bazel/workspace_status.sh

 build --experimental_repository_downloader_retries=3 # https://bazel.build/reference/command-line-reference#flag--experimental_repository_downloader_retries
@@ -90,10 +67,8 @@ test --test_env=RUST_BACKTRACE=full

 test:precommit --build_tests_only --test_tag_filters="smoke"

-build:systest --build_tag_filters= --s3_endpoint=https://s3-upload.idx.dfinity.network
 test:systest --test_output=streamed --test_tag_filters=

-build:testnet --build_tag_filters= --s3_endpoint=https://s3-upload.idx.dfinity.network --ic_version_rc_only=
 test:testnet --test_output=streamed --test_tag_filters=

which did indeed work:

ubuntu@devenv-container:/ic$ bazel run @candid//:didc encode -d rs/rosetta-api/icp_ledger/ledger.did -t '(LedgerCanisterPayload)' '(variant { Upgrade = opt record { feature_flags = opt record {icrc2 = true} } })'  --config=local | xxd -r -p > ~/ledger_arg.bin
Starting local Bazel server and connecting to it...
ERROR: -d :: Unrecognized option: -d

While looking at the configuration file we found that build --experimental_remote_build_event_upload=minimal is a deprecation option as referenced in the Bazel source code. It should rather use remote_build_event_upload.

This was done during a Twitter live stream, and I want to thank the 200 people who looked at me reading Bazel code

wpb · May 29, 2024, 2:35am

You did a fantastic job with this review @EnzoPlayer0ne. I was really cool to see your thought process and hacking skills as you stepped through the review during the WaterNeuron Twitter Live event. This is the most exciting thing about WaterNeuron in my opinion. You and @0rions plan to actively contribute to the decentralization of NNS governance on technical proposals and are building a liquid staking protocol that will provide voting power that can make WaterNeuron contributions relevant.

I think that your review today highlighted the fact that reviewing technical proposals is real work that requires skilled developers. Not only do they need to know rust, which is a less common language in general, it also requires that developers are routinely committed to reviews and independent voting multiple times every week. That’s a pretty big commitment and I hope to see incentives offered by the NNS in the future to help fund people with the skills needed to perform this work. I look forward to learning more about the automation tools you plan to develop to help minimize the time it takes.

@ZackDS @Zane @massimoalbarello @cyberowl @tiago89 @Lorimer @ilbert @hpeebles this review is relevant for proposal 130106, which is a system canister management proposal to upgrade the NNS Ledger canister. This proposal is live at this time and the CodeGov team plans to review it within the next 36 hours. You might want to take a look at Enzo’s review to see some of the issues he identified. Unfortunately, the Twitter Live didn’t seem to save, so I don’t’ think it is possible to watch his recording.

Manu · May 29, 2024, 8:13am

I also checked this proposal. The instructions for verifying the arguments seem to be incorrect, it should be

didc encode -d rs/rosetta-api/icp_ledger/ledger.did -t '(LedgerCanisterPayload)' '(variant { Upgrade = opt record { feature_flags = opt record {icrc2 = true} } })'

Wrt @EnzoPlayer0ne’s feedback:
I think it’s fair feedback that the proposal should explain where didc comes from, but the answer is pretty simple, namely get it from GitHub - dfinity/candid: Candid Library for the Internet Computer (either pre-built or if you want to double check everything, do a cargo build there). Once you have didc, you don’t need container-run.sh.

andrea · May 29, 2024, 9:35am

Thanks a lot for the feedback @EnzoPlayer0ne!

Indeed the proposal is a bit imprecise: while the proposal text refers to the argument hash, the payload does not actually specify this. What should be checked instead is that the encoding of the arguments matches the arg_hex in the payload. As @Manu pointed out, the encoding of the args can be computed using the didc command included in the instructions (before the pipe), and this only require didc, does not have to run in the container.

We’ll try to improve the instructions for next proposal, remove the argument hash, and pointing to didc.

timk11 · August 9, 2024, 2:26pm

Hey there @andrea . Should this change also be made for the current proposal 131701? I’ve made further comment in this post within CodeGov’s SCM proposal review channel.

andrea · August 12, 2024, 2:42pm

Thanks for reporting it, we did fix it for the previous proposal but somehow this was reverted in our tooling or process. We’ll fix it again for the next one

Topic		Replies	Views
NNS Update: October 19, 2023: enabling ICRC-2 on the ICP ledger Developers nns	10	1472	November 8, 2023
Ledger canister version on mainnet Governance	3	403	April 18, 2023
Ledger-transfer demo cannot be deployed Language Support Motoko , Wasm	9	399	July 19, 2023
ICP Ledger Canister Suite Upgrades 2024-08-23 Governance	0	40	August 23, 2024
Latest nns ledger wasm/candid for local testing Getting Started	4	356	July 22, 2023

Issue Verifying Proposal#130106

TLDR;

Findings

Related Topics