Validate state root hash #986
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
moreal
changed the title
moreal
force-pushed
the
feature/state-root-hash
branch
3 times, most recently
from
8cf6863
to
a683a81
Compare
moreal
force-pushed
the
feature/state-root-hash
branch
from
a683a81
to
401b999
Compare
Codecov Report
@@ Coverage Diff @@
## main #986 +/- ##
==========================================
- Coverage 89.16% 88.97% -0.20%
==========================================
Files 324 328 +4
Lines 29370 29465 +95
==========================================
+ Hits 26189 26216 +27
- Misses 1625 1696 +71
+ Partials 1556 1553 -3
|
moreal
requested review from
dahlia,
riemannulus,
earlbread,
limebell and
longfin
moreal
force-pushed
the
feature/state-root-hash
branch
2 times, most recently
from
753c27a
to
c809550
Compare
|
It seems conflicting. |
moreal
force-pushed
the
feature/state-root-hash
branch
from
c809550
to
2626326
Compare
@longfin I resolved. You can review them 😀 |
dahlia
reviewed
Sep 9, 2020
Comment on lines
+235
to
+238
| - `BlockPolicy<T>` became to validate `Block<T>.StateRootHash` property | ||
| of a `Block<T>`. [[#986]] |
There was a problem hiding this comment.
Is this necessary despite we've never released Block<T>.StateRootHash?
There was a problem hiding this comment.
The reason why I added the line, was the behavior of BlockPolicy<T>. As you said, Block<T>.StateRootHash is had never released yet, but BlockPolicy<T> does. Then isn't it right to log the change or is it unnecessary?
moreal
force-pushed
the
feature/state-root-hash
branch
from
2626326
to
389ab87
Compare
longfin
reviewed
Sep 9, 2020
riemannulus
reviewed
Sep 9, 2020
moreal
force-pushed
the
feature/state-root-hash
branch
from
5e34ea6
to
9193e58
Compare
earlbread
reviewed
Sep 10, 2020
| where T : IAction, new() | ||
| { | ||
| private readonly ILogger _logger; | ||
| private readonly IAction _blockAction; |
There was a problem hiding this comment.
Suggested change
| private readonly IAction _blockAction; | |
| private readonly IAction? _blockAction; |
| _balanceGetter; | ||
|
|
||
| internal BlockEvaluator( | ||
| IAction blockAction, |
There was a problem hiding this comment.
Suggested change
| IAction blockAction, | |
| IAction? blockAction, |
Libplanet/Store/TrieStateStore.cs
Outdated
| internal HashDigest<SHA256>? SetStates<T>( | ||
| Block<T> block, | ||
| IImmutableDictionary<string, IValue> states, | ||
| bool rehearsal) |
There was a problem hiding this comment.
Do we need this rehearsal option? This seems to be only used when making the genesis block. Since the MakeGenesisBlock method uses an in-memory DefaultKeyValueStore, wouldn't it be okay without this option?
There was a problem hiding this comment.
You're right. I will remove it.
There was a problem hiding this comment.
I fixed it in 68090f7. Please check your intention was applied well.
limebell
reviewed
Sep 10, 2020
earlbread
previously approved these changes
Sep 11, 2020
|
/rebase |
limebell
previously approved these changes
Sep 11, 2020
|
/rebase |
riemannulus
previously approved these changes
Sep 11, 2020
Originally, it was hard to evaluate blocks properly because blockchain contained the logic to evaluate and block action (i.e., block policy). BlockChain required a genesis block but it require logic to evaluate block and block action. This fixes the interdependence issue.
- Remove code duplication - Remove useless comment
`ExecuteActionsBetweenTrieStateStoreAndBlockStatesStore` is not correct anymore because `TrieStateStore` and `IBlockStatesStore` doesn't share same block layout
- `InvalidBlockStateRootHashException` [skip changelog]
Co-Authored-By: Seunghun Lee <waydi1@gmail.com>
moreal
dismissed stale reviews from riemannulus, limebell, earlbread, and longfin
via
43ad298
moreal
force-pushed
the
feature/state-root-hash
branch
from
3b31603
to
43ad298
Compare
limebell
approved these changes
Sep 11, 2020
dahlia
approved these changes
Sep 11, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Block<T>.StateRootHashfield when usingTrieStateStore.evaluationDigest(akaactionsHash)