Thoroughly fix Issue 8875: Warn in compilation when encounter an unknown attribute #9229

brmataptos · 2023-07-21T19:15:02Z

Description

Fix issue #8875 by adding a check for unknown attributes when parsing.

To handle different language variants (Aptos, third_party, EVM, Async)
we create and pass around a set of known attributes. While this may
seem a bit unnecessary, it pioneers the code path needed to pass
language extensions in the future.

Ultimately (#9228)
we should separate the configuration BuildConfig from the
code in BuildConfig so we can have that code accept "language
extension" config which is non-serializable to make it possible to
pass non-serializable config/plugins through the Aptos/third_party
compile interface. This would allow known_attributes to be passed
by reference (and be a HashSet) to be more efficient.

More details

We thoroughly fix this issue for the moment. This involves:

Issuing a warning on unknown attributes when compiling Move source code.
Distinguishing the set of known attributes based on Move dialect (base, Aptos, EVM, Async)
Making sure the dialect is selected for every one of the many different paths into the compiler
A temporary hack to avoid warning on unknown attributes used in 0x1:aptos_std module sources
Fixes to the attributes for those aptos stdlib sources so they can eventually be deployed and the hack removed
Fix to Move.toml files for internal tests to point to the local stdlib rather than pulling from network

Note that a quicker hack might have fixed this issue, and this code change is pretty involved.

However, in addition to its immediate effect, it also illustrates all the paths into the compiler (search for "known_attributes" and "skip_attribute_checks"), pioneering a path which can be used in the future to pass a "MoveDialect" container to make it cleaner to support multiple dialects through the shared third-party/ repo.

See https://pastebin.com/TH1nwNXs for some notes I took while enumerating paths through the code.

Commits:

cda4ae6 [compiler] test cases and stdlib source code updates for attributes checks PR
c123a0f [compiler] Fix issue-8875 thoroughly: error for unknown attributes, add --skip-attribute-check flag
0dac829 [compiler] Hack to move-compiler to avoid warning about unknown attributes in aptos_std=0x1. This avoids surprising warning on currently deployed library code.
31d69ee [third party/.../test, compiler] Fix Move.toml files to point to local files instead of network to avoid tests using old stdlib. Oddly, this requires fixes to capitalization of std. Fix the resolution warning to make it clear that std needs to be uncapitalized.
dfb800a (HEAD -> brm-issue-8875, origin/brm-issue-8875) [aptos stdlib] Now that we have verified that the move-compiler Aptos stdlib hack works, fix stdlib to use #[test_only] on test helper functions instead of #[testonly]. Leave struct attributes since it's a deployment problem to make it disappear.

Test Plan

Added more attribute examples and test cases.

wrwg · 2023-07-25T14:12:00Z

aptos-move/framework/src/extended_checks.rs

@@ -30,6 +36,30 @@ const RESOURCE_GROUP_NAME: &str = "group";
 const RESOURCE_GROUP_SCOPE: &str = "scope";
 const VIEW_FUN_ATTRIBUTE: &str = "view";

+// top-level attribute names, only.
+pub fn get_all_attribute_names_set() -> &'static BTreeSet<String> {
+    const ALL_ATTRIBUTE_NAMES: [&str; 4] = [


nit: you could use crate maplit, which would allow you to do do

static KNOWN_ATTRIBUTES_SET: Lazy<BTreeSet<String>> = Lazy::new(|| { btreeset!{LEGACY_ENTRY_FUN_ATTRIBUTE, RESOURCE_GROUP, ...} });

Or you just do:

static KNOWN_ATTRIBUTES_SET: Lazy<BTreeSet<String>> = Lazy::new(|| { ALL_ATTRIBUTE_NAMES.into_iter().collect() });

I missed this earlier. I'll try it if/when I make more changes.

I still think you should use at least my 2nd suggestion to make the initialization of KNOWN_ATTRIBUTES_SET more idiomatic and shorter.

Ok. I spent a while trying different alternatives to make it "cleaner", but don't think I've quite succeeded. The challenge is that I want to add both old KnownAttributes and extended_checks's KnownAttributes. I think making this clean really would involve refactoring lots of code so we can just pass a ref around. Hopefully it's a little better now, anyway.

wrwg · 2023-07-25T14:25:34Z

aptos-move/framework/src/built_package.rs

+    #[clap(long)]
+    pub skip_attribute_checks: bool,
+    #[clap(skip)]
+    pub known_attributes: BTreeSet<String>,


I don't think this need to be configurable, since the BuildConfig is only for Aptos, and in Aptos we wont use command line options to define allowed attributes. Looking at existing languages with attributes (Java/C#/Python come to mind), none of them have attributes defined this way. I'm worried we are creating technical debt here by adding this option which we now have to maintain forever. Can we remove it and hardwire the set of known attributes? I also wouldn't mind if, for everything else then Aptos (e.g. evm or async flavor), we just use --skip-attribute-checks.

The desire is not to have it be configurable by the user, but to make it configurable per Move dialect (base, Aptos, EVM, Async). As we discussed, this may be overkill for the current use case, but can serve as a guide for some more interesting dialect changes later.

wrwg · 2023-07-25T14:27:55Z

third_party/move/move-compiler/src/command_line/compiler.rs

@@ -95,6 +97,7 @@ impl<'a> Compiler<'a> {
    pub fn from_package_paths<Paths: Into<Symbol>, NamedAddress: Into<Symbol>>(
        targets: Vec<PackagePaths<Paths, NamedAddress>>,
        deps: Vec<PackagePaths<Paths, NamedAddress>>,
+        known_attributes: &BTreeSet<String>,


Do we really need to change this API here? I don't see a use case where known_attributes would be passed into the compiler like this.

The problem is that there are a huge number of paths into the compiler today. Maybe they shouldn't all be there, but I've traced them all (see https://pastebin.com/TH1nwNXs for some of my notes).

wrwg · 2023-07-25T14:30:53Z

third_party/move/move-compiler/src/expansion/translate.rs

-            None => E::AttributeName_::Unknown(sym),
+        let name_ = match KnownAttribute::resolve(sym) {
+            None => {
+                let flags = &context.env.flags();


It would have been simpler to do this in extended_checks, plus fitting well to compiler v2, because extended_checks is using the same AST/env than that one. That is, in the new compiler, we can just run extended_checks after regular type checking.

Simpler, yes. But this was more educational.

wrwg · 2023-07-25T14:32:59Z

third_party/move/move-compiler/tests/move_check/parser/attribute_placement.exp

@@ -0,0 +1,84 @@
+error[E02015]: invalid attribute


These tests are expected to pass.

I copied the same check to the /skip_attribute_checks/ directory where you can see the old result.

wrwg · 2023-07-25T14:33:12Z

third_party/move/move-compiler/tests/move_check/parser/attribute_variants.exp

@@ -0,0 +1,60 @@
+error[E02015]: invalid attribute


These too I think.

I copied the same check to the /skip_attribute_checks/ directory where you can see the old result.

wrwg · 2023-07-25T14:33:33Z

third_party/move/move-compiler/tests/move_check/parser/duplicate_attributes.exp

@@ -1,3 +1,15 @@
+error[E02015]: invalid attribute


Expected different errors here I think

I copied the same check to the /skip_attribute_checks/ directory where you can see the old result.

davidiw

I suspect @wrwg should be the definitive approver here, but it looks good to me. Modulo some tiny feedback on commenting out some code.

davidiw · 2023-08-06T04:02:00Z

aptos-move/framework/aptos-stdlib/sources/string_utils.move

@@ -107,7 +107,7 @@ module aptos_std::string_utils {
        native_format_list(&b"a = {} b = {} c = {}", &l);
    }

-    #[testonly]
+    /// #[test_only]


? why the comment out?

We can't actually use #[test_only] on this struct because it will make the module incompatible for upgrades (according to what @wrwg tells me, anyway). So instead I put it in a comment that will pass through to docs. Leaving the typo '#[testonly]' attribute could be safe, but might encourage users to keep misspelling, so it's deleted.

davidiw · 2023-08-06T04:21:36Z

third_party/move/evm/move-to-yul/src/lib.rs

@@ -102,6 +107,8 @@ pub fn run_to_abi_metadata<W: WriteColor>(
 ) -> anyhow::Result<Vec<Metadata>> {
    // Run the model builder.
    let addrs = parse_addresses_from_options(options.named_address_mapping.clone())?;
+    let flags = Flags::empty().set_flavor("async");


I guess there's no enum for flavors?

There is not. I can't boil the ocean.

davidiw · 2023-08-06T04:25:45Z

third_party/move/move-compiler/src/expansion/translate.rs

+// This is a hack to recognize APTOS StdLib to avoid warnings on some old errors.
+// This will be removed after library attributes are cleaned up.
+// (See https://github.com/aptos-labs/aptos-core/issues/9410)
+fn module_is_in_aptos_stdlib(module_address: Option<Spanned<Address>>) -> bool {


maybe we should just relabel this to "stdlib" but I guess this just exists temporarily anyway...

wrwg

This looks great and very close to land, just a few remaining nits and questions.

wrwg · 2023-08-08T03:06:31Z

aptos-move/framework/src/extended_checks.rs

@@ -30,6 +36,30 @@ const RESOURCE_GROUP_NAME: &str = "group";
 const RESOURCE_GROUP_SCOPE: &str = "scope";
 const VIEW_FUN_ATTRIBUTE: &str = "view";

+// top-level attribute names, only.
+pub fn get_all_attribute_names_set() -> &'static BTreeSet<String> {
+    const ALL_ATTRIBUTE_NAMES: [&str; 4] = [


I still think you should use at least my 2nd suggestion to make the initialization of KNOWN_ATTRIBUTES_SET more idiomatic and shorter.

wrwg · 2023-08-08T03:12:17Z

third_party/move/move-compiler/tests/move_check/parser/aptos_stdlib_attributes.move

+    #[a, a(x = 0)]
+    fun foo() {}
+
+    #[testonly]


This test case is really hard to understand, I think you are testing the warning suppression here. Perhaps name the module aptos_std::module_with_suppressed_warnings. BTW newer Move naming conventions have modules in small letters.

third_party/move/tools/move-cli/tests/build_tests/dependency_chain/args.exp

wrwg · 2023-08-13T16:44:17Z

Checking, what is the state of this PR? Are we still planning on landing this?

brmataptos · 2023-08-13T18:09:13Z

Yes.

…

On Sun, Aug 13, 2023, 9:44 AM Wolfgang Grieskamp ***@***.***> wrote: Checking, what is the state of this PR? Are we still planning on landing this? — Reply to this email directly, view it on GitHub <#9229 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A7S3T43K5M5QUUUMHCLRALTXVD76ZANCNFSM6AAAAAA2THAM2U> . You are receiving this because you authored the thread.Message ID: ***@***.***>

…hecks PR

…dd --skip-attribute-check flag

…butes in aptos_std=0x1. This avoids surprising warning on currently deployed library code.

…l files instead of network to avoid tests using old stdlib. Oddly, this requires fixes to capitalization of std. Fix the resolution warning to make it clear that std needs to be uncapitalized.

… stdlib hack works, fix stdlib to use #[test_only] on test helper functions instead of #[testonly]. Leave struct attributes since it's a deployment problem to make it disappear.

github-actions · 2023-08-15T23:40:04Z

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `085f328d92a786fe1c3eceb2bbbbe73d9538b77b`

Compatibility test results for aptos-node-v1.6.2 ==> 085f328d92a786fe1c3eceb2bbbbe73d9538b77b (PR)
1. Check liveness of validators at old version: aptos-node-v1.6.2
compatibility::simple-validator-upgrade::liveness-check : committed: 4413 txn/s, latency: 7282 ms, (p50: 6800 ms, p90: 10200 ms, p99: 17700 ms), latency samples: 167720
2. Upgrading first Validator to new version: 085f328d92a786fe1c3eceb2bbbbe73d9538b77b
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1776 txn/s, latency: 16062 ms, (p50: 19000 ms, p90: 22000 ms, p99: 22500 ms), latency samples: 92360
3. Upgrading rest of first batch to new version: 085f328d92a786fe1c3eceb2bbbbe73d9538b77b
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1848 txn/s, latency: 15732 ms, (p50: 18700 ms, p90: 22200 ms, p99: 22500 ms), latency samples: 92420
4. upgrading second batch to new version: 085f328d92a786fe1c3eceb2bbbbe73d9538b77b
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3357 txn/s, latency: 9521 ms, (p50: 9900 ms, p90: 13600 ms, p99: 14200 ms), latency samples: 144360
5. check swarm health
Compatibility test for aptos-node-v1.6.2 ==> 085f328d92a786fe1c3eceb2bbbbe73d9538b77b passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2023-08-15T23:43:02Z

✅ Forge suite `realistic_env_max_load` success on `085f328d92a786fe1c3eceb2bbbbe73d9538b77b`

two traffics test: inner traffic : committed: 6051 txn/s, latency: 6464 ms, (p50: 6300 ms, p90: 8100 ms, p99: 12300 ms), latency samples: 2626400
two traffics test : committed: 100 txn/s, latency: 3092 ms, (p50: 2900 ms, p90: 3600 ms, p99: 7400 ms), latency samples: 1820
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.272, avg: 0.214", "QsPosToProposal: max: 0.299, avg: 0.170", "ConsensusProposalToOrdered: max: 0.671, avg: 0.635", "ConsensusOrderedToCommit: max: 0.550, avg: 0.509", "ConsensusProposalToCommit: max: 1.182, avg: 1.143"]
Max round gap was 1 [limit 4] at version 1323273. Max no progress secs was 3.6993341 [limit 10] at version 1323273.
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2023-08-15T23:50:15Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `085f328d92a786fe1c3eceb2bbbbe73d9538b77b`

Compatibility test results for aptos-node-v1.5.1 ==> 085f328d92a786fe1c3eceb2bbbbe73d9538b77b (PR)
Upgrade the nodes to version: 085f328d92a786fe1c3eceb2bbbbe73d9538b77b
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 3017 txn/s, latency: 7193 ms, (p50: 7200 ms, p90: 9600 ms, p99: 14400 ms), latency samples: 168960
5. check swarm health
Compatibility test for aptos-node-v1.5.1 ==> 085f328d92a786fe1c3eceb2bbbbe73d9538b77b passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

* [storage] support sharding in fast_sync handle sharded kv batches handle sharded merkel tree save progress * [TS SDK] Support network and API endpoints on `AptosConfig` (#9549) * support network and api urls on AptosConfig * devnet as default network * Remove tutorial links from descriptions Tutorials are linked on tutorial title and again on the tutorial description just next to the title. Small nitpick, but is slightly confusing when you just arrive to the dev docs, since you expect that second link to be something different. * add profiling crate with cpu and memory profiling fix comments and warnings fix comments resolve comments add crate to cargo.toml add additional functions for different cases of profiling fix cargo.toml * remove unnecessary dependency * fix lint errors * fix lint errors * fix lint error in cargo.toml * [testsuite][pangu][pte] Creating a Pangu Command to Create a Transaction Emitter (#9495) * [testsuite][pangu][pte] Initial work for the transaction emitter. * [testsuite][pangu][pte] Finished the transaction emitter comand. * Update testsuite/test_framework/kubernetes.py good catch! Co-authored-by: Balaji Arun <[email protected]> * [testsuite][pangu][pte] Addressed Balaji's comments. * [testsuite][pangu][pte] Hotfix for Forge compatibility. * [testsuite][pangu][pte] Another hotfix. * [testsuite][pangu][pte] Added support for dry runs, and fixed a bug in get testnet * [testsuite][pangu][pte] Minor changes. * Update README.md --------- Co-authored-by: Olsen Budanur <[email protected]> Co-authored-by: Balaji Arun <[email protected]> * replay-verify: log skipped version when it happens * [dashboards] sync grafana dashboards (#9440) Co-authored-by: rustielin <[email protected]> * [forge stable] Fix HAProxy test - increase latency limits (#9575) * [indexer grpc] reduce the data service streaming channel size (#9590) * [refactoring] Remove unused genesis check from StateView (#9589) `is_genesis()` was defined in `StateView` but never used apart from a single test with `MockVM`. But the test set it to false always... Removing it to have better view interfaces. * [TS SDK] Use `account_transactions` and `account_transaction_aggregate` queries (#9403) * improve indexer client to support tokenv2 sorting and new queries * use account_transactions query * Optional argument on is not positional * [Dev Docs] Add small note about target-version in backup docs. * swich to gcp * add semgrep for github workflows (#9522) add semgrep for GitHub workflows * [GHA] Run smoke and performance tests when appropriate. * [Spec] Ensures of transaction_validation (#9461) * hp_trans_valid * hp_trans_validation * pre-pr * fix trim --------- Co-authored-by: chan-bing <[email protected]> * [GHA] Small tweaks to jobs. * [TS SDK] filter amount > 0 on getTokenOwnersData and include all query fields (#9593) * filter amount > 0 on getTokenOwnersData * export all query fields * update changelog * [Python SDK] Add a token_transfer function to the python SDK (#9422) * Adding transfer_token function to aptos_token_client.py and adding documentation tags for the new your_first_nft tutorial. * Fixed strange formatting * Created a transfer_object function in RestClient class and altered the transfer_token function in the AptosTokenClient to just call transfer_object. Left it in for ease of use * [deps] Update clap dependency to be consistent across crates (#9605) * Fix node compatibility test CI (#9609) * [gas] fix abstract gas cost for storage (#9501) * [forge] Add latency for different workloads (#9415) * remove redundant test (#9613) * [dag] Handle certified node in DAG Driver (#9312) * [dag] Handle certified node in dag driver * [dag] Trigger new round on handler start * [dag] Use only the aptos_time_service crate * [NFT Metadata Crawler] Fix error for defaulting to original raw_image_uri (#9611) * Fix error for defaulting to original raw_image_uri * move db commit outside * [ts-sdk example] Adding an example for rotation offer capability and signer offer capability with signed structs (#9425) * Adding the offer capability example. Uses signed structs in typescript. * remove unnecessary aptosClient from getAccount call Co-authored-by: Maayan <[email protected]> * Fixing network URL unnecessary code and potential unalignment between network/faucet url * Shortening # hyphens in output string * Moving the chainId to the bottom of the struct list since it's potentially undefined, and explaining in the sign struct function that the proof bytes must be in that specific order. * Cleaning up the code and making it more readable * Formatting * Use a much cleaner and reusable serializable class instead of a struct --------- Co-authored-by: Maayan <[email protected]> * bump version to 1.18.0 (#9607) * [compiler-v2] Use livevar analysis to optimize file format generation (#9361) * [compiler-v2] Use livevar analysis to optimize file format generation This connects the live-variable analysis which is already present in the Move prover to the v2 bytecode pipeline. This information is then used in the file-format generator to make better decisions when to move or copy values. Livevar is a standard compiler construction data flow analysis. It computes the set of variables which are alive (being used in subsequent reachable code) before and after each program point. The implementation from the prover uses our dataflow analysis framework and is [here](https://github.com/aptos-labs/aptos-core/blob/206f529c0c9d8488e27d2e50297178f0caf429a5/third_party/move/move-prover/bytecode/src/livevar_analysis.rs#L381). In this PR we create the first processing step in the bytecode pipeline with the new module `pipeline/livevar_analysis_step.rs` which forwards logical work to the existing analysis. * Addressing reviewer comments. * [storage] update resource requriement * [TS SDK v2] Add Hex and HexInput types (#9595) Co-authored-by: maayan <[email protected]> * [cleanup] Remove foreign contracts (#9623) * [Spec] Ensures of reconfiguration.move (#9383) * init * hp * hp reconfig * init * fix comment * fix comment --------- Co-authored-by: chan-bing <[email protected]> * Revert "Revert "[NFT Metadata Crawler] Dockerize Parser (#9541)"" (#9622) This reverts commit aa81b95. * Clearer comments in `resource_account.move` (#9555) * Explicitize that the txn_seq_num param on the epilogue is unused With this we can go through all historical txns to make sure. * [dag] Integrate Order Rule with Dag Driver (#9452) * [gas] convert output to InternalGas cost (#9603) * [move-ir-compiler] add Nop token support (#9599) * [dag] Integrate Dag Fetcher with Dag Driver (#9453) * [dag] Integrate Dag Fetcher with Dag Driver * [dag] Handle fetch response in dag handler * [TS SDK v2] Add AccountAddress (#9564) * [CLI] Disallow "rotating" to same private key (#9546) * trivial: fix gas meter mutability * Remove unused deps for CLI (#9531) * typo in functions.md * typo spec-lang.md typo * Update package-upgrades.md space added * Update developer-docs-site/docs/move/book/package-upgrades.md * Update unit-testing.md typo * [GHA] Only run all perf tests on schedule. * [TS SDK example] Add a working example of converting a MultiEd25519 account to a MultiSig account (#9516) * An example of converting a MultiEd25519 account to a MultiSig account Cleaned up the file even more, added assertions and print statements at the end Removing incorrect addresses Formatting and clarifying the steps. Changing network to DEVNET from local Removing confusing comment from prior version and adding comment about making sure secondsTilExpiration is set accordingly Fixing file name Fixing variable name Making header comments less long * Adding the ability to add metadata to the multisig account, since we were using empty arrays before. Asserted that the values on-chain match the input values at the end * Using a much cleaner and reusable serializable class for the signed message * Removing the unnecessary multisig as sender option * Update e2e flow description at top of file * Resolving rebase conflict and simplifying the signature construction * Clarify the process of creating a MultiEd25519 account and that public keys are different from addresses * Improve clarity of comments on token.move * [NFT Metadata Crawler] Fix raw image uri parse error (#9628) * Fix raw_image_uri parse bug * upsert * update compatibility test base * [Dev Docs] Add note about indexer bootstrap support. * Update developer-docs-site/docs/nodes/indexer-fullnode.md * [dashboards] sync grafana dashboards * remove payer from StateValueMetadata * Track slot allocation fee on state metadata * enable state value metadata tracking for devnet * [compiler v2] Fix bugs around compilation of vector code (#9636) * [compiler v2] Fix bugs around compilation of vector code This fixes a few bugs and closes #9629 - Usage of precompiled stdlib in transactional tests: v2 compiler currently (and probably never) supports precompiled modules, working around this - Stack-based bytecode generator created wrong target on generic function calls - Needed to make implicit conversion from `&mut` to `&` explicit in generated bytecode via Freeze operation Added some additional tests while debugging this. * Adding a new option `--print-bytecode` which can be provided to the `//# publish` and `//# run` transactional test runner command. This is the applied (for now) to the `sorter` test case only. Also introduced logic to map well-known vector functions to the associated builtin opcodes. * [TS SDK v2] Reject invalid SHORT form for special addresses in fromString (#9640) * [TS SDK v2] Reject invalid SHORT form for special addresses in fromString * Update account_address.ts * Update account_address.ts * [TS SDK v2] Add equality methods for Hex and AccountAddress (#9641) * Handle empty network address in CLI (#9576) * [CLI][e2e] Tests create-resource-account and derive-resource-account-address (#8757) * CLI e2e for resource account * add account list to test resource account * [prover] lock version dependency for z3 and boogie (#8718) (#9524) Co-authored-by: Aalok Thakkar <[email protected]> * Update workflows (#9650) * Update semgrep.yaml to also run daily * update semgrep rule * fix workflows * Update .github/workflows/semgrep.yaml Co-authored-by: Balaji Arun <[email protected]> --------- Co-authored-by: Balaji Arun <[email protected]> * [dag] bootstrap logic (#9455) * add guard field to struct * temp * add executor benchmark profiling * small fixes * lint fix * lint fix * lint fix * lint fix * lint fix * update cargo.lock * [Spec] Ensures of resources.move (#9382) * init * fix md * fix comment * [Spec] Ensures of transaction_fee (#9460) * hp trans_fee * pre-pr * add the condition that proposer == @vm_reserved * fix comment --------- Co-authored-by: chan-bing <[email protected]> * [TS SDK v2] Remove redundant code in AccountAddress.fromString (#9662) * [docs] Rename Aptos Move CLI -> Aptos CLI (#9655) * Thoroughly fix Issue 8875: Error out in compilation when encounter an unknown attribute (#9229) Fix issue-8875 thoroughly: warning for unknown attributes, add --skip-attribute-check flag. Omit warning on aptos_std library files, to avoid warning on some existing code. A few tangential fixes: * [third party/.../test, compiler] Fix Move.toml files to point to local files instead of network to avoid tests using old stdlib. Oddly, this requires fixes to capitalization of std. Fix the resolution warning to make it clear that std needs to be uncapitalized. * [compiler] Hack to move-compiler to avoid warning about unknown attributes in aptos_std=0x1. This avoids surprising warning on currently deployed library code. * [compiler] test cases and stdlib source code updates for attributes checks PR * [aptos stdlib] Now that we have verified that the move-compiler Aptos stdlib hack works, fix stdlib to use #[test_only] on test helper functions instead of #[testonly]. Leave struct attributes since it's a deployment problem to make it disappear. * [Storage Service] Add type support for subscription requests. * [Storage Service] Add subscription stream support. * [Storage Service] Adopt subscription metadata and loop when serving subscriptions. * Make AccountAddress from_str conform to AIP-40, add from_str_strict (#9186) * [TS SDK v2] Remove redundant code in AccountAddress.fromString * Make AccountAddress from_str conform to AIP-40 * Use thiserror for AccountAddressParseError * [Executor-benchmark] Make connected grps of txns fully conflicting grps (#9647) [Executor-benchmark] Make connected grps of txns fully conflicting grps Fully conflicting grps are more meaningful pattern for testing out the sharded executor because they cannot be executed parallely (that is what we intend to benchmark). Cmd: cargo run -p aptos-executor-benchmark -- --block-size 100 --connected-tx-grps 5 --shuffle-connected-txns run-executor --main-signer-accounts 1000 --data-dir /tmp/some-db --checkpoint-dir /tmp/some-checkpoint --blocks 2 For a good random workload we would want 'num_accounts_per_grp' to be much greater than 'num_txns_per_grp'. The command enforces 'num_signer_accounts' > '2 * num_txns_per_grp'. * [NFT Metadata Crawler] Stop parsing completely if token_uri has already been parsed (#9649) * stop parsing completely if token_uri has already been parsed, add logs, add more panics (failed to write to gcs, failed to commit postgres tx) * add comment to clarify * error -> warn * rename * Fixed signer to be formatted properly (#8866) * Fixed signer to be formatted properly - fixed the issue in `string_utils` - replaced `testonly` with `test_only` * Add a feature flag * [Quorum Store] A couple metrics to observe backpressure better (#9239) ### Description A couple metrics to observe backpressure better * [Execution] Do not hold the Arc of committed block in execution. (#9674) * [NFT Metadata Crawler] Increase HTTP request for large files (#9677) * retry time increase * add to constants file * remove unneeded comment * Revert "swich to gcp" This reverts commit 8f28f93. * [NFT Metadata Crawler] Maintain image aspect ration on resize (#9688) * maintain image aspect ration on resize * fix lint * [Spec] update spec for vesting.move (#9115) * new vesting * new vesting * init * fix comment * fix md * add comment * add head * add func total_accumulated_rewards time * rm boogie * fixed timeout * close timeout function's verification * rust lint fix * del head --------- Co-authored-by: chan-bing <[email protected]> * [event_v2] module event extension, attribute and extended check * [eventv2] make v0/v1 to v1/v2 * [event_v2] test * [events v2] Sample of how to check for event type attributes in the extended checker * [eventv2] add module publishing validation * [eventv2] add module event feature * [eventv2] tests and fixes * [eventv2] address comments and forbid script emitting module events * [eventv2] add script event verifier to forbid event emitting * add #[event] to known attributes * trivial: add required feature to dependency to fix tests `e2e-move-tests` when running alone doesn't pass because `pub const GAS_UNIT_PRICE: u64 = 0;` is not in effect * Revert "add #[event] to known attributes" This reverts commit 5af30fb. * Revert "[eventv2] add script event verifier to forbid event emitting" This reverts commit 6a30bcd. * Revert "[eventv2] address comments and forbid script emitting module events" This reverts commit e4a9c2c. * Revert "[eventv2] tests and fixes" This reverts commit 4040b3d. * Revert "[eventv2] add module event feature" This reverts commit 5a12afc. * Revert "[eventv2] add module publishing validation" This reverts commit 7543f6e. * Revert "[events v2] Sample of how to check for event type attributes in the extended checker" This reverts commit 72cb7b4. * Revert "[event_v2] test" This reverts commit c93e61d. * Revert "[eventv2] make v0/v1 to v1/v2" This reverts commit 0ba554c. * Revert "[event_v2] module event extension, attribute and extended check" This reverts commit ed99bf0. * [compiler v2] Move the stackless bytecode crate out of prover (#9698) This moves the `move-stackless-bytecode` crate out of its current location in the `move-prover` into the `move-model`. This is pure relocation. Subsequent PRs will split off prover specific code and move it back to the prover tree. * [gas][docs] example guides (#9633) * [refactoring] Relax trait bounds, reduce dependence on `StateView` (#9644) API only needs move resolver, and not state view. Decoupling `MoveResolverExt` into `AptosMoveResolver + StateView` so that any future changes were not visible on API side. 1. Move API checks for resource groups where it should be. 2. Make trait bounds for config not require state view. Right now state view is used all over the place although it seems this can be avoided. 3. Reduce the usage of `MoveResolverExt`: e.g. session only needs to talk to resolver. 4. State value metadata has its own resolver. This makes storage accesses almost uniform (still need to fix `read_write_set`) which should be implemented in storage adapter. 5. Some minor typo fixes and dead code removal. * [dag] network sender implementation (#9456) * [Storage Service] Add subscription stream support. * [Storage Service] Add tests for subscription streams. * [Storage Service] Add tests for stream looping. * Add get_state_value_u128 method to TStateView (#9097) * [Spec] Ensures for voting (#9462) * new_voting * run lint * little change * fix md * fix schema name * fix error in simplemap --------- Co-authored-by: chan-bing <[email protected]> * [scripts] fixes to arch linux (pacman) * Archlinux / python / pip insist that you use the package manager solutions not python for pip * The `libudev-dev` seems to exist in the default system install and under a wildly different name. * [Block Executor] Refactor view & output traits & mvhashmap (#9659) * [MVHashMap] convenient (shared) view state, aggregator v1 port * [Block executor] new traits for processing the output * [typo*] aptos-token and wallets (#9661) * Update aptos-token.md typo "maxium"/maximum * [typo*]Update wallets.md dot added * [compiler v2] Stackless Bytecode Refactoring (#9713) This is the 2nd step in the refactoring started in #9698. This splits off prover specific parts from the crate `move-model/bytecode` into `move-prover/bytecode-pipeline`. Sharable dataflow analysis and transformation processors, like livevar and reaching definitions, stay. The tests have been split as well, and a common testing driver has been moved in its own test utility crate. Github does not nicely show diffs like this, but this is functionally a no-op which only Moves code around. * [DAG] Adding randomness field in dag node (#9687) * [NFT Metadata Crawler] Fix double slash URI parsing error, add ACKing PubSub message on receive (#9706) * fix double slash uri parsing error, acking on message receive * edit var names * fix test * [api] Add consistent API testing (#9577) * [api] Add consistent API testing (#9146) This commit introduces consistent API testing where we run a consistent load through the network mimicking various user flows. See proposal for more high level information. Changes: Created aptos-api-tester crate which runs 4 user flows: Account creation Coin transfer NFT transfer Module publishing, and outputs results to stdout. Added token-client to the SDK with the methods needed to run Your First NFT. mirroring the typescript SDK and the rust coin client by using existing builders Added PartialEq to aptos_rest_client::Account for testing. Added the crate to the tools docker image to run with GCP Cloud Run. * [api] Add metrics to api tester (#9307) This commit introduces logging tools on top of the consistent API testing introduced here. See proposal for more high level information. Changes: Added metrics pusher to aptos-api-tester with 4 histogram metrics for success, error, fail rates, and latency. Added logs to tests instead of printing to stdout. * [api] Add threads and timestamps to api tests (#9349) This commit builds threads and run ID on top of the consistent API testing introduced here. See proposal for more high level information. Changes: Refactored tests into dedicated file tests.rs. Switched to creating accounts for every tests instead of using the same and created pre test set up routines in testsetups.rs. Added run ID start_time to metrics. * [api] Add support for strictness level for consistency on API tests (#9444) Description This commit builds threads and run ID on top of the consistent API testing introduced here. See proposal for more high level information. One problem we saw in the dashboard is that the problems we observed were regarding eventual consistency. We added support for adjusting the strictness level for such errors. Changes: Refactor of tests into tests module and decomposing into steps (setting up for next PR, individual step timing). Persistent checks for information retrieval. Added token support for Testnet faucet. * [api] Add individual step timing to api tester (#9502) This commit builds individual step timing on top of the consistent API testing introduced here. See proposal for more high level information. This addition allows us to dive deeper on what the issue is should we detect any flow is consistently slower. Changes: Add timer macro in macros.rs. Add helpers for emitting metrics and refactor process_result. Put publish_module in its own thread by creating a tokio runtime. Add sleeps between persistent checks. Reduce API client sleep time to 100ms from 500ms. Make test setups persistent. * [api] Add view function testing to the API tester (#9658) This commit adds a new test which tests a simple view function. * [dag] e2e integration test (#9457) * [event_v2] module event extension, attribute and extended check * [eventv2] make v0/v1 to v1/v2 * [event_v2] test * [events v2] Sample of how to check for event type attributes in the extended checker * [eventv2] add module publishing validation * [eventv2] add module event feature * [eventv2] tests and fixes * [eventv2] address comments and forbid script emitting module events * [eventv2] add script event verifier to forbid event emitting * add #[event] to known attributes * [eventv2] api test * Update staking-pool-operations.md (#8685) * Update staking-pool-operations.md corrected CLI command to create staking_contract previously the command created a direct pool type * Update staking-pool-operations.md add additional 8 0s * Update Docker images (#9699) Co-authored-by: gedigi <[email protected]> * [smart_vector] add destroy function for dropable T (#9434) * [CLI] Add support for setting faucet auth token (#9715) * [CLI] Add support for setting faucet auth token * Use FaucetClient * Unify thread name, and combine all threads in the same thread pool in framegraph produced by cpu_profiler. (#9721) * Feat/pypi ci (#9512) * add python sdk publish ci * remove check version * [framework] Reset the release yaml for upcoming 1.7 release Test Plan: ran yq / generate proposals locally ``` target/performance/aptos-release-builder generate-proposals --release-config aptos-move/aptos-release-builder/data/release.yaml --output-dir output ``` --------- Co-authored-by: Bo Wu <[email protected]> Co-authored-by: Maayan <[email protected]> Co-authored-by: Álvaro Lillo Igualada <[email protected]> Co-authored-by: yunuseozer <[email protected]> Co-authored-by: Oğuzhan (Olsen) Budanur <[email protected]> Co-authored-by: Olsen Budanur <[email protected]> Co-authored-by: Balaji Arun <[email protected]> Co-authored-by: aldenhu <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: rustielin <[email protected]> Co-authored-by: igor-aptos <[email protected]> Co-authored-by: larry-aptos <[email protected]> Co-authored-by: George Mitenkov <[email protected]> Co-authored-by: Josh Lind <[email protected]> Co-authored-by: Gerardo Di Giacomo <[email protected]> Co-authored-by: Zorrot Chen <[email protected]> Co-authored-by: chan-bing <[email protected]> Co-authored-by: Matt <[email protected]> Co-authored-by: Greg Nazario <[email protected]> Co-authored-by: Daniel Porteous (dport) <[email protected]> Co-authored-by: Victor Gao <[email protected]> Co-authored-by: Justin Chang <[email protected]> Co-authored-by: Wolfgang Grieskamp <[email protected]> Co-authored-by: maayan <[email protected]> Co-authored-by: Alin Tomescu <[email protected]> Co-authored-by: William Law <[email protected]> Co-authored-by: Vladislav ~ cryptomolot <[email protected]> Co-authored-by: David Wolinsky <[email protected]> Co-authored-by: JasperTimm <[email protected]> Co-authored-by: Jin <[email protected]> Co-authored-by: aalok-t <[email protected]> Co-authored-by: Aalok Thakkar <[email protected]> Co-authored-by: Brian R. Murphy <[email protected]> Co-authored-by: Manu Dhundi <[email protected]> Co-authored-by: Junkil Park <[email protected]> Co-authored-by: Brian (Sunghoon) Cho <[email protected]> Co-authored-by: Guoteng Rao <[email protected]> Co-authored-by: Bo Wu <[email protected]> Co-authored-by: Aaron Gao <[email protected]> Co-authored-by: Andrei Tonkikh <[email protected]> Co-authored-by: Rati Gelashvili <[email protected]> Co-authored-by: Daniel Xiang <[email protected]> Co-authored-by: Nurullah Giray Kuru <[email protected]> Co-authored-by: michelle-aptos <[email protected]> Co-authored-by: gedigi <[email protected]> Co-authored-by: Fenix <[email protected]> Co-authored-by: Perry Randall <[email protected]>

brmataptos requested review from saharct, gregnazario, 0xmigo, banool, davidiw, movekevin and wrwg as code owners July 21, 2023 19:15

brmataptos force-pushed the brm-issue-8875 branch from b3d90e4 to 662c86a Compare July 23, 2023 06:47

brmataptos marked this pull request as draft July 24, 2023 17:16

wrwg reviewed Jul 25, 2023

View reviewed changes

This was referenced Aug 1, 2023

[Bug] Turn off suppression of warnings on unknown attributes in library code when it's safe #9410

Closed

[Bug] Turn unknown attribute warnings into an error when safe to do so. #9411

Open

brmataptos force-pushed the brm-issue-8875 branch from 662c86a to dfb800a Compare August 5, 2023 02:20

brmataptos marked this pull request as ready for review August 5, 2023 02:59

brmataptos force-pushed the brm-issue-8875 branch 2 times, most recently from 970df5a to 038f378 Compare August 5, 2023 05:02

davidiw approved these changes Aug 6, 2023

View reviewed changes

wrwg reviewed Aug 8, 2023

View reviewed changes

brmataptos mentioned this pull request Aug 10, 2023

[Feature Request] Pass a MoveDialect container into move-compiler for better Move dialect support #9615

Open

brmataptos force-pushed the brm-issue-8875 branch from 038f378 to b0d354e Compare August 14, 2023 18:12

brmataptos added 8 commits August 14, 2023 13:31

[compiler] test cases and stdlib source code updates for attributes c…

0b3a5d0

…hecks PR

[compiler] Fix issue-8875 thoroughly: error for unknown attributes, a…

8e2d28f

…dd --skip-attribute-check flag

[compiler] Hack to move-compiler to avoid warning about unknown attri…

049cabe

…butes in aptos_std=0x1. This avoids surprising warning on currently deployed library code.

[third party/.../test, compiler] Fix Move.toml files to point to loca…

8771351

…l files instead of network to avoid tests using old stdlib. Oddly, this requires fixes to capitalization of std. Fix the resolution warning to make it clear that std needs to be uncapitalized.

[aptos stdlib] Now that we have verified that the move-compiler Aptos…

e8a2aff

… stdlib hack works, fix stdlib to use #[test_only] on test helper functions instead of #[testonly]. Leave struct attributes since it's a deployment problem to make it disappear.

rust_lint updates

2cb8553

a few changes inspired by Wolfgang's last review

5090368

oops, update test outputs for changed inputs

000fa2a

brmataptos force-pushed the brm-issue-8875 branch from b0d354e to 000fa2a Compare August 14, 2023 20:31

movekevin approved these changes Aug 14, 2023

View reviewed changes

wrwg approved these changes Aug 15, 2023

View reviewed changes

Merge branch 'main' into brm-issue-8875

085f328

brmataptos enabled auto-merge (squash) August 15, 2023 23:13

brmataptos changed the title ~~Thoroughly fix Issue 8875: Error out in compilation when encounter an unknown attribute~~ Thoroughly fix Issue 8875: Warn in compilation when encounter an unknown attribute Aug 15, 2023

This comment has been minimized.

Sign in to view

brmataptos merged commit 6fd6799 into aptos-labs:main Aug 16, 2023

This was referenced Sep 21, 2023

Fix issue 9410 by removing special-case hack to suppress unknown attribute warnings on aptos_stdlib #10167

Closed

Fix issue 9410 by removing special-case hack to suppress unknown attribute warnings on aptos_stdlib #10175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Thoroughly fix Issue 8875: Warn in compilation when encounter an unknown attribute #9229

Thoroughly fix Issue 8875: Warn in compilation when encounter an unknown attribute #9229

brmataptos commented Jul 21, 2023 •

edited

Loading

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

wrwg Aug 8, 2023

brmataptos Aug 14, 2023

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

wrwg Jul 25, 2023

brmataptos Aug 5, 2023

davidiw left a comment

davidiw Aug 6, 2023

brmataptos Aug 7, 2023

davidiw Aug 6, 2023

brmataptos Aug 7, 2023

davidiw Aug 6, 2023

brmataptos Aug 7, 2023

wrwg left a comment

wrwg Aug 8, 2023

wrwg Aug 8, 2023

wrwg commented Aug 13, 2023

brmataptos commented Aug 13, 2023 via email

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

github-actions bot commented Aug 15, 2023

Thoroughly fix Issue 8875: Warn in compilation when encounter an unknown attribute #9229

Thoroughly fix Issue 8875: Warn in compilation when encounter an unknown attribute #9229

Conversation

brmataptos commented Jul 21, 2023 • edited Loading

Description

More details

Commits:

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidiw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrwg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wrwg commented Aug 13, 2023

brmataptos commented Aug 13, 2023 via email

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Aug 15, 2023

✅ Forge suite compat success on aptos-node-v1.6.2 ==> 085f328d92a786fe1c3eceb2bbbbe73d9538b77b

github-actions bot commented Aug 15, 2023

✅ Forge suite realistic_env_max_load success on 085f328d92a786fe1c3eceb2bbbbe73d9538b77b

github-actions bot commented Aug 15, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.5.1 ==> 085f328d92a786fe1c3eceb2bbbbe73d9538b77b

brmataptos commented Jul 21, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.6.2` ==> `085f328d92a786fe1c3eceb2bbbbe73d9538b77b`

✅ Forge suite `realistic_env_max_load` success on `085f328d92a786fe1c3eceb2bbbbe73d9538b77b`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.5.1` ==> `085f328d92a786fe1c3eceb2bbbbe73d9538b77b`