Add Hyperblock coordinates in GetTxByHash() #2226
Conversation
andreibancioiu
changed the title
| Capacity = 20000 | ||
| Type = "LRU" | ||
| [FullHistory.HashEpochStorageConfig.DB] | ||
| [FullHistory.MiniblockHashByTxHashStorageConfig.DB] |
There was a problem hiding this comment.
what if we use the INDEXER interface in order to save all kinds of mapping like this? The node would work the same with the same storage and data, but the saved transaction structure in case of indexer would have all this metadata ? Not for current PR, but for the long term. food for taught.
There was a problem hiding this comment.
Yes, as discussed, we have to do that on the long term.
| } | ||
|
|
||
| func newErrCannotSaveMiniblockMetadata(hash []byte, originalErr error) error { | ||
| return fmt.Errorf("cannot save miniblock metadata, hash [%s]: %w", hex.EncodeToString(hash), originalErr) |
There was a problem hiding this comment.
do you really need this errors ? returning original err is not enough ? keep it simple
There was a problem hiding this comment.
I wanted to have some correlation info in case there's an issue. Can we keep them for the moment?
There was a problem hiding this comment.
there is really low possibility for this to happen
| @@ -56,11 +56,12 @@ func (hpf *historyRepositoryFactory) Create() (fullHistory.HistoryRepository, er | |||
| } | |||
|
|
|||
| historyRepArgs := fullHistory.HistoryRepositoryArguments{ | |||
There was a problem hiding this comment.
why is fullHistory in the core package ? it should not be here. core is only for simple stuff.
There was a problem hiding this comment.
Indeed. Will be moved in a future PR.
| if err != nil { | ||
| return fmt.Errorf("cannot save in storage history transaction %w", err) | ||
| return newErrCannotSaveMiniblockMetadata(hash, err) |
There was a problem hiding this comment.
just return a constant error here - no need to add hash. this will never happen. keep it simple.
There was a problem hiding this comment.
I wanted to have some correlation info in case there's an issue. Can we keep it for the moment?
There was a problem hiding this comment.
i do not see this usable - it is pretty impossible for this to happen.
| return hp.epochByHashIndex.getEpochByHash(hash) | ||
| } | ||
|
|
||
| func (hp *historyProcessor) RegisterToBlockTracker(blockTracker BlockTracker) { |
There was a problem hiding this comment.
add comment to public function
| metaBlock, ok := header.(*block.MetaBlock) | ||
| if !ok { | ||
| // Question for review: why, on every round, there's a header of type = *block.Header (even if shardID = metachain)? | ||
| log.Debug("onNotarizedBlocks(): cannot convert to *block.Metablock", "type", fmt.Sprintf("%T", header)) |
There was a problem hiding this comment.
delete this log - no need for it.
| return | ||
| } | ||
|
|
||
| for i := 0; i < len(headers); i++ { |
There was a problem hiding this comment.
for i, header := range headers
| return | ||
| } | ||
|
|
||
| log.Trace("onNotarizedBlocks()", "index", i, "len(ShardInfo)", len(metaBlock.ShardInfo)) |
There was a problem hiding this comment.
do we really need these log trace's ? if they will not be used, delete them.
| } | ||
|
|
||
| func (hp *historyProcessor) onNotarizedBlock(metaBlockNonce uint64, metaBlockHash []byte, blockHeader block.ShardData) { | ||
| log.Trace("onNotarizedBlock()", |
There was a problem hiding this comment.
logging this data already exist - please delete
| storer storage.Storer | ||
| } | ||
|
|
||
| func newMiniblockHashByTxHashIndex(storer storage.Storer) *miniblockHashByTxHashIndex { |
There was a problem hiding this comment.
there is no need for this class. this in only a storer. Add a named variable which says miniblockByHashIndexStorer and you are fine.
There was a problem hiding this comment.
Fixed, removed.
andreibancioiu
requested review from
sasurobert,
SebastianMarian and
AdoAdoAdo
…ond-go into hyperblock-coordinates
andreibancioiu
dismissed stale reviews from sasurobert and AdoAdoAdo
via
1e441f4
andreibancioiu
dismissed stale reviews from AdoAdoAdo and sasurobert
via
899e8b1
miniblock metadatastorage (used for API). These extra fields refer to the metablock that included the miniblock in question at source / at destination.NotarizedAtDestinationInMeta*are used to populate the hyperblock coordinates at proxy-side.invalidBlock)How to test:
/transaction/abracadabra, and inspect the fields:Limitations:
cannot populateFixed.NotarizedAtSourceInMetaNonce / NotarizedAtSourceInMetaHashwhen running in destination shard if the notification of "notarized at source" comes before committing the block (at destination) - since there will be no miniblock metadata in the storer yet. Can be easily fixed though.