feat: pick fixes from zkevm_migration_support-0_7_X (#1304) #1307
Conversation
- Fix estimatedSize that must consided metadata as a Hash, not the
length of bytes
- Add a new RPC end-point for aggsender `aggsender_triggerCertitificate`
to force a trigger certificate:
This is a useful feature for testing or event can be used in production
to don't wait a epoch to force a trigger
```
curl -X POST http://localhost:5576/ "Content-Type: application/json" -d '{"method":"aggsender_triggerCertitificate", "params":[], "id":1}'
```
- #1296
| r.mutex.Lock() | ||
| defer r.mutex.Unlock() |
There was a problem hiding this comment.
What are we trying to protect with this mutex? If it is the channel, it should be thread-safe by default (and also it is a bit strange setting it to nil in case of cancellation)...
We should only take care not to write into already closed channel in the ForceTriggerEvent (which if necessary we can track using some field of atomic.Bool value).
There was a problem hiding this comment.
I agree, but I added the mutex because the tests where complaining about "data race".. I think that even the channel is thread-safe is not the member of the object 'ch' that requires to be protected to access it.
I remove the unneeded mutex
There was a problem hiding this comment.
The pattern with using mutex together with channels is not necessary AFAIK.
I have tried removing the mutex field altogether and I cannot see any data race warnings in the output.
stefan@Stefan:~/go/src/Polygon/aggkit$ go test -race -count=20 github.com/agglayer/aggkit/aggsender
ok github.com/agglayer/aggkit/aggsender 56.281sTherefore I'm still suggesting to remove the mutex field. WDYT?
There was a problem hiding this comment.
You're right is no longer complaining about data race!
How do you suggest to fix this code to guarantee that after if r.ch==nil is not modified the value until line r.ch <-...?
func (r *preconfTrigger) ForceTriggerEvent() {
blockNumber, err := r.l2BridgeSync.GetLastProcessedBlock(context.Background())
if err != nil {
r.log.Errorf("ForceTriggerEvent: Failed to get last processed block: %v", err)
return
}
r.mutex.Lock()
defer r.mutex.Unlock()
if r.ch == nil {
return
}
r.ch <- aggkitsync.Block{Num: blockNumber}
}
There was a problem hiding this comment.
You can check if the channel is closed, before trying to write to it, which is if I'm not mistaken enough to protect yourself from writing into closed channel
_, ok := <-ch
if !ok {
// Channel is closed and all buffered values have been received.
fmt.Println("Channel is closed!")
}There was a problem hiding this comment.
But if it's closed beetween the check and sending data?
check_channel()
# other gorotuine close channel
r.ch <- myData
I still think that we are not protected against that, but anyway, I have removed the mutex! cdab031
|
joanestebanr
deleted the
feat/cherrypick-1304-zkevm_migration_support-0_7_X
branch
aggsender_triggerCertitificateto force a trigger certificate:This is a useful feature for testing or event can be used in production to don't wait a epoch to force a trigger
🔗 Related PRs
📝 Notes