[API] Add X-Aptos-Gas-Used header #13237

banool · 2024-05-09T17:26:10Z

Description

This PR makes the simulate and view function endpoints return how much gas they used. We will use this for compute unit based ratelimiting in API Gateway.

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

New API test. I can add one for simulation too if we want.

curl -v --request POST --url 127.0.0.1:8080/v1/view --header 'Accept: application/json' --header 'Content-Type: application/json' --data '{
    "function": "0x1::coin::balance",
    "type_arguments": [
      "0x1::aptos_coin::AptosCoin"
    ],
    "arguments": ["0x1"]
  }'

 x-aptos-gas-cost: 1

Key Areas to Review

Consider if there is a better, easier way to do this than with a header.

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

trunk-io · 2024-05-09T17:26:13Z

⏱️ 11h 28m total CI duration on this PR

Job	Cumulative Duration	Recent Runs
forge-framework-upgrade-test / forge	2h 24m	🟩 🟩
windows-build	1h 23m	🟩 🟩
rust-smoke-tests	1h 12m	🟩 🟩
rust-targeted-unit-tests	1h 8m	🟩 🟩 🟩 🟩 🟩 (+1 more)
execution-performance / single-node-performance	1h	🟥 🟥 🟩
rust-lints	47m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-move-tests	30m	🟩 🟩 🟩 🟩 🟩 (+1 more)
forge-compat-test / forge	28m	🟩 🟩
forge-e2e-test / forge	28m	🟩 🟩
run-tests-main-branch	27m	🟩 🟩 🟩 🟩 🟩 (+1 more)
rust-images / rust-all	25m	🟩 🟩
cli-e2e-tests / run-cli-tests	15m	🟥 🟩
rust-build-cached-packages	12m	🟩 🟩
general-lints	11m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check-dynamic-deps	8m	🟩 🟩 🟩 🟩 🟩 (+1 more)
check	8m	🟩 🟩
execution-performance / test-target-determinator	7m	🟩 🟩
test-target-determinator	6m	🟩 🟩
semgrep/ci	3m	🟩 🟩 🟩 🟩 🟩 (+1 more)
node-api-compatibility-tests / node-api-compatibility-tests	2m	🟩 🟩
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
file_change_determinator	1m	🟩 🟩 🟩 🟩 🟩 (+1 more)
permission-check	20s	🟩 🟩 🟩 🟩 🟩
file_change_determinator	20s	🟩 🟩
permission-check	16s	🟩 🟩 🟩 🟩 🟩
permission-check	14s	🟩 🟩 🟩 🟩 🟩
permission-check	10s	🟩 🟩 🟩 🟩 🟩
determine-docker-build-metadata	4s	🟩 🟩
permission-check	2s	🟩

🚨 4 jobs on the last run were significantly faster/slower than expected

Job	Duration	vs 7d avg
rust-build-cached-packages	8m	5m
rust-lints	10m	7m
rust-targeted-unit-tests	14m	19m
rust-move-tests	7m	12m

_{settings ⋅ feedback ⋅ docs ⋅ learn more about trunk.io}

bchocho · 2024-05-09T17:49:48Z

api/src/response.rs

