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

polish merge/beacon-chain.md #2472

Merged
merged 15 commits into from
Jun 18, 2021
Merged

polish merge/beacon-chain.md #2472

merged 15 commits into from
Jun 18, 2021

Conversation

JustinDrake
Copy link
Collaborator

@JustinDrake JustinDrake commented Jun 8, 2021

Polish merge/beacon-chain.md with mostly non-substantive changes.

Non-substantive changes

  • rename MAX_EXECUTION_TRANSACTIONS to MAX_TRANSACTIONS_PER_PAYLOAD
    • rename "execution transaction" to just "transaction" as per discussion with Danny
  • rename compute_time_at_slot to compute_timestamp_at_slot
    • the function returns a Unix timestamp
    • "timestamp" matches execution_payload.timestamp
  • be explicit about ExecutionEngine.execution_state for clarity
  • rename ExecutionPayload.number to ExecutionPayload.block_number
    • more specific ("number" is pretty vague)
    • consistent with ExecutionPayload.block_hash
  • rename new_block to on_payload
    • the on_ prefix is consistent with other event handlers (e.g. see on_tick, on_block, on_attestation here)
    • the _payload suffix is more to the point given the function accepts an execution_payload
    • avoids conflict with on_block which is already used in the fork choice
  • rework the table of contents for consistency
  • order is_execution_enabled after is_transition_completed and is_transition_block
    • is_execution_enabled refers to is_transition_completed and is_transition_block
  • rename "transition" to "merge"
    • "transition" is a bit vague—we will have other transitions at future hard forks
    • there is no need for two words to refer to the same concept
  • add a bunch of inline comments, e.g. in process_execution_payload
  • make the process_execution_payload signature consistent with the other process_ functions in process_block which take as arguments state and block.body
  • remove TRANSITION_TOTAL_DIFFICULTY
    • to be put in merge/fork-choice.md where it is used
  • various misc cleanups

Substantive changes

  • reorder ExecutionPayload fields
    • for consistency with yellow paper and Eth1
    • same for ExecutionPayloadHeader
    • added comments separating out the execution block header fields from the extra fields (cosmetic)

@JustinDrake JustinDrake added the Bellatrix CL+EL Merge label Jun 8, 2021
@JustinDrake JustinDrake requested a review from mkalinin June 8, 2021 13:38
@djrtwo
Copy link
Contributor

djrtwo commented Jun 8, 2021

Re opaque transactions -- the intention is to type these with a union type so that multiple transaction types can be supported. "execution" is the more general and proper term imo

@JustinDrake
Copy link
Collaborator Author

"execution" is the more general and proper term imo

Just to make sure I understand are you suggesting renaming every instance of "opaque transaction" to "execution transaction"? That would work for me :)

specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
@protolambda
Copy link
Collaborator

rename EXECUTION_ENGINE to execution_engine
lowercase for consistency with the bls module which is similarly exposed

It's not a static module, it's a dynamic part of the environment, something that is plugged in like configuration. E.g. tests, testnets, mainnet, different eth1-redundancy setups, etc. may all use completely different execution engines through this same protocol. The upper-snake-case to match the config style was intentional.

specs/merge/beacon-chain.md Show resolved Hide resolved
specs/merge/beacon-chain.md Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
specs/merge/beacon-chain.md Outdated Show resolved Hide resolved
Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

lgtm

specs/merge/beacon-chain.md Show resolved Hide resolved
Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

Copy link
Collaborator

@protolambda protolambda left a comment

Choose a reason for hiding this comment

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

LGTM

@JustinDrake JustinDrake marked this pull request as ready for review June 17, 2021 21:31
@JustinDrake JustinDrake merged commit 878b15d into dev Jun 18, 2021
@JustinDrake JustinDrake deleted the JustinDrake-patch-7 branch June 18, 2021 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bellatrix CL+EL Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants