Aptos gas payer #8773

gerben-stavenga · 2023-06-21T16:03:22Z

Description

Implement alternate gas payer support to aptos-blockchain. This pr reuses the already existing multi-agent framework, as a separate gas payer "is" a special case of a multi-agent transaction. However the existing multi-agent framework requires
a entry function that takes all [sender, secondary_signer...] as the first parameters, a gas payer is typically not a parameter to an entry function, but just a signature to pay for the tx.

This PR uses the MSB (high bit) of the sequence number as a boolean to indicate a gas payer in case of a multi-agent tx.
If this is set the last entry in the list of secondary signer is the "gas payer" and does not participate as signer in the
tx parameters. This is safe as 64bits is large enough for the high bit never to be reached.

Test Plan

aptos-move/aptos-release-builder/src/components/feature_flags.rs

aptos-move/aptos-vm/src/aptos_vm.rs

aptos-move/aptos-vm/src/aptos_vm_impl.rs

aptos-move/framework/move-stdlib/doc/features.md

aptos-move/aptos-vm/src/aptos_vm.rs

movekevin · 2023-06-21T22:36:36Z

aptos-move/e2e-move-tests/src/tests/scripts.rs

@@ -80,3 +81,74 @@ fn read_coin(h: &MoveHarness, account: &AccountAddress) -> u64 {
        .unwrap()
        .coin()
 }
