[randomness] Add entry function annotation #12134

georgemitenkov · 2024-02-21T17:04:25Z

Description

This adds #[randomness] annotation which should be used to mark entry functions which use randomness. For example

#[randomness]
entry fun foo() {
  let _ = randomness::u64_integer();
}

The attribute has the following semantics:

It can only be used for entry functions. Using it on other type of functions is not allowed.
It can be used on entry functions even if they don't use randomness.
If an entry function doesn't have an annotation, but uses randomness, the randomness call fails at runtime.

This attribute allows to specialise the prologue to deposit gas, simply by checking if this attribute exists. If not but randomness is used, we get a runtime failure. Otherwise, we use a deposit-refund prologue-epilogue (and it doesn't matter if function uses randomness or not).

Test Plan

Move e2e tests.

trunk-io · 2024-02-21T17:04:29Z

⏱️ 24h 5m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
rust-unit-coverage	4h 28m	🟩
rust-smoke-coverage	3h 51m	🟩
windows-build	2h 11m	🟩 🟩 🟥 🟩 🟥 (+2 more)
rust-move-unit-coverage	1h 58m	⬜ 🟩 🟩 🟥 🟩
execution-performance / single-node-performance	1h 51m	🟥 🟥 🟥 🟥 🟩 (+1 more)
rust-unit-tests	1h 46m	⬜ 🟩 🟩 🟥 🟩
rust-smoke-tests	1h 28m	⬜ 🟥 🟥 🟥 🟥
rust-move-tests	1h 7m	⬜ 🟩 🟩 🟥 🟩
rust-images / rust-all	1h	🟩 🟩 🟩 🟥 🟩 (+1 more)
forge-e2e-test / forge	56m	🟩 🟩 🟩 🟩
forge-compat-test / forge	52m	🟩 🟩 🟩 🟩
run-tests-main-branch	38m	🟥 🟥 🟥 🟥 🟥 (+1 more)
rust-lints	30m	⬜ 🟩 🟩 🟥 🟩
cli-e2e-tests / run-cli-tests	29m	🟥 🟥 🟩 🟩
check	17m	⬜ 🟩 🟩 🟩 🟩
check-dynamic-deps	11m	🟩 🟩 🟩 🟩 🟩 (+1 more)
general-lints	10m	⬜ 🟩 🟩 🟩 🟩
indexer-grpc-e2e-tests / test-indexer-grpc-docker-compose	8m	🟩 🟩 🟥 🟩
node-api-compatibility-tests / node-api-compatibility-tests	3m	🟩 🟩 🟩 🟩
semgrep/ci	3m	🟩 🟩 🟩 🟩 🟩 (+3 more)
execution-performance / file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	57s	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	53s	🟩 🟩 🟩 🟩 🟩
permission-check	23s	🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check	21s	🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check	20s	🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check	18s	🟩 🟩 🟩 🟩 🟩 (+2 more)
permission-check	14s	🟩 🟩 🟩 🟩 🟩 (+2 more)
upload-to-codecov	12s	🟩
determine-docker-build-metadata	11s	🟩 🟩 🟩 🟩 🟩 (+1 more)

🚨 2 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg	Delta
run-tests-main-branch	6m	4m
rust-move-unit-coverage	37m	30m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

codecov · 2024-02-21T17:46:21Z

Codecov Report

Attention: Patch coverage is 34.84848% with 86 lines in your changes are missing coverage. Please review.

Project coverage is 63.9%. Comparing base (d6c174c) to head (0514b94).
Report is 7 commits behind head on main.

Files	Patch %	Lines
aptos-move/framework/src/module_metadata.rs	0.0%	28 Missing ⚠️
aptos-move/aptos-vm/src/verifier/randomness.rs	0.0%	17 Missing ⚠️
aptos-move/framework/src/extended_checks.rs	38.4%	16 Missing ⚠️
aptos-move/aptos-vm/src/aptos_vm.rs	0.0%	13 Missing ⚠️
...d_party/move/move-binary-format/src/file_format.rs	0.0%	5 Missing ⚠️
third_party/move/move-vm/runtime/src/session.rs	0.0%	4 Missing ⚠️
aptos-move/framework/src/natives/randomness.rs	88.8%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             main   #12134     +/-   ##
=========================================
- Coverage    63.9%    63.9%   -0.1%     
=========================================
  Files         807      812      +5     
  Lines      178311   180090   +1779     
=========================================
+ Hits       114052   115116   +1064     
- Misses      64259    64974    +715

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alinush

Amazing! 🙏

vgao1996 · 2024-02-22T02:33:28Z

aptos-move/aptos-vm/src/verifier/uses_randomness.rs

+    let module_bytes = session.load_module(entry_fn.module())?;
+    let module = CompiledModule::deserialize_with_config(
+        &module_bytes,
+        &session.get_vm_config().deserializer_config,
+    )
+    .map_err(|e| e.finish(Location::Undefined))?;


Wonder if this would have perf issues

Yeah, single node perf is failing
https://github.com/aptos-labs/aptos-core/actions/runs/7993394538/job/21829130781?pr=12134

davidiw · 2024-02-23T09:19:29Z

Curious if there's a design doc, aip, or something here? I suspect the issue is we're depositing something like 1 storage slot fee and so if the app doesn't need it, it shouldn't be taken? do we actually need this? the devx is a bit frustrating. Imagine the case where I move from a pseudorandom function to our random function within a library, I would break every application that uses my library. Curious if we have evaluated other options, such as requiring the transaction sender to set a bit on randomness or just not doing this at all? An AIP would be useful to facilitate community feedback on devx.

georgemitenkov · 2024-02-23T16:55:20Z

Storage fees? Do you mean the deposits?

We had a discussion with @alinush @vgao1996 @zekun000 @danielxiangzl @igor-aptos @msmouse, but no real design doc (though there is a summary of the discussion). The goal is to make randomness unbiasable, but a good callout about AIP. Let's discuss on slack.

If you set a bit, you cannot check it at runtime when you make randomness call (unless we expose it via context). Not doing at all leads to test and abort attack?

alinush · 2024-02-23T17:28:43Z

@davidiw, this annotation is already part of AIP-41 here.

zekun000

like those tests!

zekun000 · 2024-03-12T05:38:19Z

aptos-move/aptos-vm/src/verifier/randomness.rs

+) -> VMResult<bool> {
+    let module = session
+        .get_move_vm()
+        .load_module(entry_fn.module(), resolver)?;


it's a bit weird that this would require resolver, isn't the session already holding a module_store?

Yes, I was trying to re-use it from the session, but I think the problem is that we store a trait object dyn AptosMoveResolver in session, which rust doesn't allow to upcast to MoveResolver (unless I add AsBaseMoveResolver trait or similar), so I kept it this way. Will give it another try!

github-actions · 2024-03-13T15:33:14Z

✅ Forge suite `compat` success on `aptos-node-v1.9.5` ==> `0514b9480af8b4301b811f1334c3b42557c1137d`

Compatibility test results for aptos-node-v1.9.5 ==> 0514b9480af8b4301b811f1334c3b42557c1137d (PR)
1. Check liveness of validators at old version: aptos-node-v1.9.5
compatibility::simple-validator-upgrade::liveness-check : committed: 5972 txn/s, latency: 5017 ms, (p50: 5000 ms, p90: 8100 ms, p99: 10800 ms), latency samples: 232920
2. Upgrading first Validator to new version: 0514b9480af8b4301b811f1334c3b42557c1137d
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 711 txn/s, latency: 34185 ms, (p50: 36600 ms, p90: 55800 ms, p99: 57200 ms), latency samples: 57620
3. Upgrading rest of first batch to new version: 0514b9480af8b4301b811f1334c3b42557c1137d
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 256 txn/s, submitted: 562 txn/s, expired: 306 txn/s, latency: 28084 ms, (p50: 29600 ms, p90: 45200 ms, p99: 59700 ms), latency samples: 23090
4. upgrading second batch to new version: 0514b9480af8b4301b811f1334c3b42557c1137d
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 2299 txn/s, latency: 12474 ms, (p50: 12800 ms, p90: 17200 ms, p99: 18400 ms), latency samples: 110360
5. check swarm health
Compatibility test for aptos-node-v1.9.5 ==> 0514b9480af8b4301b811f1334c3b42557c1137d passed
Test Ok

github-actions · 2024-03-13T15:34:10Z

✅ Forge suite `realistic_env_max_load` success on `0514b9480af8b4301b811f1334c3b42557c1137d`

two traffics test: inner traffic : committed: 7701 txn/s, latency: 5093 ms, (p50: 5000 ms, p90: 6000 ms, p99: 9500 ms), latency samples: 3327180
two traffics test : committed: 100 txn/s, latency: 1895 ms, (p50: 1800 ms, p90: 2200 ms, p99: 3600 ms), latency samples: 1760
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.248, avg: 0.205", "QsPosToProposal: max: 0.300, avg: 0.280", "ConsensusProposalToOrdered: max: 0.482, avg: 0.433", "ConsensusOrderedToCommit: max: 0.300, avg: 0.285", "ConsensusProposalToCommit: max: 0.728, avg: 0.719"]
Max round gap was 1 [limit 4] at version 1542496. Max no progress secs was 4.108862 [limit 15] at version 1542496.
Test Ok

georgemitenkov added the CICD:run-e2e-tests when this label is present github actions will run all land-blocking e2e tests from the PR label Feb 21, 2024

georgemitenkov requested review from alinush, zjma and danielxiangzl February 21, 2024 17:04

georgemitenkov requested review from davidiw, movekevin, wrwg, zekun000 and vgao1996 as code owners February 21, 2024 17:04

georgemitenkov force-pushed the george/randomness/annotation branch from 07a894e to a8da2b5 Compare February 21, 2024 17:05