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

Add random value to block metadata and fix sys_prevrandao #1207

Merged
merged 11 commits into from
Sep 25, 2023

Conversation

wborgeaud
Copy link
Contributor

See #1204 for details.

evm/src/proof.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@LindaGuiga LindaGuiga left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this! Looks great, but there's just one missing thing: block_random is not set in set_block_metadata_target.

Copy link
Contributor

@LindaGuiga LindaGuiga Sep 20, 2023

Choose a reason for hiding this comment

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

Should sys_prevrandao be removed from syscall_stubs.asm and put in (for example) metadata.asm instead?

@@ -336,6 +346,9 @@ impl BlockMetadataTarget {
builder.connect(bm0.block_timestamp, bm1.block_timestamp);
builder.connect(bm0.block_number, bm1.block_number);
builder.connect(bm0.block_difficulty, bm1.block_difficulty);
for i in 0..8 {
builder.connect(bm0.block_random[i], bm1.block_random[i]);
Copy link
Contributor

Choose a reason for hiding this comment

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

block_random is connected here, but it isn't set in set_block_metadata_target. This leads to a generators not run error in a recursive proof.

@@ -10,11 +10,9 @@ global sys_blockhash:
EXIT_KERNEL

// This is a temporary version that returns the block difficulty (i.e. the old version of this opcode).
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this comment be deleted or updated?

Copy link
Collaborator

@Nashtare Nashtare 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!

@@ -791,6 +793,7 @@ fn test_two_txn() -> anyhow::Result<()> {
block_timestamp: 0x03e8.into(),
block_number: 1.into(),
block_difficulty: 0x020000.into(),
block_random: H256::from_low_u64_le(0x020000),
Copy link
Collaborator

Choose a reason for hiding this comment

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

tiny nit: we're using H256::from_uint(n.into()) in other places, though doesn't really matter.

@wborgeaud wborgeaud merged commit 8c78271 into main Sep 25, 2023
2 checks passed
@wborgeaud wborgeaud deleted the real_prevrandao branch September 25, 2023 16:20
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.

3 participants