ethereum / consensus-spec-tests

Common tests for the Ethereum proof-of-stake consensus layer
MIT License
74 stars 23 forks source link

Attestation signatures used in tests are invalid due to ignored custody bit #6

Closed michaelsproul closed 5 years ago

michaelsproul commented 5 years ago

While running the block sanity tests for Lighthouse we noticed something funky. It seems that in verify_indexed_attestation, the two message hashes for custody bit 0 and 1 can sometimes end up being equal! This seems to be due to the custody bits being supplied as integer literals (0b0 or 0b1), as changing them to False and True causes the message hashes to be distinct again (and to match what Lighthouse thinks they should be).

There's a demo of this on the ignored-custody-field branch on my fork of the spec here: https://github.com/ethereum/eth2.0-specs/compare/v0.6.3...michaelsproul:ignored-custody-bit

I've been running the test_attestation test from a venv in test_libs/pyspec like:

pytest --config=minimal eth2spec -k test_attestation -s

Which produces the message hash 891e0385800ff598f80d2339ff1cd2ac46c3717dca00d099a7008c104946e765 for both messages when using the 0b1 syntax, and 891e0385800ff598f80d2339ff1cd2ac46c3717dca00d099a7008c104946e765 or 5a36957157fb41a2c7ecd1e8358f6896b15b6ef6b7ac7344f851f8c64f37f9f5 when using False/True.

The signature used in the tests must also be constructed badly, because the test passes

protolambda commented 5 years ago

Found the source of the bug:

https://github.com/ethereum/eth2.0-specs/blob/cb9301a9fece8864d97b6ff6b0bb3a662fa21484/test_libs/pyspec/eth2spec/utils/minimal_ssz.py#L155

What fixes it:

-        return b'\x01' if value is True else b'\x00'
+        return b'\x01' if value else b'\x00'

Coincidentally, we refactored SSZ completely on dev a week ago, and the new implementation is: https://github.com/ethereum/eth2.0-specs/blob/cf9169411e52611cb55e025b7a4662a394e4011a/test_libs/pyspec/eth2spec/utils/ssz/ssz_impl.py#L25

if value:
    return b'\x01'
else:
    return b'\x00'

A simple test shows that it is running correctly on byte / integer values now:

print("bytes:")
if b"": print("no empty!")
if b"\x00": print("yes zero!")
if b"\x01": print("yes one!")
print("binary literal:")
if 0b0: print("no!")
if 0b1: print("yes!")
print("int literal:")
if 0: print("no!")
if 1: print("yes!")

And we don't really pass byte strings to it, so that's fine.

bytes:
yes zero!
yes one!
binary literal:
yes!
int literal:
yes!

Thanks for reporting, but it won't need a fix, as it should be working on the dev branch correctly now, which will be released as v0.7.0 shortly :)