fix(blockchain): sequential blob Persist() DB writes
#2258
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2258 +/- ##
==========================================
+ Coverage 30.88% 31.08% +0.19%
==========================================
Files 331 331
Lines 15321 15319 -2
Branches 20 20
==========================================
+ Hits 4732 4762 +30
+ Misses 10268 10231 -37
- Partials 321 326 +5
|
There was a problem hiding this comment.
Two points:
- I would add
-raceflag formake test-unitcommand inscript\build\testing.mkfile. Note that we do have-raceenabled for a similar command, so I do not have objection is setting race detector in every test - If having two separate blobsidecars in the same slot with the same KzgCommitment is illegal, I would add the check in this PR, as a way to fully handle the race. As the PR currently is, I believe we do not fully solve the race
|
Another note: enabling race cause some linker warnings on macs, which should be benign (see golang/go#61229) |
Having two separate blobsidecars in the same slot with the same KzgCommitment is indeed illegal and is a unique problem to beacon-kit. That is a separate issue that I have halfway written a fix for this in a separate branch in response to an issue brought up in audit. But I have paused that fix so that I could wait for all of the generics stuff to be resolved first. Having said all of that, even if there were two blobs with the same KzgCommitments currently, the fix in this PR would still prevent it from causing a race condition in the DB. Edit: will push up |
The
store.Persist()function storedBlobSidecarupdates concurrently usingiter.Map()from "github.com/sourcegraph/conc/iter" package. However, the underlying RangeDB is not equipped to handle the concurrent writes. This resulted in a race condition that could corrupt the DA store.This PR fixes this issue and adds a
racetest case. Currently, this test case is disabled until the build flag "race" is added. Please advise on the best way to get that included in unit tests.This race condition exists in the
db.firstNonNilIndexfield inRangeDB. There could also arise another concurrency issue if the same file is written to concurrently, which could happen if two separate blobsidecars in the same slot have the same KzgCommitment (which is currently not prevented).I was originally considering making the
Set()function concurrency safe. However, this could be very difficult do to the aforementioned concurrent writes to the same file. In addition, the sequential implementation is onlyl marginally slower than the concurrent implementation. Here are the differences for a test that callsPersist()3 times with 20 blobs each: