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

Refactor signature handling #633

Merged
merged 4 commits into from
Feb 18, 2019
Merged

Refactor signature handling #633

merged 4 commits into from
Feb 18, 2019

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Feb 15, 2019

Goes with #625.

Related issue: #540

# Signature
'signature': 'bytes96',
}
```
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we even need to do this? Why not be more radical and just remove the Proposal class entirely and directly sign the block? Or is that to reduce the number of slashing conditions down the line when we have shard blocks?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do like the idea of having two separate slashing conditions for two reasons:

  1. It's somewhat cleaner (don't need Proposal and BEACON_CHAIN_SHARD_NUMBER)
  2. Beacon chain proposer slashing is arguably more important than shard proposer slashing. So with two separate transaction types we have guaranteed throughput for beacon chain proposer slashings.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's not completely trivial if we remove Proposal. The reason is that for the proposer slashing you would need a Merkle path to the block root to prove the slot number.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, why? Why not just have a ProposerSlashing object that contains two block headers? Sure it's maybe a hundred bytes longer, but that's nothing compared to attester slashings in any case...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We could replace Proposal with a BeaconBlockHeader object but would that be more verbose (Proposal only have 4 fields, BeaconBlockHeader would have 7) and slightly less efficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Aah I see, BeaconBlock objects themselves are too big, and we technically don't have a BeaconBlockHeader (well, from any block you can create a block header with an equivalent SSZ root by replacing the body with a hash of the body, but if we're going to take advantage of this fact in the spec it would be good to document it well...

I see a few possibilities:

  1. Modify the BeaconBlock object so it has an explicit header that contains hash(body) and a separate body
  2. Define a BeaconBlockHeader as a header that can be derived from any BeaconBlock
  3. Keep the current approach

Note that in terms of hashes and bytes, (1) and (2) are exactly equivalent; object1 containing the SSZ root of object2 with object2 living "somewhere else", and obect1 literally containing object2 in its entirety, are in some sense exactly the same thing and will have exactly the same SSZ root. So it's a choice of presentation rather than content.

(3) would change things less relative to the status quo and it would not introduce this weirdness of the header being kinda part of and kinda separate from the rest of the block, though it would lead to a slightly longer spec. I guess my "don't change things at this stage of the game unless it's a really really good idea" bias does make me lean slightly toward (3)....

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll keep as is and mull over it a bit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you approve the PR?

@vbuterin
Copy link
Contributor

Yay! Love PRs that delete more LoCs than they add as usual. Made two comments, otherwise can't find any issues yet.

Copy link
Contributor

@vbuterin vbuterin left a comment

Choose a reason for hiding this comment

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

👍

@djrtwo
Copy link
Contributor

djrtwo commented Feb 17, 2019

I'd like to take a quick look at this before merge. Give me today

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants