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

api: add skip_randao_verification for produceBlockV2 #3837

Merged
merged 3 commits into from
Sep 21, 2022

Conversation

michaelsproul
Copy link
Contributor

@michaelsproul michaelsproul commented Jul 4, 2022

Implement ethereum/beacon-APIs#222.

If my Nim isn't too atrocious hopefully I might be able to get this to a mergeable state. I'm putting it up as a draft for gentle feedback (this is my first time ever writing Nim).

Does the broad angle of attack seem acceptable? My next step would be to work out how to put a zero randao_reveal in when none is provided, and to add a new UpdateFlag to disable verification of the RANDAO signature only.

@michaelsproul michaelsproul marked this pull request as ready for review July 5, 2022 00:57
beacon_chain/extras.nim Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Jul 6, 2022

Unit Test Results

       12 files  ±0       860 suites  ±0   1h 14m 13s ⏱️ + 4m 6s
  1 982 tests ±0    1 835 ✔️ ±0  147 💤 ±0  0 ±0 
10 662 runs  ±0  10 472 ✔️ ±0  190 💤 ±0  0 ±0 

Results for commit 9e22583. ± Comparison against base commit 80f44f4.

♻️ This comment has been updated with latest results.

@arnetheduck
Copy link
Member

this is my first time ever writing Nim

welcome to the bright side - fortunately, it's not that easy to write atrocious Nim code (which perhaps is the main raison d'etre of it), and yours is already looking quite good ;)

I guess there are two things to look at here: is the PR good, and is the feature desired in general - I'll focus on the former and then we can have the latter discussion in the spec PR, upon which this PR would depend :)

beacon_chain/extras.nim Outdated Show resolved Hide resolved
@arnetheduck
Copy link
Member

@michaelsproul since ethereum/beacon-APIs#222 is merged, we can go ahead with this PR as well as soon as it's rebased / updated to the new param names etc

@michaelsproul michaelsproul changed the title api: add verify_randao for produceBlockV2 api: add skip_randao_verification for produceBlockV2 Sep 5, 2022
@michaelsproul
Copy link
Contributor Author

michaelsproul commented Sep 5, 2022

I've updated this PR to use the skip_randao_verification flag that was decided upon in the beacon-APIs repo. I've tested it manually and it seems to be working well. If you'd like me to add some automated tests please point me in the right direction.

@arnetheduck
Copy link
Member

If you'd like me to add some automated tests please point me in the right direction.

The relevant location is here:

randao_reveal =
: we need to teach this function to take a valid/invalid randao and pass on the new randao flag, then teach
suite "Block pool processing" & preset():
to call it with the various combinations - although the suite creates some dummy blocks, a new test in it will do fine.

@arnetheduck
Copy link
Member

Thanks for the feature and the upstream spec! I'll merge this now and throw in some tests in a separate PR.

@arnetheduck arnetheduck merged commit d6325b1 into status-im:unstable Sep 21, 2022
@michaelsproul
Copy link
Contributor Author

Thanks for having me on as a novice Nimbus contributor! I'll wear the badge with pride 😁

@michaelsproul michaelsproul deleted the verify-randao branch September 21, 2022 07:40
arnetheduck added a commit that referenced this pull request Sep 26, 2022
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