-
Notifications
You must be signed in to change notification settings - Fork 363
Make builders non-validating staked actors #10261
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
Make builders non-validating staked actors #10261
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
zilm13
left a comment
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, just few nits
...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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
...in/java/tech/pegasys/teku/spec/logic/versions/gloas/withdrawals/WithdrawalsHelpersGloas.java
Show resolved
Hide resolved
9ed3f94 to
fe759c4
Compare
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
| stateGloas.getNextWithdrawalBuilderIndex().plus(processedBuildersSweepCount); | ||
| final UInt64 nextBuilderIndex = nextIndex.mod(stateGloas.getBuilders().size()); | ||
| stateGloas.setNextWithdrawalBuilderIndex(nextBuilderIndex); | ||
| } |
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.
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
mehdi-aouadi
left a comment
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
Just few nits
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.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
| final UInt64 amount) { | ||
| return new Builder( | ||
| pubkey, | ||
| withdrawalCredentials.get(0), |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.