Logic vulnerability when handling IPersistencePlugin during block persisting

[Qiao-Jin]

Describe the bug

IPersistencePlugins have DBs themselves, which is used to store specified data. This design, however, can bring data inconsistency in case that node closes or crashes within these lines:

neo/src/neo/Ledger/Blockchain.cs

Lines 418 to 427 in a4d2edd

	foreach (IPersistencePlugin plugin in Plugin.PersistencePlugins)
	plugin.OnPersist(system, block, snapshot, all_application_executed);
	snapshot.Commit();
	List<Exception> commitExceptions = null;
	foreach (IPersistencePlugin plugin in Plugin.PersistencePlugins)
	{
	try
	{
	plugin.OnCommit(system, block, snapshot);
	}

Data can be writen mutiple times or missing from DBs of IPersistencePlugins.

To Reproduce
Close nodes while persisting blocks.

Solution

1. Add a Height parameter in IPersistencePlugin, which is recorded in its DB
2. Fetch the height of neo-core snapshot and run func OnCommit and OnPersist of missing blocks if needed, in the constructor of IPersistencePlugin
3. Change IPersistencePlugin to Akka actors and send corresponding trigger messages to them during block persisting, instead of call OnPersist and OnCommit directly.

The 3rd can also be replaced by this logic: don't persist new blocks until all IPersistencePlugins finished all OnPersist and OnCommit, similiar to logic of Neo2.

[shargon]

The 3rd can also be replaced by this logic: don't persist new blocks until all IPersistencePlugins finished all OnPersist and OnCommit, similiar to logic of Neo2.

I think that it's not a bad idea

[roman-khimov]

Looks like I'm not familiar enough with this part of C# codebase. I've never thought it's possible to persist new blocks until Persist() is finished dealing with the previous one. At the same time even if that's the case there is some possibility for data loss between snapshot.Commit() and plugin.OnCommit() in at least RpcNep17Tracker because it's only writes in OnCommit (technically, in StatesDumper too but it's not often a part of regular node).

[roman-khimov]

BTW, maybe there is a more generic problem here in that some plugins keep their own DBs and we can't atomically flush to disk all of them, so there always is a chance for inconsistency (when one DB syncs its data to disk and another doesn't).

[Qiao-Jin]

BTW, maybe there is a more generic problem here in that some plugins keep their own DBs and we can't atomically flush to disk all of them, so there always is a chance for inconsistency (when one DB syncs its data to disk and another doesn't).

Adding a Height parameter whose value is got from DB should solve this problem

[roman-khimov]

It will allow to detect inconsistency, but what can we do with it? Say we have block N persisted and NEP-17 tracker didn't make it before something killed the process, so it only has data for N-1, now on node start we can see this mismatch, but I don't think we can get data plugin needs for block N again, so probably the only thing we could do is to ask user to resync.

[Qiao-Jin]

It will allow to detect inconsistency, but what can we do with it? Say we have block N persisted and NEP-17 tracker didn't make it before something killed the process, so it only has data for N-1, now on node start we can see this mismatch, but I don't think we can get data plugin needs for block N again, so probably the only thing we could do is to ask user to resync.

We can change the order so that snapshot in neo-core is committed in the last, please refer to #2576