-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
EIP-3675: Upgrade consensus to Proof-of-Stake #3675
Conversation
Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):
@MicahZoltu @lightclient @Arachnid @cdetrio @Souptacular @nicksavers @wanderer @gcolvin @axic |
🚀 🚀 |
Co-authored-by: Jorge Olivero <jorgeaolivero@gmail.com> Co-authored-by: terence tsao <terence@prysmaticlabs.com>
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.
a few nits; glad to see it all coming together though
Co-authored-by: lightclient <14004106+lightclient@users.noreply.github.com>
Head branch was pushed to by a user without write access
Head branch was pushed to by a user without write access
@mkalinin this is looking pretty good as a draft, please tag me when you're done making changes and are ready to merge. |
Head branch was pushed to by a user without write access
These anchors are not working:
I think they are made lowercase in the rendering, and/or the dashes are replaced with underscores. |
Head branch was pushed to by a user without write access
I have turned them all to lowercase, the dashes are in place on the site so they should work (if this output is an attempt to render for the EIP site) |
Head branch was pushed to by a user without write access
I think it's ready for the merge. cc @axic @lightclient @MicahZoltu |
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.
Left some comments that I recommend reviewing, but nothing blocking this from merging as a Draft.
|
||
### PoW block processing | ||
|
||
PoW blocks that are descendants of any terminal PoW block **MUST NOT** be imported. This implies that a terminal PoW block will be the last PoW block in the canonical chain. |
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 may be worth mentioning here that execution clients should accept, validate, store, and gossip all peers to the terminal proof of work block until the first POS_BLOCK_FINALIZED
event is received. This note may need to be split up between this section and the networking section.
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.
You mean that there is a bit of inconsistency between the two sections? Like, this section says keep processing terminal PoW blocks and their ancestors and puts no time boundary on it while the network section implies more strict behaviour and deprecates gossip at a strict boundary. And we might want to reflect this restriction here as well?
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'd suggest to add the following kind of statement:
Beginning with the first
POS_BLOCK_FINALIZED
event, client software MUST NOT import any PoW block that doesn't belong to the canonical chain.
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.
My concern is that currently I don't think clients gossip siblings until they see a new longest chain (maybe I'm wrong about this)? If that assumption is true, then this will be a behavior change that clients need to implement.
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.
Hmm, interesting. I would assume they do verify PoW upon receiving a block and propagate this block further if the verification passed. In the mean time this block is stored. And here we have options, it either processed right away or processing is deferred until the fork that this block does belong to becomes the longest chain.
And this is where we may have a real issue (not related to gossip). If the terminal PoW block that is picked by the beacon chain proposer has been received but hasn't been processed by our node due to conditions described above (our node may receive another terminal block with the same TD and thus refuse to re-org). Then our node won't be able to verify the beacon block that has been received because it will stuck verifying the PoW block that is the parent of the execution payload. So, it means that the spec must force every terminal PoW block to be processed, not only stored.
cc @holiman
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.
An alternative solution for this edge case would be the corresponding method in the Consensus API, let's say verifyTerminalPoWBlock(block_hash: Hash32)
. This method would enforce the execution of a sibling terminal PoW block if it hasn't been executed yet. The downside of this approach is the delay between receiving a beacon block and considering it for further processing due to calling to this method, especially, if there are more than one block on this sibling fork that must be executed. But this approach seems to be less intrusive in terms of execution clients change set.
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.
A solution with verifyTerminalPoWBlock(block_hash: Hash32)
or a similar call aids mitigation of the DoS vector that is described in #3675 (comment). If miners have a cheap way to produce terminal PoW blocks to spam the network it is better for a node to avoid executing them all and execute terminal PoW blocks from non-canonical chains on demand.
* Remove all validation rules that are evaluated over the list of ommers and each member of this list, except for enforcing this list to be empty. | ||
* Add verification of the fields noted in [block structure](#block-structure) as their specified constant value, including the enforcement of empty ommers list. |
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.
On these two lines you call out the ommers list specifically, but you don't call out any other fields specifically. This is confusing because it makes it seem like ommers is special when I believe it is not, it should be validated as a constant just like all other fields mentioned in the Block structure
section.
* Remove increasing the balance of the block's **`beneficiary`** account by the ommer inclusion reward per each ommer. | ||
* Remove increasing the balance of the ommer's **`beneficiary`** account by the ommer block reward per each ommer. |
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.
This feels redundant, since PoS blocks cannot have any ommers. As mentioned above, calling this out specially makes it feel to the reader like something special is happening with ommers when in reality nothing special is happening. This can lead the reader to searching for answers to "why are ommers special" when there is nothing to be found.
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 may fill redundant but the purpose of this statement is to be consistent with respect to the YP when removing PoW rewards, i.e. the intention is to explicitly remove each source of these rewards.
### Block and ommer rewards | ||
|
||
Beginning with `TRANSITION_BLOCK`, block and ommer rewards are deprecated. Specifically, the following actions **MUST** be taken: | ||
* Remove increasing the balance of the block's **`beneficiary`** account by the basic block reward. |
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 this is referred to just as block reward
in most places. Calling it basic block reward
may lead to confusion since that phrase isn't commonly used.
* [`NewBlockHashes (0x01)`](https://github.com/ethereum/devp2p/blob/master/caps/eth.md#newblockhashes-0x01) | ||
* [`NewBlock (0x07)`](https://github.com/ethereum/devp2p/blob/master/caps/eth.md#newblock-0x07) | ||
|
||
Beginning with the first `POS_BLOCK_FINALIZED` event, peers that keep sending these messages **SHOULD** be disconnected. |
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 this will introduce a race condition. Not all peers will receive POS_BLOCK_FINALIZED
at the same time, so some may still be broadcasting NewBlockHashes
and NewBlock
for siblings of the terminal PoW block after some of their peers have received the POS_BLOCK_FINALIZED
event.
Consider changing this rule to be that after the second POS_BLOCK_FINALIZED
event peers are disconnected, as by that time all peers should have received the first POS_BLOCK_FINALIZED
event and should no longer be gossping sibling terminal PoW blocks.
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.
Fair, but there is also the following statement above:
- The networking stack SHOULD NOT send the following messages if they advertise the descendant of any terminal PoW block
Which means that effectively block gossip SHOULD be stopped before POS_BLOCK_FINALIZED
.
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.
Why would block gossip stop before POS_BLOCK_FINALIZED
? I think we should assume that there will be a steady stream of new siblings until POS_BLOCK_FINALIZED
, since the protocol rules as I understand them suggest you continue to gossip those.
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.
Because there is enough time between the terminal PoW block event and the first POS_BLOCK_FINALIZED
event that is issued when the first PoS block gets finalized (it is not issued when the beacon block with empty execution payload is finalized). The gap between these two events is at least 2 epochs and unless there is a party that is constantly mining terminal PoW blocks there should be a plenty of time to disseminate all terminal PoW blocks existing in the network.
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, I feel like we should assume that one or more miners mine terminal PoW siblings. They have a huge pile of hashing power and nothing to do with it. Maybe we'll get lucky and they don't, but I feel like we should prepare for them doing this.
I wonder if a miner could do something interesting where once the first terminal PoW block is produced (the most likely one to be finalized) they switch to mining a forked chain some number of blocks back that lowers the difficulty with each block. The goal here would be to drive the difficulty down as far as possible in this fork given their hashpower while still reaching the target total difficulty. Once you get to one block shy of target total difficulty, you then produce terminal PoW blocks over and over as fast as you can in order to spam the network?
I'm not sure how much you could manipulate given 2 epochs.
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'm not sure how much you could manipulate given 2 epochs.
Good question. There are possibilities depending on whether whole hashing power participates in anything like that, whether this attack started in advance by switching a portion of hashing power from mainnet to start producing such a chain before the transition. If all hashing power participates it may produce roughly 60
blocks during 2 epochs period but with lowering the difficulty this number increases; like 20 blocks with 1000 seconds gap between their timestamps is required to reduce the difficulty ~8 times and produce roughly 40*8=320
blocks during the rest of the period, it also doesn't count how many blocks needed to reach the terminal total difficulty threshold. If we assume that somehow miners become able to produce a block in a second then during two epochs we would got 768
terminal PoW blocks, I don't know how bad is it for the network if we assume that these blocks are not executed, only propagated with PoW verification.
As for the network stack, if handlers are removed won't peers sending unknown messages be disconnected by clients?
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.
For the health of the network under this kind of attack it makes sense for clients to stop propagating any blocks upon receiving the first POS_BLOCK_FINALIZED
and remove handlers with the corresponding penalty upon receiving the second POS_BLOCK_FINALIZED
. So, the updated spec should look like:
The networking stack SHOULD NOT send the following messages if they advertise the descendant of any terminal PoW block:
Beginning with the first
POS_BLOCK_FINALIZED
event, the networking stack MUST discard the following ingress messages:Beginning with the second
POS_BLOCK_FINALIZED
event, the networking stack MUST remove the handlers corresponding to the following messages:Peers that keep sending these messages after the handlers have been removed SHOULD be disconnected.
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 this conversation to the discussions-to link. PR comments should be assumed to be transient, especially on merged PRs.
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.
|
||
Deprecated block fields are replaced with constant values to ensure the block format remains backwards compatible. Preserving the block format aids existing smart contracts and services in providing uninterrupted service during and after this transition. | ||
|
||
Particularly, this is important for those smart contracts that verify Merkle proofs of transaction/receipt inclusion and state by validating the hash of externally provided block header string against the corresponding value returned by the `BLOCKHASH` operation. |
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.
Particularly, this is important for those smart contracts that verify Merkle proofs of transaction/receipt inclusion and state by validating the hash of externally provided block header string against the corresponding value returned by the `BLOCKHASH` operation. | |
Particularly, this is important for those smart contracts that verify Merkle proofs of transaction/receipt inclusion and state by validating the hash of externally provided block header against the corresponding value returned by the `BLOCKHASH` operation. |
The string
there I think makes the sentence a bit more confusing, as you may receive a block header in many forms such as a structured format, a byte array, a hex string, etc.
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.
You're right, theoretically they may receive a block header as a set of field values, then RLP encode it, compute a hash and verify this hash. Although it does seem like a suboptimal solution we better remove this "string".
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.
If you need to pull data out of the header, it may be easier to serialize from raw inputs than it is to parse/decerialize to pull bits out of the header.
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.
Arguably it may be cheaper given a string to verify it against the corresponding BLOCKHASH
value by parsing out a number
using some optimized RLP decoder that jumps between list elements and then read the needed field in the same fashion than RLP encoding the whole thing as a preliminary to the verification. Anyway, I am going to remove the "string" term.
## Test Cases | ||
*TBD* | ||
|
||
## Reference Implementation | ||
*TBD* | ||
|
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.
## Test Cases | |
*TBD* | |
## Reference Implementation | |
*TBD* |
Consider removing these until you are ready to fill them in. They are optional sections, and thus not required to be present to merge this as a draft and if they never get filled in that is technically acceptable. Generally, we want to write EIPs in a way that is "ready to merge as final" without TODOs.
### Further threat analysis | ||
_TBD_ | ||
|
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.
### Further threat analysis | |
_TBD_ |
As above, recommend leaving out unfinished sections until you have something to put into them.
Specification of the consensus mechanism upgrade on Ethereum Mainnet that introduces Proof-of-Stake.
Specification of the consensus mechanism upgrade on Ethereum Mainnet that introduces Proof-of-Stake (a.k.a. The Merge)