Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Pure consensus upgrade version of the merge #2257
Pure consensus upgrade version of the merge #2257
Changes from 21 commits
0dec828
ee16163
f6f3687
3fb5f2e
5435324
3c9cd85
a368f5d
b8e16c1
bf15164
46fc8a1
3420e51
24dc8a2
38a455c
83453d2
7e6ac4e
96de910
ea5f606
63ae9f2
ee5ecf8
a23bde3
260a0a5
81a2c2c
41a087a
223aba3
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Post-London, we'll want to add the
BASE FEE
here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe define this as
transactions: List[Union[Transaction], MAX_APPLICATION_TRANSACTIONS]
to add a 4 byte selector to enable different transaction types in the future?Optimization note: the Union type in SSZ is unused in Eth2 so far. We can parametrize the selector length, to shorten it to a 1 byte selector.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to this EIP https://eips.ethereum.org/EIPS/eip-2718? If yes then I would wait until the EIP is approved and shipped on the Mainnet. The intention is to replicate the application layer structures from those that will be on the Mainnet at the point of merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also see #2270, to support
Union
with a 1-byte selectorThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that
Eth1Data
name will look odd after the merge but we might want to keep it as it this time. The follow up cleanups are going to reduce the follow distance ofEth1Data
and it would be a good time to make the renaming too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more of
DepositContractData
if we are going to rename it.It is more specific than application layer data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need
get_application_state()
at all? (also see other comment below)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to pass in the randao or some other seed for
DIFFICULTY
/BLOCKHASH
here?obviously, easiest to just stick them on the
application_payload
so the eth1 engine doesn't even have to know about this new logic.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think it would make sense to prepare a consensus bundle that will be passed onto this function with randao mix and further extended by other bits. I'd add it later once we made the decision about difficulty and whether to use randao or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a need for the Eth2 node to access
application_state
?A cleaner approach would be to outsource all the work to
application_state_transition()
, so that it operates using theapplication_state_root
. (see suggestion for theapplication_state_transition
function)Also, need to check that whatever is returned by
application_state_transition()
actually matches withbody.application_payload.state_root
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely see the value in simplifying this part. But then it would not read like the beacon state and the application state are tightly coupled.
For the root check we can do the following:
But that would require yet another abstract function. So, it might really be better to have the following (according to your suggestion):
The latter form also fits stateless verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpicking:
application_state_root = application_state_transition(...)
requiresapplication_state_transition
to returnBytes32
, but it doesn't align with the naming pattern as beacon state transition functionstate_transition(...) -> None
. So having aget_application_state_root(application_state)
makes sense to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making this call conformant with
state_transition(...)
was the original intention put behind this extra function call and using application state object rather than the state root.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may also define
ApplicationState
in a following wayAnd then add an explicit check of the state root after application state transition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
is_processed
just a super set ofis_valid
?Do eth1 clients remember if an invalid block has already been processed? or does it just drop it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Current behaviour is to return error if either invalid or not yet processed block is requested meaning that these two statuses are indistinguishable. So, if we want this level of granularity then JSON-RPC implementation will have to be adjusted but we probably don't want it. It's worth discussing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with some configuration it stores invalid blocks to be able to serve
debug_getBadBlock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this granularity required?
uint's larger than 64 have been entirely avoided in the consensus-layer so far
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
total difficulty is on the order of
10**22
so it doesn't fit in uint64...We could reduce precision to avoid uint256. Need to consider our options here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, total difficulty falls into
> 2**74
interval today. The other potential way of handling this is to return an offset wrt some absolute total difficulty value. Current block's difficulty is around2**64
, so we indeed may divide total difficulty by2**20
without loss of generality.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it worth noting that
uint256
type is used byTransaction
data structure to define several fields.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, right. I don't thnk we'll be able to get around
value
with 256 granularityI suppose beacon clients don't actually have to support arithmetic on these TX values because all arithmetic and validations happen in application layer so as long as they can serialize and deserialize, that's enough support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably move some of his logic into sub-functions on phase0 so we can have better code reuse. Can do that in a separate pr