Introduce StateRootInHeader
#4161
Conversation
AnnaShaleva
requested review from
shargon,
cschuchardt88,
Jim8y,
vncoelho,
roman-khimov,
superboyiii and
Wi1l-B0t
Required to test nspcc-dev/neo-go#3566 and neo-project/neo#4161. Includes nspcc-dev/neo-go#4001. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
| sizeof(uint) + // Index | ||
| sizeof(byte) + // PrimaryIndex | ||
| UInt160.Length + // NextConsensus | ||
| (version is (uint)BlockVersion.Zero ? 0 : UInt256.Length) + // PrevStateRoot |
There was a problem hiding this comment.
| (version is (uint)BlockVersion.Zero ? 0 : UInt256.Length) + // PrevStateRoot | |
| (version == 0 ? 0 : UInt256.Length) + // PrevStateRoot |
| { | ||
| if (primaryIndex >= settings.ValidatorsCount) | ||
| return false; | ||
| if (version != (uint)(settings.IsHardforkEnabled(Hardfork.HF_Faun, Index) ? BlockVersion.One : BlockVersion.Zero)) |
There was a problem hiding this comment.
This is very sloppy and makes more work for others to maintain. At least create a private method for this (uint)(settings.IsHardforkEnabled(Hardfork.HF_Faun, Index) ? BlockVersion.One : BlockVersion.Zero) code.
| Hashes.Length | ||
| ]); | ||
| }; | ||
| if (Header.Version == (uint)BlockVersion.One) |
There was a problem hiding this comment.
If we add them at the end, we don't break a smart contract that use Hashes.Length in previous version
There was a problem hiding this comment.
Current order has some logic behind it and is also more clear wrt hashable/non-hashable parts, but the concern is valid. We can check current Ledger callers, maybe it's not a problem practically and hardforks are hardforks for a reason. The other way to solve this is to have a separate call to return v1 structure.
There was a problem hiding this comment.
for now, for simplicity, everything should be added in the end I think
There was a problem hiding this comment.
In fact, I worry less about the communication protocol itself because it will not cause problems since nodes will be aligned with the format.
There was a problem hiding this comment.
Not fixed yet, we need to come to an agreement. To me, moving PrevStateRoot to the end of the Header won't help much, because there's always a set of transactions attached to the Block, so we still need to update dependent tools anyway.
| "UnhandledExceptionPolicy": "StopNode" | ||
| } | ||
| }, | ||
| "Dependency": [ |
There was a problem hiding this comment.
Why add this extra dependency ?
There was a problem hiding this comment.
Because dBFT should have an access to stateroot at every block, and stateroot is managed by StateService plugin.
| if (system.Settings.Network != StateServiceSettings.Default.Network) return; | ||
| StateStore.Singleton.UpdateLocalStateRootSnapshot(block.Index, | ||
|
|
||
| // Rebuild MPT from scratch on node bootstrap if StateService plugin is not up-to-date wrt the current block height. |
There was a problem hiding this comment.
This part is not clear to me, why rebuild here and why the plugin would be out of sync
There was a problem hiding this comment.
It's to avoid node resynchronization for existing CNs. We can skip doing this (frankly, It'd be even easier for us), but then CNs would need to enable the plugin and restart from genesis.
There was a problem hiding this comment.
We need the plugin to restart, not the whole node. Maybe we need this feature
There was a problem hiding this comment.
We need the plugin to restart,
This may be moved to a separate issue, but for now it's implemented as is. I'm open to suggestions on a place where we can move this functionality, because right now it's not a proper place for it (right now we rebuild MPT after block processing first time when the node is started with StateService plugin in fact).
AnnaShaleva
force-pushed
the
stateroot-in-header
branch
from
5ca3c02 to
a2e716d
Compare
|
@neo-project/core let's review one more time, the implementation is finished and passed my tests with NeoBench and mixed Go+C# CNs privnet: @superboyiii the following minimum functionality should be tested:
|
| // TODO: the following exception must be critical for the node functioning, need to | ||
| // refactor this code to stop the node if it happens. | ||
| if (nextH is not null && root != nextH.PrevStateRoot) | ||
| throw new InvalidOperationException($"Stateroot mismatch at {block.Index}: expected {nextH.PrevStateRoot}, got {root}"); |
There was a problem hiding this comment.
@shargon I need some help with this. This exception must stop the block processing because it means that local stateroot doesn't match the one included into the block by CNs (so either block is invalid or local MPT is invalid, both situations are critical). But I'm not sure what's the best way to implement it.
There was a problem hiding this comment.
An exception will deny the service, But maybe we can create a new exception, catch it, and revert the snapshot
There was a problem hiding this comment.
I think in this case DoS is exactly what we want. We know the state should be A at height H, if we have state B at the same height something is wrong and node can't be used reliably.
There was a problem hiding this comment.
Roman is right, this is exactly the purpose of this pr.
| } | ||
| }, | ||
| "Dependency": [ | ||
| "StateService" |
There was a problem hiding this comment.
@neo-project/core There's a pitfall that may be moved to a separate issue: dBFT needs StateService because dBFT should have an access to stateroots, and stateroots are managed by StateService plugin. However, StateService plugin requires RPCServer plugin to be installed because of RPC method extensions. So in other words, dBFT requires RPC server which is not a proper dependency for dBFT.
We may split StateService plugin functionality into two plugins: one part will manage stateroots, another part will expose additional RPC methods. Let me know if you agree or have other suggestions on how to solve this issue.
AnnaShaleva
requested review from
shargon,
vncoelho,
cschuchardt88 and
Wi1l-B0t
| if (Header.Version == (uint)BlockVersion.V1) | ||
| block.Add(Header.PrevStateRoot.ToArray()); | ||
|
|
||
| // Block properties | ||
| block.Add(Hashes.Length); |
There was a problem hiding this comment.
I'm still thinking that if we append at the end, we don't affect SC that checks the Hashes.Length without check the version
| NativeContract.NEO.ComputeNextBlockValidators(Snapshot, neoSystem.Settings) : | ||
| NativeContract.NEO.GetNextBlockValidators(Snapshot, neoSystem.Settings.ValidatorsCount)) | ||
| NativeContract.NEO.GetNextBlockValidators(Snapshot, neoSystem.Settings.ValidatorsCount)), | ||
| PrevStateRoot = isFaun ? StateService.StatePlugin.GetStateRootHash(height) : null |
There was a problem hiding this comment.
will this null here cause any issue? is the codebase robust enough to tolerate a null prevstateroot?
| h.Version = (uint)BlockVersion.V1; | ||
| // blockbase 4 + 64 + 1 + 32 + 4 + 4 + 20 + 4 + 32 | ||
| // header 1 | ||
| Assert.AreEqual(145, uut.Size); // 105 + nonce + prevHash |
There was a problem hiding this comment.
still asserts on uut.Size after
creating the V1 header in h, so the test always compares against the V0 size (113) and fails.
There was a problem hiding this comment.
Yes, UTs fixes are still in progress.
| json["nextconsensus"] = nextConsensus.ToAddress(settings.AddressVersion); | ||
| json["witnesses"] = new JArray(Witness.ToJson()); | ||
| if (version == (uint)BlockVersion.V1) | ||
| json["previousstateroot"] = new JArray(prevStateRoot.ToString()); |
AnnaShaleva
force-pushed
the
stateroot-in-header
branch
2 times, most recently
from
d802e39 to
b620e4c
Compare
Close #1526. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
Do not require StateService plugin to be enabled from genesis block. Node can build MPT based on the current contract storage state from scratch at the arbitrary height. This feature is required to avoid chain resync from genesis for those nodes who don't have StateService plugin enabled. Signed-off-by: Anna Shaleva <shaleva.ann@nspcc.ru>
AnnaShaleva
force-pushed
the
stateroot-in-header
branch
from
b620e4c to
ab4be97
Compare
|
@neo-project/core I think I need your help with unit-tests: GitHub testing jobs are failing for dBFT plugin (probably due to timeout): And also locally I have |
|
@Jim8y @roman-khimov Just for the record, I just checked what is the time of rebuilding MPT on the current Mainnet at height 8060917 (current height), it takes 348277ms on my local machine (~6min) which is pretty much affordable. So users may safely install StateService plugin without full node resynchronisation. Just for the record, I measured it with this change: diff --git a/src/Plugins/StateService/StatePlugin.cs b/src/Plugins/StateService/StatePlugin.cs
index 788934c89..1417b7c06 100644
--- a/src/Plugins/StateService/StatePlugin.cs
+++ b/src/Plugins/StateService/StatePlugin.cs
@@ -111,6 +111,7 @@ public override void Dispose()
var stateRoot = stateSnapshot.GetStateRoot(persistedHeight);
if (stateRoot is null)
{
+ var watch = System.Diagnostics.Stopwatch.StartNew();
using var coreSnapshot = system.GetSnapshotCache();
// TODO: this will only work with completely empty MPT. If there's some out-of-date state,
// then need to rebuild the trie from empty root.
@@ -120,6 +121,10 @@ public override void Dispose()
.Select(si => new KeyValuePair<StorageKey, DataCache.Trackable>(si.Key, new DataCache.Trackable(si.Value, TrackState.Added)))
.ToList());
StateStore.Singleton.UpdateLocalStateRoot(persistedHeight);
+ watch.Stop();
+ var elapsedMs = watch.ElapsedMilliseconds;
+ Console.WriteLine($"###### REBUILDING MPT for ${persistedHeight}: {elapsedMs}");
+ Utility.Log(nameof(StatePlugin), LogLevel.Warning, $"###### REBUILDING MPT for ${persistedHeight}: {elapsedMs}");
}
}The resulting log is (I checked it two times, for small height and for the full chain): |
Description
Close #1526.
It's the initial version that has a couple of TODOs, so the work is still in progress, but @neo-project/core may perform the initial review of this PR. I tried to introduce as less changes to the existing codebase as possible.
This version is tested on NeoBench with StateService plugin enabled from the genesis,
Faunenabled from non-zero height and using homogeneous C# nodes setup: at least blocks are being accepted, Faun transition happens seamlessly and state root is being included into header starting from Faun. I'll do more tests once implementation is finished and I'll notify @superboyiii once this PR is ready for NGD testing.TODO:
DBFT->StateService->RPCServerdependency toDBFT->StateService, probably splitStateServiceinto two plugins.Type of change
How Has This Been Tested?
Checklist: