-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix: limit builder submissions by name instead of pubkey #3241
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: develop
Are you sure you want to change the base?
Conversation
|
Currently, each builder can have at most 6 addresses, each address can bid twice for each block at most. 2 feedbacks:
|
|
@zzzckck thank you for response 🔧 Assumption Behind the Proposal This isn’t malicious — just rational behavior under the new rules: since builders are only competing against their own previous bids, they’ll send a new block as long as it shows any improvement. ✅ Feedback 1: Limit from 12 to 6 ✅ Feedback 2: Need more time |
|
If this limitation is going to be enforced, we suggest first updating the configuration format used by the Good Will Alliance. This would simplify the implementation and make the enforcement logic much cleaner in code. |
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.
Pull Request Overview
This PR changes the bid submission limiting mechanism from builder address-based to builder name-based for more flexible configuration. The change allows for better grouping of builders that may use different addresses but share the same name or domain.
Key Changes:
- Extended
BuilderConfigstruct with optionalNamefield and smart name extraction from URL domains - Updated bid limiting logic to use builder names instead of addresses in
bidSimulator - Added comprehensive test coverage for name extraction and configuration handling
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
miner/minerconfig/config.go |
Adds Name field to BuilderConfig, implements name extraction logic and getter methods |
miner/minerconfig/config_test.go |
Comprehensive test suite covering name extraction, config methods, and edge cases |
miner/bid_simulator.go |
Updates bid limiting to use builder names instead of addresses with dual tracking system |
| pendingMu sync.RWMutex | ||
| pending map[uint64]map[common.Address]map[common.Hash]struct{} // blockNumber -> builder -> bidHash -> struct{} | ||
| pendingByName map[uint64]map[string]map[common.Hash]struct{} // blockNumber -> builderName -> bidHash -> struct{} |
Copilot
AI
Jul 28, 2025
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.
The code now maintains two separate pending maps (pending and pendingByName) that track the same data but with different keys. This duplication increases memory usage and complexity. Consider consolidating into a single data structure or clearly documenting why both are necessary.
| pendingMu sync.RWMutex | |
| pending map[uint64]map[common.Address]map[common.Hash]struct{} // blockNumber -> builder -> bidHash -> struct{} | |
| pendingByName map[uint64]map[string]map[common.Hash]struct{} // blockNumber -> builderName -> bidHash -> struct{} | |
| pendingMu sync.RWMutex | |
| pending map[uint64]map[builderKey]map[common.Hash]struct{} // blockNumber -> builderKey -> bidHash -> struct{} | |
| // builderKey is a composite key containing both common.Address and builderName. |
miner/bid_simulator.go
Outdated
|
|
||
| b.pending[blockNumber][builder][bidHash] = struct{}{} | ||
|
|
||
| // Also add to pendingByName | ||
| builderName := b.getBuilderName(builder) |
Copilot
AI
Jul 28, 2025
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.
The code assumes that b.pending[blockNumber][builder] exists, but there's no guarantee that the nested maps have been initialized. This will cause a panic if the maps don't exist. The same initialization pattern used for pendingByName should be applied here.
| b.pending[blockNumber][builder][bidHash] = struct{}{} | |
| // Also add to pendingByName | |
| builderName := b.getBuilderName(builder) | |
| // Ensure nested maps are initialized | |
| if _, ok := b.pending[blockNumber]; !ok { | |
| b.pending[blockNumber] = make(map[common.Address]map[common.Hash]struct{}) | |
| } | |
| if _, ok := b.pending[blockNumber][builder]; !ok { | |
| b.pending[blockNumber][builder] = make(map[common.Hash]struct{}) | |
| } | |
| b.pending[blockNumber][builder][bidHash] = struct{}{} | |
| // Also add to pendingByName | |
| builderName := b.getBuilderName(builder) | |
| if _, ok := b.pendingByName[blockNumber]; !ok { | |
| b.pendingByName[blockNumber] = make(map[string]map[common.Hash]struct{}) | |
| } | |
| if _, ok := b.pendingByName[blockNumber][builderName]; !ok { | |
| b.pendingByName[blockNumber][builderName] = make(map[common.Hash]struct{}) | |
| } |
| // For invalid formats, return the original URL | ||
| return url |
Copilot
AI
Jul 28, 2025
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.
When the URL format is invalid, returning the original URL as the domain suffix could lead to unexpected behavior. Consider returning a more descriptive fallback value or logging a warning about the invalid URL format.
| // For invalid formats, return the original URL | |
| return url | |
| // For invalid formats, log a warning and return a descriptive fallback value | |
| log.Warn("Invalid URL format encountered in extractDomainSuffix", "url", url) | |
| return "invalid" |
| } | ||
|
|
||
| if len(b.pendingByName[blockNumber][builderName]) >= int(b.maxBidsPerBuilder) { | ||
| return fmt.Errorf("too many bids: exceeded limit of %d bids per builder name (%s) per block", b.maxBidsPerBuilder, builderName) |
Copilot
AI
Jul 28, 2025
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.
[nitpick] The error message includes the builder name, which could potentially expose sensitive information in logs. Consider whether the builder name should be included in error messages or if a sanitized version should be used.
Template for builders [Name of Builder] |
Summary
Change bid submission limit mechanism from builder address (pubkey) to builder name for more flexible configuration.
Problem
The original bid limiting mechanism was based on builder address, but in some scenarios, the same builder might use different addresses, or we want to group limits by builder name.
Solution
Namefield to support explicit builder name configurationChanges
1. Config structure update
2. New methods
GetBuilderName(): Get builder name, prioritize configured Name, otherwise extract from URLextractDomainSuffix(): Extract builder name from URL domain3. Limit logic adjustment
maxBidsPerBuilderconfig parameterConfig example
Backward compatibility
maxBidsPerBuilderconfig parameterTesting
Impact