ethereum / consensus-spec-tests

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

Hexadecimal integers should not be in quotes #8

Closed prestonvanloon closed 5 years ago

prestonvanloon commented 5 years ago

Here are the formats for integers in the yaml spec.

canonical: 12345
decimal: +12345
octal: 0o14
hexadecimal: 0xC

When a hexadecimal integer is written as '0xC' then it is interpreted as a string by yaml spec compliant libraries. This only seems to happen with domain fields in the bls yaml tests so far.

See: Example 2.19. Integers and 10.3.2. Tag Resolution in yaml spec.

protolambda commented 5 years ago

See https://github.com/ethereum/eth2.0-specs/blob/dev/specs/test_formats/bls/sign_msg.md Domain is typed as bytes in the tests. I do see now that it's uint64 in the BLS spec doc. But ultimately it's up to implementations.

Regarding the spec-tests in general: all integers should be in unquoted decimal, and if it's in quoted hex, the value really is some form of bytes (even if short enough for an integer, it should be a bytes type).

Also note that the fork-version changed from int to bytes in the spec not that long ago, that may be related with what you are struggling with? We are avoiding integer arithmetic when constructing the domain for BLS signatures, since it's more precise to just define where the bytes are coming from (BLS-domain-type and fork version)

domain construction spec here: https://github.com/ethereum/eth2.0-specs/blob/v0.7.1/specs/core/0_beacon-chain.md#bls_domain

prestonvanloon commented 5 years ago

Here it is mentioned as uint64 https://github.com/ethereum/eth2.0-specs/blob/ab012b8adf17a3d76dbb47ddffc5c0db6c7565ac/test_generators/bls/main.py#L30

We're fine to use it as bytes if that is what the spec is supposed to be or uint64. The problem is that when we have a struct like

type TestObject struct {
   Domain uint64 `yaml:"domain"`
}

and try to unmarshal a yaml with domain written as

domain: '0x01'

which fails to parse.

However, if we write the domain value in the correct format:

domain: 0x01

Then it parses without issue. The correct format could be any of the formats listed in my original issue.

This leaves us with two options:

I'm asking for the second option here. The first option is simply not viable or maintainable.

protolambda commented 5 years ago

See https://github.com/ethereum/eth2.0-specs/pull/1238/commits/91f55f55b532eaa422459702153876b961844dfb

prestonvanloon commented 5 years ago

That works for me. Thanks!!