Support of EIP-1559 in the Neon EVM #409

evgenyzdanovich · 2024-05-13T09:14:49Z

Introduce struct for the new transaction type.
Validate Operator's and charge User's priority fee.
Add logging to be able to parse from the proxy.
Implement BASEFEE opcode.
Support the new transaction type in the emulation.

alfiedotwtf

Again, take my comments with a grain of salt... they're more to give you "things to think about just in case" rather than unresolved comments that actually need changing.

evm_loader/program/src/account/sysvar.rs

alfiedotwtf · 2024-05-14T12:29:58Z

evm_loader/program/src/account/sysvar.rs

+            .and_then(|res| res.checked_div(CONVERSION_DIVISOR.into()))
+            .ok_or(Error::PriorityFeeError("base_fee_per_gas * ".into()))?;
+
+        if actual_priority_fee_per_gas >= trx_max_priority_fee_per_gas {


Maybe I'm misunderstanding here, but why is X >= MAX_X ok and X < MAX_X the error?

The operator may be willing to pay more for priority fee to speed up transactions (on top of what user agreed to pay).

Link to the relevatn section in the DD:
https://www.notion.so/neonfoundation/Support-of-EIP-1559-in-Neon-40d76ecf1e5f4e609e71b45376fb5352?pvs=4#6d352913d9244ca293bac01038a4303d

evm_loader/program/src/account/sysvar.rs

alfiedotwtf · 2024-05-14T12:37:56Z

evm_loader/lib/src/types/mod.rs

+                let dynamic_fee_tx = DynamicFeeTx {
+                    nonce,
+                    max_priority_fee_per_gas,
+                    max_fee_per_gas: self.max_fee_per_gas.unwrap_or(max_priority_fee_per_gas * 2),


Just wondering if the 2 here is arbitrary or something agreed on.

for library code I don't this it's very relevant (it's not used currently). The only catch is to have base_fee_per_gas > 0 , otherwise some validation may fail when this code is called from the api/cli for example.

base_fee_per_gas = max_fee_per_gas - max_priority_fee_per_gas =>

So I just assigned base_fee_per_gas = max_priority_fee_per_gas and both are valid.

alfiedotwtf · 2024-05-14T12:39:42Z

evm_loader/program/src/instruction/transaction_step.rs

+    // In case the transaction is DynamicFee:
+    // (1) validate that the Operator specified the priority fee according to transaction.
+    // (2) charge the User in favor of Operator with amount of `priority_fee_per_gas` * `LAMPORTS_PER_SIGNATURE`.
+    if let TransactionPayload::DynamicFee(dynamic_fee_payload) = &storage.trx().transaction {


Should handle_priority_fee() be called here instead?

alfiedotwtf · 2024-05-14T12:44:13Z

evm_loader/program/src/types/transaction.rs

@@ -266,13 +266,123 @@ impl rlp::Decodable for AccessListTx {
    }
 }



We don't do it elsewhere (yet) and it may be a useless idea, but thinking in terms of optimizing space - should we consider reordering fields in favour of optimising for struct packing here? Like I said - it might be useless trying to save a few bytes, but just thought I would bring it up (at least to keep in the back of our collective minds for the future).

It's a good idea in general, I support it!

However, this work should be done carefully with heap-inside-account effort - there I already introduced repr(C) for transaction struct which implies strict C-like memory layout. Not sure how repr(C) would impact effectiveness of storage.

Also, for such an effort, we have to think carefully about the roll-out process, so that older transactions (with the previous, default-rust-optimized, memory layour) can be digested by the new order of fields.

Feel free to create a task, we can discuss it as a team if it's worth doing and what other things may pop up during impl.

alfiedotwtf · 2024-05-14T12:45:43Z

evm_loader/program/src/types/transaction.rs

+
+                for key in &entry.at(1)? {
+                    storage_keys.push(key.as_val()?);
+                }


Maybe I'm reading this wrong, but since entry doesn't look like it's changing, isn't this an infinite loop?

entry is being "changed" by the outer loop, right? Here we just iterate over the inner list of addresses. Maybe I'm missing something?:)

Off-topic, some context around access list state in Neon:

You see, AccessList transactions are supported by our EVM for quite a long time.

The problem, they are NOT supported by the proxy (operator never creates such transactions and always falls back to creation of legacy ones).
Moreover, they are NOT very well supported even by Ethereum's infra. For example, Metamask is yet to add creation of such transactions: MetaMask/metamask-extension#11863

Summing things up: AccessList transactions mechanism is not battle tested at all :) What I did is basically copied this piece of rlp parsing from access list transactions into dynamic gas transactions. It's quite possible there are some bugs that should be discovered during testing.

P.S. Also, last piece of my long rant: access list is an Ethereum mechanism designed to save some gas given transaction specifies addresses/memory cells it's going to touch. For Neon, I can't see how it plays out simply because in solana operator is being charged per-signature (and not per memory/address access).

evgenyzdanovich · 2024-05-24T11:53:41Z

@alfiedotwtf Thank you for the review! Sorry that I took long, I was quite busy with proxy changed and didnt want to contextswitch :)
I replied and addressed what made sense to me.
Feel free to have another look!

anton-lisanin · 2024-06-28T11:44:28Z

evm_loader/program/src/account/sysvar.rs

+    pub fn get_compute_budget_priority_fee(&self) -> Result<(u32, u64), Error> {
+        let compute_budget_account_pubkey =
+            Pubkey::from_str(COMPUTE_BUDGET_ADDRESS).map_err(|_| {
+                Error::PriorityFeeParsingError("Invalid Compute budget address.".to_string())


Please avoid string conversion at runtime inside evm program. It could take a lot of compute units.
Solana provides pubkey! macro to convert str to Pubkey at compile time
https://docs.rs/solana-program/1.18.17/solana_program/macro.pubkey.html

you are right. thanks done.

anton-lisanin · 2024-06-28T11:48:50Z

evm_loader/program/src/account/sysvar.rs

+            Pubkey::from_str(COMPUTE_BUDGET_ADDRESS).map_err(|_| {
+                Error::PriorityFeeParsingError("Invalid Compute budget address.".to_string())
+            })?;
+        // Intent is to check all the instructions before the current one.


Are you sure? Can compute budget instruction be included after the current one?
Check how compute budget processed in the solana validator.
https://github.com/anza-xyz/agave/blob/73c7150bb15aea95e92b359ea9a41208cd4dbe15/program-runtime/src/compute_budget_processor.rs#L68

Thanks, checked. It seems that for transaction to be valid, the solana runtime first need to sanitize transaction:

check that compute budget limit are set correctly (e.g. limit < 1.4M, heap < 256kb, etc)

actually enforce those limit by creating SVM with specified values (heap size, CU limit, etc).

https://github.com/anza-xyz/agave/blob/70efe819bcf70476926f06c473f29015b3269536/runtime-transaction/src/runtime_transaction.rs#L89

So long story short, CB handling goes before actual transaction logic and indeed, it's not required to have CB placed first in the instruction list. I doubled checked it locally by placing CB instructions at the end.

On the other hand:

proxy does exactly this,

most of transactions I've seen are doing the same (it's quite logical),

we have no other way, but to rely this fact (after I removed Sysvar and substituted it with the syscall instruction::get_**processed**_sibling_instruction, please see recent changes).

The processed word is important here, it implies that CB instructions should go first. Otherwise syscall does not see CB instuction, I also checked it locally.

What's your opinion?

anton-lisanin · 2024-06-28T11:56:30Z

evm_loader/program/src/account_storage/apply.rs

-
-        source.increment_revision(&self.rent, &self.accounts)?;
+        if inc_revision {
+            source.increment_revision(&self.rent, &self.accounts)?;


Revision of account should always be incremented when it's balance changed

anton-lisanin · 2024-06-28T12:00:03Z

evm_loader/program/src/instruction/mod.rs

@@ -190,6 +196,7 @@ pub enum EvmInstruction {
    ///  `[WRITE]` Treasury
    ///  `[WRITE]` Operator Balance
    ///  `[]` System program
+    ///  `[]` Sysvar account


It may be better to create a separate set of instructions with sysvar accounts.
That way we still have an option to run the transaction in the old format without it.

Discuss this with @afalaleev

Ye, I discussed it.
I managed to remove sysvar account and get the necessary data about compute budget instructions via syscall, so it's done.

…rity fee.

…lude it in the account list.

…rams.

…fee_per_gas, max_priority_fee_per_gas.

…ty fee.

…riority_fee validation with get_processed_sibling_instruction syscall.

neonlabstech · 2024-08-01T21:48:49Z

Dapps report

Cost report for "Uniswap V3" dApp

Action	Fee	Cost in $	Accounts	TRx	Estimated Gas	Used Gas	Used % of EG
Token approve	0.00234464	0.0012148048768	9	1	1192320	1172320	98.32
NonfungiblePositionManager - Mint position	0.02747064	0.014233087996800002	4	7	14000320	13735320	98.11
NonfungiblePositionManager - Increase liquidity	8e-05	4.1449600000000004e-05	23	6	230000	40000	17.39
NonfungiblePositionManager - Decrease Liquidity	0.00139808	0.0007243732096	17	1	829040	699040	84.32
NonfungiblePositionManager - Collect Fees	2e-05	1.0362400000000001e-05	20	1	80000	10000	12.5
NonfungiblePositionManager - Burn Liquidity Position	2e-05	1.0362400000000001e-05	15	1	50000	10000	20.0
Direct swap	2e-05	1.0362400000000001e-05	17	1	180000	10000	5.56
Burn transaction	0.00047936	0.0002483660032	11	1	279680	239680	85.7
Collect transaction	0.00234464	0.0012147579839999999	12	1	1202320	1172320	97.5

evgenyzdanovich requested review from alfiedotwtf, anton-lisanin, spylogsster, andreisilviudragnea and s-medvedev May 13, 2024 09:14

alfiedotwtf previously approved these changes May 14, 2024

View reviewed changes

evgenyzdanovich dismissed alfiedotwtf’s stale review via e5c44ec May 24, 2024 11:12

evgenyzdanovich force-pushed the ndev_2960_eip_1559_support branch from 95ed233 to e5c44ec Compare May 24, 2024 11:12

anton-lisanin assigned evgenyzdanovich May 28, 2024

evgenyzdanovich force-pushed the ndev_2960_eip_1559_support branch 3 times, most recently from 2200889 to 479cea6 Compare May 28, 2024 09:18

anton-lisanin removed the request for review from spylogsster June 28, 2024 11:36

anton-lisanin requested changes Jun 28, 2024

View reviewed changes

evgenyzdanovich added 14 commits July 31, 2024 14:40

Introduce DynamicGas transaction type.

c93cb1a

Implement instruction inspection for Compute Budget to calculate prio…

654a89a

…rity fee.

Introduce Sysvar account and modify transaction_* instructions to inc…

c335a9f

…lude it in the account list.

Implement validation of priority fee and transfer.

775b03e

Add PRIORITYFEE logging to be able to parse it from the proxy.

0347ff4

Implement BASEFEE opcode, take it from the transaction.

6fcc0a3

Minor fix of the comment.

9b94193

Support DynamicFee transaction type in the transaction emulation TxPa…

c93fbd7

…rams.

Expose additional transaction data from get_holder API: tx_type, max_…

99ad0e0

…fee_per_gas, max_priority_fee_per_gas.

Minor review changes, part1.

02dcdd6

Handle transaction parsed rlp gracefully in the get_holder api method.

deb5d50

Fix after rebase on develop.

858f977

Bugfix: do not increment source account revision when charging priori…

570063c

…ty fee.

Fixes after rebase.

cb7ac49

evgenyzdanovich force-pushed the ndev_2960_eip_1559_support branch from 8736faf to cb7ac49 Compare July 31, 2024 12:40

Remove Sysvar account for evm instructions account list, substitute p…

0b1b756

…riority_fee validation with get_processed_sibling_instruction syscall.

evgenyzdanovich requested review from alfiedotwtf and removed request for alfiedotwtf August 1, 2024 22:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support of EIP-1559 in the Neon EVM #409

Support of EIP-1559 in the Neon EVM #409

evgenyzdanovich commented May 13, 2024

alfiedotwtf left a comment

alfiedotwtf May 14, 2024

evgenyzdanovich May 24, 2024

alfiedotwtf May 14, 2024

evgenyzdanovich May 24, 2024

alfiedotwtf May 14, 2024

alfiedotwtf May 14, 2024

evgenyzdanovich May 24, 2024

alfiedotwtf May 14, 2024

evgenyzdanovich May 24, 2024

evgenyzdanovich commented May 24, 2024

anton-lisanin Jun 28, 2024

evgenyzdanovich Aug 1, 2024

anton-lisanin Jun 28, 2024

evgenyzdanovich Aug 1, 2024 •

edited

Loading

anton-lisanin Jun 28, 2024

anton-lisanin Jun 28, 2024

evgenyzdanovich Aug 1, 2024

neonlabstech commented Aug 1, 2024

		@@ -266,13 +266,123 @@ impl rlp::Decodable for AccessListTx {
		}
		}

Support of EIP-1559 in the Neon EVM #409

Are you sure you want to change the base?

Support of EIP-1559 in the Neon EVM #409

Conversation

evgenyzdanovich commented May 13, 2024

alfiedotwtf 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

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

evgenyzdanovich commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgenyzdanovich Aug 1, 2024 • edited Loading

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

neonlabstech commented Aug 1, 2024

evgenyzdanovich Aug 1, 2024 •

edited

Loading