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

[Prover] add support of abort in spec function #14939

Merged
merged 3 commits into from
Oct 16, 2024
Merged

Conversation

rahxephon89
Copy link
Contributor

@rahxephon89 rahxephon89 commented Oct 11, 2024

Description

Currently, when rewriting a Move expression into a spec expression, abort will be rewritten into empty () expression. Later when generating boogie, the error Tuple not yet supported will be raised. This PR introduces uninterpreted functions to the Boogie native for each type generated in the program and abort operation is translated into calling the corresponding function based on its type.

close #14793

How Has This Been Tested?

  1. Existing tests pass;
  2. Add a new test case to the prover test.

Key Areas to Review

Type of Change

  • New feature
  • Bug fix
  • Breaking change
  • Performance improvement
  • Refactoring
  • Dependency update
  • Documentation update
  • Tests

Which Components or Systems Does This Change Impact?

  • Validator Node
  • Full Node (API, Indexer, etc.)
  • Move/Aptos Virtual Machine
  • Aptos Framework
  • Aptos CLI/SDK
  • Developer Infrastructure
  • Move Compiler
  • Prover

Checklist

  • I have read and followed the CONTRIBUTING doc
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I identified and added all stakeholders and component owners affected by this change as reviewers
  • I tested both happy and unhappy path of the functionality
  • I have made corresponding changes to the documentation

Copy link

trunk-io bot commented Oct 11, 2024

⏱️ 6h 27m total CI duration on this PR
Slowest 15 Jobs Cumulative Duration Recent Runs
execution-performance / single-node-performance 42m 🟩🟩
dispatch_event 19m 🟩
rust-targeted-unit-tests 19m 🟩
rust-move-unit-coverage 17m 🟥🟩
rust-cargo-deny 16m 🟩🟩🟩🟩 (+5 more)
dispatch_event 16m 🟩
dispatch_event 16m 🟩
dispatch_event 15m 🟩
dispatch_event 15m 🟩
rust-move-unit-coverage 15m 🟩
rust-move-unit-coverage 15m 🟩
rust-move-unit-coverage 14m 🟩
rust-move-tests 11m 🟩
rust-move-unit-coverage 10m 🟩
rust-move-tests 10m 🟩

settingsfeedbackdocs ⋅ learn more about trunk.io

Copy link
Contributor Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @rahxephon89 and the rest of your teammates on Graphite Graphite