@@ -143,6 +143,8 @@ macro_rules! generate_error_response {
                #[oai(header = "X-Aptos-Block-Height")] Option<u64>,
                /// Oldest non-pruned block height of the chain
                #[oai(header = "X-Aptos-Oldest-Block-Height")] Option<u64>,
+                /// The cost of the call in terms of gas
+                #[oai(header = "x-aptos-gas-cost")] Option<u64>,


nit: Any reason not to follow the existing case conventions? (X-Aptos-Gas-Cost)

Should we just be consistent with "gas used" as in the code? Especially with txn simulation I think it might confuse people in thinking this cost is "APT/octas" paid instead of just "gas units" used

Headers are actually meant to be lower case it turns out. Most libraries handle them case insensitively anyway. But I can make this follow the existing convention.

Good call.

bchocho · 2024-05-09T22:35:12Z

api/src/tests/simulation_test.rs

@@ -70,6 +70,8 @@ const LARGE_TRANSFER_AMOUNT: u64 = 1_000_000_000;
 async fn test_simulate_transaction_with_valid_signature() {
    let mut context = new_test_context(current_function_name!());
    let resp = simulate_aptos_transfer(&mut context, true, SMALL_TRANSFER_AMOUNT, 400).await;
+    // Assert the gas used header is present.
+    //assert!(resp.headers().contains_key("X-Aptos-Gas-Used"));


Why is it commented?

Oop let me address that.

bchocho

LGTM

vgao1996 · 2024-05-14T21:59:24Z

api/src/tests/simulation_test.rs

+            assert_eq!(
+                resp.headers().get("X-Aptos-Gas-Used").unwrap(),
+                expected_gas_used.to_string().as_str()
+            );


Yeah this looks a bit fragile to me, since the gas cost of a transaction could change if we modify or re-calibrate the gas schedule.

Would it make sense if you just assert that resp.headers().get("X-Aptos-Gas-Used").is_some()?

I had that at first actually because I also thought it was fragile, but I wanted a better test. How about we assert gas is at least > 0?

gregnazario · 2024-05-14T18:44:27Z

api/src/response.rs

+                /// The cost of the call in terms of gas
+                #[oai(header = "X-Aptos-Gas-Used")] Option<u64>,


I'm not sure this belongs here, would it be on every response? I guess it shows up only on simulations?

View function and simulation yeah.

gregnazario · 2024-05-14T22:48:09Z

api/src/tests/view_function.rs

+    // Confirm the gas used header is present and has the expected value.
+    assert_eq!(resp.headers().get("X-Aptos-Gas-Used").unwrap(), "2");


Probably best to just check it exists, rather than that it's 2

github-actions · 2024-05-15T17:20:56Z

✅ Forge suite `realistic_env_max_load` success on `aa3785c84e334da36c7a3f9c246669469db2cc8a`

two traffics test: inner traffic : committed: 7547.132901414482 txn/s, latency: 5171.9997392716705 ms, (p50: 5100 ms, p90: 6000 ms, p99: 10200 ms), latency samples: 3275440
two traffics test : committed: 99.88504902082137 txn/s, latency: 1947.276404494382 ms, (p50: 1900 ms, p90: 2200 ms, p99: 3600 ms), latency samples: 1780
Latency breakdown for phase 0: ["QsBatchToPos: max: 0.205, avg: 0.201", "QsPosToProposal: max: 0.264, avg: 0.244", "ConsensusProposalToOrdered: max: 0.485, avg: 0.433", "ConsensusOrderedToCommit: max: 0.375, avg: 0.359", "ConsensusProposalToCommit: max: 0.803, avg: 0.792"]
Max round gap was 1 [limit 4] at version 1605469. Max no progress secs was 4.517688 [limit 15] at version 1605469.
Test Ok

github-actions · 2024-05-15T17:22:27Z

✅ Forge suite `compat` success on `01b24e7e3548382dd25440b39a0438a993387f12` ==> `aa3785c84e334da36c7a3f9c246669469db2cc8a`

Compatibility test results for 01b24e7e3548382dd25440b39a0438a993387f12 ==> aa3785c84e334da36c7a3f9c246669469db2cc8a (PR)
1. Check liveness of validators at old version: 01b24e7e3548382dd25440b39a0438a993387f12
compatibility::simple-validator-upgrade::liveness-check : committed: 5861.178817007249 txn/s, latency: 5416.137746999077 ms, (p50: 5100 ms, p90: 8700 ms, p99: 15000 ms), latency samples: 216600
2. Upgrading first Validator to new version: aa3785c84e334da36c7a3f9c246669469db2cc8a
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 1804.7078912934946 txn/s, latency: 15949.636307007786 ms, (p50: 19900 ms, p90: 22900 ms, p99: 23300 ms), latency samples: 89900
3. Upgrading rest of first batch to new version: aa3785c84e334da36c7a3f9c246669469db2cc8a
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 1534.2240808099648 txn/s, latency: 18771.26283218504 ms, (p50: 19900 ms, p90: 22400 ms, p99: 22900 ms), latency samples: 81280
4. upgrading second batch to new version: aa3785c84e334da36c7a3f9c246669469db2cc8a
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 3544.6656623751337 txn/s, latency: 8816.541157356194 ms, (p50: 9600 ms, p90: 12600 ms, p99: 12700 ms), latency samples: 144640
5. check swarm health
Compatibility test for 01b24e7e3548382dd25440b39a0438a993387f12 ==> aa3785c84e334da36c7a3f9c246669469db2cc8a passed
Test Ok

github-actions · 2024-05-15T18:24:42Z

✅ Forge suite `framework_upgrade` success on `01b24e7e3548382dd25440b39a0438a993387f12` ==> `aa3785c84e334da36c7a3f9c246669469db2cc8a`

Compatibility test results for 01b24e7e3548382dd25440b39a0438a993387f12 ==> aa3785c84e334da36c7a3f9c246669469db2cc8a (PR)
Upgrade the nodes to version: aa3785c84e334da36c7a3f9c246669469db2cc8a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1374.7883860655768 txn/s, submitted: 1376.6065885930234 txn/s, failed submission: 1.8182025274466216 txn/s, expired: 1.8182025274466216 txn/s, latency: 2363.2069680939 ms, (p50: 1800 ms, p90: 4200 ms, p99: 5700 ms), latency samples: 120980
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1133.325097967686 txn/s, submitted: 1135.7500300971635 txn/s, failed submission: 2.4249321294776394 txn/s, expired: 2.4249321294776394 txn/s, latency: 2615.6350807235945 ms, (p50: 2100 ms, p90: 4500 ms, p99: 6300 ms), latency samples: 102820
5. check swarm health
Compatibility test for 01b24e7e3548382dd25440b39a0438a993387f12 ==> aa3785c84e334da36c7a3f9c246669469db2cc8a passed
Upgrade the remaining nodes to version: aa3785c84e334da36c7a3f9c246669469db2cc8a
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 1176.853573465486 txn/s, submitted: 1179.3618837363872 txn/s, failed submission: 2.5083102709010556 txn/s, expired: 2.5083102709010556 txn/s, latency: 2612.5839856616935 ms, (p50: 2100 ms, p90: 4500 ms, p99: 6600 ms), latency samples: 103220
Test Ok

bchocho reviewed May 9, 2024

View reviewed changes

banool force-pushed the banool/gas-cost-header branch from 6a27200 to b13552c Compare May 9, 2024 21:30

banool marked this pull request as ready for review May 9, 2024 21:31

banool requested review from gregnazario and 0xmaayan as code owners May 9, 2024 21:31

bchocho reviewed May 9, 2024

View reviewed changes

banool force-pushed the banool/gas-cost-header branch 2 times, most recently from ca7ce2d to 528ad6c Compare May 10, 2024 16:01

banool changed the title ~~[API] Add x-aptos-gas-cost header~~ [API] Add x-aptos-gas-used header May 10, 2024

banool changed the title ~~[API] Add x-aptos-gas-used header~~ [API] Add X-Aptos-Gas-Used header May 10, 2024

bchocho approved these changes May 10, 2024

View reviewed changes

banool enabled auto-merge (squash) May 11, 2024 00:22

This comment has been minimized.

Sign in to view

vgao1996 reviewed May 14, 2024

View reviewed changes

gregnazario reviewed May 14, 2024

View reviewed changes

[API] Add X-Aptos-Gas-Used header

aa3785c

banool force-pushed the banool/gas-cost-header branch from 528ad6c to aa3785c Compare May 15, 2024 16:53

vgao1996 approved these changes May 15, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

banool merged commit 55e0d6a into main May 15, 2024
51 checks passed

banool deleted the banool/gas-cost-header branch May 15, 2024 18:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[API] Add X-Aptos-Gas-Used header #13237

[API] Add X-Aptos-Gas-Used header #13237

banool commented May 9, 2024 •

edited

trunk-io bot commented May 9, 2024 •

edited

bchocho May 9, 2024

banool May 9, 2024

bchocho May 9, 2024

banool May 10, 2024

bchocho left a comment

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.

This comment has been minimized.

vgao1996 May 14, 2024

banool May 15, 2024

gregnazario May 14, 2024

banool May 15, 2024

gregnazario May 14, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 15, 2024

		/// The cost of the call in terms of gas
		#[oai(header = "X-Aptos-Gas-Used")] Option<u64>,

		// Confirm the gas used header is present and has the expected value.
		assert_eq!(resp.headers().get("X-Aptos-Gas-Used").unwrap(), "2");

[API] Add X-Aptos-Gas-Used header #13237

[API] Add X-Aptos-Gas-Used header #13237

Conversation

banool commented May 9, 2024 • edited

Description

Type of Change

Which Components or Systems Does This Change Impact?

How Has This Been Tested?

Key Areas to Review

Checklist

trunk-io bot commented May 9, 2024 • edited

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

bchocho left a comment

Choose a reason for hiding this comment

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.

This comment has been minimized.

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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented May 15, 2024

✅ Forge suite realistic_env_max_load success on aa3785c84e334da36c7a3f9c246669469db2cc8a

github-actions bot commented May 15, 2024

✅ Forge suite compat success on 01b24e7e3548382dd25440b39a0438a993387f12 ==> aa3785c84e334da36c7a3f9c246669469db2cc8a

github-actions bot commented May 15, 2024

✅ Forge suite framework_upgrade success on 01b24e7e3548382dd25440b39a0438a993387f12 ==> aa3785c84e334da36c7a3f9c246669469db2cc8a

banool commented May 9, 2024 •

edited

trunk-io bot commented May 9, 2024 •

edited

✅ Forge suite `realistic_env_max_load` success on `aa3785c84e334da36c7a3f9c246669469db2cc8a`

✅ Forge suite `compat` success on `01b24e7e3548382dd25440b39a0438a993387f12` ==> `aa3785c84e334da36c7a3f9c246669469db2cc8a`

✅ Forge suite `framework_upgrade` success on `01b24e7e3548382dd25440b39a0438a993387f12` ==> `aa3785c84e334da36c7a3f9c246669469db2cc8a`