Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce reasonable boundaries on field values #192

Closed
arnetheduck opened this issue Nov 29, 2018 · 9 comments
Closed

Introduce reasonable boundaries on field values #192

arnetheduck opened this issue Nov 29, 2018 · 9 comments
Labels
general:enhancement New feature or request

Comments

@arnetheduck
Copy link
Contributor

With the new spec defaulting to uint64 for serialization, it would be useful to introduce the notion of minimally supported values or bounds on acceptable values, so as to separate serialization from execution.

As an example, uint24 is used because occasionally by design it is safe to do so, based on maximum possible validator counts.
Other values might not be bounded the same way, but it remains essential that they get treated the same way by clients - for example, what is an upper bound on the slot number that we realistically want to support?

Introducing this in the spec will help ensure that implementations are conforming both in theory and in practice:

  • JavaScript for example has no native uint64 support, and in many languages, indexing is done with a smaller or signed type - the mechanical alignment between spec and reality suffers encouraging compromises
  • unsigned arithmetic is surprising sometimes, where order of operations matters might make you over- or underflow temporarily, even though the end result is within bounds - more clarity will help statically analyze problem spots.
  • In both geth and in parity, optimizations have resulted in test cases being ignored and formal verification stumbling - due to uint256 being specified but there being no practical uses for such a wide range - addressing this early on in the design would be beneficial
@JustinDrake
Copy link
Collaborator

I'd say "acceptable" is subjective in the sense that in theory different clients can have different acceptable bounds. Do you want your implementation to be relevant for 10 years, 100 years, 1000 years? Some quick calculations:

  • Slots: In one century, assuming 6-second slots, we will use log2(100 * 365.25 * 24 * 60 * 60 / 6) < 29 bits. So uint32 should do for the foreseeable future.
  • Shards: At the moment we only have 1024 shards, so uint16 should do.
  • Kind/status: At the moment we have < 10 different kinds and statuses, so uint8 should be fine.
  • Fork version: I could see someone making use of all 64 bits here, so I'd stick with uint64.
  • Votes: Shouldn't go above POW_RECEIPT_ROOT_VOTING_PERIOD so uint16 is probably fine.
  • Exit counter: I'd say we won't have more than 8 exits per slot for the next century, so uint32 should also be fine.
  • Justified slot bitfield: Seems to use all 64 bits, so uint64.
  • Balances: The most granular token is the Gwei so stick with uint64.

In terms of underflows, the main one to look out for is when when substracting from a balance, i.e. applying a penalty. The spec has the following:

Note: When applying penalties in the following balance recalculations implementers should make sure the uint64 does not underflow.

@hwwhww hwwhww added the general:enhancement New feature or request label Dec 1, 2018
@JustinDrake
Copy link
Collaborator

Thanks Jacek for all your bug reports :)

Do you still have concerns regarding reasonable upper bounds on field values? In short, the spec uses 2**64 - 1 as the implicit upper bound for uint64 fields though implementations can subjectively optimise with more aggressive upper bounds if they want.

Regarding overflows, there's a issue dedicated to that here.

@djrtwo
Copy link
Contributor

djrtwo commented Dec 4, 2018

Specifying the BEACON_CHAIN_SHARD_NUMBER as 2**64 - 1 does put the 64 bit requirement on all clients.

@arnetheduck
Copy link
Contributor Author

A motivating rationale for this exercise is to allow the establishment of a formal specification, and later to prove properties about the system that will help client implementers make good choices and produce clients that are more safe against certain categories of exploits:

  • Overflows and underflows
  • Out-of-bounds accesses (validity of array indices)
  • Out-of-order execution issues (basically, which parts are parallelizable safely)

From a practical point of view, establishing or describing bounds means that clients can forgo certain checks and focus on others - for example, if the spec is proven to be undeflow-free as a design property, there is no need to explicitly check for underflows at various stages.

For the network as a whole, it's also nice to be able to say that all clients support at least X years of slot time as a requirement - though there are good arguments to keep this an implementation-defined detail as well.

@JustinDrake
Copy link
Collaborator

Out-of-bounds accesses (validity of array indices)

There's a relevant discussion here.

there are good arguments to keep this an implementation-defined detail as well

I think that's the status quo. One idea is to document reasonable bounds in the validator doc or an implementer doc. Feel free to reopen :)

@Nashatyrev
Copy link
Member

As soon as we already have 'dedicated' uint24 type for validator index I would also support narrowing at least Shard type since it's also used as array index across the spec

Shards: At the moment we only have 1024 shards, so uint16 should do.

I would stick to the same uint24 for uniformity.
@JustinDrake I would love to make a PR for this. What do you think?

@JustinDrake JustinDrake reopened this Jan 18, 2019
@JustinDrake
Copy link
Collaborator

I would stick to the same uint24 for uniformity.

We've actually settled to use uint64 everywhere for uniformity. The only exceptions were bytes48 and bytes96 for the BLS stuff, and uint24 for validator indices to save space in the (now deprecated) committees_at_slots data structures. So maybe we can now make validator indices uint64 for consistency.

@Nashatyrev
Copy link
Member

So maybe we can now make validator indices uint64 for consistency.

Sounds reasonable

@JustinDrake
Copy link
Collaborator

Validator indices are now uint64. Typing is clean and simple now!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
general:enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants