Make builders non-validating staked actors#10261
Make builders non-validating staked actors#10261StefanBratanov merged 21 commits intoConsensys:masterfrom
Conversation
...in/java/tech/pegasys/teku/spec/logic/versions/gloas/withdrawals/WithdrawalsHelpersGloas.java
Outdated
Show resolved
Hide resolved
...ava/tech/pegasys/teku/spec/logic/versions/capella/withdrawals/WithdrawalsHelpersCapella.java
Show resolved
Hide resolved
...main/java/tech/pegasys/teku/spec/logic/versions/gloas/helpers/BeaconStateAccessorsGloas.java
Outdated
Show resolved
Hide resolved
.../main/java/tech/pegasys/teku/spec/logic/versions/gloas/helpers/BeaconStateMutatorsGloas.java
Show resolved
Hide resolved
...ch/pegasys/teku/spec/logic/versions/phase0/operations/validation/VoluntaryExitValidator.java
Outdated
Show resolved
Hide resolved
...main/java/tech/pegasys/teku/statetransition/execution/DefaultExecutionPayloadBidManager.java
Outdated
Show resolved
Hide resolved
|
Thanks a lot @jtraglia actually all reference tests should be passing now after the last 3 commits (apart from few minimal failing tests) |
...ava/tech/pegasys/teku/spec/logic/versions/electra/withdrawals/WithdrawalsHelpersElectra.java
Show resolved
Hide resolved
...a/tech/pegasys/teku/spec/logic/versions/gloas/execution/ExecutionRequestsProcessorGloas.java
Show resolved
Hide resolved
.../spec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/helpers/PredicatesGloas.java
Show resolved
Hide resolved
...m/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/versions/gloas/Builder.java
Outdated
Show resolved
Hide resolved
...in/java/tech/pegasys/teku/spec/logic/versions/gloas/withdrawals/WithdrawalsHelpersGloas.java
Show resolved
Hide resolved
...ava/tech/pegasys/teku/spec/logic/versions/capella/withdrawals/WithdrawalsHelpersCapella.java
Show resolved
Hide resolved
...m/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/versions/gloas/Builder.java
Outdated
Show resolved
Hide resolved
ethereum/spec/src/testFixtures/java/tech/pegasys/teku/spec/util/BuilderBuilder.java
Outdated
Show resolved
Hide resolved
...main/java/tech/pegasys/teku/spec/logic/versions/gloas/helpers/BeaconStateAccessorsGloas.java
Show resolved
Hide resolved
...va/tech/pegasys/teku/spec/logic/versions/gloas/execution/ExecutionPayloadProcessorGloas.java
Outdated
Show resolved
Hide resolved
...spec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/helpers/MiscHelpersGloas.java
Show resolved
Hide resolved
31349a2 to
61f8099
Compare
...egasys/teku/spec/logic/versions/gloas/operations/validation/VoluntaryExitValidatorGloas.java
Show resolved
Hide resolved
...m/spec/src/main/java/tech/pegasys/teku/spec/datastructures/state/versions/gloas/Builder.java
Show resolved
Hide resolved
...va/tech/pegasys/teku/spec/logic/versions/gloas/execution/ExecutionPayloadProcessorGloas.java
Outdated
Show resolved
Hide resolved
| stateGloas.getNextWithdrawalBuilderIndex().plus(processedBuildersSweepCount); | ||
| final UInt64 nextBuilderIndex = nextIndex.mod(stateGloas.getBuilders().size()); | ||
| stateGloas.setNextWithdrawalBuilderIndex(nextBuilderIndex); | ||
| } |
There was a problem hiding this comment.
Builder index corrupts next withdrawal validator index calculation
High Severity
WithdrawalsHelpersGloas does not override updateNextWithdrawalValidatorIndex, inheriting the Capella implementation that assumes all withdrawals have real validator indices. In Gloas, builder withdrawals use convertBuilderIndexToValidatorIndex which sets BUILDER_INDEX_FLAG (2^40). When the withdrawal list is full and the last withdrawal is from a builder, withdrawals.getLast().getValidatorIndex() returns a large value (e.g., 2^40 + builderIndex). Computing .mod(state.getValidators().size()) on this produces an arbitrary position, causing validators to be skipped or processed out of order in subsequent withdrawal sweeps.
Additional Locations (1)
There was a problem hiding this comment.
Fixed here, will be included in the next release:
...pec/src/main/java/tech/pegasys/teku/spec/logic/versions/gloas/block/BlockProcessorGloas.java
Show resolved
Hide resolved
8676609 to
a138da7
Compare
...in/java/tech/pegasys/teku/spec/logic/versions/gloas/withdrawals/WithdrawalsHelpersGloas.java
Show resolved
Hide resolved
...in/java/tech/pegasys/teku/spec/logic/versions/gloas/withdrawals/WithdrawalsHelpersGloas.java
Show resolved
Hide resolved
b04c2a7 to
12d53e7
Compare
...in/java/tech/pegasys/teku/spec/logic/versions/gloas/withdrawals/WithdrawalsHelpersGloas.java
Show resolved
Hide resolved
9ed3f94 to
fe759c4
Compare
| stateGloas.getNextWithdrawalBuilderIndex().plus(processedBuildersSweepCount); | ||
| final UInt64 nextBuilderIndex = nextIndex.mod(stateGloas.getBuilders().size()); | ||
| stateGloas.setNextWithdrawalBuilderIndex(nextBuilderIndex); | ||
| } |
There was a problem hiding this comment.
Missing override for validator sweep index with builder withdrawals
High Severity
WithdrawalsHelpersGloas inherits updateNextWithdrawalValidatorIndex from Capella without override. When the withdrawal limit is reached during builder withdrawals (before validator sweep), the last withdrawal has a validatorIndex with BUILDER_INDEX_FLAG (2^40) set. The inherited method computes withdrawals.getLast().getValidatorIndex().plus(1).mod(validators.size()), producing an incorrect next validator sweep index since the builder index is astronomically larger than the validator count.
Additional Locations (1)
...eum/spec/src/main/java/tech/pegasys/teku/spec/logic/common/block/AbstractBlockProcessor.java
Show resolved
Hide resolved
...ch/pegasys/teku/spec/logic/versions/electra/execution/ExecutionRequestsProcessorElectra.java
Outdated
Show resolved
Hide resolved
.../main/java/tech/pegasys/teku/spec/logic/versions/gloas/forktransition/GloasStateUpgrade.java
Show resolved
Hide resolved
| final UInt64 amount) { | ||
| return new Builder( | ||
| pubkey, | ||
| withdrawalCredentials.get(0), |
There was a problem hiding this comment.
Signed byte to int conversion fails for values >= 128
Low Severity
In getBuilderFromDeposit, withdrawalCredentials.get(0) returns a Java signed byte which is automatically promoted to a signed int when passed to the Builder constructor. For byte values >= 0x80 (128), this produces a negative int (e.g., 0x80 → -128), which will fail the checkArgument(value >= 0 && value <= 255) validation in SszByte.asUInt8. The getter Builder.getVersion() already uses ByteUtil.toUnsignedInt for correct unsigned conversion, but the setter path doesn't.
| protected int processBuilderWithdrawals( | ||
| final BeaconState state, final List<Withdrawal> withdrawals) { | ||
| UInt64 withdrawalIndex = getNextWithdrawalIndex(state, withdrawals); | ||
|
|
There was a problem hiding this comment.
Missing override causes wrong next validator index with builder withdrawals
Medium Severity
WithdrawalsHelpersGloas doesn't override updateNextWithdrawalValidatorIndex. The inherited Capella implementation assumes when withdrawals.size() == maxWithdrawals, the last withdrawal is from validator sweep and uses withdrawals.getLast().getValidatorIndex() to set the next index. In Gloas, if builder withdrawals fill the list first, the last withdrawal has BUILDER_INDEX_FLAG set (a value around 2^40), causing getValidatorIndex().plus(1).mod(validatorCount) to produce an essentially arbitrary validator index for the next sweep.
PR Description
As per ethereum/consensus-specs#4788
Also enabling back al the Gloas consensus-specs reference tests from: https://github.com/ethereum/consensus-specs/releases/tag/v1.7.0-alpha.1
The major change TLDR is that we track builders in the state separate from validators and process their deposits/withdrawals separate
Fixed Issue(s)
N/A
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Implements builders as non-validating staked actors and aligns with consensus-specs v1.7.0-alpha.1.
BuilderSSZ container and registry inBeaconStatewith caches (buildersPubKeys,builderIndexCache); addnext_withdrawal_builder_indexandpayload_expected_withdrawalsBuilderIndex(incl.BUILDER_INDEX_SELF_BUILD), updateExecutionPayloadBid/Envelopeand signature checks to use builder pubkeys; special handling for self-buildsnext_withdrawal_*indices; extendExpectedWithdrawalsinitiateBuilderExit); map builder/validator indices via new helpersBuilder.versionuint8; removewithdrawable_epochfromBuilderPendingWithdrawal)ssz_*, fork-choice tests; update test fixtures/utilities to useBUILDER_INDEX_SELF_BUILD; bump specrefs to v1.7.0-alpha.1Written by Cursor Bugbot for commit cbd4670. This will update automatically on new commits. Configure here.