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

feat: implements process bls to execution change #251

Conversation

distributedstatemachine
Copy link
Contributor

@distributedstatemachine distributedstatemachine commented Sep 18, 2023

Took a stab at this and submitting a draft PR for feedback.

This PR closes #249

@distributedstatemachine distributedstatemachine marked this pull request as draft September 18, 2023 21:56
Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice work!

there are a few changes to go ahead and make now

after those, we will want to change the use of assert! to errors but we can handle that after the first round of updates

ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
@distributedstatemachine distributedstatemachine marked this pull request as ready for review September 20, 2023 05:50
Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice updates! left some comments

Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice updates! we are getting closer to merge :)

I left some comments and also I think can tighten up our implementation to drop some use of clone and be a bit less verbose

  1. keep signed_address_change as &mut SignedBlsToExecutionChange
    you can then immediately do this
let address_change = &mut signed_address_change.message;
let signature = &signed_address_change.signature;

// ... rest of code
  1. let's follow the spec and grab a ref to our validator (it looks like this won't collide anywhere else as far as the borrow checker is concerned)
let mut validator = &mut state.validators[address_change.validator_index];
  1. we can then also bind the withdrawal credentials so we don't have to keep going through the state data
let withdrawal_credentials = &validator.withdrawal_credentials;
  1. let's not clone the from_bls_public_key,
let public_key = &address_change.from_bls_public_key;

you will need to clone it when assembling the InvalidBlsToExecutionChange error, but if you only clone inside the if body then we will only pay the cost when necessary

  1. I may have misled you w/ the verify_signed_data helper as we will run into some issues w/ the borrow checker

instead lets just inline its implementation:

    let signing_root = compute_signing_root(address_change, domain)?;
    verify_signature(&address_change.from_bls_public_key, signing_root.as_ref(), signature).map_err(Into::into)

ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
yash-atreya added a commit to yash-atreya/ethereum-consensus that referenced this pull request Sep 25, 2023
@yash-atreya
Copy link

Hey @ralexstokes have made the requested changes in PR #265

@distributedstatemachine
Copy link
Contributor Author

distributedstatemachine commented Sep 26, 2023

@ralexstokes I have made changes , but want to clarify the following :

let's follow the spec and grab a ref to our validator (it looks like this won't collide anywhere else as far as the borrow checker is concerned)
let mut validator = &mut state.validators[address_change.validator_index];

IIUC, we already take a reference , and let mut validator generates a warning

warning: variable does not need to be mutable

verify_signature(&address_change.from_bls_public_key, signing_root.as_ref(), signature).map_err(Into::into)

I have mapped this to the actual error enum in state_transation::errors, as the suggestion generates this error

error: type annotations needed
label: cannot infer type of the type parameter `T` declared on the trait `Into`

Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice updates! left some nits below

ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

let's just clean up the errors and then this should be ready to merge!

ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
Copy link
Owner

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

excellent! thanks for staying w/ me on all the back-and-forth

a few minor nits but I think can I merge from here

ethereum-consensus/src/state_transition/error.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
ethereum-consensus/src/capella/block_processing.rs Outdated Show resolved Hide resolved
@ralexstokes ralexstokes merged commit 16947e7 into ralexstokes:main Sep 28, 2023
2 checks passed
@distributedstatemachine distributedstatemachine deleted the feat/process_bls_to_execution_change branch October 2, 2023 07:28
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.

capella: impl process_bls_to_execution_change
3 participants