+
+#[test]
+fn test_two_to_two_transfer_gas_payer() {


Can you add some more tests around cases such as:

Feature flag is not enabled.

Gas payer doesn't have enough gas

Extra senders without gas payer bit set.

etc. Any other edge cases? Let's be as thorough as possible here to make sure there are no unforeseen issues

Still missing some failure cases:

Gas payer doesn't have sufficient balances.

Invalid gas payer signature

Etc.
I'll leave it to you to determine what can be covered with unit vs move e2e tests. We'll also have a more concise integration test later in TS as well for a true e2e.

Given that this code is just using multi-agent signing and reusing the epilogue. Most of these flows are tested by regular tests

I have tests that cover (scripts/entry functions)/(no signer/single signer/multiple signers)/(gas bit/no-gas bit)(feature/no feature)

aptos-move/aptos-vm/src/aptos_vm_impl.rs

wrwg

This looks good to me from the code perspective, wo/ understanding the design goals. More tests would be good.

aptos-move/aptos-vm/src/aptos_vm.rs

zekun000

lgtm

aptos-move/aptos-vm/src/aptos_vm_impl.rs

movekevin · 2023-06-22T15:37:13Z

aptos-move/aptos-vm/src/aptos_vm_impl.rs

+        } else {
+            // Gas payer tx
+            let gas_payer = *txn_data.secondary_signers.last().ok_or_else(|| {
+                PartialVMError::new(StatusCode::UNKNOWN_INVARIANT_VIOLATION_ERROR)


We shouldn't use UNKNOWN_INVARIANT_VIOLATION_ERROR here as this case can happen if user sets the gas payer bit for a normal tx with no secondary signers

No it's invariant error, as we shouldn't even get to the epilogue if the gas payer account is missing. Was tempted to do unwrap but decided invariant error is better

I added a proper abort code in the multi_agent prologue to verify this

We're being a bit strict and trying to avoid invariant violation as much as possible. cc @runtian-zhou regarding whether this is a legit case

github-actions · 2023-06-23T21:04:55Z

✅ Forge suite `compat` success on `aptos-node-v1.4.4` ==> `cbcab072cd84a8e12f44beb937c96488e0f7abb8`

Compatibility test results for aptos-node-v1.4.4 ==> cbcab072cd84a8e12f44beb937c96488e0f7abb8 (PR)
1. Check liveness of validators at old version: aptos-node-v1.4.4
compatibility::simple-validator-upgrade::liveness-check : committed: 7169 txn/s, latency: 4627 ms, (p50: 4500 ms, p90: 6800 ms, p99: 8400 ms), latency samples: 258100
2. Upgrading first Validator to new version: cbcab072cd84a8e12f44beb937c96488e0f7abb8
compatibility::simple-validator-upgrade::single-validator-upgrade : committed: 3691 txn/s, latency: 8629 ms, (p50: 9400 ms, p90: 11000 ms, p99: 11500 ms), latency samples: 147660
3. Upgrading rest of first batch to new version: cbcab072cd84a8e12f44beb937c96488e0f7abb8
compatibility::simple-validator-upgrade::half-validator-upgrade : committed: 4347 txn/s, latency: 7344 ms, (p50: 8200 ms, p90: 10300 ms, p99: 11000 ms), latency samples: 165220
4. upgrading second batch to new version: cbcab072cd84a8e12f44beb937c96488e0f7abb8
compatibility::simple-validator-upgrade::rest-validator-upgrade : committed: 6523 txn/s, latency: 4941 ms, (p50: 4900 ms, p90: 6900 ms, p99: 8100 ms), latency samples: 234840
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> cbcab072cd84a8e12f44beb937c96488e0f7abb8 passed
Test Ok

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

github-actions · 2023-06-23T21:07:47Z

✅ Forge suite `land_blocking` success on `cbcab072cd84a8e12f44beb937c96488e0f7abb8`

performance benchmark : committed: 5588 txn/s, submitted: 5589 txn/s, expired: 1 txn/s, latency: 7091 ms, (p50: 5100 ms, p90: 13200 ms, p99: 27400 ms), latency samples: 2386092
Max round gap was 1 [limit 4] at version 724436. Max no progress secs was 3.96412 [limit 10] at version 1514058.
Test Ok

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

github-actions · 2023-06-23T21:12:25Z

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.4.4` ==> `cbcab072cd84a8e12f44beb937c96488e0f7abb8`

Compatibility test results for aptos-node-v1.4.4 ==> cbcab072cd84a8e12f44beb937c96488e0f7abb8 (PR)
Upgrade the nodes to version: cbcab072cd84a8e12f44beb937c96488e0f7abb8
framework_upgrade::framework-upgrade::full-framework-upgrade : committed: 3175 txn/s, latency: 7118 ms, (p50: 8000 ms, p90: 9500 ms, p99: 12000 ms), latency samples: 171500
5. check swarm health
Compatibility test for aptos-node-v1.4.4 ==> cbcab072cd84a8e12f44beb937c96488e0f7abb8 passed
Test Ok

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

* initial commit * Support gas payer * Remove relics * Fix * Make verification know about gas payer bit * update comments * Remove merge conflict * Give MSB a good name * Reformat * improve * add comment * Fix spec

gerben-stavenga added 3 commits May 30, 2023 19:13

initial commit

054a910

Merge branch 'main' into aptos-gas-payer

54dbda6

Support gas payer

af458e4

gerben-stavenga marked this pull request as ready for review June 21, 2023 20:48

gerben-stavenga requested review from saharct, junkil-park, davidiw, movekevin, wrwg and zekun000 as code owners June 21, 2023 20:48

gerben-stavenga added 4 commits June 21, 2023 13:51

Remove relics

e1fcb6b

Fix

77504bc

Make verification know about gas payer bit

fe4ad39

update comments

53ed2c8

gerben-stavenga requested review from xindingw, vgao1996 and runtian-zhou June 21, 2023 21:06

Merge branch 'main' into aptos-gas-payer

60e0d2b

gerben-stavenga requested a review from lightmark June 21, 2023 21:09

Remove merge conflict

63c09f4

movekevin reviewed Jun 21, 2023

View reviewed changes

aptos-move/aptos-release-builder/src/components/feature_flags.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm/src/aptos_vm.rs Outdated Show resolved Hide resolved

aptos-move/aptos-vm/src/aptos_vm_impl.rs Outdated Show resolved Hide resolved

Give MSB a good name

43b4d71

gerben-stavenga requested review from alinush and bowenyang007 June 21, 2023 21:55

bowenyang007 reviewed Jun 21, 2023

View reviewed changes

aptos-move/framework/move-stdlib/doc/features.md Outdated Show resolved Hide resolved

Reformat

df6e2ef

movekevin reviewed Jun 21, 2023

View reviewed changes

wrwg approved these changes Jun 21, 2023

View reviewed changes

aptos-move/aptos-vm/src/aptos_vm.rs Show resolved Hide resolved

zekun000 reviewed Jun 21, 2023

View reviewed changes

aptos-move/aptos-vm/src/aptos_vm_impl.rs Show resolved Hide resolved

movekevin reviewed Jun 22, 2023

View reviewed changes

gerben-stavenga requested review from msmouse, JoshLind, gregnazario, brianolson, banool, 0xmaayan, 0xmigo, a team, ibalajiarun, geekflyer, gelash, sasha8, danielxiangzl and georgemitenkov as code owners June 23, 2023 20:02

gerben-stavenga force-pushed the aptos-gas-payer branch from a31cc57 to 23298a4 Compare June 23, 2023 20:10

gerben-stavenga added 3 commits June 23, 2023 13:18

Fix prover + add tests

c0ec675

Insufficient funds test

9b0b86f

Merge branch 'main' into aptos-gas-payer

cbcab07

This comment has been minimized.

Sign in to view

movekevin approved these changes Jun 23, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

gerben-stavenga merged commit 6d4b495 into main Jun 23, 2023

gerben-stavenga deleted the aptos-gas-payer branch June 23, 2023 21:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aptos gas payer #8773

Aptos gas payer #8773

gerben-stavenga commented Jun 21, 2023 •

edited

Loading

movekevin Jun 21, 2023

movekevin Jun 23, 2023

gerben-stavenga Jun 23, 2023

gerben-stavenga Jun 23, 2023

wrwg left a comment

zekun000 left a comment

movekevin Jun 22, 2023

gerben-stavenga Jun 22, 2023

gerben-stavenga Jun 22, 2023

movekevin Jun 22, 2023

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jun 23, 2023

github-actions bot commented Jun 23, 2023

github-actions bot commented Jun 23, 2023

Aptos gas payer #8773

Aptos gas payer #8773

Conversation

gerben-stavenga commented Jun 21, 2023 • edited Loading

Description

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

wrwg left a comment

Choose a reason for hiding this comment

zekun000 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

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Jun 23, 2023

✅ Forge suite compat success on aptos-node-v1.4.4 ==> cbcab072cd84a8e12f44beb937c96488e0f7abb8

github-actions bot commented Jun 23, 2023

✅ Forge suite land_blocking success on cbcab072cd84a8e12f44beb937c96488e0f7abb8

github-actions bot commented Jun 23, 2023

✅ Forge suite framework_upgrade success on aptos-node-v1.4.4 ==> cbcab072cd84a8e12f44beb937c96488e0f7abb8

gerben-stavenga commented Jun 21, 2023 •

edited

Loading

✅ Forge suite `compat` success on `aptos-node-v1.4.4` ==> `cbcab072cd84a8e12f44beb937c96488e0f7abb8`

✅ Forge suite `land_blocking` success on `cbcab072cd84a8e12f44beb937c96488e0f7abb8`

✅ Forge suite `framework_upgrade` success on `aptos-node-v1.4.4` ==> `cbcab072cd84a8e12f44beb937c96488e0f7abb8`