WIP - Native Staking post Pectra upgrade #2559
Conversation
Added native staking value transitions diagram
Started beacon proofs unit tests
…ain pendingDeposits
proveBalances does old doAccounting first
Upgraded @nomiclabs/hardhat-ethers
Added more Natspec
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2559 +/- ##
==========================================
+ Coverage 35.13% 40.07% +4.94%
==========================================
Files 111 121 +10
Lines 5303 5664 +361
Branches 1405 1501 +96
==========================================
+ Hits 1863 2270 +407
+ Misses 3438 3393 -45
+ Partials 2 1 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| consolidationTargetStrategy = address(0); | ||
|
|
||
| // Unpause the strategy to allow further operations | ||
| _unpause(); |
There was a problem hiding this comment.
I think we can add a separate consolidation strategy to manage the accounting during the consolidation. That will make changes to the existing strategies minimal and move most of the consolidation verification logic out of the new strategy.
I'll experiment on a branch
|
|
||
| // TODO what if the last validator was exited rather than consolidated? | ||
| // slither-disable-start reentrancy-no-eth | ||
| function verifyConsolidation( |
There was a problem hiding this comment.
Unfortunately the consolidation queue on the beacon chain does not contain the slot the request was made like the pending deposit queue. This means we can't do similar logic to verifying deposits of looking at the front of the queue and working out if our deposit must have been processed or not.
class PendingConsolidation(Container):
source_index: ValidatorIndex
target_index: ValidatorIndexhttps://ethereum.github.io/consensus-specs/specs/electra/beacon-chain/#pendingconsolidation
class PendingDeposit(Container):
pubkey: BLSPubkey
withdrawal_credentials: Bytes32
amount: Gwei
signature: BLSSignature
slot: Slothttps://ethereum.github.io/consensus-specs/specs/electra/beacon-chain/#pendingdeposit
| // for each validator | ||
| for (uint256 i = 0; i < verifiedValidatorsCount; ++i) { | ||
| // for each validator in reserve order so we can pop off exited validators at the end | ||
| for (uint256 i = verifiedValidatorsCount; i > 0; ) { |
There was a problem hiding this comment.
you could also put the whole for statement in the same line:
for (uint256 i = verifiedValidatorsCount - 1; i > 0; i--) {
The verifiedValidatorsCount will always be >= 1
There was a problem hiding this comment.
Ah, yes, because of the earlier if (verifiedValidatorsCount > 0) {
Thanks
There was a problem hiding this comment.
Maybe not. The last loop when i = 0 is skipped.
There was a problem hiding this comment.
yeah true should be:
for (uint256 i = verifiedValidatorsCount - 1; i >= 0; i--) {
|
|
||
| // Move the last validator that has already been verified to the current index. | ||
| // There's an extra SSTORE if i is the last active validator but that's fine, | ||
| // It's not a common case and the code is simpler this way. | ||
| verifiedValidators[i] = verifiedValidators[ | ||
| verifiedValidatorsCount |
There was a problem hiding this comment.
do we need a comment explaining that this would typically be verifiedValidatorsCount - 1 but is without -1 because the verifiedValidatorsCount has been subtracted from 1 step before?
| bytes calldata targetPubKey, | ||
| address targetStakingStrategy | ||
| ) external nonReentrant whenNotPaused onlyRegistrator { | ||
| require( |
There was a problem hiding this comment.
should we check that the consolidationCount == 0. I know that the whenNotPaused will prevent 2 consolidations from happening at the same time, but as Daniel mentioned other things can pause/unpause the strategy. Would be cool to have an extra check.
| view | ||
| returns (bytes32 parentRoot) | ||
| { | ||
| // Commented out the following checks as it makes unit and fork testing very difficult. |
There was a problem hiding this comment.
🟡 Is this a temporary comment out? If so, seems like we need a way to ensure it gets turned on for production.
There was a problem hiding this comment.
I dug into this today. We can safely remove the timestamp too old checks as the BeaconRoots contract from EIP-4788 will revert if the stored timestamp does not match the timestamp being requested.
FYI, I have Hardhat task that will get the beacon block root for a particular block. For example, here's the first block from the Denub release which included EIP-4788
npx hardhat beaconRoot --block 19426587 --mainnet true
Block 19426587 has parent beacon block root 0xb35bb80bc5f4e3d8f19b62f6274add24dca334db242546c3024403027aaf6412
The hardhat task can get blocks older than 8,191 blocks as it calls with a blockTag one after the block we are interested in
async function beaconRoot({ block, mainnet }) {
// Either use mainnet or local fork to get the block timestamp
const provider = mainnet ? getProvider() : ethers.provider;
// Get timestamp of the block
const fetchedBlock = await provider.getBlock(block);
if (fetchedBlock == null) throw Error(`Block ${block} not found`);
const { timestamp } = fetchedBlock;
log(`Block ${block} has timestamp ${timestamp}`);
const data = defaultAbiCoder.encode(["uint256"], [timestamp]);
log(`Encoded timestamp data: ${data}`);
const root = await provider.call(
{
to: addresses.mainnet.beaconRoots,
data,
},
block + 1 // blockTag
);
console.log(`Block ${block} has parent beacon block root ${root}`);
return { root, timestamp };
}
| bytes calldata slotProof, | ||
| bytes calldata blockProof | ||
| ) external returns (bytes32 blockRoot) { | ||
| require(_blockToSlot[blockNumber] == 0, "Block already mapped"); |
There was a problem hiding this comment.
This code could double run for slot zero, since the require statement would not trigger.
(Don't know if that actually matters)
| bytes calldata validatorPubKeyProof, | ||
| bytes32 balancesLeaf, | ||
| bytes calldata validatorBalanceProof | ||
| ) external onlyRegistrator { |
There was a problem hiding this comment.
It would seem like clean symmetrical code to have this also check the contract is paused before allowing a call to it. (The ValidatorRegistrar does do this this check)
Developer Security ReviewEasy ChecksAuthentication
Ethereum
Cryptographic code
Gas problems
Black magic
Overflow
License
Proxy
Events
Medium ChecksRounding and casts
Dependencies
External calls
Tests
Deploy
Strategy SpecificRemove this section if the code being reviewed is not a strategy. Strategy checks
Downstream
ThinkingLogicAre there bugs in the logic?
Deployment ConsiderationsAre there things that must be done on deploy, or in the wider ecosystem for this code to work. Are they done? Internal State
Does this code do that? AttackWhat could the impacts of code failure in this code be. What conditions could cause this code to fail if they were not true. Does this code successfully block all attacks. FlavorCould this code be simpler? Could this code be less vulnerable to other code behaving weirdly? |
…l withdrawals work
…ots contract will revert Removed the old deploy of MockBeaconRoots to mainnet
Code Change Checklist
To be completed before internal review begins:
Internal review:
Contracts
The scope of the contracts of this build is
CompoundingStakingSSVStrategyis the new staking contract.CompoundingValidatorManageris inherited byCompoundingStakingSSVStrategyBeaconOraclecalled fromCompoundingValidatorManagerBeaconProofscalled fromCompoundingValidatorManagerThe following libraries have also be built
BeaconRootsBeaconProofsLibPartialWithdrawalBeaconConsolidationMerkleEndianBuild
Keeping the contract under 24Kb has been a problem during development. The following will show the contract size of the compile contracts including
CompoundingStakingSSVStrategy.Testing
Unit Tests
Are main strategy unit tests in
contracts/test/strategies/compoundingSSVStaking.jsThere are also unit tests of the beacon merkle proofs in
contracts/test/beacon/beaconProofs.jsFork Tests
There are no fork tests of the strategy as its hard to mock the beacon chain. But there are fork tests of various beacon chain contracts and libraries. These includes:
Hoodi Testnet
Contracts have been deployed to Hoodi with deployment script
contracts/deploy/hoodi/001_core.jsLocal testing
Testing against a local fork node using real beacon chain data
Deployment