Check BLS changes when requesting from pool #11718
Conversation
| ethpb "github.com/prysmaticlabs/prysm/v3/proto/prysm/v1alpha1" | ||
| "github.com/sirupsen/logrus" | ||
| ) | ||
|
|
||
| // 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() ([]*ethpb.SignedBLSToExecutionChange, error) |
There was a problem hiding this comment.
Shouldn't you apply this for PendingBLSToExecChanges or we just simply care less for that?
There was a problem hiding this comment.
I can see the value in returning the full list, someone may be interested in knowing what are all changes in the node. No one uses this endpoint yet so I am not sure what are the applications yet. Also, performing a linear scan with signature verification in an unbounded list scares me, so I'd rather have the caller making these checks.
There was a problem hiding this comment.
That's fair. I don't know whether beacon API explicitly calls this out. We can add this behavior to the commentary if it doesn't
| if err != nil { | ||
| logrus.Warning("removing invalid BLSToExecutionChange from pool") | ||
| // MarkIncluded removes the invalid change from the pool | ||
| p.lock.RUnlock() |
There was a problem hiding this comment.
These lock/unlock scares me. Any reason to pick this over defer?
There was a problem hiding this comment.
MarkIncluded needs a lock, so we can't defer.
| return nil, err | ||
| } | ||
| _, err = blocks.ValidateBLSToExecutionChange(st, change) | ||
| if err != nil { | ||
| logrus.Warning("removing invalid BLSToExecutionChange from pool") |
There was a problem hiding this comment.
Should we include the err in this warning log?
| // We now verify the signatures in batches | ||
| cSet, err := blocks.BLSChangesSignatureBatch(st, result) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not get BLSToExecutionChanges signatures") |
There was a problem hiding this comment.
Optional: I think you can continue here and check signatures individually
| } | ||
| ok, err := cSet.Verify() | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "could not batch verify BLSToExecutionChanges signatures") |
There was a problem hiding this comment.
Optional: I think you can continue here and check signatures individually
terencechain
added
Blocked
|
Please don't merge it. A few more changes are coming |
potuz
added
Ready For Review
and removed
Blocked
| logrus.WithError(err).Warning("could not batch verify BLSToExecutionChanges signatures") | ||
| } else if ok { | ||
| return result, nil | ||
|
|
* Check BLS changes when requesting from pool * Terence's suggestions * Radek's suggestion Co-authored-by: terencechain <terence@prysmaticlabs.com> Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
This PR checks BLS changes when requesting them from the pool (this is performed when proposing a block). |
It checks the validity of the operation individually (except signatures)
If any is found invalid it's removed from the pool
Then it checks the signatures in batch
If the batch is found invalid it checks signatures individually
If any signature is found invalid it removes it from the pool.
If a change is invalid then we continue scanning until filling the block or exhausting the pool. If a signature is invalid we do not rescan.