NethermindEth / nethermind

A robust execution client for Ethereum node operators.
https://nethermind.io/nethermind-client
GNU General Public License v3.0
1.23k stars 428 forks source link

BLS MapToG1 consensus failure #2231

Closed holiman closed 4 years ago

holiman commented 4 years ago

From EIP:

Field-to-curve call expects 64 bytes an an input that is interpreted as a an element of the base field. Output of this call is 128 bytes and is G1 point following respective encoding rules.

Error cases:

Input has invalid length Input is not a valid field element

https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Evm/Precompiles/Bls/Shamatar/MapToG1Precompile.cs#L48

Nethermind reads the first 64 bytes of input, but does not throw an error if input is larger than 64 bytes. Geth and OE does.

Credits to goevmlab fuzzing and @MariusVanDerWijden 's bls-fuzzer

holiman commented 4 years ago

cc @shamatar

shamatar commented 4 years ago

This is strange. If Nethermind uses the same Rust lib as OE then this check happens internally https://github.com/matter-labs/eip1962/blob/c8cec9ebf3dbb70e05dc39ecc3dffc9b3c6b8a0e/src/public_interface/eip2537/mod.rs#L312 and an error is immediately returned. There may be something in C glue that OE does not use, but are you sure that length check is a cause of the error?

tkstanczak commented 4 years ago

Yes, we adjust the length before passing to your lib but we do not throw an error if it is of wrong length.

This is how it was with bn254 libs

On Tue, 18 Aug 2020, 08:48 Alexander, notifications@github.com wrote:

This is strange. If Nethermind uses the same Rust lib as OE then this check happens internally https://github.com/matter-labs/eip1962/blob/c8cec9ebf3dbb70e05dc39ecc3dffc9b3c6b8a0e/src/public_interface/eip2537/mod.rs#L312 and an error is immediately returned. There may be something in C glue that OE does not use, but are you sure that length check is a cause of the error?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/NethermindEth/nethermind/issues/2231#issuecomment-675317751, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADZZYLFQU3Q7I7BS3E2D33SBIW6JANCNFSM4QC6NVUQ .

shamatar commented 4 years ago

As Martin has pointed invalid length must be an error. And I think invalid length for bn254 should also be an error if I remember code from Geth.

tkstanczak commented 4 years ago

As Martin has pointed invalid length must be an error. And I think invalid length for bn254 should also be an error if I remember code from Geth.

for bn254 length can be longer than required for addition and multiplication so the behaviour will be different I have updated Nethermind to follow the BLS EIP

shamatar commented 4 years ago

Interesting, I should have missed that in a spec for BN.