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

NDEV-2505 Fix EXTHASHCODE #253

Merged
merged 9 commits into from
Jan 17, 2024
Merged

Conversation

mickvangelderen
Copy link

No description provided.

@mickvangelderen mickvangelderen force-pushed the NDEV-2505-fix-extcodehash branch from 3c99bd3 to 5b348e8 Compare January 11, 2024 11:35
evm_loader/program/src/evm/buffer.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
@mickvangelderen
Copy link
Author

@anton-lisanin is this more in line with what you were thinking of?

@mickvangelderen mickvangelderen marked this pull request as ready for review January 11, 2024 12:51
@mickvangelderen mickvangelderen changed the title WIP Fix EXTHASHCODE NDEV-2505 Fix EXTHASHCODE Jan 11, 2024
@anton-lisanin
Copy link
Collaborator

anton-lisanin commented Jan 11, 2024

It may be good idea to move this to lower level here - https://github.com/neonlabsorg/neon-evm/blob/develop/evm_loader/program/src/evm/opcode.rs#L651

And remove everything related to codehash from executor state and backend

In #251 @s-medvedev added an another Executor State and now both of them would need the same codehash logic.

@mickvangelderen
Copy link
Author

It may be good idea to move this to lower level here - https://github.com/neonlabsorg/neon-evm/blob/develop/evm_loader/program/src/evm/opcode.rs#L651

And remove everything related to codehash from executor state and backend

In #251 @s-medvedev added an another Executor State and now both of them would need the same codehash logic.

I was thinking if we could we use a trait default fn implementation?

Copy link

@andreisilviudragnea andreisilviudragnea left a comment

Choose a reason for hiding this comment

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

I left a few comments. I also added some unit tests to validate my suggestions from comments here.

evm_loader/program/src/evm/buffer.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
@andreisilviudragnea
Copy link

@anton-lisanin I believe we should treat Database::code_size the same as Database::code_hash, by using the Database::code().len() inside Database::code_size and remove AccountStorage::code_size method completely. This would remove the duplication of:

        for action in &self.actions {
            if let Action::EvmSetCode { address, code, .. } = action {
                if &from_address == address {
                    return Ok(<something>);
                }
            }
        }

but it would also create a Buffer only to retrieve its len().

And what should Database::code and Database::code_hash return for a precompiled contract address? I see that Database::code_size returns 1 for precompiled contracts, but Database::code_hash looks inconsistent with Database::code_size returning 1. Or are Database::code and Database::code_hash never called for precompiled contracts?

@anton-lisanin
Copy link
Collaborator

@anton-lisanin I believe we should treat Database::code_size the same as Database::code_hash, by using the Database::code().len() inside Database::code_size and remove AccountStorage::code_size method completely. This would remove the duplication of:

        for action in &self.actions {
            if let Action::EvmSetCode { address, code, .. } = action {
                if &from_address == address {
                    return Ok(<something>);
                }
            }
        }

but it would also create a Buffer only to retrieve its len().

And what should Database::code and Database::code_hash return for a precompiled contract address? I see that Database::code_size returns 1 for precompiled contracts, but Database::code_hash looks inconsistent with Database::code_size returning 1. Or are Database::code and Database::code_hash never called for precompiled contracts?

@andreisilviudragnea Database::code and Database::code_hash are never called for precompiled contracts.
And doing something with code_size is out of scope of this pr

evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
evm_loader/program/src/executor/state.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/buffer.rs Outdated Show resolved Hide resolved
@andreisilviudragnea
Copy link

@anton-lisanin While checking the go-ethereum implementation of opExtCodeHash, I saw this comment:

// It is worth noting that in order to avoid unnecessary create and clean, all precompile
// accounts on mainnet have been transferred 1 wei, so the return here should be
// emptyCodeHash. If the precompile account is not transferred any amount on a private or
// customized chain, the return value will be zero.

Should Database::balance also return non-zero balance for is_precompile_address(address) == true addresses?

mickvangelderen and others added 2 commits January 12, 2024 14:51
* Remove unused AccountStorage::code_hash method

* Replace Buffer::buffer_is_empty with [T]::is_empty

* Rename from_address to address (consistent with trait)

* Rename buffer to code

* Inline data_account_exists method

* Remove TODOs

* Reformat

* Add test_keccak_hash_empty_slice

* Add Buffer::is_empty tests

* Use early return to simplify code flow
@mickvangelderen mickvangelderen force-pushed the NDEV-2505-fix-extcodehash branch 2 times, most recently from 94a700d to faa8db4 Compare January 12, 2024 14:30
Added `DatabaseExt` which implements functions that can be implemented
in terms of `Database`.
@mickvangelderen mickvangelderen force-pushed the NDEV-2505-fix-extcodehash branch from faa8db4 to 43ddf36 Compare January 12, 2024 14:32
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
@mickvangelderen
Copy link
Author

@anton-lisanin @andreisilviudragnea I think this PR is in a mergeable state right now. If you can find the time, I'd like to wrap this up.

@anton-lisanin
Copy link
Collaborator

anton-lisanin commented Jan 16, 2024

Should Database::balance also return non-zero balance for is_precompile_address(address) == true addresses?

No. Transferring Neons to the precompile address is valid usecase

evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
evm_loader/program/src/evm/database.rs Outdated Show resolved Hide resolved
@kristinaNikolaevaa kristinaNikolaevaa added the fullTestSuite Run OZ tests and part of dapps tests label Jan 16, 2024
The maybe_async::test predicate does not work with `cargo test-sbf`
right now anyway.
@neonlabstech
Copy link

Dapps report

Cost report for "Uniswap V3" dApp

Action Fee Cost in $ Accounts TRx Estimated Gas Used Gas Used % of EG
Token approve 0.00228896 0.0035186337636032 9 1 1164480 1144480 98.28
NonfungiblePositionManager - Mint position 0.02730384 0.0419719930885728 4 28 30739120 13651920 44.41
NonfungiblePositionManager - Increase liquidity 0.00044 0.0006763765448 23 23 4364240 220000 5.04
NonfungiblePositionManager - Decrease Liquidity 0.00139808 0.0021491557267135997 17 1 4274240 699040 16.35
NonfungiblePositionManager - Collect Fees 0.00455792 0.0070071984402336 20 1 3497360 2278960 65.16
NonfungiblePositionManager - Burn Liquidity Position 2e-05 3.07441302e-05 15 1 3954560 10000 0.25
Direct swap 2e-05 3.07443674e-05 17 1 1328400 10000 0.75
Burn transaction 0.00047936 0.0007368790564352001 11 1 739040 239680 32.43
Collect transaction 0.00228896 0.0035187448239424 12 1 1633840 1144480 70.05

Cost report for "Aave" dApp

Action Fee Cost in $ Accounts TRx Estimated Gas Used Gas Used % of EG
Token mint 0.00228896 0.0035187448239424 9 1 1164480 1144480 98.28
Token approve 0.00228896 0.0035187448239424 9 1 1164480 1144480 98.28
Deposit to lending pool 0.0077856 0.011968567054176002 4 5 4761840 3892800 81.75
Borrow from lending pool 0.00508792 0.0078208486050688 4 30 3737360 2543960 68.07
Repay 0.00049 0.0007531989136 4 27 2586800 245000 9.47
Flashloan 0.00501792 0.007713248760268801 4 28 7360480 2508960 34.09
Withdraw 7e-05 0.0001075945885 4 5 1857760 35000 1.88
Liquidation 0.00335896 0.005162941699828 4 57 4940000 1679480 34.0

Cost report for "Saddle Finance" dApp

Action Fee Cost in $ Accounts TRx Estimated Gas Used Gas Used % of EG
Add liquidity 0.00229896 0.0035338192822512004 3 2 3181920 1149480 36.13
Swap DAI -> USDC 2e-05 3.074131100000001e-05 21 1 1797760 10000 0.56
Swap USDC -> DAI 2e-05 3.07413e-05 21 1 1797760 10000 0.56
Remove liquidity 2e-05 3.07413e-05 23 1 1937440 10000 0.52
Add liquidity in 3 Tokens 0.00278896 0.0042876525303664 4 30 4220640 1394480 33.04
Swap DAI -> USDC 3 pool 2e-05 3.0745189600000005e-05 21 1 1807760 10000 0.55
Swap USDC -> USDT 3 pool 2e-05 3.07435602e-05 21 1 1807760 10000 0.55
Swap USDC -> DAI 3 pool 2e-05 3.07435602e-05 21 1 1807760 10000 0.55
Remove liquidity 3 pool 3e-05 4.61178111e-05 3 2 2646480 15000 0.57
Add liquidity in metapool 0.00285896 0.0043955488779488 3 31 3271920 1429480 43.69
Swap SUSD -> LP metapool 2e-05 3.07452252e-05 22 1 1807760 10000 0.55
Swap USDC -> SUSD metapool 0.00103 0.0015833790978 4 53 3775520 515000 13.64
Swap DAI -> USDT metapool 0.00056 0.0008608663055999999 4 30 3086160 280000 9.07
Remove liquidity in Metapool 2e-05 3.0746388e-05 23 1 1937440 10000 0.52

@mickvangelderen mickvangelderen merged commit 9b55e5e into develop Jan 17, 2024
4 checks passed
@mickvangelderen mickvangelderen deleted the NDEV-2505-fix-extcodehash branch January 17, 2024 10:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fullTestSuite Run OZ tests and part of dapps tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants