-
Notifications
You must be signed in to change notification settings - Fork 231
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
implement bulk of 0.5.0 state transition function #192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, it does not seem to improve or worsen the beacon_node simulation
|
||
## The 2nd/3rd/4th most recent epochs are all justified, the 2nd using the | ||
## 4th as source | ||
if (bitfield shr 1) mod 8 == 0b111 and |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make that a proc bitAllOne(bf: BitField, numBits: int, offset: int)
proc? and use
if (bitfield shr 1) and 7 == 0b111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. Clarifies the intent of that code considerably. Mixing logical and arithmetic operations is kind of odd, too.
I'll do another PR including that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the justification bitfield is not a BitField
though.. it's an uint64
..
if new_justified_epoch != state.current_justified_epoch: | ||
state.current_justified_epoch = new_justified_epoch | ||
state.current_justified_root = | ||
get_block_root(state, get_epoch_start_slot(new_justified_epoch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be very useful if we log the block and slot that has just been justified
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is slightly less useful, because you can have multiple justified blocks that are valid candidates.. also, generally, the logging of this stuff is probably better done at the decision point where the justified block is used.. keep in mind that the state transition function runs for blocks that have yet to pass validation, during block production etc..
if new_finalized_epoch != state.finalized_epoch: | ||
state.finalized_epoch = new_finalized_epoch | ||
state.finalized_root = | ||
get_block_root(state, get_epoch_start_slot(new_finalized_epoch)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and finalized
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do, just not here.
@@ -179,6 +179,7 @@ const | |||
type | |||
ValidatorIndex* = range[0'u32 .. 0xFFFFFF'u32] # TODO: wrap-around | |||
|
|||
Shard* = uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe these should all be distinct
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably. Given the fallout of the last distinct
thing I did, I'm 100% not doing it in this PR. I agree though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, though distinct ranges are utterly broken.
- Error when trying to iterate a distinct type based array nim-lang/Nim#7167 for array indexing
- Cannot infer the value of static param nim-lang/Nim#10220 with compile-time parametrized ranges.
nimble test
(obviously) works.research/state_sim
seems to works out as far as I let it run.USE_MULTITAIL="yes" ./tests/simulation/start.sh
has run stably for a while for me.This isn't, by any means, all of what's left of 0.5.0. The main slot/epoch aspect left is that https://github.com/ethereum/eth2.0-specs/blob/v0.5.0/specs/core/0_beacon-chain.md#per-slot-processing says
increment
state.slot
. Since currently, that means it never changesstate.slot
at all (I text-searched through the 0.5.0 spec forstate.slot
and didn't see at a glance where/how it'd ever get incremented fromGENESIS_SLOT
). https://github.com/ethereum/eth2.0-specs/blob/v0.5.0/specs/core/0_beacon-chain.md#on-genesis specifically setsslot
asGENESIS_SLOT
, not asGENESIS_SLOT + 1
or similar. So I don't know.In any case, this all works at least as well for me as previous versions.