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

feature: Add new assertion instruction models #1939

Merged

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Oct 3, 2024

Summary

Adds the following instruction models, including compilation, decompilation and validation:

  • ASSERT_WORKTOP_RESOURCES_ONLY
  • ASSERT_WORKTOP_RESOURCES_INCLUDE
  • ASSERT_NEXT_CALL_RETURNS_ONLY
  • ASSERT_NEXT_CALL_RETURNS_INCLUDE
  • ASSERT_BUCKET_CONTENTS

Also:

  • Changes the existing cuttlefish instruction, ASSERT_WORKTOP_IS_EMPTY, to be an instruction alias.
  • Improves static validation for the existing resource assertions (e.g. checks amount is positive, checks we don't assert against non-fungible ids with non fungible resources).
  • Improves the failure output in e2e.rs to properly format the error messages, making debugging much easier.
  • Unifies some of the types with Feature/resource movements static analysis #1909
  • The new commands have been integrated into the visitor from Feature/resource movements static analysis #1909 (should be very easy!)

Further work

  • The instructions need to be implemented in the engine

Testing

  • Add instructions to a test ine2e.rs
  • Add tests to static_resource_movements_visitor.rs

Copy link

github-actions bot commented Oct 3, 2024

Docker tags
docker.io/radixdlt/private-scrypto-builder:a93970bb30

Copy link

github-actions bot commented Oct 3, 2024

Benchmark for a93970b

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 44.8±0.13ms 44.7±0.17ms -0.22%
costing::decode_encoded_i8_array_to_manifest_raw_value 18.9±0.04ms 18.9±0.05ms 0.00%
costing::decode_encoded_i8_array_to_manifest_value 42.5±0.04ms 41.3±0.04ms -2.82%
costing::decode_encoded_tuple_array_to_manifest_raw_value 68.7±0.13ms 72.2±0.09ms +5.09%
costing::decode_encoded_tuple_array_to_manifest_value 97.4±0.14ms 98.4±0.14ms +1.03%
costing::decode_encoded_u8_array_to_manifest_raw_value 26.0±0.06µs 32.2±0.09µs +23.85%
costing::decode_encoded_u8_array_to_manifest_value 42.5±0.04ms 41.4±0.04ms -2.59%
costing::decode_rpd_to_manifest_raw_value 14.9±0.05µs 14.4±0.03µs -3.36%
costing::decode_rpd_to_manifest_value 11.0±0.04µs 10.6±0.02µs -3.64%
costing::deserialize_wasm 1261.1±2.92µs 1282.1±2.43µs +1.67%
costing::execute_transaction_creating_big_vec_substates 700.6±9.96ms 705.3±8.09ms +0.67%
costing::execute_transaction_reading_big_vec_substates 586.2±0.64ms 597.1±0.69ms +1.86%
costing::instantiate_flash_loan 998.1±926.31µs 939.2±478.47µs -5.90%
costing::instantiate_radiswap 981.8±802.89µs 1065.6±1523.54µs +8.54%
costing::spin_loop 21.3±0.04ms 22.2±0.06ms +4.23%
costing::validate_sbor_payload 32.2±0.13µs 32.6±0.16µs +1.24%
costing::validate_sbor_payload_bytes 242.7±0.45ns 261.7±1.09ns +7.83%
costing::validate_secp256k1 76.7±0.07µs 76.6±0.05µs -0.13%
costing::validate_wasm 33.7±0.04ms 33.6±0.04ms -0.30%
decimal::add/0 8.4±0.00ns 8.4±0.00ns 0.00%
decimal::add/rust-native 9.9±0.01ns 9.9±0.02ns 0.00%
decimal::add/wasmi 225.2±0.29ns 226.8±0.24ns +0.71%
decimal::add/wasmi-call-native 2.1±0.00µs 2.1±0.01µs 0.00%
decimal::div/0 187.6±0.07ns 187.6±0.12ns 0.00%
decimal::from_string/0 158.2±0.35ns 154.5±0.16ns -2.34%
decimal::mul/0 149.3±0.35ns 149.1±0.15ns -0.13%
decimal::mul/rust-native 147.3±0.09ns 147.5±0.09ns +0.14%
decimal::mul/wasmi 11.9±0.01µs 12.1±0.05µs +1.68%
decimal::mul/wasmi-call-native 2.3±0.00µs 2.3±0.01µs 0.00%
decimal::pow/0 697.9±0.83ns 696.9±0.64ns -0.14%
decimal::pow/rust-native 667.3±1.10ns 672.3±0.45ns +0.75%
decimal::pow/wasmi 60.4±0.26µs 58.9±0.30µs -2.48%
decimal::pow/wasmi-call-native 2.5±0.00µs 3.4±0.02µs +36.00%
decimal::root/0 8.1±0.01µs 8.0±0.00µs -1.23%
decimal::sub/0 8.3±0.28ns 9.1±0.00ns +9.64%
decimal::to_string/0 441.0±0.80ns 444.9±1.20ns +0.88%
large_transaction_processing::prepare 2.6±0.00ms 2.6±0.00ms 0.00%
large_transaction_processing::prepare_and_decompile 6.9±0.01ms 6.8±0.02ms -1.45%
large_transaction_processing::prepare_and_decompile_and_recompile 25.1±0.12ms 32.7±0.40ms +30.28%
precise_decimal::add/0 9.0±0.03ns 9.0±0.02ns 0.00%
precise_decimal::add/rust-native 10.8±0.02ns 10.7±0.02ns -0.93%
precise_decimal::add/wasmi 272.4±1.00ns 325.6±0.36ns +19.53%
precise_decimal::add/wasmi-call-native 2.8±0.00µs 3.1±0.01µs +10.71%
precise_decimal::div/0 315.3±0.31ns 315.1±0.44ns -0.06%
precise_decimal::from_string/0 214.9±0.34ns 202.0±0.16ns -6.00%
precise_decimal::mul/0 381.7±2.76ns 362.9±0.58ns -4.93%
precise_decimal::mul/rust-native 319.7±0.65ns 320.8±1.81ns +0.34%
precise_decimal::mul/wasmi 34.1±0.18µs 34.5±0.15µs +1.17%
precise_decimal::mul/wasmi-call-native 3.2±0.00µs 3.2±0.01µs 0.00%
precise_decimal::pow/0 1937.2±3.51ns 1936.0±3.14ns -0.06%
precise_decimal::pow/rust-native 1541.1±7.08ns 1524.9±2.63ns -1.05%
precise_decimal::pow/wasmi 163.6±1.17µs 170.3±0.31µs +4.10%
precise_decimal::pow/wasmi-call-native 5.7±0.00µs 5.8±0.01µs +1.75%
precise_decimal::root/0 57.4±0.02µs 58.4±0.04µs +1.74%
precise_decimal::sub/0 9.2±0.08ns 9.1±0.08ns -1.09%
precise_decimal::to_string/0 708.9±2.75ns 702.6±1.44ns -0.89%
schema::validate_payload 363.6±0.78µs 366.1±0.80µs +0.69%
transaction::radiswap 5.3±0.04ms 5.2±0.03ms -1.89%
transaction::transfer 1879.5±13.65µs 1871.3±4.28µs -0.44%
transaction_validation::validate_manifest 43.1±0.03µs 43.1±0.03µs 0.00%
transaction_validation::verify_bls_2KB 1094.2±32.17µs 1085.0±25.75µs -0.84%
transaction_validation::verify_bls_32B 1024.6±27.98µs 1002.9±8.35µs -2.12%
transaction_validation::verify_ecdsa 74.6±0.07µs 74.6±0.06µs 0.00%
transaction_validation::verify_ed25519 47.5±0.08µs 42.1±0.11µs -11.37%

LowerBound::Inclusive(decimal) => *decimal,
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem correct to me with the ONE. Also, why not LowerBound(Decimal)?

Copy link
Contributor Author

@dhedey dhedey Oct 4, 2024

Choose a reason for hiding this comment

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

Well Omar was the one advocating for it, but actually I think it makes sense to capture the intention of the caller with e.g. NonZero.

e.g. NonZero for a divisibility 18 Fungible means "at-least one atto", but for a Non-Fungible is actually "at least one". In practice, we interpret NonZero to mean "an infinitesimal amount" when interpreting it as a lower bound.

match self {
ManifestResourceConstraint::NonZeroAmount => true,
ManifestResourceConstraint::ExactAmount(amount) => !amount.is_negative(),
ManifestResourceConstraint::AtLeastAmount(amount) => !amount.is_negative(),
Copy link
Member

Choose a reason for hiding this comment

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

Minor: do we want to restrict granularity to 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice idea, have added these constraints.

Copy link

github-actions bot commented Oct 4, 2024

Phylum Report Link

@dhedey dhedey merged commit 074c9dd into develop Oct 4, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants