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

Alternative crosslink data construction, take 1 #1278

Closed
wants to merge 6 commits into from
Closed

Conversation

vbuterin
Copy link
Contributor

@vbuterin vbuterin commented Jul 8, 2019

Properties:

  • Optimal data packing
  • Fixed depth
  • All data is encoded; makes fraud proofs not-too-difficult

Weaknesses:

  • Uses SSZ serialization.
  • Positions of blocks dynamic, so accesses require an extra Merkle branch

Properties:

* Optimal data packing
* Fixed depth
* All data is encoded; makes fraud proofs not-too-difficult

Weaknesses:

* Uses SSZ serialization.
* Positions of blocks dynamic, so accesses require an extra Merkle branch
@vbuterin vbuterin mentioned this pull request Jul 13, 2019
10 tasks
)
MAXLEN = (
BYTES_PER_SHARD_BLOCK_HEADER + BYTES_PER_SHARD_BLOCK_BODY
) * SHARD_SLOTS_PER_EPOCH * MAX_EPOCHS_PER_CROSSLINK
Copy link
Contributor

Choose a reason for hiding this comment

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

SHARD_SLOTS_PER_EPOCH is undefined. Should it be SHARD_SLOTS_PER_BEACON_SLOT(#1276) * SLOTS_PER_EPOCH?

specs/core/1_shard-data-chains.md Outdated Show resolved Hide resolved
specs/core/1_shard-data-chains.md Outdated Show resolved Hide resolved
@@ -76,7 +79,7 @@ The following types are defined, mapping into `DomainType` (little endian):

| Name | Value |
| - | - |
| `PLACEHOLDER` | `2**32` |
| `PLACEHOLDER` | `2**3` |
Copy link
Contributor

Choose a reason for hiding this comment

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

I changed it to 2**3 for testing.

@@ -107,7 +110,7 @@ class ShardBlock(Container):
shard: Shard
beacon_chain_root: Bytes32
parent_root: Bytes32
data: ShardBlockBody
body: ShardBlockBody
Copy link
Contributor

Choose a reason for hiding this comment

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

Also hotfix in #1298.

def compute_crosslink_data(blocks: Sequence[ShardBlock]) -> Bytes32:
"""
Flattens a series of blocks. Designed to be equivalent to SSZ serializing a list of
flattened blocks with one empty block at the end as a termination marker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you elaborate what do you mean by "one empty block at the end" here?

Copy link
Contributor

@hwwhww hwwhww Jul 15, 2019

Choose a reason for hiding this comment

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

Could you elaborate what's the main advantage of this one over just using SSZ serialize like:

class CrosslinkingBlocks:
    data: List[ShardBlock, SHARD_SLOTS_PER_EPOCH * MAX_EPOCHS_PER_CROSSLINK]

assert len(blocks) <= SHARD_SLOTS_PER_EPOCH * MAX_EPOCHS_PER_CROSSLINK
crosslinking_blocks = CrosslinkingBlocks(data=blocks)
return serialize(crosslinking_blocks)

?


```python
def flatten_block(block: ShardBlock) -> Bytes:
return zpad(serialize(get_shard_header(block)), BYTES_PER_SHARD_BLOCK_HEADER) + serialize(block.body)
Copy link
Contributor

Choose a reason for hiding this comment

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

I added serialize() here, correct?

positions = [4 * len(blocks)]
bodies = []
for block in blocks:
bodies.append(flatten_block(block))
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't flattening the block here reduce our ability to construct proofs about components of a block through the crosslink root? Still possible but not as clean due to flattening to bytes

@dankrad
Copy link
Contributor

dankrad commented Jul 22, 2019

Uses SSZ serialization.

I wouldn't see that as a weakness, if it is the right tool for the job, I think we should be able to bring it into consensus.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 23, 2019

I wouldn't see that as a weakness, if it is the right tool for the job, I think we should be able to bring it into consensus.

agreed.

@djrtwo
Copy link
Contributor

djrtwo commented Jul 29, 2019

@vbuterin This can be closed as it was superseded by #1294, right?

@djrtwo
Copy link
Contributor

djrtwo commented Aug 1, 2019

Closing -- superseded by #1294

@djrtwo djrtwo closed this Aug 1, 2019
@djrtwo djrtwo deleted the vbuterin-patch-6 branch September 8, 2019 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants