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

Add intent hash to the error code #1986

Merged
merged 3 commits into from
Oct 30, 2024
Merged

Conversation

iamyulong
Copy link
Member

Summary

This PR adds intent hash to the rejection error code, so clients can know whether transaction intent hash or subintent hash was failing.

Copy link

github-actions bot commented Oct 29, 2024

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

Copy link

github-actions bot commented Oct 29, 2024

Benchmark for b8167e8

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 44.3±0.09ms 44.3±0.28ms 0.00%
costing::decode_encoded_i8_array_to_manifest_raw_value 19.3±0.03ms 19.6±0.01ms +1.55%
costing::decode_encoded_i8_array_to_manifest_value 40.7±0.05ms 41.3±0.05ms +1.47%
costing::decode_encoded_tuple_array_to_manifest_raw_value 70.5±0.18ms 71.3±0.09ms +1.13%
costing::decode_encoded_tuple_array_to_manifest_value 98.8±0.15ms 119.1±1.11ms +20.55%
costing::decode_encoded_u8_array_to_manifest_raw_value 25.6±0.09µs 25.9±0.06µs +1.17%
costing::decode_encoded_u8_array_to_manifest_value 40.6±0.04ms 41.4±0.05ms +1.97%
costing::decode_rpd_to_manifest_raw_value 14.6±0.02µs 14.6±0.02µs 0.00%
costing::decode_rpd_to_manifest_value 10.8±0.05µs 10.7±0.03µs -0.93%
costing::deserialize_wasm 1216.4±4.62µs 1223.9±5.54µs +0.62%
costing::execute_transaction_creating_big_vec_substates 681.8±3.47ms 689.9±8.94ms +1.19%
costing::execute_transaction_reading_big_vec_substates 576.1±1.25ms 589.3±2.14ms +2.29%
costing::instantiate_flash_loan 1089.7±1619.81µs 913.1±467.98µs -16.21%
costing::instantiate_radiswap 986.6±862.21µs 1035.3±1165.57µs +4.94%
costing::scrypto_malloc 451.6±1.95ms 463.0±1.00ms +2.52%
costing::scrypto_sbor_decode 460.0±1.19ms 472.3±3.70ms +2.67%
costing::scrypto_sha256 347.5±0.68ms 348.4±1.34ms +0.26%
costing::spin_loop_v1 462.4±0.77ms 462.2±2.46ms -0.04%
costing::spin_loop_v2 936.2±1.01ms 938.1±1.68ms +0.20%
costing::validate_sbor_payload 29.8±0.09µs 29.0±0.07µs -2.68%
costing::validate_sbor_payload_bytes 241.4±1.31ns 239.0±0.47ns -0.99%
costing::validate_secp256k1 76.8±0.11µs 76.7±0.12µ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.8±0.00ns 9.8±0.00ns 0.00%
decimal::add/wasmi 217.7±0.47ns 218.4±0.55ns +0.32%
decimal::add/wasmi-call-native 2.1±0.02µs 2.2±0.00µs +4.76%
decimal::div/0 168.9±0.11ns 168.1±1.00ns -0.47%
decimal::from_string/0 155.8±0.08ns 156.6±0.18ns +0.51%
decimal::mul/0 129.4±0.08ns 127.6±0.05ns -1.39%
decimal::mul/rust-native 127.5±0.20ns 126.8±0.08ns -0.55%
decimal::mul/wasmi 11.6±0.09µs 11.4±0.05µs -1.72%
decimal::mul/wasmi-call-native 2.3±0.00µs 2.3±0.01µs 0.00%
decimal::pow/0 594.3±0.20ns 594.5±0.42ns +0.03%
decimal::pow/rust-native 590.0±0.52ns 606.9±0.15ns +2.86%
decimal::pow/wasmi 57.7±0.37µs 58.3±1.19µs +1.04%
decimal::pow/wasmi-call-native 3.2±0.00µs 3.3±0.01µs +3.12%
decimal::root/0 8.2±0.01µs 8.0±0.00µs -2.44%
decimal::sub/0 8.3±0.01ns 8.3±0.00ns 0.00%
decimal::to_string/0 451.0±0.72ns 442.8±0.81ns -1.82%
large_transaction_processing::prepare 2.5±0.00ms 2.5±0.00ms 0.00%
large_transaction_processing::prepare_and_decompile 6.2±0.03ms 6.2±0.03ms 0.00%
large_transaction_processing::prepare_and_decompile_and_recompile 27.6±2.99ms 30.9±2.03ms +11.96%
metadata_validation::validate_urls 5.3±0.03µs 5.2±0.07µs -1.89%
precise_decimal::add/0 8.7±0.13ns 8.7±0.04ns 0.00%
precise_decimal::add/rust-native 10.8±0.07ns 10.7±0.02ns -0.93%
precise_decimal::add/wasmi 277.3±0.49ns 277.1±0.55ns -0.07%
precise_decimal::add/wasmi-call-native 2.8±0.00µs 2.8±0.00µs 0.00%
precise_decimal::div/0 295.9±0.43ns 292.1±0.77ns -1.28%
precise_decimal::from_string/0 203.1±0.14ns 200.8±0.08ns -1.13%
precise_decimal::mul/0 336.2±0.33ns 334.7±0.66ns -0.45%
precise_decimal::mul/rust-native 289.3±0.77ns 296.7±1.36ns +2.56%
precise_decimal::mul/wasmi 33.7±0.50µs 33.3±0.29µs -1.19%
precise_decimal::mul/wasmi-call-native 3.1±0.00µs 3.1±0.01µs 0.00%
precise_decimal::pow/0 1776.6±5.54ns 1723.7±2.18ns -2.98%
precise_decimal::pow/rust-native 1360.6±2.58ns 1358.7±1.73ns -0.14%
precise_decimal::pow/wasmi 162.2±0.46µs 164.6±1.75µs +1.48%
precise_decimal::pow/wasmi-call-native 5.3±0.01µs 5.4±0.01µs +1.89%
precise_decimal::root/0 58.4±0.04µs 58.4±0.05µs 0.00%
precise_decimal::sub/0 9.0±0.18ns 9.0±0.02ns 0.00%
precise_decimal::to_string/0 690.1±0.44ns 692.5±0.52ns +0.35%
schema::validate_payload 381.2±0.36µs 383.0±0.61µs +0.47%
transaction::radiswap 4.8±0.03ms 4.8±0.04ms 0.00%
transaction::transfer 1840.8±4.51µs 1843.6±4.08µs +0.15%
transaction_validation::validate_manifest 43.3±0.04µs 43.2±0.22µs -0.23%
transaction_validation::verify_bls_2KB 1006.1±12.62µs 1001.1±5.00µs -0.50%
transaction_validation::verify_bls_32B 1057.6±26.77µs 1000.2±2.27µs -5.43%
transaction_validation::verify_ecdsa 74.7±0.07µs 74.6±0.05µs -0.13%
transaction_validation::verify_ed25519 42.2±0.08µs 42.3±0.11µs +0.24%

