-
Notifications
You must be signed in to change notification settings - Fork 360
10175 execution payload bid gloas gossip #10198
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
base: master
Are you sure you want to change the base?
10175 execution payload bid gloas gossip #10198
Conversation
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Outdated
Show resolved
Hide resolved
...ition/src/main/java/tech/pegasys/teku/statetransition/validation/GossipValidationHelper.java
Outdated
Show resolved
Hide resolved
...main/java/tech/pegasys/teku/statetransition/execution/DefaultExecutionPayloadBidManager.java
Fixed
Show fixed
Hide fixed
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Outdated
Show resolved
Hide resolved
44d6922 to
c590df1
Compare
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Outdated
Show resolved
Hide resolved
...main/java/tech/pegasys/teku/statetransition/execution/DefaultExecutionPayloadBidManager.java
Show resolved
Hide resolved
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Show resolved
Hide resolved
2d10e2d to
f006907
Compare
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Outdated
Show resolved
Hide resolved
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Show resolved
Hide resolved
...ition/src/main/java/tech/pegasys/teku/statetransition/validation/GossipValidationHelper.java
Show resolved
Hide resolved
...in/java/tech/pegasys/teku/statetransition/validation/ExecutionPayloadBidGossipValidator.java
Outdated
Show resolved
Hide resolved
| public boolean isValidBuilderIndex( | ||
| final UInt64 builderIndex, final BeaconState state, final UInt64 slot) { | ||
| if (builderIndex.isGreaterThanOrEqualTo(state.getValidators().size()) | ||
| || builderIndex.longValue() < 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.
intValue, as we use it as int right after that?
| final int index = builderIndex.intValue(); | ||
| final Validator builder = state.getValidators().get(index); | ||
| final boolean isActiveBuilder = | ||
| spec.getActiveValidatorIndices(state, spec.computeEpochAtSlot(slot)).contains(index); |
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.
Look heavy for this, we already have Validator structure for this specific state. Can we use predicates.isActiveValidator for the check?
| } | ||
|
|
||
| public boolean isBlockHashKnown(final Bytes32 blockHash, final Bytes32 blockRoot) { | ||
| // TODO-GLOAS check this logic. We might need to check the block hash existence in the store |
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 we have the same in Store, we use both ForkChoiceStrategy and Store for such checks in validation helpers like it's interchangeable.
Spec is literally saying for this check: bid.parent_block_hash is the block hash of a known execution payload in fork choice., so you could remove TODO
| public static final int VALID_PAYLOAD_ATTESTATION_SET_SIZE = 512 * 2; | ||
| // Target holding two slots worth of aggregators (16 aggregators, 64 committees and 2 slots) | ||
| public static final int VALID_AGGREGATE_SET_SIZE = 16 * 64 * 2; | ||
| // Target 2 different attestation data (aggregators normally agree) for two slots |
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.
Comment is from VALID_ATTESTATION_DATA_SET_SIZE probably, you've inserted in the middle
| final SigningRootUtil signingRootUtil; | ||
|
|
||
| private final Set<BuilderIndexAndSlot> seenExecutionPayloadBids = | ||
| LimitedSet.createSynchronized(SEEN_EXECUTION_PAYLOAD_BID_SET_SIZE); |
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 it's better to base this one number of slots rather than some random big number always occupying memory. Maybe Map<Slot, Set<UInt64>> for 10 slots could be a better for memory? You will also need synchronized internal sets.
What do you think?
| final Optional<UInt64> maybeParentBlockSlot = | ||
| gossipValidationHelper.getSlotForBlockRoot(bid.getParentBlockRoot()); | ||
| if (maybeParentBlockSlot.isEmpty()) { | ||
| LOG.trace("Bid's parent block does not exist. It will be saved for future processing"); |
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.
we don't expect this with previous check passed, but anyway, let's add blockRoot to the log string
| LOG.trace( | ||
| "State for block root {} and slot {} is unavailable.", | ||
| bid.getParentBlockRoot(), | ||
| bid.getSlot()); |
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.
parentBlockSlot?
| * [REJECT] bid.builder_index is a valid, active, and non-slashed builder index. | ||
| */ | ||
|
|
||
| final UInt64 buildrIndex = bid.getBuilderIndex(); |
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.
builderIndex
| LOG.trace("Invalid payload execution bid signature"); | ||
| return reject("Invalid payload execution bid signature"); | ||
| } | ||
| highestBids.merge(bidValueKey, bid.getValue(), UInt64::max); |
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.
why? I don't understand. If it's really needed, we need a comment here
| assertThatSafeFuture(bidValidator.validate(signedBid)).isCompletedWithValue(ACCEPT); | ||
|
|
||
| // a same value bid from a different builder with the same parent block hash and slot | ||
| final SignedExecutionPayloadBid lowerValueBid = |
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.
sameValueBid?
PR Description
Add Gloas
execution_payload_bidgossip rulesFixed Issue(s)
#10175
Documentation
doc-change-requiredlabel to this PR if updates are required.Changelog
Note
Adds Gloas execution_payload_bid gossip validation with helper utilities and integrates it into the bid manager and node, plus constants, test utilities, and updated tests/fixtures.
ExecutionPayloadBidGossipValidatorwith checks (slot window, parent hash/root known, builder validity/credentials/balance, non-zeroexecution_paymentreject, signature, highest-bid tracking) using LRU caches (SEEN_EXECUTION_PAYLOAD_BID_SET_SIZE,HIGHEST_BID_SET_SIZE).DefaultExecutionPayloadBidManager.validateAndAddBid.BeaconChainController.initExecutionPayloadBidManager().GossipValidationHelper(current/next slot, builder index/credentials/balance, block-hash known, signature helpers).MiscHelpersGloas.hasBuilderWithdrawalCredential.SEEN_EXECUTION_PAYLOAD_BID_SET_SIZE,HIGHEST_BID_SET_SIZEinConstants.DataStructureUtilto generateExecutionPayloadBid/SignedExecutionPayloadBidwith explicit fields/variants.signed_execution_payload_bidvalues to match changes.Written by Cursor Bugbot for commit 100dec8. This will update automatically on new commits. Configure here.