@rahxephon89 rahxephon89 changed the title [single-node-performance] Add runner information in the output (#14932) [WIP] add support of abort in spec and spec function Oct 11, 2024
@rahxephon89 rahxephon89 changed the title [WIP] add support of abort in spec and spec function [WIP][Prover] add support of abort in spec and spec function Oct 11, 2024
Copy link

codecov bot commented Oct 11, 2024

Codecov Report

Attention: Patch coverage is 74.50980% with 13 lines in your changes missing coverage. Please review.

Project coverage is 60.1%. Comparing base (b481f94) to head (fe7f555).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
.../move-prover/boogie-backend/src/spec_translator.rs 56.6% 13 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #14939   +/-   ##
=======================================
  Coverage    60.1%    60.1%           
=======================================
  Files         856      856           
  Lines      211035   211086   +51     
=======================================
+ Hits       126896   126938   +42     
- Misses      84139    84148    +9     

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

@rahxephon89 rahxephon89 force-pushed the teng/fix-14793 branch 3 times, most recently from c06c408 to 6142f77 Compare October 11, 2024 21:42
@rahxephon89 rahxephon89 changed the title [WIP][Prover] add support of abort in spec and spec function [WIP][Prover] add support of abort in spec function Oct 11, 2024
@rahxephon89 rahxephon89 changed the title [WIP][Prover] add support of abort in spec function [Prover] add support of abort in spec function Oct 11, 2024
@rahxephon89 rahxephon89 marked this pull request as ready for review October 11, 2024 23:15
Copy link
Contributor

@wrwg wrwg left a comment

Choose a reason for hiding this comment

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

Nice solution

{%- set S = "'" ~ instance.suffix ~ "'" -%}
{%- set T = instance.name -%}

function $Uninterpreted_value_of_{{S}}(): {{T}};
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is a bit of off, because "uninterpreted value" does not make a lot of sense in contrast to "uninterpreted function". Perhaps call this "$Arbitrary_value_of"

| ExpData::Mutate(id, ..)
| ExpData::SpecBlock(id, ..)
| ExpData::LoopCont(id, ..) => {
self.env.error(
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a test that illustrates this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -184,6 +184,27 @@ pub fn add_prelude(
sh_instances = vec![];
bv_instances = vec![];
}

let mut all_types = mono_info
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this going to be very big? Is mono_info every type in the program?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For aptos-framework, there are about 500 uninterpreted functions generated while the generated boogie code is about 300k line. So IMHO as long as the program itself is reasonable, the number of types in the program is not a big concern.

@@ -991,15 +996,31 @@ impl<'env> SpecTranslator<'env> {
self.translate_call(node_id, oper, &[args[args.len() - 1].clone()]);
emit!(self.writer, &")".repeat(count));
},
Operation::Abort => {
let exp_bv_flag = global_state.get_node_num_oper(node_id) == Bitwise;
emit!(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we show this error, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think you mean the error below. All operations should already be handled in early stages so it is hard to add a test case to generate this error.

Copy link
Contributor

@junkil-park junkil-park left a comment

Choose a reason for hiding this comment

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

I understand that it's fine to use such Move functions (which potentially abort) in ensures specs because they are evaluated under the condition that the function does not abort.

However, I haven't fully grasped the use of such functions in aborts_if specs yet. Nevertheless, this shouldn't be an issue as long as we clearly explain this behavior to users in the documentation.

igor-aptos and others added 2 commits October 15, 2024 17:32
* [single-node-performance] Add runner information in the output

* adding skip move e2e

* recalibration
@rahxephon89 rahxephon89 enabled auto-merge (squash) October 16, 2024 03:20
@rahxephon89 rahxephon89 disabled auto-merge October 16, 2024 03:21

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@rahxephon89 rahxephon89 enabled auto-merge (squash) October 16, 2024 03:50

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Copy link
Contributor

✅ Forge suite compat success on 7eeba4cd15892717741a614add1afde004c7855f ==> fe7f555381e5d5d0ef8583daf80dc581240a5950

Compatibility test results for 7eeba4cd15892717741a614add1afde004c7855f ==> fe7f555381e5d5d0ef8583daf80dc581240a5950 (PR)
1. Check liveness of validators at old version: 7eeba4cd15892717741a614add1afde004c7855f
compatibility::simple-validator-upgrade::liveness-check : committed: 12713.56 txn/s, latency: 2651.53 ms, (p50: 2100 ms, p70: 2400, p90: 4900 ms, p99: 13700 ms), latency samples: 413080
2. Upgrading first Validator to new version: fe7f555381e5d5d0ef8583daf80dc581240a5950
compatibility::simple-validator-upgrade::single-validator-upgrading : committed: 6868.08 txn/s, latency: 4065.63 ms, (p50: 4700 ms, p70: 4900, p90: 5000 ms, p99: 5100 ms), latency samples: 126840
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 6700.88 txn/s, latency: 4749.38 ms, (p50: 5000 ms, p70: 5400, p90: 6400 ms, p99: 6800 ms), latency samples: 224800
3. Upgrading rest of first batch to new version: fe7f555381e5d5d0ef8583daf80dc581240a5950
compatibility::simple-validator-upgrade::half-validator-upgrading : committed: 6483.52 txn/s, latency: 4222.71 ms, (p50: 4800 ms, p70: 5200, p90: 5500 ms, p99: 5900 ms), latency samples: 118800
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 5000.16 txn/s, latency: 6487.05 ms, (p50: 6700 ms, p70: 7800, p90: 8400 ms, p99: 9200 ms), latency samples: 171260
4. upgrading second batch to new version: fe7f555381e5d5d0ef8583daf80dc581240a5950
compatibility::simple-validator-upgrade::rest-validator-upgrading : committed: 10029.57 txn/s, latency: 2641.81 ms, (p50: 2700 ms, p70: 3100, p90: 3600 ms, p99: 3900 ms), latency samples: 182500
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 8738.93 txn/s, latency: 3565.86 ms, (p50: 3100 ms, p70: 3700, p90: 6800 ms, p99: 8500 ms), latency samples: 306360
5. check swarm health
Compatibility test for 7eeba4cd15892717741a614add1afde004c7855f ==> fe7f555381e5d5d0ef8583daf80dc581240a5950 passed
Test Ok

Copy link
Contributor

✅ Forge suite realistic_env_max_load success on fe7f555381e5d5d0ef8583daf80dc581240a5950

two traffics test: inner traffic : committed: 13581.22 txn/s, latency: 2927.06 ms, (p50: 2700 ms, p70: 3000, p90: 3100 ms, p99: 3600 ms), latency samples: 5163860
two traffics test : committed: 100.09 txn/s, latency: 2754.26 ms, (p50: 2500 ms, p70: 2700, p90: 2800 ms, p99: 12700 ms), latency samples: 1740
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.246, avg: 0.221", "QsPosToProposal: max: 0.289, avg: 0.266", "ConsensusProposalToOrdered: max: 0.341, avg: 0.304", "ConsensusOrderedToCommit: max: 0.509, avg: 0.481", "ConsensusProposalToCommit: max: 0.811, avg: 0.785"]
Max non-epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 1.08s no progress at version 2363079 (avg 0.21s) [limit 15].
Max epoch-change gap was: 0 rounds at version 0 (avg 0.00) [limit 4], 8.37s no progress at version 2363077 (avg 8.37s) [limit 15].
Test Ok

Copy link
Contributor

✅ Forge suite framework_upgrade success on 7eeba4cd15892717741a614add1afde004c7855f ==> fe7f555381e5d5d0ef8583daf80dc581240a5950

Compatibility test results for 7eeba4cd15892717741a614add1afde004c7855f ==> fe7f555381e5d5d0ef8583daf80dc581240a5950 (PR)
Upgrade the nodes to version: fe7f555381e5d5d0ef8583daf80dc581240a5950
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1237.53 txn/s, submitted: 1239.95 txn/s, failed submission: 2.42 txn/s, expired: 2.42 txn/s, latency: 2529.79 ms, (p50: 2100 ms, p70: 2700, p90: 4200 ms, p99: 6900 ms), latency samples: 112640
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1089.92 txn/s, submitted: 1091.92 txn/s, failed submission: 2.00 txn/s, expired: 2.00 txn/s, latency: 2681.51 ms, (p50: 2400 ms, p70: 3000, p90: 4200 ms, p99: 6700 ms), latency samples: 98080
5. check swarm health
Compatibility test for 7eeba4cd15892717741a614add1afde004c7855f ==> fe7f555381e5d5d0ef8583daf80dc581240a5950 passed
Upgrade the remaining nodes to version: fe7f555381e5d5d0ef8583daf80dc581240a5950
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1125.27 txn/s, submitted: 1127.69 txn/s, failed submission: 2.42 txn/s, expired: 2.42 txn/s, latency: 2727.30 ms, (p50: 2400 ms, p70: 3000, p90: 4800 ms, p99: 6300 ms), latency samples: 102300
Test Ok

@rahxephon89 rahxephon89 merged commit d2eaddb into main Oct 16, 2024
132 of 140 checks passed
@rahxephon89 rahxephon89 deleted the teng/fix-14793 branch October 16, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug][Prover] Abort is translated into empty tuple when rewriting Move expression in a spec
5 participants