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

Add Bitlist and Bitvector #1224

Merged
merged 31 commits into from Jun 28, 2019
Merged

Add Bitlist and Bitvector #1224

merged 31 commits into from Jun 28, 2019

Conversation

dankrad
Copy link
Contributor

@dankrad dankrad commented Jun 27, 2019

Replaces #1096 for Bitfields only

  • Adding Bitvector (static length Bitfield) and Bitlist (dynamic length Bitfield)

Further fixes:

scripts/build_spec.py Outdated Show resolved Hide resolved
@JustinDrake JustinDrake mentioned this pull request Jun 27, 2019
@@ -272,6 +272,16 @@ def get_custody_chunk_count(crosslink: Crosslink) -> int:
return crosslink_length * chunks_per_epoch
```

### `get_bitfield_bit`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this function, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not change this logic at the moment because that will likely change quite a bit anyway. I think in the future, we will not need it, but I wanted to keep the old implementation alive for now.

@dankrad
Copy link
Contributor Author

dankrad commented Jun 27, 2019

So the test that is currently failing is for the bridge to py-ssz. I think we probably can't get that to work until py-ssz has the new Bitvector and Bitlist classes. (Also, I noticed that at the moment, py-ssz does not seem to support the new max-length lists)

specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
specs/core/0_beacon-chain.md Outdated Show resolved Hide resolved
@JustinDrake JustinDrake added the milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet label Jun 27, 2019
@protolambda
Copy link
Collaborator

protolambda commented Jun 27, 2019

See a5154da on the bitfield-suggestion branch.
Changed things:

  • made the base class "Bitfield"
  • made the metaclass consistent with the metaclass for bytes
  • made it a subclass of elements-base, so that it's a list, and slicing works (slice of boolean as slice result`, can be coerced back into a bitfield
  • some updates to the ssz-implementation; handle chunk count as special case for bitfields, since it's not like the other subclasses of Elements where we just assume a basic type is at least 1 byte.

Edit; ssz implementation still needs testing, we don't want every bit to be encoded as byte, and need alignment checks for non-multiple-of-8 bits in bytes.

@JustinDrake
Copy link
Collaborator

@protolambda: Can we avoid throwing an error with Checkpoint(previous_epoch, get_block_root(state, previous_epoch))?

>           state.current_justified_checkpoint = Checkpoint(current_epoch, get_block_root(state, current_epoch))
E           TypeError: __init__() takes 1 positional argument but 3 were given

@protolambda
Copy link
Collaborator

@JustinDrake Containers expect named arguments, not bare arguments. I prefer named arguments, as it enables you to shorten the names for input variables, without being less descriptive where it matters. And it enables you to leave out unused arguments. If we really want, I can add support for unnamed args, but it feels error prone (things change, and phase 1 will keep changing for a while).

@protolambda
Copy link
Collaborator

@dankrad @JustinDrake Can you take a look at the more explicit wording for list-limits? It's important, since it is part of consensus (max operations in a block), and merkleization with padding is a subtle combined thing, possibly not familiar to some readers.

@protolambda
Copy link
Collaborator

Resolving the merge conflict now

@djrtwo
Copy link
Contributor

djrtwo commented Jun 28, 2019

Great work @dankrad!
I have a number of minor comments but very close to merge time :)

specs/simple-serialize.md Outdated Show resolved Hide resolved
specs/simple-serialize.md Outdated Show resolved Hide resolved
Copy link

@naterush naterush left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick questions around max length merkelization

dankrad and others added 4 commits June 28, 2019 20:23
Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
…rocess_attestation.py

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
Update test_libs/pyspec/eth2spec/test/phase_0/block_processing/test_process_attestation.py

Co-Authored-By: Danny Ryan <dannyjryan@gmail.com>
@protolambda
Copy link
Collaborator

Fixed the last suggestion: removed unused import for code linter

@protolambda protolambda merged commit b21c9cc into dev Jun 28, 2019
@protolambda protolambda deleted the dankrad-patch-8 branch June 28, 2019 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
milestone:June 30 freeze 🥶 Phase 0 spec freeze for long-lived cross-client testnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants