NDEV-2505 Fix EXTHASHCODE (#253)

* Fix EXTHASHCODE

* NDEV-2505: Code review suggestions (#254)

* 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

* Move `Database` convenience functions to extension trait

Added `DatabaseExt` which implements functions that can be implemented
in terms of `Database`.

* Update comments and naming

* Add tests

* Address comment

* Refactor unit tests

* Replace maybe_async::test with tokio::test

The maybe_async::test predicate does not work with `cargo test-sbf`
right now anyway.

* Fix typo

---------

Co-authored-by: Andrei Silviu Dragnea <[email protected]>

evm_loader/lib/src/account_storage.rs

@@ -623,27 +623,6 @@ impl<T: Rpc> AccountStorage for EmulatorAccountStorage<'_, T> {
         address.find_solana_address(self.program_id())
     }
 
-    async fn code_hash(&self, address: Address, chain_id: u64) -> [u8; 32] {
-        use solana_sdk::keccak::hash;
-
-        info!("code_hash {address} {chain_id}");
-
-        let code = self.code(address).await.to_vec();
-        if !code.is_empty() {
-            return hash(&code).to_bytes();
-        }
-
-        // https://eips.ethereum.org/EIPS/eip-1052
-        // https://eips.ethereum.org/EIPS/eip-161
-        if (self.balance(address, chain_id).await == 0)
-            && (self.nonce(address, chain_id).await == 0)
-        {
-            return <[u8; 32]>::default();
-        }
-
-        hash(&[]).to_bytes()
-    }
-
     async fn code_size(&self, address: Address) -> usize {
         info!("code_size {address}");
 

evm_loader/program/Cargo.toml

@@ -62,6 +62,9 @@ async-trait = { version = "0.1.73", optional = true }
 version = "0.2.7"
 features = ["is_sync"]
 
+[dev-dependencies]
+tokio = { version = "1.0", features = ["full"] }
+
 [lib]
 crate-type = ["cdylib", "lib"]
 

evm_loader/program/src/account_storage/backend.rs

@@ -73,29 +73,6 @@ impl<'a> AccountStorage for ProgramAccountStorage<'a> {
             .contract_with_bump_seed(self.program_id(), address)
     }
 
-    fn code_hash(&self, address: Address, chain_id: u64) -> [u8; 32] {
-        use solana_program::keccak;
-
-        if let Ok(contract) = self.contract_account(address) {
-            keccak::hash(&contract.code()).to_bytes()
-        } else {
-            // https://eips.ethereum.org/EIPS/eip-1052
-            // https://eips.ethereum.org/EIPS/eip-161
-            if let Ok(account) = self.balance_account(address, chain_id) {
-                if account.nonce() > 0 || account.balance() > 0 {
-                    // account without code
-                    keccak::hash(&[]).to_bytes()
-                } else {
-                    // non-existent account
-                    <[u8; 32]>::default()
-                }
-            } else {
-                // non-existent account
-                <[u8; 32]>::default()
-            }
-        }
-    }
-
     fn code_size(&self, address: Address) -> usize {
         self.contract_account(address).map_or(0, |a| a.code_len())
     }

evm_loader/program/src/account_storage/mod.rs

@@ -58,8 +58,6 @@ pub trait AccountStorage {
     /// Get contract solana address
     fn contract_pubkey(&self, address: Address) -> (Pubkey, u8);
 
-    /// Get code hash
-    async fn code_hash(&self, address: Address, chain_id: u64) -> [u8; 32];
     /// Get code size
     async fn code_size(&self, address: Address) -> usize;
     /// Get code data

evm_loader/program/src/evm/database.rs

@@ -21,8 +21,6 @@ pub trait Database {
         chain_id: u64,
         value: U256,
     ) -> Result<()>;
-
-    async fn code_hash(&self, address: Address, chain_id: u64) -> Result<[u8; 32]>;
     async fn code_size(&self, address: Address) -> Result<usize>;
     async fn code(&self, address: Address) -> Result<Buffer>;
     fn set_code(&mut self, address: Address, chain_id: u64, code: Vec<u8>) -> Result<()>;
@@ -51,3 +49,257 @@ pub trait Database {
         is_static: bool,
     ) -> Option<Result<Vec<u8>>>;
 }
+
+/// Provides convenience methods that can be implemented in terms of `Database`.
+#[maybe_async(?Send)]
+pub trait DatabaseExt {
+    /// Returns whether an account exists and is non-empty as specified in
+    /// https://eips.ethereum.org/EIPS/eip-161.
+    async fn account_exists(&self, address: Address, chain_id: u64) -> Result<bool>;
+
+    /// Returns the code hash for an address as specified in
+    /// https://eips.ethereum.org/EIPS/eip-1052.
+    async fn code_hash(&self, address: Address, chain_id: u64) -> Result<[u8; 32]>;
+}
+
+#[maybe_async(?Send)]
+impl<T: Database> DatabaseExt for T {
+    async fn account_exists(&self, address: Address, chain_id: u64) -> Result<bool> {
+        Ok(self.nonce(address, chain_id).await? > 0 || self.balance(address, chain_id).await? > 0)
+    }
+
+    async fn code_hash(&self, address: Address, chain_id: u64) -> Result<[u8; 32]> {
+        // The function `Database::code` returns a zero-length buffer if the account exists with
+        // zero-length code, but also when the account does not exist. This makes it necessary to
+        // also check if the account exists when the returned buffer is empty.
+        //
+        // We could simplify the implementation by checking if the account exists first, but that
+        // would lead to more computation in what we think is the common case where the account
+        // exists and contains code.
+        let code = self.code(address).await?;
+        let bytes_to_hash: Option<&[u8]> = if !code.is_empty() {
+            Some(&*code)
+        } else if self.account_exists(address, chain_id).await? {
+            Some(&[])
+        } else {
+            None
+        };
+
+        Ok(bytes_to_hash.map_or([0; 32], |bytes| {
+            solana_program::keccak::hash(bytes).to_bytes()
+        }))
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use std::collections::HashMap;
+
+    use super::*;
+
+    struct TestDatabaseEntry {
+        balance: U256,
+        nonce: u64,
+        code: Vec<u8>,
+    }
+
+    impl TestDatabaseEntry {
+        fn empty() -> Self {
+            Self {
+                balance: U256::from(0u8),
+                nonce: 0,
+                code: Vec::default(),
+            }
+        }
+
+        fn without_code() -> Self {
+            Self {
+                balance: U256::from(1u8),
+                nonce: 1,
+                code: Vec::default(),
+            }
+        }
+
+        fn with_code(code: Vec<u8>) -> Self {
+            assert!(!code.is_empty());
+            Self {
+                balance: U256::from_words(0, 1),
+                nonce: 1,
+                code,
+            }
+        }
+    }
+
+    #[derive(Default)]
+    struct TestDatabase(HashMap<Address, TestDatabaseEntry>);
+
+    impl FromIterator<(Address, TestDatabaseEntry)> for TestDatabase {
+        fn from_iter<T: IntoIterator<Item = (Address, TestDatabaseEntry)>>(iter: T) -> Self {
+            Self(iter.into_iter().collect())
+        }
+    }
+
+    #[maybe_async(?Send)]
+    #[allow(unused_variables)]
+    impl Database for TestDatabase {
+        fn default_chain_id(&self) -> u64 {
+            unimplemented!();
+        }
+
+        fn is_valid_chain_id(&self, chain_id: u64) -> bool {
+            unimplemented!();
+        }
+
+        async fn contract_chain_id(&self, address: Address) -> Result<u64> {
+            unimplemented!();
+        }
+
+        async fn nonce(&self, address: Address, chain_id: u64) -> Result<u64> {
+            Ok(self
+                .0
+                .get(&address)
+                .map(|entry| entry.nonce)
+                .unwrap_or_default())
+        }
+
+        fn increment_nonce(&mut self, address: Address, chain_id: u64) -> Result<()> {
+            unimplemented!();
+        }
+
+        async fn balance(&self, address: Address, chain_id: u64) -> Result<U256> {
+            Ok(self
+                .0
+                .get(&address)
+                .map(|entry| entry.balance)
+                .unwrap_or_default())
+        }
+
+        async fn transfer(
+            &mut self,
+            source: Address,
+            target: Address,
+            chain_id: u64,
+            value: U256,
+        ) -> Result<()> {
+            unimplemented!();
+        }
+
+        async fn code_size(&self, address: Address) -> Result<usize> {
+            unimplemented!();
+        }
+
+        async fn code(&self, address: Address) -> Result<Buffer> {
+            Ok(self
+                .0
+                .get(&address)
+                .map(|entry| Buffer::from_slice(&entry.code))
+                .unwrap_or_default())
+        }
+
+        fn set_code(&mut self, address: Address, chain_id: u64, code: Vec<u8>) -> Result<()> {
+            unimplemented!();
+        }
+
+        fn selfdestruct(&mut self, address: Address) -> Result<()> {
+            unimplemented!();
+        }
+
+        async fn storage(&self, address: Address, index: U256) -> Result<[u8; 32]> {
+            unimplemented!();
+        }
+
+        fn set_storage(&mut self, address: Address, index: U256, value: [u8; 32]) -> Result<()> {
+            unimplemented!();
+        }
+
+        async fn block_hash(&self, number: U256) -> Result<[u8; 32]> {
+            unimplemented!();
+        }
+
+        fn block_number(&self) -> Result<U256> {
+            unimplemented!();
+        }
+
+        fn block_timestamp(&self) -> Result<U256> {
+            unimplemented!();
+        }
+
+        async fn map_solana_account<F, R>(&self, address: &Pubkey, action: F) -> R
+        where
+            F: FnOnce(&AccountInfo) -> R,
+        {
+            unimplemented!();
+        }
+
+        fn snapshot(&mut self) {
+            unimplemented!();
+        }
+
+        fn revert_snapshot(&mut self) {
+            unimplemented!();
+        }
+
+        fn commit_snapshot(&mut self) {
+            unimplemented!();
+        }
+
+        async fn precompile_extension(
+            &mut self,
+            context: &Context,
+            address: &Address,
+            data: &[u8],
+            is_static: bool,
+        ) -> Option<Result<Vec<u8>>> {
+            unimplemented!();
+        }
+    }
+
+    #[maybe_async]
+    async fn code_hash(database_entry: Option<TestDatabaseEntry>) -> [u8; 32] {
+        let address = Address::default();
+        let database: TestDatabase = database_entry
+            .map(|entry| (address, entry))
+            .into_iter()
+            .collect();
+        database
+            .code_hash(address, crate::config::DEFAULT_CHAIN_ID)
+            .await
+            .unwrap()
+    }
+
+    #[tokio::test]
+    async fn code_hash_of_non_existing_account() {
+        let actual = code_hash(None).await;
+        assert_eq!(actual, [0; 32]);
+    }
+
+    #[tokio::test]
+    async fn code_hash_of_empty_account() {
+        let actual = code_hash(Some(TestDatabaseEntry::empty())).await;
+        assert_eq!(actual, [0; 32]);
+    }
+
+    #[tokio::test]
+    async fn code_hash_of_existing_account_without_code() {
+        let actual = code_hash(Some(TestDatabaseEntry::without_code())).await;
+        assert_eq!(
+            actual,
+            [
+                197, 210, 70, 1, 134, 247, 35, 60, 146, 126, 125, 178, 220, 199, 3, 192, 229, 0,
+                182, 83, 202, 130, 39, 59, 123, 250, 216, 4, 93, 133, 164, 112,
+            ]
+        );
+    }
+
+    #[tokio::test]
+    async fn code_hash_of_existing_account_with_code() {
+        let actual = code_hash(Some(TestDatabaseEntry::with_code(vec![0; 10]))).await;
+        assert_eq!(
+            actual,
+            [
+                107, 210, 221, 107, 212, 8, 203, 238, 51, 66, 147, 88, 191, 36, 253, 198, 70, 18,
+                251, 248, 177, 180, 219, 96, 69, 24, 244, 15, 253, 52, 182, 7
+            ]
+        );
+    }
+}

evm_loader/program/src/evm/opcode.rs

@@ -3,7 +3,10 @@ use ethnum::{I256, U256};
 use maybe_async::maybe_async;
 use solana_program::log::sol_log_data;
 
-use super::{database::Database, tracing_event, Context, Machine, Reason};
+use super::{
+    database::{Database, DatabaseExt},
+    tracing_event, Context, Machine, Reason,
+};
 use crate::{
     error::{Error, Result},
     evm::{trace_end_step, Buffer},

evm_loader/program/src/executor/state.rs

@@ -284,20 +284,6 @@ impl<'a, B: AccountStorage> Database for ExecutorState<'a, B> {
         Ok(self.backend.code_size(from_address).await)
     }
 
-    async fn code_hash(&self, from_address: Address, chain_id: u64) -> Result<[u8; 32]> {
-        use solana_program::keccak::hash;
-
-        for action in &self.actions {
-            if let Action::EvmSetCode { address, code, .. } = action {
-                if &from_address == address {
-                    return Ok(hash(code).to_bytes());
-                }
-            }
-        }
-
-        Ok(self.backend.code_hash(from_address, chain_id).await)
-    }
-
     async fn code(&self, from_address: Address) -> Result<crate::evm::Buffer> {
         for action in &self.actions {
             if let Action::EvmSetCode { address, code, .. } = action {