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

Bring-up of state trie hash verification function #3

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

shamb0
Copy link
Owner

@shamb0 shamb0 commented Feb 16, 2023

Bring-up of state trie hash verification function.

Observing mismatch in hash value under FEVM context.

Summary

Modified test vector with single transaction, common test vector used in both fevm & revm context.

Mismatch in computed state trie hash value under FEVM context

Computed state trie hash value under revm context

Difference in state properties values, used for calculating state trie hash.

Mismatch in bytecode hash

FEVM context

revm context

Mismatch in slot status @0000000000000000000000000000000000000400

FEVM context

revm context

@shamb0
Copy link
Owner Author

shamb0 commented Feb 16, 2023

CC @Stebalien, @vyzo

@Stebalien
Copy link
Contributor

Stebalien commented Feb 16, 2023

For the first case, can you print out the state trees from both cases? I.e., the slots so I can see where the difference is?

@Stebalien
Copy link
Contributor

Ah... there aren't separate sub tests.

@Stebalien
Copy link
Contributor

But yes, this is exactly what we're looking for.

One thing that's confusing me is that both FEVM and REVM agree on the state of b94f5374fce5edbc8e2a8697c15331677e6ebf0b, but the tests don't.

@Stebalien
Copy link
Contributor

But I solved the bytecode hash issue.

fn init_fevm(&mut self, code: Bytes, nonce: u64, kamt_config: KamtConfig) -> Result<Cid> {
let state_tree = self.executor.as_mut().unwrap().state_tree_mut();
let hasher = Code::try_from(SupportedHashes::Keccak256 as u64).unwrap();
let code_hash = multihash::Multihash::wrap(
SupportedHashes::Keccak256 as u64,
&hasher.digest(&code).to_bytes(),
)
.expect("failed to hash bytecode with keccak");
let code_cid = state_tree
.store()
.put(Code::Blake2b256, &Block::new(IPLD_RAW, code))
.expect("failed to write bytecode");
let mut slots = StateKamt::new_with_config(state_tree.store(), kamt_config);
let evm_state = EvmState {
bytecode: code_cid,
bytecode_hash: BytecodeHash::try_from(&code_hash.to_bytes()[..32]).unwrap(),
contract_state: slots.flush().expect("failed to flush contract state"),
nonce,
tombstone: None,
};
state_tree.store().put_cbor(&evm_state, Code::Blake2b256)
}

The bytecodehash as stored in the state is the hash digest, not a multihash. I.e., there's no need to wrap the hash digest in a multihahs, convert it to bytes, then take the first 32. Just take the entire digest:

&hasher.digest(&code).to_bytes(),

If possible, I recommend that you use the EVM's builtin constructor.

@Stebalien
Copy link
Contributor

#4

@Stebalien
Copy link
Contributor

Mismatch in slot status @0000000000000000000000000000000000000400

It looks like you aren't inserting the slots from the "pre" section before running the test.

@shamb0
Copy link
Owner Author

shamb0 commented Feb 26, 2023

Hello @Stebalien,

Update-01 :: Quick fix to suppress the error ExitCode::SYS_SENDER_INVALID | "Send not from valid sender"

  • Based on state of the condition flag, error "Send not from valid sender" is due to is_placeholder_actor() is not set.

  • Indeed state of is_placeholder_actor() not set is valid, since fevm actor is created during runtime & it's not part of the manifest file.

  • As a quick hack, Re-factored the logic as below to suppress the error. Could you please kindly suggest the better alternate to handle this.

https://github.com/shamb0/ref-fvm/blob/shamb0/mirror-master/fvm/src/executor/default.rs#L422

@shamb0
Copy link
Owner Author

shamb0 commented Feb 26, 2023

Update-02 :: Status on analysis of mismatch in state trie hash calculated value.

Here are some observation on the properties of ActorState for the address
0xa94f5374fce5edbc8e2a8697c15331677e6ebf0b

  • Observation-01 :: Difference in gas_used between revm & fevm