Copy link
Contributor

@dhedey dhedey left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -540,7 +540,7 @@ impl<V: SystemCallbackObject> System<V> {

fn validate_intent_hash_uncosted<S: CommitableSubstateStore>(
Copy link
Contributor

Choose a reason for hiding this comment

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

What were your thoughts on that drive-by refactor to deduplicate the read of TransactionTrackerField::TransactionTracker?

Copy link
Member Author

@iamyulong iamyulong Oct 30, 2024

Choose a reason for hiding this comment

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

My default position is always not to change unless there is a problem.

I'm not too sure if this will cause any performance issue as the duplicate reads will be from cache.

@@ -65,14 +66,17 @@ pub enum RejectionReason {
valid_to_exclusive: Instant,
current_time: Instant,
},
IntentHashPreviouslyCommitted,
IntentHashPreviouslyCancelled,
DeprecatedIntentHashPreviouslyCommitted,
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess if it's not in the LocalTransactionExecution / persisted in node; and we no longer serialize transaction receipts, then we don't actually need this to be maintained over time, and could remove these after all?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's my initial thought. I kept it just in case Gateway is serializing rejection reason somewhere. Happy to remove them if that's not the case.

@iamyulong iamyulong merged commit db8452f into develop Oct 30, 2024
3 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.

2 participants