Add blockchain sharding #169
Conversation
casper/protocols/sharding/block.py
Outdated
| def is_valid_estimate(cls, estimate): | ||
| if not isinstance(estimate, dict): | ||
| return False | ||
| if not isinstance(estimate['prev_blocks'], set): |
There was a problem hiding this comment.
estimate['prev_blocks'] will throw a KeyError if 'prev_blocks' is not set. Should probably check for inclusion of keys and then if the value is in fact a set. It's probably more likely (or at least just as likely) that the key is forgotten as the value being incorrect.
casper/protocols/sharding/block.py
Outdated
| if block.on_shard(shard_id): | ||
| return block | ||
|
|
||
| raise KeyError("Should have found previous block on shard!") |
There was a problem hiding this comment.
Might be useful to add why we should have found a previous block:
"shard_id in estimate['shard_ids']. Should have found previous block on shard!"
casper/protocols/sharding/block.py
Outdated
| raise KeyError("No previous block on that shard") | ||
|
|
||
| for block in self.estimate['prev_blocks']: | ||
| if block is None: |
There was a problem hiding this comment.
Might be worth a comment on what is going on here.
casper/protocols/sharding/block.py
Outdated
|
|
||
| @property | ||
| def is_merge_block(self): | ||
| return len(self.estimate['shard_ids']) == 2 |
There was a problem hiding this comment.
I know we're in binary sharding so we are checking for == rather than >=, but if we aren't checking for >= here, we should probably enforce len(estimate['shard_ids']) in is_valid_estimate.
Maybe consider adding a CONSTANT to the class that enforces this max: MAX_MERGE_BLOCK_SIZE or something.
There was a problem hiding this comment.
If we add such a constant then this method would be checking that the len in range(2, MAX_MERGE_BLOCK_SIZE+1)
| @@ -0,0 +1,79 @@ | |||
| """The forkchoice module implements the estimator function a blockchain""" | |||
| @@ -0,0 +1,105 @@ | |||
| """The blockchain plot tool implements functions for plotting blockchain data structures""" | |||
| curr_shard_idx = 0 | ||
| curr_shard_ids = [''] | ||
|
|
||
| """Shard ID's look like this: |
| return cls.shard_genesis_blocks[''] | ||
|
|
||
| @classmethod | ||
| def get_new_shard_id(cls): |
There was a problem hiding this comment.
From my experience, I see these functions often named with "next" so get_next_shard_id. That said... follow your heart.
| shard_id = cls.get_new_shard_id() | ||
|
|
||
| estimate = {'prev_blocks': set([None]), 'shard_ids': set([shard_id])} | ||
| cls.shard_genesis_blocks[''] = Block(estimate, dict(), validator, -1, 0) |
There was a problem hiding this comment.
What's going on right here? Why do we use '' as the key each time? Ultimately we are just storing the last initial_message and all the others are overwritten, no?
There was a problem hiding this comment.
Good catch! Yeah, meant to save the blocks (in case we don't want to number of shards to be equal to the number of validators :) ).
|
|
||
| @classmethod | ||
| def initial_message(cls, validator): | ||
| """Returns a dict from shard_id -> shard genesis block""" |
There was a problem hiding this comment.
This comment is incorrect. Returns a Block
|
|
||
| def select_random_shards(self, shards_forkchoice): | ||
| """Randomly selects a shard to build on, and sometimes selects another child shard""" | ||
| shards_to_build_on = [r.choice([key for key in self.starting_blocks.keys()])] |
There was a problem hiding this comment.
[r.choice(list(self.starting_blocks.keys()))] slightly less verbose but not sure what reads better..
| def select_random_shards(self, shards_forkchoice): | ||
| """Randomly selects a shard to build on, and sometimes selects another child shard""" | ||
| shards_to_build_on = [r.choice([key for key in self.starting_blocks.keys()])] | ||
| if (r.randint(0, 1) == 1): |
There was a problem hiding this comment.
I like random.choice([True, False]). very readable.
| """Randomly selects a shard to build on, and sometimes selects another child shard""" | ||
| shards_to_build_on = [r.choice([key for key in self.starting_blocks.keys()])] | ||
| if (r.randint(0, 1) == 1): | ||
| child = str(r.randint(0, 1)) |
There was a problem hiding this comment.
declare child_shard_id = shards_to_build_on[0] + child so you don't have to dynamically spell it out twice below and for clarity.
| self.check_forkchoice_atomicity(shards_forkchoice) | ||
|
|
||
| shards_to_build_on = self.select_shards(shards_forkchoice) | ||
| return {'prev_blocks': {shards_forkchoice[shard_id] for shard_id in shards_to_build_on}, |
There was a problem hiding this comment.
Why are we only returning the prev_blocks for the shards_to_build_on? Might a user want to see the entire shards_forkchoice as that is the full current estimate?
There was a problem hiding this comment.
As this is the previous block links, we only point to blocks that we are explicitly building on top of w/ this new block (at most, two shards as of now).
We can add other select_shards rules in the future to modify this slightly, but in all cases I think the prev_blocks themselves will just be those of the shards selected.
|
|
||
| return set(shards_to_build_on) | ||
|
|
||
| def check_forkchoice_atomicity(self, shards_forkchoice): |
There was a problem hiding this comment.
Okay here right now, but might be more appropriate in some sort of testing utils when we add testing for sharding.
| def update_safe_estimates(self, validator_set): | ||
| """Checks safety on messages in views forkchoice, and updates last_finalized_block""" | ||
| # check the safety of the top shard! | ||
| pass |
There was a problem hiding this comment.
Yeah. Doesn't work yet. Have to do a bit of thinking on how to fix this up (don't think it should be bad).
|
|
||
| tip = None | ||
|
|
||
| while tip and tip != self.starting_blocks['']: |
There was a problem hiding this comment.
That's the root shard id. I'll define it as a constant so it's more clear.
There was a problem hiding this comment.
Actually, just going to remove unused code for now. Will add this constant when I figure out safety stuff.
|
|
||
| max_score = max(scores.values()) | ||
|
|
||
| assert max_score != 0, "max_score of a block should never be zero" |
There was a problem hiding this comment.
maybe max_score > 0, "max_score should be greater than zero"
naterush
force-pushed
the
feat/sharding
branch
from
8469ee3 to
307e7d4
Compare
A working binary blockchain sharded data structure (what a mouthful). Currently, all validators see all shards (so we don't really get any scalability gains), but I think this is fine for a first version. The number of shards is currently limited to the number of validators (an artifact of how initial message creation works).