Conversation
| for { | ||
| select { | ||
| case <-nc.channel: | ||
| if len(kzgCommitments) == len(nc.indices) { |
There was a problem hiding this comment.
Did you mean to make chanForRoot a map[[32]*blobNotifierChan? As currently written, nc.indices will never be updated by the blob side, because the read from the map makes a copy of the blobNotifierChan struct, so the slice append modifies the local copy. Using references on both sides will fix that.
There was a problem hiding this comment.
for thread safety i think you want also treat the slice length computation as the critical section. or to avoid that lock, and since you aren't looking at the indices and are just checking the length, you could count how many times you read from the channel and change this if statement to len(kzCommitments) == nChannelReads. that would also clean up the writers to the channel, which could be leaking as currently written, because you could delete the blobNotifierChan without reading and unblocking them.
| // for the blocroot `root` is ready in the database | ||
| func (s *Service) SendNewBlobEvent(root [32]byte, index uint64) { | ||
| s.blobNotifier.Lock() | ||
| defer s.blobNotifier.Unlock() |
There was a problem hiding this comment.
you're holding the lock until the read happens, which will deadlock the reader that needs to hold the lock before it gets to the channel read code.
| s.blobNotifier.chanForRoot[root] = nc | ||
| } else { | ||
| if !slice.IsInUint64(index, nc.indices) { | ||
| nc.indices = append(nc.indices, index) |
There was a problem hiding this comment.
this shouldn't be in an else, you'll miss the first index where the map entry is created
There was a problem hiding this comment.
the index was added in that creation as well wasn't it?
There was a problem hiding this comment.
oh right the problem is if it's created in the reader
| func (s *Service) SendNewBlobEvent(root [32]byte, index uint64) { | ||
| s.blobNotifier.Lock() | ||
| var ok bool | ||
| var nc *blobNotifierChan |
There was a problem hiding this comment.
you could get rid of this pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
* Add a new blob channel * fix mock * reset the channel * keep a map of channels * gazelle * do not overwrite map * remove pre-declaration
| var ErrMissingClockSetter = errors.New("blockchain Service initialized without a startup.ClockSetter") | ||
|
|
||
| type blobNotifierChan struct { | ||
| indices map[uint64]struct{} |
There was a problem hiding this comment.
Why do we need a map? it seems like a counter should be sufficient
There was a problem hiding this comment.
I see. We are worried about duplication, but p2p validation should block that
| if err != nil { | ||
| return errors.Wrap(err, "could not get blob sidecars") |
There was a problem hiding this comment.
If err is not nil, shouldn't we also do something like return early?
There was a problem hiding this comment.
nvm, I think this is by design, perhaps it's safer to check the error for not found
| s.blobNotifier.Lock() | ||
| var nc *blobNotifierChan | ||
| var ok bool | ||
| nc, ok = s.blobNotifier.chanForRoot[root] |
There was a problem hiding this comment.
Can move this line before right before if !ok
3 participants
Adds a new channel to the blockchain package to keep track of blobs that have been added to database.
It's an unbuffered channel that notifies the blob sidecar's
blockroot.When processing blocks, if the blob is not in the database, we block until the channel receives the right blockroot or the slot context deadline is done.
The channel is unbuffered since blobs are always consumed by blocks.