prop fevm revm
base_fee 0.00000000000000001 TODO
initial_balance 17592186044416 17592186044416
balance_after_transaction 17592186044416 17592184564106
balance_diff 0 1480310
gas_used 4367801 148031

from the traces, looks like ActorState::deduct_funds() is always passed with value zero, Can I have some clarity on this behavior ?

  • Observation-02 :: Please correct me if I'm wrong, Looks like functionality of the property ActorState::sequence & fil_actor_evm::State::nonce are duplicates, but the values are in out-of sync.

@Stebalien
Copy link
Contributor

Send not from valid sender

You need to send messages from an account or placeholder, not an EVM actor. The EVM doesn't distinguish between accounts and contracts, but we do.

To handle this, you likely need to check AccountInfo::code. If code is empty, create an account actor here:

let evm_code_cid = self.tester.code_by_id(ActorType::EVM as u32).unwrap();

If it's non-empty, create an EVM actor as usual.

@Stebalien
Copy link
Contributor

from the traces, looks like ActorState::deduct_funds() is always passed with value zero, Can I have some clarity on this behavior ?

I'm not sure where you're getting those traces, I assume you've modified something?

I'm guessing that you're setting the base-fee to 0. In which case yes, we'll charge nothing for gas.

Observation-02 :: Please correct me if I'm wrong, Looks like functionality of the property ActorState::sequence & fil_actor_evm::State::nonce are duplicates, but the values are in out-of sync.

That's a bit tricky:

  1. ActorState::sequence is the Filecoin nonce and is only used by accounts to order off-chain messages.
  2. fil_actor_evm::State::nonce is the EVM nonce and is used in CREATE to derive the address.

fil_actor_evm::State::nonce exists because the EVM needs to update this nonce every time it creates a new contract, but actors aren't allowed to manually increment their Filecoin nonce (ActorState::sequence).

Short version: For accounts (ethereum-style accounts and native accounts), always look at ActorState::sequence. For everything else, ignore it.

@shamb0
Copy link
Owner Author

shamb0 commented Feb 28, 2023

To handle this, you likely need to check AccountInfo::code. If code is empty, create an account actor here:

let evm_code_cid = self.tester.code_by_id(ActorType::EVM as u32).unwrap();

If it's non-empty, create an EVM actor as usual.

Looks like I'm missing some information piece, in managing place-holder address & fevm actor address. Could you please kindly review the latest integration ?

https://github.dev/shamb0/fevm-eth-compliance-test/blob/shamb0/verify-merkle-trie/src/statetest/executor.rs#L406

@shamb0 shamb0 closed this Feb 28, 2023
@shamb0 shamb0 reopened this Feb 28, 2023
@shamb0
Copy link
Owner Author

shamb0 commented Feb 28, 2023

I'm guessing that you're setting the base-fee to 0. In which case yes, we'll charge nothing for gas.

Using the base-fee value from the test vector property. Can we use it straight or any transformation required ?

https://github.com/shamb0/fevm-eth-compliance-test-trace/blob/b333f5adef012b83ef287ded0bc4de1a8c77eafc/GeneralStateTests/EIPTests/stEIP3855/TID-01-04-01-mod-push0.json#LL22C2-L22C2

image

@Stebalien
Copy link
Contributor

Looks like I'm missing some information piece, in managing place-holder address & fevm actor address. Could you please kindly review the latest integration ?

@Stebalien
Copy link
Contributor

Using the base-fee value from the test vector property. Can we use it straight or any transformation required ?

What are you trying to test? The gas used by FEVM will always differ from the gas used by the EVM.

@shamb0
Copy link
Owner Author

shamb0 commented Mar 1, 2023

Using the base-fee value from the test vector property. Can we use it straight or any transformation required ?

What are you trying to test? The gas used by FEVM will always differ from the gas used by the EVM.

Oops, then we can't use the state root hash to test against the post::testcase::hash, which is part of the test vector.

Computation of state root hash, has dependency on state of contract balance. (After executing the transaction use-cases, gas_used is
deducted from the contract account balance).

fix: create an ethaccount when applicable
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants