add header hash support to block proposal state management - #7962
Conversation
| if headerHandler.IsHeaderV3() { | ||
| // skip intra-shard miniblocks. | ||
| // they are returned later when this block’s execution result is included in a future block. | ||
| // they are returned later when this block\u2019s execution result is included in a future block. |
There was a problem hiding this comment.
Change the unicode back to `
| c.retainedMut.Unlock() | ||
| return nil, nil | ||
| } | ||
| delete(c.stateAccessesForHeader, string(headerHash)) |
There was a problem hiding this comment.
Is it ok to delete here, even if it is not equal to expected? Is it possible that expected is wrong, and this will be needed in the future?
There was a problem hiding this comment.
there should be no production path where the roothash does not match for the given hash, when reaching commit. Also the clean up here is safe, as there is no retrial if we reached this point, and a potential mismatch has already been logged.
| } else { | ||
| log.Debug("state accesses already collected for root hash, keeping previous accesses", "rootHash", rootHash) | ||
| c.headerScopeMut.RLock() | ||
| headerHash := append([]byte(nil), c.headerHash...) |
There was a problem hiding this comment.
c.headerHash could be replaced by BeginExecution while the commit is running.
maybe pass the headerHash explicitly to CommitCollectedAccesses alongside the rootHash
There was a problem hiding this comment.
There should be only synchronous calls for this, valid as a robustness against future changes, but currently there is no path that would call it asynchronously/in parallel.
I would postpone a robustness change, as it would touch multiple places.
|
|
||
| // Error returns the root mismatch message | ||
| func (e *StateAccessesRootMismatchError) Error() string { | ||
| return ErrStateAccessesRootMismatch.Error() |
There was a problem hiding this comment.
could add HeaderHash, ExpectedRoot, ActualRoot to the error for debugging purposes
| func (ert *executionResultsTracker) unlockAndNotifyDismissed() { | ||
| notifications := ert.dismissedNotifications | ||
| ert.dismissedNotifications = nil | ||
| ert.mutex.Unlock() |
There was a problem hiding this comment.
i think it's better to keep them separated, Unlock and the NotifyDismissed, to keep Lock/Unlock in the same context func
|
|
||
| c.retainedMut.Lock() | ||
| c.stateAccessesForHeader[string(headerHash)] = &committedStateAccesses{ | ||
| rootHash: append([]byte(nil), rootHash...), |
There was a problem hiding this comment.
is this needed? we should not end up with a nil rootHash at this point, maybe early return?
There was a problem hiding this comment.
the roothash is not nil at this point, as the only caller is AccountsDB.commit, and it does an error check before (and RootHash() never returns nil without an error).
the append is used here to copy the roothash and not use the trie internal one.
| // EndExecution clears the identity when the generation is still current | ||
| func (c *collector) EndExecution(generation uint64) { | ||
| c.headerScopeMut.Lock() | ||
| if c.headerGeneration == generation { |
There was a problem hiding this comment.
is headerGeneration needed? add a header hash check?
There was a problem hiding this comment.
right, the header check is the better protection, especially since we don't allow overlapping collections.
e157263
into
fix-state-accesses-root-hash-overwrite
5 participants
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?