Add `BeaconStateMachine` outline #1373

hwwhww · 2018-10-08T20:04:25Z

What was wrong?

#1339 - part 3: Introduce eth.beacon.state_machines.base.BaseStateMachine(Configurable)

How was it fixed?

Note: the spec changed recently, so it doesn’t match the current spec! This PR is closer to this version.
State storage
1. The beacon chain states are quite different from EVM chain:
  - The crystallized state recalculation, which happens only if block.slot_number >= last_state_recalc + CYCLE_LENGTH, and affects the CrystallizedState and ActiveState
  - The per-block processing, which happens every block (if during a crystallized state recalculation block, it happens after the crystallized state recalculation), and affects the ActiveState only
  - Both states are being serialized with RLP (or SSZ later). No tree data structure currently, but possible will be. Now the state root is actually just hashed state.
2. The CrystallizedState is recalculated every cycle (~17 minutes with the latest spec). Now my strategy is simply store it by:
  1. crystallized_state_root: Hash32 -> crystallized_state: CystallizedState
  2. last_state_recalc slot number -> crystallized_state_root: Hash32 for canonical chain
  3. If there is forking (e.g., the same slot number conflicted), records the old state root to deletable_state_roots DB storage. Could be clean up periodically or when some blocks are finalized.
3. The ActiveState is recalculated every block (~16 seconds).
  1. The ActiveState will be backed up when CrystallizedState is persisted.
  2. Since the CYCLE_LENGTH is 64 slots, with the backup ActiveState + recent blocks, it can be reproduced.
4. Not a perfect solution (high disk I/O!), but since the spec hasn't been finalized, the data structure might be changed soon. (In fact, ActiveState grows to 4 fields now!) I think we can leave the storage optimization later.
Config
1. After another look, I think if we want to mock important parameters in tests, it can be configured more like opcodes in PoW chain, not constants. But now it’s pretty messy.
StateMachine
1. Copied the structure of VM.
2. Assumed that the DB already has essential data.

TODO

Now there are two BeaconConfigs, needs to be merged.
Check and refactor from_old_active_and_blocks thing.

Cute Animal Picture

pipermerriam · 2018-10-08T21:04:25Z

eth/beacon/db/schema.py

+    #
+    @staticmethod
+    def make_slot_to_crystallized_state_lookup_key(slot: int) -> bytes:
+        return b'beacon:slot-to-crystallized-state:%d' % slot


still probably want the v1 namespace to be present somewhere in this string.

Should I just add v1 to all lookup keys?

pipermerriam · 2018-10-08T21:08:41Z

eth/beacon/state_machines/base.py

+            self._active_state = self.get_active_state_class().from_old_active_and_blocks(
+                old_active_state,
+                blocks,
+                self.config.CYCLE_LENGTH * 2,


It appears that this is the only value that is accessed in self.config. Does it make sense to just have this be a class level variable?

Ohh, I think there will be more when more becon_chain state_transition.py functions are torn down into StateMachine.

pipermerriam · 2018-10-09T15:22:11Z

eth/beacon/helpers.py

@@ -268,11 +267,14 @@ def _get_shards_and_committees_for_shard_indices(


 @to_tuple
-def get_new_shuffling(seed: Hash32,
+def get_new_shuffling(*,


❤️

I just deleted a comment suggesting that you adopt this exact pattern!

One thing to think about. Invoking functions using keyword arguments has a runtime cost. I suspect that for this function, that runtime cost is going to be trivial compared to the execution of the body of the functions, but for any function that needs to be highly performant, this should be taken into account.

If we move it into StateMachine, we can reduce some parameters by using self.config.CYCLE_LENGTH, self.config.MIN_COMMITTEE_SIZE, self.config.SHARD_COUNT.

pipermerriam · 2018-10-09T15:23:26Z

eth/beacon/state_machines/base.py

@@ -74,7 +74,7 @@ def logger(self):
    @classmethod
    def get_block_class(cls) -> Type[BaseBeaconBlock]:
        """
-        Return the :class:`~eth.beacon.types.blocks.BeaconBlock` class that this
+        Returns the :class:`~eth.beacon.types.blocks.BeaconBlock` class that this


I believe the original Return was the proper format we are using for docstrings (cc @cburgdorf)

I don't have any preferences! 😆 Searched in eth with this PR, there are 81 Returns and 64 Return now. I can change it back.

Correct, we try to streamline docs to always be imperative present tense. I believe that is what most documentation uses but it is at least inline with what the official Python docs use.

Here's a rule of thumb. If this sounds correct, it is correct:

If this method is called it will: <doc-string>

Example:

If this method is called it will return the BeaconBlock class

Just noting that this is the same for git commit messages which at least makes it easier for me to remember.

pipermerriam · 2018-10-09T15:24:07Z

eth/beacon/state_machines/base.py

+    def get_prev_blocks(cls,
+                        last_block_hash: Hash32,
+                        chaindb: BaseBeaconChainDB,
+                        searching_depth: int,


nitpick

I think search_depth is a more natural name for this. And maybe even max_search_depth since this seems to be an upper bound.

pipermerriam · 2018-10-09T15:30:22Z

eth/beacon/state_machines/base.py

+        Returns the previous blocks.
+
+        Since the slot numbers might be not continuous. The searching depth is ``searching_depth``
+        and the target slot depth is ``target_slot_number``.


Suggested rephrasing.

Slot numbers are not guaranteed to be contiguous since it is possible for there to be no block at a given slot. The search is bounded by two parameters. - `max_search_depth` - The maximum number of slots below the slot of the block denoted by `last_block_hash` that we should search. - `min_slot_depth` - The slot number for which we should not search any deeper if reached, though we do include the first block encountered below this slot number before terminating the search.

pipermerriam · 2018-10-09T15:31:03Z

eth/beacon/state_machines/base.py

+                        last_block_hash: Hash32,
+                        chaindb: BaseBeaconChainDB,
+                        searching_depth: int,
+                        target_slot_number: int) -> Iterable[BaseBeaconBlock]:


Maybe min_slot_number since minimum is somewhat of an optional constraint and target is likely to be interpreted as a firm constraint. With the term target, my intuition would say that this search would continue until it hit this slot number.

pipermerriam · 2018-10-09T15:33:34Z

eth/beacon/state_machines/base.py

            yield block
            try:
                block = chaindb.get_block_by_hash(block.parent_hash)
            except (IndexError, BlockNotFound):
                break
-            if block.slot_number < start_slot:
+            if block.slot_number < target_slot_number:


I find it interesting and not intuitive that this search will include blocks which have a slot number lower than target_slot_number. I assume that is part of the spec, but it might be something worth leaving an inline comment to be sure future readers of this code know that behavior is intentional.

I find it interesting and not intuitive that this search will include blocks which have a slot number lower than target_slot_number

Ohh, my goal is making this search return the blocks that are greater than or equal to target_slot_number (min_slot_number). It should return at least one block (the block that produced backup_active_state).

Long story version:

Startup: Setup empty states

Apply the genesis block (B0) and get the genesis states. Persist these states (C0 and A0)

recent_block_hashes: [ZERO_HASH32] * (CYCLE_LENGTH * 2)

Apply block B1, update active state (A1)
recent_block_hashes = get_new_recent_block_hashes( old_block_hashes=recent_block_hashes, parent_slot=B0.slot_number, current_slot=B1.slot_number, parent_hash=B0.hash )

recent_block_hashes: [ZERO_HASH32] * (CYCLE_LENGTH * 2 - 1) + [B0.hash]

Apply block B2, update active state (A2)
recent_block_hashes = get_new_recent_block_hashes( old_block_hashes=recent_block_hashes, parent_slot=B1.slot_number, current_slot=B2.slot_number, parent_hash=B1.hash )

recent_block_hashes: [ZERO_HASH32] * (CYCLE_LENGTH * 2 - 2) + [B0.hash] + [B1.hash]

Apply block B3, update active state (A3)
…

Apply block B64, update active state (A64) and crystallized state (C64), and also persist states.

Assume that we have the backup active state A0 and want to create B3’ (!= B3), so we have to reproduce A2. It requires the blocks starting from the parent of “the first block after backup_active_state_slot”. This parent block should be the block that produced A0, so I set min_slot_number=backup_active_state_slot.

Sorry, it's long. Do you think it makes sense?

Just noting that I was wrong in my first read of the code and yes, this does indeed only return down to block greater than or equal to target_slot_number

pipermerriam · 2018-10-09T15:36:04Z

eth/beacon/state_machines/base.py

+                    searching_depth=searching_depth,
+                    target_slot_number=backup_active_state_slot
+                )
+                blocks = blocks[::-1]


I had to go check that this was reversing the blocks. I think the idiomatic way to do this is to call reversed() on blocks, or better yet, to do blocks = reversed(self.get_prev_blocks(...)) so that you don't have to define the blocks variable twice.

pipermerriam · 2018-10-09T15:37:28Z

eth/beacon/state_machines/forks/serenity/active_states.py

-        pending_attestations = old_active_state.pending_attestations
+    def from_backup_active_state_and_blocks(cls,
+                                            backup_active_state: ActiveState,
+                                            blocks: Sequence['BaseBeaconBlock']) -> ActiveState:


👍 to using Sequence. I think we (and largely me) have been defining these as Tuple or List too often which isn't strictly necessary and doesn't provide much extra safety.

BTW, I also used Iterable for input fields of other functions. Do you think I should change most input fields from Iterable to Sequence?

Once I was using value: Iterable[Any] and doing len(value) in this function, mypy returned that "len" has incompatible type "Iterable[Any]"; expected "Sized". Changing to Sequence saved me in this case.

When judging between Iterable and Sequence my preference is to always use the most flexible type that fulfills the goal.

For example, if you do not need to access len() (or e.g index access), Iterable will do it and is more flexible. For instance, it allows you to also pass in an iterator (yield "foo").

But if you need to get the length or need to access elements at a specific index, then one has to tighten the type to something like Sequence because Iterable simply can't give you that (e.g. it is perfectly valid for an Iterable to be infinite, so not having an answer to len() at all).

@cburgdorf thank you for elaborating! I will keep the current Iterables.

pipermerriam · 2018-10-09T15:38:13Z

eth/beacon/state_machines/forks/serenity/configs.py

@@ -3,12 +3,11 @@
 from eth.beacon.state_machines.configs import BeaconConfig


-SENERITY_CONFIG = BeaconConfig(


cburgdorf · 2018-10-10T14:22:37Z

eth/beacon/db/chain.py

    #
    @abstractmethod
-    def exists(self, key: bytes) -> bool:
+    def get_crystallized_state_by_root(self, state_root: Hash32) -> CrystallizedState:


ntpick: I think we recently settled on a preference to just use pass here instead of raising a NotImplementedError because the ABC module already takes care of not allowing these to be called, rendering the raise obsolete.

cburgdorf · 2018-10-10T14:23:28Z

eth/beacon/db/chain.py

+    #
+    # Active State
+    #
+    def get_active_state_by_root(self, state_root: Hash32) -> ActiveState:


Any reason these aren't @abstractmethod?

cburgdorf · 2018-10-10T14:25:19Z

eth/beacon/db/chain.py

+    @staticmethod
+    def _get_deletable_state_roots(db: BaseDB) -> Tuple[Hash32]:
+        """
+        Returns deletable_state_roots.


nitpick: Returns

cburgdorf · 2018-10-10T14:25:32Z

eth/beacon/db/chain.py

+                                               db: BaseDB,
+                                               crystallized_state: CrystallizedState) -> None:
+        """
+        Sets a record in the database to allow looking up this block by its


nitpick: Sets

cburgdorf · 2018-10-10T14:44:28Z

eth/beacon/state_machines/forks/serenity/configs.py

+    MIN_COMMITTEE_SIZE=128,  # validators
+    MIN_DYNASTY_LENGTH=256,  # slots
+    SHARD_COUNT=1024,  # shards
+    SLOT_DURATION=8,  # seconds


Should that rather be named SLOT_DURATION_SECONDS and SQRT_E_DROP_SECONDS? Alternatively, instead of assigning int here, we could assign Second which would be a NewType and prevent arbitrary int values to be assigned. /cc @pipermerriam @carver

Renaming: now these constants (configs) names follow the previous spec - and the spec is changing. Recently Justin proposed that using slot as the common time unit, except for SLOT_DURATION. e.g., SQRT_E_DROP_TIME in second became SQRT_E_DROP_TIME in slot. I will clean up all these recent changes after porting old codebase.

For beacon chain, some other NewType candidates: Slot, ShardID. And with the latest spec: ValidatorStatus, SpecialRecordType, ValidatorSetDeltaFlag.

cburgdorf · 2018-10-10T14:45:51Z

eth/beacon/state_machines/configs.py

+        ('MIN_COMMITTEE_SIZE', int),
+        ('MIN_DYNASTY_LENGTH', int),
+        ('SHARD_COUNT', int),
+        ('SLOT_DURATION', int),


As noted in another comment, the following two are good candidates to use a Second NewType rather than using int.

hwwhww · 2018-10-10T18:39:55Z

@pipermerriam @cburgdorf thanks for the review! I fixed PR feedback except for the Second NewType. I will open an issue for the beacon chain related NewType.

hwwhww added the PR state: WIP label Oct 8, 2018

pipermerriam reviewed Oct 8, 2018

View reviewed changes

hwwhww requested a review from pipermerriam October 9, 2018 15:09

hwwhww removed the PR state: WIP label Oct 9, 2018

pipermerriam approved these changes Oct 9, 2018

View reviewed changes

hwwhww mentioned this pull request Oct 9, 2018

Port beacon chain #1381

Closed

18 tasks

cburgdorf reviewed Oct 10, 2018

View reviewed changes

hwwhww added 6 commits October 11, 2018 02:33

CrystallizedState storage

70d7db3

ActiveState storage

8da56e8

Added BeaconStateMachine

2ba1d27

Removed eth.beacon.config and added v1 namespace in DB schema

b4aa281

refactors

3fc3790

fix

1733d53

hwwhww force-pushed the beacon_sm branch from 31a0e2b to 7f0daa5 Compare October 10, 2018 18:36

PR feedback - mostly enhance docstring and rename some parameters

340f0af

hwwhww force-pushed the beacon_sm branch from 7f0daa5 to 340f0af Compare October 10, 2018 18:44

hwwhww mentioned this pull request Oct 10, 2018

Add beacon chain / eth2.0 related NewType #1383

Closed

hwwhww merged commit fc7ed34 into ethereum:master Oct 10, 2018

hwwhww mentioned this pull request Oct 10, 2018

Refactor BeaconConfig related fixtures #1340

Closed

cburgdorf mentioned this pull request Oct 12, 2018

Per block transition and attestation #1390

Merged

This was referenced Oct 17, 2018

Add section on how to do correct Sphinx friendly docstrings. ethereum/snake-charmers-tactical-manual#25

Open

Http backend ethpm/py-ethpm#61

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `BeaconStateMachine` outline #1373

Add `BeaconStateMachine` outline #1373

hwwhww commented Oct 8, 2018 •

edited

Loading

pipermerriam Oct 8, 2018

hwwhww Oct 9, 2018

pipermerriam Oct 8, 2018

hwwhww Oct 9, 2018

pipermerriam Oct 9, 2018

hwwhww Oct 9, 2018 •

edited

Loading

pipermerriam Oct 9, 2018

hwwhww Oct 9, 2018 •

edited

Loading

cburgdorf Oct 10, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 9, 2018

pipermerriam Oct 9, 2018

pipermerriam Oct 9, 2018

pipermerriam Oct 9, 2018

hwwhww Oct 9, 2018

pipermerriam Oct 10, 2018

pipermerriam Oct 9, 2018

pipermerriam Oct 9, 2018

hwwhww Oct 9, 2018

cburgdorf Oct 10, 2018

hwwhww Oct 10, 2018

pipermerriam Oct 9, 2018

cburgdorf Oct 10, 2018

cburgdorf Oct 10, 2018

cburgdorf Oct 10, 2018

cburgdorf Oct 10, 2018

cburgdorf Oct 10, 2018

hwwhww Oct 10, 2018

hwwhww Oct 10, 2018

cburgdorf Oct 10, 2018 •

edited

Loading

hwwhww commented Oct 10, 2018

		@@ -3,12 +3,11 @@
		from eth.beacon.state_machines.configs import BeaconConfig


		SENERITY_CONFIG = BeaconConfig(

Add BeaconStateMachine outline #1373

Add BeaconStateMachine outline #1373

Conversation

hwwhww commented Oct 8, 2018 • edited Loading

What was wrong?

How was it fixed?

TODO

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hwwhww Oct 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

hwwhww commented Oct 10, 2018

Add `BeaconStateMachine` outline #1373

Add `BeaconStateMachine` outline #1373

hwwhww commented Oct 8, 2018 •

edited

Loading

hwwhww Oct 9, 2018 •

edited

Loading

hwwhww Oct 9, 2018 •

edited

Loading

cburgdorf Oct 10, 2018 •

edited

Loading