Skip to content

builder demotion for block validation errors (#283)#315

Open
tosynthegeek wants to merge 1 commit intogattaca-com:developfrom
tosynthegeek:develop
Open

builder demotion for block validation errors (#283)#315
tosynthegeek wants to merge 1 commit intogattaca-com:developfrom
tosynthegeek:develop

Conversation

@tosynthegeek
Copy link
Contributor

This PR addresses issue #283 by implementing builder demotion for BlockValidationFailed errors (e.g., block hash mismatch). The changes ensure that the builder for the invalid block is demoted once per incident, using both cache (local_cache.demote_builder) and database (db.db_demote_builder) for immediate and persistent effects, respectively.

Changes

  • Added builder_pubkey: Option<BlsPublicKeyBytes> to GetPayloadResultData to pass the builder's public key from bid_sorter.get_builder_pub_key.
  • Modified handle_get_payload to retrieve builder_pubkey from ForkState using parent_hash.
  • Updated ProposerApi::_get_payload to demote builders on BlockValidationFailed errors after MultiBeaconClient::publish_block.

@tosynthegeek tosynthegeek changed the title Builder Demotion for Block Validation Errors (#283) builder demotion for block validation errors (#283) Oct 24, 2025
self.forks.get(parent_hash).and_then(|s| s.curr_bid.as_ref().map(|b| b.1.block_hash))
}

pub fn get_builder_pub_key(&self, parent_hash: &B256) -> Option<BlsPublicKeyBytes> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the builder pubkey should be already in the "local" payload we have in the payload entry, if not let's add it there instead of getting it from the bid sorter

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the builder pubkey should be already in the "local" payload we have in the payload entry, if not let's add it there instead of getting it from the bid sorter

Okay I will add builder_pubkey to PayloadEntry and pass it through to handle_get_payload, but since new_gossip payloads may not include a builder key, I may have to keep it as optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the builder pubkey should be already in the "local" payload we have in the payload entry, if not let's add it there instead of getting it from the bid sorter

Okay I will add builder_pubkey to PayloadEntry and pass it through to handle_get_payload, but since new_gossip payloads may not include a builder key, I may have to keep it as optional

@ltitanb please let me know

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes sounds good

pub to_publish: VersionedSignedProposal,
pub trace: GetPayloadTrace,
pub fork: ForkName,
pub builder_pubkey: Option<BlsPublicKeyBytes>,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why optional?

Comment on lines 319 to 324
error!(
%builder_pubkey,
%err,
%block_hash_clone,
"failed to demote builder in database! Pausing all optimistic submissions"
);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you are not really pausing submission right? this is a copy of the demotion logic in handle_simulation_result however id rather we keep the demotion in one place. Let's make a new event , "BuilderDemotion" to be sent to the auctioneer , and refactor the existing logic to be used by both that event and the handle_simulation_result

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you are not really pausing submission right? this is a copy of the demotion logic in handle_simulation_result however id rather we keep the demotion in one place. Let's make a new event , "BuilderDemotion" to be sent to the auctioneer , and refactor the existing logic to be used by both that event and the handle_simulation_result

Make sense, I will resume this once I get clarification from the first review

@tosynthegeek tosynthegeek force-pushed the develop branch 2 times, most recently from dd6c038 to f9b3ff9 Compare November 6, 2025 22:58
@tosynthegeek
Copy link
Contributor Author

@ltitanb I’ve pushed the latest changes for this, please take a look when you can.

@tosynthegeek
Copy link
Contributor Author

Hello @ltitanb I saw that my PR now has merge conflicts. Before I spend time resolving them, I wanted to check if this PR is still needed or if there are any changes required on my side. Please let me know how you'd like me to proceed. Thanks!

@ltitanb
Copy link
Collaborator

ltitanb commented Nov 30, 2025

hey @tosynthegeek , yes apologies for the delay! If you could please resolve the branch conflicts then me and @Owen66 will review!

- Introduced `BuilderDemotion` event to trigger when `BeaconClientError::BlockValidationFailed` occurs
- Added `send_event` method to `AuctioneerHandle` for safe event dispatch using private auctioneer sender
- Used `builder_pubkey` from payload bid data to identify the demoted builder
- Implemented `handle_builder_demotion` in `Context` to process demotion events
@tosynthegeek
Copy link
Contributor Author

hey @tosynthegeek , yes apologies for the delay! If you could please resolve the branch conflicts then me and @Owen66 will review!

Thanks for the update @ltitanb, I’ve resolved all merge conflicts and pushed the updated changes. Please let me know if anything else is needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants