-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Eip6110 queue deposit requests #14430
Conversation
139db22
to
b008a3b
Compare
defer span.End() | ||
index, ok := st.ValidatorIndexByPubkey(bytesutil.ToBytes48(deposit.PublicKey)) | ||
if !ok { | ||
valid, err := blocks.IsValidDepositSignature(ðpb.Deposit_Data{ |
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 wonder if i should be doing any batch verification before hand ...
@@ -290,7 +290,7 @@ filegroup( | |||
visibility = ["//visibility:public"], | |||
) | |||
""", | |||
integrity = "sha256-BoXckDxXnDcEmAjg/dQgf/tLiJsb6CT0aZvmWHFijrY=", | |||
integrity = "sha256-D/HPAW61lKqjoWwl7N0XvhdX+67dCEFAy8JxVzqBGtU=", |
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.
can someone double check this one for me
beacon-chain/core/altair/deposit.go
Outdated
@@ -115,13 +115,13 @@ func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verif | |||
// # Increase balance by deposit amount | |||
// index = ValidatorIndex(validator_pubkeys.index(pubkey)) | |||
// increase_balance(state, index, amount) | |||
func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, verifySignature bool) (state.BeaconState, error) { | |||
func ApplyDeposit(beaconState state.BeaconState, data *ethpb.Deposit_Data, verified bool) (state.BeaconState, error) { |
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.
Do we want to change verifySignature
to verified
in the same PR? It seems to me it would be better to do it in another PR, and maybe open it beforehand to eliminate potential sources of errors and avoid doing too much in one PR. It's up to you.
beacon-chain/core/blocks/deposit.go
Outdated
if err := verifyDepositDataWithDomain(ctx, deposits, domain); err != nil { | ||
log.WithError(err).Debug("Failed to batch verify deposits signatures, will try individual verify") | ||
verified = true |
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 see why this was confusing before, and what you've done here is a welcome improvement. Thanks
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.
#14526 now as part of this pr
@@ -159,3 +173,44 @@ func verifyDepositDataWithDomain(ctx context.Context, deps []*ethpb.Deposit, dom | |||
} | |||
return nil | |||
} | |||
|
|||
func verifyPendingDepositDataWithDomain(ctx context.Context, deps []*ethpb.PendingDeposit, domain []byte) error { |
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.
In general, all of these may benefit from unit tests, but I’ll leave that decision to you:
verifyPendingDepositDataWithDomain
BatchVerifyPendingDepositsSignatures
batchProcessNewPendingDeposits
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.
Added one for batch ProcessNewPendingDeposits, can add more
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.
added more tests
@@ -82,7 +82,7 @@ func ProcessDeposits( | |||
// amount=deposit.data.amount, | |||
// signature=deposit.data.signature, | |||
// ) | |||
func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verifySignature bool) (state.BeaconState, error) { | |||
func ProcessDeposit(beaconState state.BeaconState, deposit *ethpb.Deposit, verified bool) (state.BeaconState, error) { |
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.
Unrelated to this PR but I don't see why we need to repeat ProcessDeposits
-> ProcessDeposit
, the majority of the difference comes from ApplyDeposit
. Even then, I think ApplyDeposit
could be reused across different forks by using conditional logic.
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 creates circular dependencies if i put it in the blocks package
Co-authored-by: terence <terence@prysmaticlabs.com>
@@ -180,6 +180,7 @@ func TestModifiedE2E(t *testing.T) { | |||
|
|||
func TestLoadConfigFile(t *testing.T) { | |||
t.Run("mainnet", func(t *testing.T) { | |||
t.Skip("TODO: add back in after all spec test features are in.") |
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.
not sure if #14509 solves this one
What type of PR is this?
Feature
What does this PR do? Why is it needed?
implements ethereum/consensus-specs#3818
Which issues(s) does this PR fix?
Fixes #
Other notes for review
Acknowledgements