alloy-rs / core

High-performance, well-tested & documented core libraries for Ethereum, in Rust
https://alloy.rs
Apache License 2.0
763 stars 137 forks source link

[Bug] Incorrect keccak256 hash computation on linux-x86_64 with keccak-asm #711

Closed nbaztec closed 1 week ago

nbaztec commented 1 month ago

Component

primitives

What version of Alloy are you on?

0.7.7

Operating System

Linux

Describe the bug

Summary

When running the following test with --features asm-keccak on linux-x86_64, the following test fails under Ubuntu and Arch (x86_64), but passes on Mac:

fn test_foo_fails() {
    let a = alloy_primitives::keccak256("testFoo()");   // want: 0x79adbd5094e60c1bc2b963678ff44695d1430b8ccff0b1cd57c03a7f63567822
                                                        // got : 0xae33576402a61ff3a4a241b4a91d20ce98b70aa7bf0f388fb857111ec83aef73
    let b = alloy_primitives::keccak256("test_Foo()");
    println!("testFoo() : {:?}", a);
    println!("test_Foo(): {:?}", b);
    assert_eq!(format!("{a:?}"), String::from("0x79adbd5094e60c1bc2b963678ff44695d1430b8ccff0b1cd57c03a7f63567822"));
    assert_eq!(format!("{b:?}"), String::from("0x45c48c2bd4afc6adc7884fe296b9af10e234ddbc44f2f99f40cfb8b6391e9798"));   // this is always correct
}
testFoo() : 0xae33576402a61ff3a4a241b4a91d20ce98b70aa7bf0f388fb857111ec83aef73
test_Foo(): 0x45c48c2bd4afc6adc7884fe296b9af10e234ddbc44f2f99f40cfb8b6391e9798
thread 'tests::test_foo_fails' panicked at src/main.rs:19:9:
assertion `left == right` failed
  left: "0xae33576402a61ff3a4a241b4a91d20ce98b70aa7bf0f388fb857111ec83aef73"
 right: "0x79adbd5094e60c1bc2b963678ff44695d1430b8ccff0b1cd57c03a7f63567822"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests::test_foo_fails

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

The issue seems to be that the keccak256 hash is incorrectly calculated on linux but only for testFoo() and not for test_Foo().

The example has been minimized from foundry-zksync repository here https://github.com/nbaztec/repro-keccak-asm-issue It is observed that exactly these collection of dependencies and their respective imports cause this issue to occur. If either one of the dependencies or their respective imports is removed, then the issue goes away.

It seems this issue could be related to openssl discrepancies but given a lot of unsafe rust is involved in keccak-asm repository, could might as well be something else.

DaniPopes commented 1 month ago

Hey I cannot reproduce your result, in fact I can't even build the project

Can you please try updating to the latest keccak-asm version, or reverting to 0.1.0 if already at latest?

DaniPopes commented 1 month ago

Might be some sort of symbol collision, but I still can't get it to reproduce. I would probably recommend trying to get off of openssl in general, but it might also be a fix for this.

nbaztec commented 4 weeks ago

I added an action that compiles and runs the test https://github.com/nbaztec/repro-keccak-asm-issue/actions/runs/10470617272/job/28996125941?pr=1#step:5:1235 first without asm-keccak, then with it. The first test passes but the second one fails.

You can see here:

running test without asm-keccak

running 1 test
test tests::test_foo_fails ... ok

successes:

---- tests::test_foo_fails stdout ----
testFoo() : 0x79adbd5094e60c1bc2b963678ff44695d1430b8ccff0b1cd57c03a7f63567822
test_Foo(): 0x45c48c2bd4afc6adc7884fe296b9af10e234ddbc44f2f99f40cfb8b6391e9798

successes:
    tests::test_foo_fails

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

..
running test with asm-keccak

running 1 test
test tests::test_foo_fails ... FAILED

successes:

successes:

failures:

---- tests::test_foo_fails stdout ----
testFoo() : 0xae33576402a61ff3a4a241b4a91d20ce98b70aa7bf0f388fb857111ec83aef73
test_Foo(): 0x45c48c2bd4afc6adc7884fe296b9af10e234ddbc44f2f99f40cfb8b6391e9798
thread 'tests::test_foo_fails' panicked at src/main.rs:19:9:
assertion `left == right` failed
  left: "0xae33576402a61ff3a4a241b4a91d20ce98b70aa7bf0f388fb857111ec83aef73"
 right: "0x79adbd5094e60c1bc2b963678ff44695d1430b8ccff0b1cd57c03a7f63567822"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

failures:
    tests::test_foo_fails

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

error: test failed, to rerun pass `--bin foundry-zksync-release-debug`

test_Foo() returns the correct keccak, but testFoo() does not. Unfortunately it's not trivial to figure out or remove openssl deps due to a complex dependency tree, but most seem to be native-tls.

Tried the crate version 0.1.0 and was still able to reproduce the bug.

nbaztec commented 4 weeks ago

Note that I was unable to reproduce it in the keccak-asm crate as well and this behavior only occurs when using this library through alloy_primitives and that's why I've opened the issue here.

This test passes just fine for me too, if I include keccak-asm directly in the repro and not use alloy-primitives::keccak256.

DaniPopes commented 4 weeks ago

Reduced:

src/main.rs

extern crate alloy_primitives;
extern crate openssl;

fn main() {
    assert_eq!(
        alloy_primitives::keccak256("testFoo()"),
        alloy_primitives::b256!("79adbd5094e60c1bc2b963678ff44695d1430b8ccff0b1cd57c03a7f63567822"),
    );
}

Cargo.toml

[package]
name = "foundry-zksync-release-debug"
version = "0.1.0"
edition = "2021"

[dependencies]
alloy-primitives = { version = "0.7", default-features = false }
openssl = { version = "0.10", features = ["vendored"] }

[features]
asm-keccak = ["alloy-primitives/asm-keccak"]

Running cargo run --features asm-keccak panics.

Applying this patch makes the command run fine again:

diff --git a/src/main.rs b/src/main.rs
index c82c4a9..1ae767f 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,5 +1,5 @@
-extern crate alloy_primitives;
 extern crate openssl;
+extern crate alloy_primitives;

 fn main() {
     assert_eq!(

I am not sure how to fix this, it seems like it's a symbol conflict between the vendored openssl static library and keccak-asm's.

nbaztec commented 4 weeks ago

Thanks for isolating it to the openssl itself. Yeah I'm also out of ideas, seems the crate cannot be used if anything in the dependency tree uses vendored openssl.

DaniPopes commented 1 week ago

Moved to https://github.com/DaniPopes/keccak-asm/issues/9

DaniPopes commented 1 week ago

This has been fixed in keccak-asm 0.1.4 with https://github.com/DaniPopes/keccak-asm/pull/10