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: New Executable model (and other misc markups) #1885

Merged
merged 13 commits into from
Aug 28, 2024

Conversation

dhedey
Copy link
Contributor

@dhedey dhedey commented Aug 21, 2024

Honestly, with a big review backlog, I've given up with making my PRs coherent / separate. This is a bit of a mish-mash, of tweaks and improvements.

Details

Headlines include:

  • Add --no-fail-fast to unit tests so we see all failures
  • Making Executable and IntentDetails traits, and preparing for a world of separation of Transaction from Intent.
  • Renaming Executable to ExecutableTransactionV1 and making it a primary implementer of Executable
  • Tweaking engine init and the executable model to be easy to add the new executor to:
    • Refactored the System init function to be better sub-divided and nicer to read; and to facilitate creation of an abortion receipt more easily.
    • Transaction receipt no longer needs to know how to create an abortion receipt (which hardcoded lots of config statically, which felt wrong) - that is now with System.
    • System no longer stores or is parametrized by an E: Executable - this was gnarly and it turns out it wasn't needed. Instead, System stores actions it needs to perform on finalization. If we want to add it back, I propose storing it as a Box<dyn Executable> which will be nicer to work with.
  • Started on some model tweaks - e.g. changing tip to allow it being a u16.
  • Added a LocalTransactionReceiptV1 model from the node, to catch model divergence sooner... and then improved the usability of the SborAssertion macros to be less fragile, and much easier to kick off:
    • Test names don't change
    • Allow generation and regeneration very easily
    • Allow reconfiguring of comparison configuration more easily
  • Slight tweaks to protocol builder / scenarios (basically markups against previous PRs)
    • Can now turn off bootstrapping

Testing

Mostly refactors - existing tests apply.

Need to add tests for tips and things - I assume these will come in due course when we add concrete implementations of the new transaction model and can test assertions against it.

Copy link

Phylum Report Link

Copy link

github-actions bot commented Aug 21, 2024

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

Copy link

github-actions bot commented Aug 21, 2024

Benchmark for 23d7658

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 63.0±0.12ms 63.8±0.20ms +1.27%
costing::decode_encoded_i8_array_to_manifest_raw_value 19.4±0.03ms 19.3±0.01ms -0.52%
costing::decode_encoded_i8_array_to_manifest_value 42.3±0.12ms 41.3±0.04ms -2.36%
costing::decode_encoded_tuple_array_to_manifest_raw_value 61.2±0.15ms 63.0±0.16ms +2.94%
costing::decode_encoded_tuple_array_to_manifest_value 98.7±0.18ms 118.8±1.17ms +20.36%
costing::decode_encoded_u8_array_to_manifest_raw_value 32.4±0.10µs 26.0±0.06µs -19.75%
costing::decode_encoded_u8_array_to_manifest_value 42.4±0.03ms 41.5±0.07ms -2.12%
costing::decode_rpd_to_manifest_raw_value 12.8±0.02µs 12.3±0.03µs -3.91%
costing::decode_rpd_to_manifest_value 10.8±0.03µs 10.7±0.03µs -0.93%
costing::deserialize_wasm 1272.5±4.28µs 1247.9±5.15µs -1.93%
costing::execute_transaction_creating_big_vec_substates 705.7±14.36ms 705.3±12.02ms -0.06%
costing::execute_transaction_reading_big_vec_substates 588.9±1.75ms 588.3±2.02ms -0.10%
costing::instantiate_flash_loan 935.9±584.56µs 1039.1±981.82µs +11.03%
costing::instantiate_radiswap 983.0±972.38µs 976.6±919.29µs -0.65%
costing::spin_loop 21.3±0.16ms 21.3±0.14ms 0.00%
costing::validate_sbor_payload 32.5±0.10µs 32.3±0.12µs -0.62%
costing::validate_sbor_payload_bytes 246.1±1.70ns 245.4±1.03ns -0.28%
costing::validate_secp256k1 76.8±0.18µs 76.7±0.10µs -0.13%
costing::validate_wasm 33.6±0.07ms 34.1±0.07ms +1.49%
decimal::add/0 8.4±0.00ns 8.4±0.01ns 0.00%
decimal::add/rust-native 9.8±0.00ns 9.8±0.00ns 0.00%
decimal::add/wasmi 218.8±0.79ns 225.8±0.54ns +3.20%
decimal::add/wasmi-call-native 2.1±0.00µs 2.0±0.00µs -4.76%
decimal::div/0 187.2±0.16ns 187.4±0.26ns +0.11%
decimal::from_string/0 158.6±0.16ns 168.3±0.20ns +6.12%
decimal::mul/0 150.0±0.35ns 149.9±0.22ns -0.07%
decimal::mul/rust-native 147.1±0.32ns 145.4±0.21ns -1.16%
decimal::mul/wasmi 10.6±0.17µs 10.8±0.07µs +1.89%
decimal::mul/wasmi-call-native 2.2±0.01µs 2.2±0.00µs 0.00%
decimal::pow/0 694.8±0.84ns 698.7±0.99ns +0.56%
decimal::pow/rust-native 666.9±0.39ns 671.6±0.65ns +0.70%
decimal::pow/wasmi 52.4±0.29µs 52.7±0.25µs +0.57%
decimal::pow/wasmi-call-native 2.4±0.00µs 2.4±0.01µs 0.00%
decimal::root/0 7.7±0.15µs 7.7±0.02µs 0.00%
decimal::sub/0 8.3±0.01ns 8.3±0.02ns 0.00%
decimal::to_string/0 443.3±0.90ns 447.1±0.65ns +0.86%
precise_decimal::add/0 8.9±0.03ns 9.0±0.01ns +1.12%
precise_decimal::add/rust-native 10.8±0.15ns 10.8±0.06ns 0.00%
precise_decimal::add/wasmi 272.9±0.46ns 261.2±0.49ns -4.29%
precise_decimal::add/wasmi-call-native 2.7±0.00µs 2.7±0.00µs 0.00%
precise_decimal::div/0 358.4±0.71ns 333.6±11.47ns -6.92%
precise_decimal::from_string/0 200.0±0.34ns 198.5±0.51ns -0.75%
precise_decimal::mul/0 363.2±0.56ns 364.8±1.40ns +0.44%
precise_decimal::mul/rust-native 319.8±0.93ns 316.6±0.29ns -1.00%
precise_decimal::mul/wasmi 33.6±0.14µs 30.5±0.06µs -9.23%
precise_decimal::mul/wasmi-call-native 3.1±0.01µs 3.1±0.01µs 0.00%
precise_decimal::pow/0 1929.6±2.57ns 1924.4±2.73ns -0.27%
precise_decimal::pow/rust-native 1537.6±2.25ns 1582.8±8.14ns +2.94%
precise_decimal::pow/wasmi 148.4±0.94µs 146.9±0.28µs -1.01%
precise_decimal::pow/wasmi-call-native 5.5±0.02µs 5.5±0.01µs 0.00%
precise_decimal::root/0 58.0±0.14µs 58.1±0.03µs +0.17%
precise_decimal::sub/0 9.0±0.13ns 9.2±0.07ns +2.22%
precise_decimal::to_string/0 709.7±1.33ns 706.2±2.68ns -0.49%
schema::validate_payload 381.9±1.14µs 381.2±0.62µs -0.18%
transaction::radiswap 5.3±0.01ms 5.3±0.03ms 0.00%
transaction::transfer 1953.0±6.12µs 1901.1±4.67µs -2.66%
transaction_processing::prepare 2.4±0.00ms 2.4±0.00ms 0.00%
transaction_processing::prepare_and_decompile 6.1±0.03ms 6.0±0.02ms -1.64%
transaction_processing::prepare_and_decompile_and_recompile 25.8±1.78ms 24.3±0.10ms -5.81%
transaction_validation::validate_manifest 42.8±0.14µs 42.8±0.06µs 0.00%
transaction_validation::verify_bls_2KB 1006.4±12.27µs 1022.5±27.23µs +1.60%
transaction_validation::verify_bls_32B 1030.9±31.25µs 1059.0±17.16µs +2.73%
transaction_validation::verify_ecdsa 74.7±0.17µs 74.7±0.15µs 0.00%
transaction_validation::verify_ed25519 55.5±0.14µs 55.9±0.61µs +0.72%

@dhedey dhedey force-pushed the feature/versioned-executable branch from ef10e20 to 0c51979 Compare August 22, 2024 03:08
@dhedey dhedey marked this pull request as ready for review August 22, 2024 03:11
@dhedey dhedey changed the title feature: Introduce versioned TransactionExecutable feature: New Executable model (and other misc markups) Aug 23, 2024
@dhedey dhedey changed the base branch from feature/add-cuttlefish to develop August 28, 2024 20:58
@dhedey dhedey merged commit 908a807 into develop Aug 28, 2024
29 checks passed
@dhedey
Copy link
Contributor Author

dhedey commented Aug 28, 2024

Heads up - I merged #1884 into the wrong branch, so this PR was merged with the changes from #1884 as well :)

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.

2 participants