-
Notifications
You must be signed in to change notification settings - Fork 839
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
EIP-7002: Validator Exit contract helper and adding exits to created blocks #6883
EIP-7002: Validator Exit contract helper and adding exits to created blocks #6883
Conversation
ced776d
to
53758f0
Compare
…tion payload for engineV4 methods Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
…ocks Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
24d400a
to
7e80b83
Compare
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
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.
Nice testing, PR looks good. I have added some comments.
if (!validateExits(block, body.getExits().get())) { | ||
return false; | ||
} |
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.
Nit:
if (!validateExits(block, body.getExits().get())) { | |
return false; | |
} | |
return validateExits(block, body.getExits().get()); |
/** | ||
* Helper for interacting with the Validator Exit Contract (https://eips.ethereum.org/EIPS/eip-7002) | ||
* | ||
* <p>Please note that this is not the spec-way of interacting with the Validator Exit contract. See | ||
* https://github.com/hyperledger/besu/issues/6918 for more information. | ||
*/ |
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.
/** | |
* Helper for interacting with the Validator Exit Contract (https://eips.ethereum.org/EIPS/eip-7002) | |
* | |
* <p>Please note that this is not the spec-way of interacting with the Validator Exit contract. See | |
* https://github.com/hyperledger/besu/issues/6918 for more information. | |
*/ | |
/** | |
* Helper for interacting with the Validator Exit Contract (<a | |
* href="https://eips.ethereum.org/EIPS/eip-7002">eip-7002</a>) | |
* | |
* <p>TODO: Please note that this is not the spec-way of interacting with the Validator Exit | |
* contract. See <a href="https://github.com/hyperledger/besu/issues/6918">#6918</a> for more | |
* information. | |
*/ |
} | ||
|
||
final List<ValidatorExit> exitsInBlock = block.getBody().getExits().get(); | ||
// TODO Do we need to allow for customization? (e.g. if the value changes in the next fork) |
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.
Suggestion: We could rename it to PrageExitsValidator, and make it a standalone class. If the next fork needs to change this we can easily abstract a class override what we need.
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.
nit: Also, giving the validations, the semantics here are more like a RequiredExitsValidator
than an AllowedExistsValidator
.
@@ -197,6 +197,12 @@ public BlockProcessingResult processBlock( | |||
} | |||
} | |||
|
|||
final ValidatorExitsValidator exitsValidator = protocolSpec.getExitsValidator(); | |||
if (exitsValidator instanceof ValidatorExitsValidator.AllowedExits) { |
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 think that the verification to perform the popping should belong to the validator/processor itself and not the blockprocessor. Maybe an extra method allowExists()?
if (block.getHeader().getExitsRoot().isEmpty()) { | ||
LOG.warn("Block {} must contain exits_root", blockHash); | ||
return false; | ||
} |
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.
This piece should be probably in a headervalidationrules instead of here
final Hash expectedExitsRoot = BodyValidation.exitsRoot(exitsInBlock); | ||
if (!expectedExitsRoot.equals(block.getHeader().getExitsRoot().get())) { | ||
LOG.warn( | ||
"Block {} exits_root does not match expected hash root for exits in block", blockHash); | ||
return false; | ||
} |
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.
s.a
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
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.
LGTM
…blocks (hyperledger#6883) Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com> Signed-off-by: Antonio Mota <antonio.mota@citi.com>
…blocks (hyperledger#6883) Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
…blocks (hyperledger#6883) Signed-off-by: Lucas Saldanha <lucascrsaldanha@gmail.com>
Sorry for the big PR! I tried to keep it more concise but lots of the pieces kinda work together so it was hard to separate them! Hopefully, it isn't too bad to review as a bulk of the changes are adding a parameter to a constructor or something like that... ❤️
This is an experimental implementation of EIP-7002 logic for adding exits from the Validator Exit smart contract into blocks (and including them on the execution payload object of the Engine API).
Summary of changes
fixes #6881 and #6882
Open Questions/Future Improvements
Do we need extra validation onAbstractBlockProcessor
(e.g. check that we do not have more exits than expected). I have a feeling we do not need to as we are running the system logic as part of processing, so any difference between the exits in the block we are validating would be caught by a different exits_root post our processing. But I might be wrong. The same question applies toMainnetBlockBodyValidator. validateBodyLight()
.ValidatorExitContractHelper
behind a protocol schedule setup in case the contract goes through some changes in future upgrades?Engine API changes
Most of the logic was already there, few pieces had to be added (e.g. constructor parameters but the core of the logic hasn't changed). The more important change is how GetPayloadV4 includes the exits in the execution payload.
Validator Exit Contract Helper
This is an initial implementation based on the work done by lightclient on the Validator Exit smart contract (https://github.com/lightclient/7002asm), and also using the bytecode defined in the EIP (https://eips.ethereum.org/EIPS/eip-7002#deployment).
Each exit uses 3 storage slots (each slot has 32 bytes). There is a bit of trickery to ensure they fit in exactly on 3 slots (some right/left padding depending on the field). The detailed diagram of the expected storage can be seen in this code.
UPDATE: contract manipulation is not the best way of implementing this. The smart contract actually has all the logic to handle the system call. All we need to do is implement the mechanism to perform "send a tx" from the system account. I believe we do not have this logic in Besu at the moment, the closest thing I can think of is how we use a fake transaction in the
TransactionSimulator
, but we would need a way of ensuring that changes in contract storage are persisted. See #6918Including exits on blocks
The system call logic has been implemented in a way that manipulates the contract storage as if we were executing the call through the EVM (although no EVM is involved).
This seems to be the expected behaviour for system calls.The heart of this is howAbstractBlockCreator
usesValidatorExitContractHelper.popExitsFromQueue(..)
.