Operations pool for BLS-to-execution-changes #11631
Conversation
# Conflicts: # beacon-chain/rpc/eth/beacon/config_test.go # config/params/config.go # config/params/mainnet_config.go # config/params/minimal_config.go
| } | ||
|
|
||
| // Binary search to check if the index exists in the list of pending objects. | ||
| func existsInList(pending []*ethpb.SignedBLSToExecutionChange, searchingFor types.ValidatorIndex) (bool, int) { |
There was a problem hiding this comment.
I don't allow two objects for the same validator index. Someone could send two messages with the same index and different ETH1 addresses, but there is no guarantee which one will be picked by the block proposer, so I simply disallow it.
There was a problem hiding this comment.
The gossip validation will drop the second object
[IGNORE] The signed_bls_to_execution_change is the first valid signed bls to execution change received for the validator with index signed_bls_to_execution_change.message.validator_index.
But it's good that you are checking here. +1 more defensive
| @@ -0,0 +1,112 @@ | |||
| package blstoexecchanges | |||
There was a problem hiding this comment.
package name looks funny but I dont have a better suggestion, maybe just blstoexec
| sort.Slice(p.pending, func(i, j int) bool { | ||
| return p.pending[i].Message.ValidatorIndex < p.pending[j].Message.ValidatorIndex | ||
| }) |
There was a problem hiding this comment.
This also means that validators with lower indices will have preferential treatment. Validators at 400k range might not be happy
There was a problem hiding this comment.
Newer validators don't need to send this message because they already have an ETH1 withdrawal credential to begin with. I think @potuz will have an answer to how many validators will need to change their credentials. Out of those, I believe preferential treatment of earlier stakers is the right thing to do. Of course we can always randomize it and order by the BLS pubkey.
There was a problem hiding this comment.
On a related note, we have the same ordering for voluntary exits. Whatever we decide on, I think we should keep these two consistent.
There was a problem hiding this comment.
Newer validators don't need to send this message because they already have an ETH1 withdrawal credential
Is this true? They could still send in BLS without knowing ETH1
On a related note, we have the same ordering for voluntary exits.
I'd argue exit is wrong because it's unfair and we should change that
There was a problem hiding this comment.
They could still send in BLS without knowing ETH1
Will need to double check with Potuz, but if this is true then they have ETH1 creds already, and thus any message they send will be ignored because of the v.HasETH1WithdrawalCredential() check.
I'd argue exit is wrong because it's unfair and we should change that
OK, I am convinced that FIFO is better. So in that case the discussion around already having ETH1 creds is irrelevant.
|
|
||
| // PendingBLSToExecChanges returns objects that are ready for inclusion at the given slot. | ||
| // Without returnAll, this method will not return more than the block enforced MaxBlsToExecutionChanges. | ||
| func (p *Pool) PendingBLSToExecChanges(returnAll bool) []*ethpb.SignedBLSToExecutionChange { |
There was a problem hiding this comment.
returnAll is useful for Beacon API.
| // PoolManager maintains pending and seen BLS-to-execution-change objects. | ||
| // This pool is used by proposers to insert BLS-to-execution-change objects into new blocks. | ||
| type PoolManager interface { | ||
| PendingBLSToExecChanges(noLimit bool) []*ethpb.SignedBLSToExecutionChange |
There was a problem hiding this comment.
remove noLimit here or rename in the function implementation.
| return p.pending | ||
| } | ||
| maxChanges := params.BeaconConfig().MaxBlsToExecutionChanges | ||
| pending := make([]*ethpb.SignedBLSToExecutionChange, maxChanges) |
There was a problem hiding this comment.
This makes a slice of the size maxChanges even if p.pending has less elements than this
| pending := make([]*ethpb.SignedBLSToExecutionChange, maxChanges) | ||
| for i, ch := range p.pending { | ||
| pending[i] = ch | ||
| if uint64(len(pending)) == maxChanges { | ||
| break | ||
| } | ||
| } | ||
| return pending |
There was a problem hiding this comment.
Why for loop over the full range instead of simply
| pending := make([]*ethpb.SignedBLSToExecutionChange, maxChanges) | |
| for i, ch := range p.pending { | |
| pending[i] = ch | |
| if uint64(len(pending)) == maxChanges { | |
| break | |
| } | |
| } | |
| return pending | |
| length := Min(maxChanges, len(p.pending)) | |
| pending := make([]*ethpb.SignedBLSToExecutionChange, length) | |
| for i := 0; i < length; i++ { | |
| pending[i] = p.pending[i] | |
| } | |
| return pending |
| if change == nil || | ||
| change.Message == nil || | ||
| len(change.Signature) != fieldparams.BLSSignatureLength || | ||
| len(change.Message.FromBlsPubkey) != fieldparams.BLSPubkeyLength || | ||
| len(change.Message.ToExecutionAddress) != fieldparams.ExecutionAddressLength { | ||
| return | ||
| } |
There was a problem hiding this comment.
Why are we having these checks here? the Beacon Api prevents that we even gossip them if we haven't checked the conditions in process_bls_to_execution_changes.
| } | ||
|
|
||
| // Do we already have a pending object for the validator? | ||
| existsInPending, _ := existsInList(p.pending, change.Message.ValidatorIndex) |
There was a problem hiding this comment.
Same here, this is an explicit condition on the P2P layer. Why would we run this check in the worst possible case (testing existence on a list of an object that we are guaranteed does not exist).
|
|
||
| // Does the validator already have ETH1 withdrawal credentials? | ||
| v, err := state.ValidatorAtIndexReadOnly(change.Message.ValidatorIndex) | ||
| if err != nil || v.HasETH1WithdrawalCredential() { |
|
|
||
| // Insert into pending list and sort. | ||
| p.pending = append(p.pending, change) | ||
| sort.Slice(p.pending, func(i, j int) bool { |
There was a problem hiding this comment.
I suspect this is the discussion you had with @terencechain, I agree that a FIFO would be more appropriate here, but implementing a queue that can be very large (at the fork) can be tricky
| // MarkIncluded is used when an object has been included in a beacon block. Every block seen by this | ||
| // node should call this method to include the object. This will remove the object from | ||
| // the pending objects slice. | ||
| func (p *Pool) MarkIncluded(change *ethpb.SignedBLSToExecutionChange) { |
There was a problem hiding this comment.
How are we going to make this reorg-resistant?
| p.lock.RLock() | ||
| defer p.lock.RUnlock() | ||
|
|
||
| if returnAll { |
There was a problem hiding this comment.
This looks like a bool param that DeepSource will complain, probably better to have two different getters.
| return result | ||
| } | ||
|
|
||
| length := int(math.Min(float64(params.BeaconConfig().MaxBlsToExecutionChanges), float64(p.pending.len))) |
There was a problem hiding this comment.
Is the float conversion really necessary?
There was a problem hiding this comment.
We should create a generic min function in separate PR so that we can really avoid floats. Not a big deal for this one right now though
| length := int(math.Min(float64(params.BeaconConfig().MaxBlsToExecutionChanges), float64(p.pending.len))) | ||
| result := make([]*ethpb.SignedBLSToExecutionChange, length) | ||
| node := p.pending.first | ||
| for i := 0; i < length; i++ { |
There was a problem hiding this comment.
This works, but somehow I think it's safer to make an empty slice and append on a for loop from first until node=last. Either way we need to have a safety check that the linked list wasn't broken or that there's no nil node in the middle.
There was a problem hiding this comment.
Looping until node == last won't work with a nil node in the middle. I will add a node != nil condition.
| node.next.prev = node.prev | ||
| } | ||
| } | ||
| delete(p.m, node.value.Message.ValidatorIndex) |
There was a problem hiding this comment.
I don't think we want to delete here cause then we will add again a change later wouldn't we?
| return result | ||
| } | ||
|
|
||
| length := int(math.Min(float64(params.BeaconConfig().MaxBlsToExecutionChanges), float64(p.pending.len))) |
There was a problem hiding this comment.
We should create a generic min function in separate PR so that we can really avoid floats. Not a big deal for this one right now though
| // This pool is used by proposers to insert BLS-to-execution-change objects into new blocks. | ||
| type PoolManager interface { | ||
| PendingBLSToExecChanges() []*ethpb.SignedBLSToExecutionChange | ||
| BLSToExecChangesForInclusion() []*ethpb.SignedBLSToExecutionChange |
There was a problem hiding this comment.
What's the difference between this and the method above? Pending and awaiting inclusion feel the same to me as a new reader of this code
There was a problem hiding this comment.
// PendingBLSToExecChanges returns all objects from the pool.
// BLSToExecChangesForInclusion returns objects that are ready for inclusion at the given slot.
// This method will not return more than the block enforced MaxBlsToExecutionChanges.
| length := int(math.Min(float64(params.BeaconConfig().MaxBlsToExecutionChanges), float64(p.pending.len))) | ||
| result := make([]*ethpb.SignedBLSToExecutionChange, length) | ||
| node := p.pending.first | ||
| for i := 0; node != nil && i < length; i++ { |
There was a problem hiding this comment.
This loop feels almost the same except for the middle conditional. The functions are very similar and not sure if we should have it as a separate one. If the intention of pending vs. awaiting inclusion is to cap the amounts needed in the latter, perhaps we only need the latter. How would the former be used in the codebase?
There was a problem hiding this comment.
There is an API endpoint that returns all pending SignedBLSToExecutionChange objects. And although I agree these methods are similar, they are both very short, so there is not that much repetition.
| } | ||
|
|
||
| var node *listNode | ||
| if p.pending.first == nil { |
There was a problem hiding this comment.
Could changes list be implemented as a slice that has associated methods rather than a struct? That way you don't need need fields for len, prev, next, etc.
type changesList []*ethpb.BLSToExecChange
func (c changesList) Append()
func (c changesList) Last()and that way you can also use the built-in len() function
| p.lock.Lock() | ||
| defer p.lock.Unlock() | ||
|
|
||
| node := p.m[change.Message.ValidatorIndex] |
There was a problem hiding this comment.
I think we should use something else for the key in this map than just validator index to make reorg resistant as Potuz mentioned...or be aware of block roots
| return | ||
| } | ||
|
|
||
| if node == p.pending.first { |
There was a problem hiding this comment.
A lot of this complexity can be fixed by just using a slice type with methods, I think
| return nil | ||
| } | ||
|
|
||
| delete(p.m, change.Message.ValidatorIndex) |
There was a problem hiding this comment.
We delete the item here because we will need to re-insert objects after a reorg.
A BLS to execution change may be included in a block and then that block is reorged we need to go back and add those entries back to the pool
What type of PR is this?
Feature
What does this PR do? Why is it needed?
Implements a pool for
SignedBLSToExecutionChangeobjects. The pool uses a doubly-linked list internally for memory efficiency. I added a simple doubly-linked list implementation.