ChainSafe / ChainSafe/lodestar
non-atomic write during the migration of data sidecars
- Dominant language
- TypeScript
- Stars
- 1.4k
- Forks
- 483
- Avg merge
- 1d 18h
- Merged PRs (30d)
- 156
Description
### Describe the bug
During refactoring of the `dataSidecars` observed that we have a non-atomic write for migration of the data-column sidecars and we reply on just the promise fulfillment to make sure whole operation is finished.
```ts
promises.push(
db.dataColumnSidecarArchive.putManyBinary(
block.slot,
dataColumnSidecars.map(({id, value}) => ({id, value}))
)
);
promises.push(db.dataColumnSidecar.deleteMany(canonicalBlocks.map((block) => block.root)));
await Promise.all(promises);
```
So there the `delete` and `put` are run independant to each other and each can fail individually. This will create inconsistent state on the database. e.g. We may put dataColumns to archive but the delete options failed. Or worst what if the delete works out and put failed and we will lost the data and have to sync again.
### Expected behavior
These both operations should be atomic batch operation. Either it passes fully or fail fully. So we never lost the data or have inconsistent db state.
### Steps to reproduce
The highlighted case is just one case, but there are other places in the code where we can get benifit from fixing this approach.
Can happen with many weird edge cases. Currently it never triggered because `db.close()` tries to complete all requests before closing. But the process can also terminate or even the single promise is yet left to execute in the event queue.
### Additional context
The underlaying `leveldb` have that support for atomic batch requests but our `@lodestar/db` is designed in the way to not utilize that feature.
### Operating system
Linux
### Lodestar version or commit hash
unstable
Contributor guide
Research direction
Start in the @lodestar/db implementation behind dataColumnSidecarArchive.putManyBinary and dataColumnSidecar.deleteMany, then review how db.close() handles pending requests and how the underlying LevelDB batch support is exposed. Done means the migration's archive and delete operations are committed atomically, with failure leaving no inconsistent or lost data.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- typescript
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100