[filedao] get recent blocks from staging buffer #4488
Conversation
dustinxie
requested review from
CoderZhi,
Liuhaai and
envestcc
as code owners
| buffer []*block.Store | ||
| deser *block.Deserializer |
| // stagingKey is the position of block in the staging buffer | ||
| func stagingKey(height uint64, header *FileHeader) uint64 { | ||
| return (height - header.Start) % header.BlockStoreSize | ||
| } |
There was a problem hiding this comment.
replaced by stagingBuffer.slot()
| func (fd *fileDAOv2) getFromStagingBuffer(height uint64) (*block.Store, error) { | ||
| if fd.loadTip().Height-height >= fd.header.BlockStoreSize { | ||
| return nil, ErrNotSupported | ||
| } |
There was a problem hiding this comment.
improve the logic, nothing to do with blkStore.Size() now
staging buffer contains the top BlockStoreSize blocks, for example, if tip height = 100, it has blocks 100, 99, ... 85
There was a problem hiding this comment.
The return value of blkStore.Size() is currently incorrect, and it seems that this PR did not fix it. Shouldn't the Size method be removed? If Size is used elsewhere, it should also be checked for similar issues.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4488 +/- ##
==========================================
- Coverage 74.83% 72.86% -1.97%
==========================================
Files 378 382 +4
Lines 31624 32565 +941
==========================================
+ Hits 23666 23730 +64
- Misses 6747 7612 +865
- Partials 1211 1223 +12 ☔ View full report in Codecov by Sentry. |
| @@ -178,12 +173,7 @@ func (fd *fileDAOv2) getBlock(height uint64) (*block.Block, error) { | |||
| return nil, db.ErrNotExist | |||
| } | |||
| // check whether block in staging buffer or not | |||
| storeKey := blockStoreKey(height, fd.header) | |||
| if storeKey >= fd.blkStore.Size() { | |||
There was a problem hiding this comment.
root-cause of the bug:
the batch has not been written to DB yet, but blkStore.Size() already increased 1. This caused code go to L190 attempting to read the block from DB, which is not yet written to disk yet.
blockchain/filedao/staging_buffer.go
Outdated
| func (s *stagingBuffer) Serialize() ([]byte, error) { | ||
| blkStores := []*iotextypes.BlockStore{} | ||
| // blob sidecar data are stored separately | ||
| s.lock.Lock() |
There was a problem hiding this comment.
seems unnecessary to lock (rlock at most), b/c Put and Serialize will not be called parallelly.
There was a problem hiding this comment.
- in current code, they are not called parallelly. As 2 public methods, should consider synchronize access?
Serializeis actually also a read operation, so can change toRLock
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if blkStore, err := fd.getFromStagingBuffer(height); err == nil { |
There was a problem hiding this comment.
It is better to check for errors and throw if unexpected
|
Description
as title
Fixes #4489
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
Test Configuration:
Checklist: