k256 later

[rakita]

Start running tests on: "bins/revme/tests/GeneralStateTests/"
███████████████████████████████████████████████████████████████████████████████████████████████████████████████████░░░░░░░░░ 2418/2596
thread '' panicked at 'called Result::unwrap() on an Err value: signature::Error {}', /home/rakita/.cargo/registry/src/github.com-1ecc6299db9ec823/k256-0.10.2/src/ecdsa/verify.rs:158:81
Error: Statetest(SystemError)

something crashed k256 when enabled berlin/instanbul tests

[gakonst]

FYI @tarcieri seems like state tests pass when using secp256k1 but verification fails w k256. @rakita what's the best way to repro?

[rakita]

It is maybe usage problem i didnt check anything.
To reproduce:
Change from secp256k1 feature to k256 here:

revm/bins/revme/Cargo.toml

Line 21 in 76f8d00

revm = { path = "../../crates/revm", version = "1.1", default-features = false, features = ["web3db","std","secp256k1"] }

download ethereum/tests
and execute statetests (last arg is path to GeneralStateTests): cargo run --release -p revme -- statetest tests/GeneralStateTests

[gakonst]

Thanks! @tarcieri this is how we use k256: https://github.com/bluealloy/revm/blob/main/crates/revm_precompiles/src/secp256k1.rs#L14-L37

[tarcieri]

Looking at this line of code:

thread '' panicked at 'called Result::unwrap() on an Err value: signature::Error {}',
/home/rakita/.cargo/registry/src/github.com-1ecc6299db9ec823/k256-0.10.2/src/ecdsa/verify.rs:158:81

...it seems to be an issue with round tripping a point encoding:

https://docs.rs/k256/0.10.2/src/k256/ecdsa/verify.rs.html#158

(it seems like the conversion could possibly be a bit more straightforward there, but I digress)

As it were, we just fixed an issue related to point decompression:

RustCrypto/elliptic-curves#530

Can you try upgrading to the latest release of k256: v0.10.4?

[rakita]

@tarcieri it is still happening.

Here is a litlle bit more info:

sig:"79be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f817986b8d2c81b11b2d699528dde488dbdf2f94293d0d33c32e347f255fa4a6c1f0a900"  msg:"6b8d2c81b11b2d699528dde488dbdf2f94293d0d33c32e347f255fa4a6c1f0a9"
thread '<unnamed>' panicked at 'called `Result::unwrap()` on an `Err` value: signature::Error {}', /home/rakita/.cargo/registry/src/github.com-1ecc6299db9ec823/k256-0.10.4/src/ecdsa/verify.rs:158:81
stack backtrace:
   0: rust_begin_unwind
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/panicking.rs:116:14
   2: core::result::unwrap_failed
             at /rustc/9d1b2106e23b1abd32fce1f17267604a5102f57a/library/core/src/result.rs:1690:5
   3: k256::ecdsa::recoverable::Signature::recover_verify_key_from_digest_bytes
   4: revm_precompiles::secp256k1::secp256k1::ecrecover
   5: revm_precompiles::secp256k1::ec_recover_run
   6: revm::evm_impl::EVMImpl<GSPEC,DB,_>::call_inner
   7: revm::instructions::host::call
   8: revm::interpreter::Interpreter::run
   9: revm::evm_impl::EVMImpl<GSPEC,DB,_>::call_inner
  10: <revm::evm_impl::EVMImpl<GSPEC,DB,_> as revm::evm_impl::Transact>::transact
  11: revm::evm::EVM<DB>::transact_commit
  12: revme::statetest::runner::execute_test_suit
Error: Statetest(SystemError)

[tarcieri]

Interesting. It appears to be recovering the additive identity (a.k.a. point at infinity) for that particular signature/msg combination. Is that at all expected?

This does highlight a deeper issue: the From<&AffinePoint> impl on VerifyingKey needs to be changed to a TryFrom, as the additive identity is not allowed as a public key and an error needs to be returned in that case. That's a breaking change though.

I could backport a fix which checks if the point is the identity inside of the recovery function, and returns an error if that's helpful.

[rakita]

Interesting. It appears to be recovering the additive identity (a.k.a. point at infinity) for that particular signature/msg combination. Is that at all expected?

This is from eth/tests, maybe it is made expecially to test that case for consistency between implementations (will dig the test later)

This does highlight a deeper issue: the From<&AffinePoint> impl on VerifyingKey needs to be changed to a TryFrom, as the additive identity is not allowed as a public key and an error needs to be returned in that case. That's a breaking change though.

I could backport a fix which checks if the point is the identity inside of the recovery function, and returns an error if that's helpful.

This should be helpful, but let me check what exact behaviour is expected from secp256k1 lib in this case.

[tarcieri]

This commit to the k256 v0.11 branch will change this particular case into an error: RustCrypto/elliptic-curves#535

[rakita]

Tested it agains master and it now passes. Thank you @tarcieri

[rakita]

btw test that was failing is called: GeneralStateTests/stTransactionTest/PointAtInfinityECRecover.json lol