ethereum / beacon_chain

MIT License
211 stars 65 forks source link

Bitfield shortening #46

Closed paulhauner closed 6 years ago

paulhauner commented 6 years ago

https://github.com/ethereum/beacon_chain/blob/509387c5a1eb4eaa9f636ae01dedf4b340c4f09e/beacon_chain/state/state_transition.py#L235

I am looking to understand the meaning behind this line. I presently see two options:

  1. Declaring that the length of the attestation bitfield must always hold a bit for each active state attester (i.e., if the last 8 attesters did not vote you can't shorten the bitfield by a byte)
  2. Avoiding the case where the program attempts to access an out-of-bounds index.

I ask because option (1) is relevant to other clients, whereas (2) is not so much.

The primary question derived from this is: "can we shorten bitfields to reduce state size?"

Thanks :relaxed:

djrtwo commented 6 years ago

Because the attestation_bitfield is a part of the block that must be signed, reproduced, and verified across clients, we should conform on a standard representation -- in this case (1), are we allowed to shorten the bitfield if possible. I would lean toward not being able to shorten the bitfield so as to more easily conform the standard in creation and verifying the validity of blocks. The extremely small savings potential in bytesize is not at all worth the added complexity IMO. Unless there is a regular and large savings, I would lean toward simplicity in the spec. These are the devilish details we'll all need to hash out eventually and put into a cross client test suite :)

As for this particular check, it is a sanity check that made its way into the code rather than being independently checked in the test suite.

paulhauner commented 6 years ago

Cool, seems reasonable to me.

I'll make sure my bitifields are adequately large for now.