Add receipts hash storer #2235
Add receipts hash storer #2235
Conversation
… key and all the receipts hashes list marshalized as a value
process/block/baseProcess.go
Outdated
| log.Warn("saveBody.CreateMarshalizedReceipts", "error", errNotCritical.Error()) | ||
| } | ||
|
|
||
| errNotCritical = bp.store.Put(dataRetriever.ReceiptsUnit, finalReceiptHash, marshalizedReceiptsHashes) |
There was a problem hiding this comment.
the problem is not resolved. The saving key for this data structure should come from shardHdr.ReceiptsHash not a new computed hash. otherwise you will not find it,
There was a problem hiding this comment.
Actually is not a new computed, it will be exactly the one from shardHdr as this check and the hash calculation is already done in ProcessBlock by calling VerifyCreatedBlockTransactions wich uses the same pattern.
process/coordinator/process.go
Outdated
| return nil, nil, err | ||
| } | ||
|
|
||
| return finalReceiptHash, marshalizedReceiptsHashes, nil |
There was a problem hiding this comment.
no need to return finalReceiptHash - use the one from shard Header
There was a problem hiding this comment.
Yes. I do not have header in caller method but could be added
cmd/node/config/config.toml
Outdated
| Name = "ReceiptsStorage" | ||
| Capacity = 1000 | ||
| Type = "SizeLRU" | ||
| SizeInBytes = 104857600 #100MB |
There was a problem hiding this comment.
usually we set a larger cache capacity if there are reads on recent data so we can have them prepared, in this case I don't think we need 100 MB cache as this is currently seldom read.
I would set it to at most 50 MB.
There was a problem hiding this comment.
Yes I also thought that this size is too much when I have copied paste this setting from miniblocks but I forgot to adjust it. I think 10 MB is enough for this kind of receipts as regularly should be < 100 bytes per record/block (1 hash as a key and 1 marshalized hash as a value)
| FilePath = "Receipts" | ||
| Type = "LvlDBSerial" | ||
| BatchDelaySeconds = 2 | ||
| MaxBatchSize = 100 |
There was a problem hiding this comment.
this batch size is for separate receipts or aggregated receipts?
If it is for individual receipts, then I would set the size equal to the transaction batch size.
There was a problem hiding this comment.
it is for the aggregated receipts: key = aggregated receipts hash and value = marshalized individual receipts hashes list. So it is max one record per each block
update/process/shardBlock.go
Outdated
| log.Warn("saveAllTransactionsToStorageIfSelfShard.CreateMarshalizedReceipts", "error", errNotCritical.Error()) | ||
| } | ||
|
|
||
| errNotCritical = s.storage.Put(dataRetriever.ReceiptsUnit, shardHdr.GetReceiptsHash(), marshalizedReceiptsHashes) |
There was a problem hiding this comment.
what if there was an errNotCritical, do we save nil for the marshalizedReceiptHashes? I think in that case the put should be skipped
There was a problem hiding this comment.
Yep, also it should be avoided if len(shardHdr.GetReceiptsHash()) is 0 I think
process/block/baseProcess.go
Outdated
| log.Warn("saveBody.CreateMarshalizedReceipts", "error", errNotCritical.Error()) | ||
| } | ||
|
|
||
| errNotCritical = bp.store.Put(dataRetriever.ReceiptsUnit, header.GetReceiptsHash(), marshalizedReceiptsHashes) |
There was a problem hiding this comment.
if there was an error previously you are storing nil, I think you should skip instead
There was a problem hiding this comment.
Yep, also it should be avoided if len(shardHdr.GetReceiptsHash()) is 0 I think
…dd-receipts-hash-storer
Testing steps:
a) Prerequisites
b) Test steps
Start a normal testnet
c) Expected results
We should have another storage/folder/db created in: .../Epoch_X/Shard_Y/Receipts which holds cumulative receipt hash as a key and all the receipts hashes list marshalized as a value, for each committed block
d) Actual results
Test process is in pending