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

Set the value of the random field to the previous randao_mix #2581

Merged
merged 5 commits into from
Sep 20, 2021

Conversation

mkalinin
Copy link
Contributor

@mkalinin mkalinin commented Sep 1, 2021

What is changed?

The randao_mix that is passed as the value to the random field is now the mix that is computed with the reveal of the previous block instead of the reveal of the current block as it was before this change.

This PR introduces a straightforward implementation. An alternative may look as follows:

def process_block(state: BeaconState, block: BeaconBlock) -> None:
    process_block_header(state, block)
    prev_randao_mix = get_randao_mix(state, get_current_epoch(state))  # [New in Merge]
    process_randao(state, block.body)
    process_eth1_data(state, block.body)
    process_operations(state, block.body)
    process_sync_aggregate(state, block.body.sync_aggregate)
    if is_execution_enabled(state, block.body):
        process_execution_payload(state, block.body.execution_payload, prev_randao_mix, EXECUTION_ENGINE)  # [New in Merge]

The above code makes randao_mix as a parameter of the process_execution_payload function and requires more changes including test vectors of this function (they would require yielding prev_randao_mix).

Rationale

Future compatibility with the proposer/builder separation. Without this change the proposer would have to communicate the most recent randao_mix to the block builder and make this communication in some advance to give the builder a time to re-execute a block, ideally as fast as the mix becomes known. This communication would likely need to happen over the public channel to avoid the situation when the builder needs to reveal its network identity to the proposer. There might also be an adverse effect of re-executing a block when the MEV opportunity that the builder have found is getting reduced or disappearing after the re-execution and the builder will have to adjust the proposer's bribe. So, the proposer should communicate the mix to all builders in order to see all bids after builders re-executes their blocks.

Security implications

The privacy and re-execution issues makes the public communication of the randao mix that is happening in as much advance as it's possible is very likely the case with proposer/builder separation. Which is effectively not the far from using the randao_mix computed with the reveal of the previous block with an exception for the previous slot being empty. But we don't expect empty slots to be frequent on the mainnet. From this standpoint we might experience reduction in the level of security provided by RANDOM opcode with the separation introduced to the system.

Moreover, the most recent randao_mix is always known to the proposer of a block and, obviously, this knowledge might be exploited which significantly reduces the advantage of using the current mix instead of the previous one.

Another argument is that currently BLOCKHASH(n-1) (the hash of the previous block) is used as a source of randomness by some apps. And using randao_mix[n-1] would be aligned with this approach. By the end of the day, the level of randomness that is provided by randao_mix[n-1] is still much bigger than BLOCKHASH(n-1) can give.

@mkalinin mkalinin force-pushed the set-random-to-prev-randao-mix branch from b49968e to 02057cb Compare September 1, 2021 14:59
@djrtwo
Copy link
Contributor

djrtwo commented Sep 1, 2021

I think this has some deeper implications than I think are being addressed in the above discussion given out applications currently likely expect DIFFICULTY to be just-in-time. I'd like to discuss a bit more before we quickly merge

I see the value in shifting to prev_randao for the builder separation but having the randomness publicly known at block N for use in block N+1 likely opens a ton of manipulation on current uses of DIFFICULTY.

I hadn't realized it, but it makes sense that BLOCKHASH[n-1] is used (otherwise it would be a circular dependency), but I worry that existing DIFFICULTY randomness applications make different assumptions about when the information is available.

If you were designing randomness applications knowing this delay, you might be able to alter the game such that it is hard to profit off of the slot lookahead, but I worry that some existing applications will be vulnerable to frontrunning in unexpected ways by this change.

Note -- on the long list of security improvements is an "early randao reveal slashing" where if you reveal your reveals early you can get slashed. This would potentially prevent a market where you provide your randao at slot N-1 to builders, but in practice it wouldn't really because you can't be slashed after slot N for an early reveal and you are the assigned proposer at N so not time for it to be included unless a re-org to a different N proposer.

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.

The changeset looks good

want to discuss the change a bit more though. see comment!

@mkalinin
Copy link
Contributor Author

mkalinin commented Sep 2, 2021

Good point on the DIFFICULTY 👍 The only parameter that is known to the miner is the timestamp of a block, other difficulty inputs are know to the network as well. And cur_randao is more aligned with the difficulty approach while prev_randao is aligned with the BLOCKHASH.

If we go deeper into the analysis of known to the miner/proposer only vs known to the whole network in advance we can deduce the following:

  • there is many participants in the network vs only one actor which are seeking for MEV opportunities related to the breach in the randomness; unless such MEV opportunities are somehow democratised, I believe it make sense to democratise them if they could give a tangible gain wrt DeFi opportunities, democratisation in this case would have an additional complexity because randomness is not know to the searcher but some workaround might be found here
  • a miner doesn't know the timestamp in advance but the timestamp is malleable and this can be exploited up to a certain extent (miner don't want to increase difficulty that much)
  • a proposer knows its reveal in advance and depending on the case might have more time than a miner to find or accommodate an MEV opportunity related to the randomness breach

So, the cur_randao looks stronger than the prev_randao but it's difficult to say of how much. Since the timestamp is malleable we can assume that the guarantees of the difficulty and the cur_randao are the same despite the randao is known in advance.

What worries me in the perspective of proposer/builder separation is that if we use cur_randao now then we give some promise to the applications which might become broken when proposer/builder separation comes in as cur_randao will start to be known to the network before the block which means it will be giving the same guarantee as the prev_randao does give. From this standpoint we might want to use prev_randao from the beginning which also allows to avoid an additional round of communication that would be required to send the reveal to builders and reduces the complexity of proposer/builder separation design. If we'd have no plans to include the separation in the roadmap the cur_randao would definitely be a winner.

cc @vbuterin @MicahZoltu

@MicahZoltu
Copy link
Contributor

From a security standpoint, I think we should model any value that is known to any party at some particular point in time to be known to the whole network at that point in time. If a block builder can know timestamp and difficulty before executing transactions, then they can conditionally include transactions based on those values. Even without revealing those values to the network, they can still delegate decision making based on those values by giving transactions an opportunity to conditionally execute based on them (e.g., don't get included unless those are specific values).

Today, a miner knows block.difficulty and block.timestamp prior to executing transactions. This means transactions can be included or not based on those values. Further, the miner can manipulate timestamp for the current block (I don't think they can manipulate difficulty, please correct me if I'm wrong here) to fulfill whatever needs transactions have. This means that effectively anyone wanting to extract MEV could have either direct or indirect access to these variables.

After PoS with the current spec (without this PR), the situation is the same in that the proposer has access to these values in advance of execution and they can choose to provide either direct or indirect access to these values to anyone they want by way of broadcasting the values ahead of time or giving people a mechanism for executing code in an environment controlled by the proposer to "probe" those values and make decisions based on them.

After PoS with this PR, the only change in the situation is that everyone will have access to one of those values (RANDAO) without needing to engage in backchannel communication with the proposer. This democratizes access to information that otherwise would be harder (but not impossible) to get, which I think is generally a good thing, not a bad thing.

@mkalinin
Copy link
Contributor Author

mkalinin commented Sep 2, 2021

After PoS with the current spec (without this PR), the situation is the same in that the proposer has access to these values in advance of execution and they can choose to provide either direct or indirect access to these values to anyone they want by way of broadcasting the values ahead of time or giving people a mechanism for executing code in an environment controlled by the proposer to "probe" those values and make decisions based on them.

The risk of an early RANDAO reveal will be mitigated by the slashing condition that @djrtwo has mentioned (slash validator if there is a proof of revealing its randao_reveal value ahead of time, namely in any time before the current slot). But, this slashing protection stops working after a block of the previous slot is received by the network. So, it still leaves an opportunity for a validator to reveal the value in some advance.

@mkalinin
Copy link
Contributor Author

mkalinin commented Sep 8, 2021

Sorry, I've been mistaken previously by saying that the proposer will have to reveal its randao_reveal, it will have to reveal randao_mix after his reveal is mixed in. And if the spec requires cur_randao_mix to be passed onto payload building process then there will be an incentive for a proposer to share computed cur_randao_mix with block builders as soon as the parent block is received. The randao mix sharing will potentially use channels that many builders are listening to. It could put applications relying on the assumption that the random value is known only by the proposer at a higher risk than if the prev_randao_mix were used. This might be another argument in favour of using the prev_randao_mix.

@hwwhww hwwhww added the Bellatrix CL+EL Merge label Sep 8, 2021
@MicahZoltu
Copy link
Contributor

It should definitely be assumed that as soon as one person knows the next RANDAO result, then all financially motivated parties know it as well.

@djrtwo
Copy link
Contributor

djrtwo commented Sep 15, 2021

I've come over to the dark side. This change makes sense to me.

Re-reviewing for detail/correctness

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.

Looks good! need to remove the unnecessary randao_reveal arg in the validator helper functions. after that should be good to merge 👍

epoch = get_current_epoch(state)
return xor(get_randao_mix(state, epoch), hash(randao_reveal))


def produce_execution_payload(state: BeaconState,
parent_hash: Hash32,
randao_reveal: BLSSignature,
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to remove randao_reveal as an arg here and from the caller in get_execution_payload

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Should be fixed now

@mkalinin mkalinin force-pushed the set-random-to-prev-randao-mix branch from 0d0eb68 to 26c78b5 Compare September 17, 2021 10:01
@mkalinin mkalinin mentioned this pull request Sep 17, 2021
2 tasks
@djrtwo djrtwo force-pushed the set-random-to-prev-randao-mix branch 2 times, most recently from 72cb240 to 26c78b5 Compare September 17, 2021 17:01
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.

great!

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.

4 participants