Revamp 1_shard-data-chains.md#1383
Conversation
WIP! * Significant simplifications * A few bug fixes * Lots of cleanups and reorganising (making it consistent with `0_beacon-chain.md`) * Likely a few bugs introduced
1) Make `ShardBlock` and `ShardState` flat containers (as opposed to plain containers) 2) Make Gwei deltas `int64` (as opposed `uint64`) 3) Make `older_committee_deltas` a `Vector` (as opposed to `List`) 4) Apply size fee on block body only (as opposed to block header and body) 5) Enshrine minimum "extra" block body fee for proposers (reusing `PROPOSER_REWARD_QUOTIENT`) 6) Fix bugs reported by @terencechain and @hwwhww 👍
Co-Authored-By: terence tsao <terence@prysmaticlabs.com>
|
|
||
| ```python | ||
| class ShardBlockCore(Container): | ||
| class ShardBlock(FlatContainer): |
There was a problem hiding this comment.
This is actually a (big) problem with this approach. With a flat container, you no longer have this nice property that an object with an entire subtree substituted with its root necessarily has the same hash tree root as the original object, so you can't have shard blocks and shard block headers in the same way. Not sure how to get around this within the FlatContainer model.
This is arguably one reason why not doing flat containers and instead taking Danny's suggestion and having "embedded vectors (and containers)" would be better. Basically, the idea would be that if you have a container as follows:
class Foo(Container):
bar: uint64
baz: Embedded[Vector[uint64, 4]]
bav: Bytes32It would be treated identically to:
class Foo(Container):
bar: uint64
baz_0: uint64
baz_1: uint64
baz_2: uint64
baz_3: uint64
bav: Bytes32This way you avoid introducing a separate type of container and instead have flattening as a feature that you can use or not use for specific elements at will.
There was a problem hiding this comment.
Was just about to question if we indeed get this property anymore. The embedded looks like a reasonable approach
There was a problem hiding this comment.
So are we going to replace FlatContainer with Embedded here? I guess the attestations and signature would need to be part of a 2-item Embedded container where the items are themselves Embedded to get flatness while gaining the ability to use the standard signature root method?
djrtwo
left a comment
There was a problem hiding this comment.
This is awesome! super pleased with the parallels to the structure in phase 0. It makes it really easy to reason about and check for correctness.
Great work :)
I have a number of comments. I'm not 100% done reviewing, but don't want to leave you hanging as I'll be afk for much of the rest of the day. Trying to finish my review up tonight
|
|
||
| ```python | ||
| class ShardBlockCore(Container): | ||
| class ShardBlock(FlatContainer): |
There was a problem hiding this comment.
Was just about to question if we indeed get this property anymore. The embedded looks like a reasonable approach
| body_root: Hash | ||
| block_size_sum: uint64 | ||
| aggregation_bits: Bitvector[2 * MAX_PERIOD_COMMITTEE_SIZE] | ||
| attestations: BLSSignature |
There was a problem hiding this comment.
Any reasons we not using attestation_signatures? I find attestations a bit miss-leading
| # Return post-state | ||
| return shard_state | ||
| ``` | ||
|
|
There was a problem hiding this comment.
| ### Slot processing |
Shard chain CI cleanup
Shard chain sanity test
djrtwo
left a comment
There was a problem hiding this comment.
great work! still things to be done and some potential changes to discuss with crosslinking, but this is good enough to get into dev for now
TODO:
PR dependencies:
compute_proposer_indexfrom Improve beacon proposer selection logic #1371Renamings:
Substantive changes:
FlatContainerfor shard blocks and stateVectors instead ofLists in shard state to be friendly to flatteningShardCheckpoints (as opposed toparent_roots)SHARD_HEADER_SIZE(simpler and partially cachable Merkleisation)get_shard_proposer_index(also make it consistent with phase 0—see Improve beacon proposer selection logic #1371)receipt_root(instead useolder_committee_rewardsand compute indices on the beacon chain)Gweieverywhere (and reuseget_base_rewardfrom phase 0)_deltasList[byte, SHARD_BLOCK_SIZE_LIMIT - SHARD_HEADER_SIZE](as opposed toBytes[SHARD_BLOCK_SIZE_LIMIT - SHARD_HEADER_SIZE])2 *inrange(len(persistent_committee), 2 * MAX_PERSISTENT_COMMITTEE_SIZE)get_shard_committeeMAX_BLOCK_SIZE_PRICEless conservative (32 ETH as opposed to 1 ETH)—fees are split across the shard committee anywayshardcheck inprocess_shard_block_headerbeacon_chain_rootcheck inprocess_shard_block_headerlen(data.body)tolen(data.body) + SHARD_HEADER_SIZEinprocess_shard_block(as forblock_size_sum)process_shard_block_size_feewhen computingMAX_BLOCK_SIZE_PRICE(should have been// SLOTS_PER_EPOCHas opposed to* SLOTS_PER_EPOCH)block.parent_root == Bytes32()to bypass parent root checkMisc cosmetic changes:
1_beacon-chain-misc.md—see Starting on phase 1 misc beacon changes #1323)MIN_BLOCK_SIZE_PRICEparameterPHASE_1_FORK_SLOT,CROSSLINK_LOOKBACK,PLACEHOLDERparametersSHARD_SLOTS_PER_BEACON_SLOTin favour ofSHARD_SLOTS_PER_EPOCHraise Exceptionwhich is not used in phase 0shard_block_transitioninto three functions (header, attestations, block size fee)BeaconStateparameter, thenShardState, thenShard)stateeverywhere forBeaconState, andshard_stateforShardState)compute_slot_of_shard_slot