-
Notifications
You must be signed in to change notification settings - Fork 100
core, util: attach stateroot witness to NeoFS state objects #4099
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4099 +/- ##
==========================================
- Coverage 83.44% 83.34% -0.10%
==========================================
Files 352 352
Lines 42748 42854 +106
==========================================
+ Hits 35669 35718 +49
- Misses 5324 5371 +47
- Partials 1755 1765 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'll test this code on real network once we finalize the PR. |
| } | ||
|
|
||
| lastFoundIdx, err := strconv.ParseUint(lastItem.Attributes[0], 10, 32) | ||
| lastItemH, err := neofs.ObjectHead(ctx, s.Pool, s.Account.PrivateKey(), s.ContainerID, lastItem.ID) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why an additional Head? You can request the attribute from ObjectSearch directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How exactly?
From what I understood, we need to include attribute to a search filter. The problem is that this attribute is optional, and there's no such filter like "check attribute if it's present".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, just add it to the list of requested attributes, you'll get it if it's present.
| continue | ||
| } | ||
| if err = s.chain.AddContractStorageItems(batch, syncHeight, expectedRoot); err != nil { | ||
| if err = s.chain.AddContractStorageItems(batch, syncHeight, expectedRoot, witness); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you're passing it always when processing batches of KVs. Intermediate roots will never be correct, only the final one can be, so to me you should be:
- checking stateroot witness once before performing any state changes
- adding sets of KVs as usual
- checking the resulting stateroot to be equal to the one we proved to be correct at the first step
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure why you're passing it always when processing batches of KVs
I'm just following the current implementation since we're passing expectedRoot every time as far. It was like that since #3844, let's refactor this code.
checking stateroot witness once before performing any state changes
It was an initial implementation, but then I discovered stateroot verification functions bound to the state service and decided to reuse this code (and it can be done only after state service cache initialization on the proper height). Fail-fast strategy is good, but we'll be sure that the whole state is correct only after full MPT recovery at the third step.
So are we OK with the stateroot witness checking code duplication?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in a separate commit.
pkg/core/blockchain.go
Outdated
| // TODO: @roman-khimov, we need to notify NeoFS nodes about chain state reset so that they update | ||
| // NeoFSAlphabet-dependent services accordingly (the current code won't work because it's based | ||
| // on notifications, and there are no notifications during state sync). I suggest to add a callback | ||
| // to the Blockchain that will be called once state reset is finished. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It only happens on a new node normally and this can happen with non-local RPC as well, so callbacks can't solve the problem.
e0fdcb0 to
a26d6a8
Compare
a26d6a8 to
3d7e28d
Compare
|
|
A part of #4049. Signed-off-by: Anna Shaleva <[email protected]>
Oracle, StateRoot and Notary services should properly react to state jump (either occured after state reset or after statesync finalization). Otherwise, these services use an out-of-date roles information retrieved from the previous node state. Signed-off-by: Anna Shaleva <[email protected]>
A part of #4049. 1. Check the stateroot witness. 2. Ensure witness' public keys match block's validators at the state sync height. Signed-off-by: Anna Shaleva <[email protected]>
This callback is set when module is not active yet, no locking required. Signed-off-by: Anna Shaleva <[email protected]>
(*Module).syncStage is a subject of change, it may be updated by statechanging Module's callbacks concurrently. Signed-off-by: Anna Shaleva <[email protected]>
27fb9c8 to
8aacb96
Compare
Signed-off-by: Anna Shaleva <[email protected]>
8aacb96 to
c902752
Compare
|
|
||
| // JumpToState performs jump to the state specified by given stateroot index. | ||
| func (s *Module) JumpToState(sr *state.MPTRoot) { | ||
| func (s *Module) JumpToState(sr *state.MPTRoot) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always returns nil (at least as of the commit where it's added).
| s.isActive.CompareAndSwap(true, false) | ||
| return 0, fmt.Errorf("failed to decode state root from attribute: %w", err) | ||
| } | ||
| if len(lastItem.Attributes) > 2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They will always be returned, but they can be empty is missing from object.
pkg/core/statesync/module.go
Outdated
| return s.defineSyncStage() | ||
| } | ||
|
|
||
| func (s *Module) InitContractStorageItemsSync(syncHeight uint32, expectedRoot util.Uint256, witness transaction.Witness) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty in 7523f7c
| // IntermediateRoot is a computed intermediate root of non-complete MPT | ||
| // at the StateSyncCheckpoint. | ||
| IntermediateRoot util.Uint256 | ||
| // TODO: @roman-khimov, technically this and previous commit include a DB schema change since StateSyncCheckpoint is stored in the DB. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a note to release, that's it. State sync should be finished with the old node if it's in process.
| // height is returned. | ||
| func (s *Service) FindStateObject(minH uint32, maxH uint32) (uint32, error) { | ||
| if maxH > 0 && maxH < minH { | ||
| // Program bug. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it's a public interface, can be used by IR, for example.
| // TODO: @roman-khimov, we don't include len(KVs) in the state object, | ||
| // so the only indicator of state object end is EOF. I suggest | ||
| // to change the format of state object files and include | ||
| // len(KVs) to the header. Reuploading is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Streams are easier to create, how many KVs your DB has? You'll only know this after complete iteration. You have object data hash for consistency check and stateroot hash as well. Shouldn't be a problem.
Close #4049.
As an alternative, we may consider attaching a full serialized stateroot instead of just witness (right now stateroot version is missing from the state object which may be a problem in future if stateroot version will be changed).