Remove `BlockChain<T>.Fork()` and `BlockChain<T>.Swap()` in `Swarm.PreloadAsync()`

[riemannulus]

Context

BlockChain<T>.Swap() and BlockChain<T>.Fork in Swarm.PrealodAsync() and Swarm.CompleteBlocksAsync() is useless in the PBFT scenario. There is only one canonical chain in the network so we do not support multiple chain branches.

And BlockChain<T>.Swap() is slow as much as when branches are long. So we suggest removing BlockChain<T>.Swap() in Swarm.PreloadAsync()

[riemannulus]

If we remove BlockChain<T>.Swap() in Swarm.PreloadAsync(), we need to decision for inevitable behavior changes.

• In Swarm.CompleteBlockAsync(), it appends all candidate blocks without evaluation action. https://github.com/planetarium/libplanet/blob/ff823f8435133689fcbd3f26508e40a2eab969ae/Libplanet.Net/Swarm.BlockSync.cs#L517-L522
• After finishing appends, it executes the action to lazy-eval https://github.com/planetarium/libplanet/blob/ff823f8435133689fcbd3f26508e40a2eab969ae/Libplanet.Net/Swarm.BlockSync.cs#L542-L547
• But if we process Swarm.CompleteBlockAsync() directly to the current main BlockChain<T>, main BlockChain<T> is processed to the topmost block but local states do not.
• If the user or whatever tries to quit the node while appending a block to main BlockChain<T>, TrieState does not reflect BlockChain<T> when it's node run again.

[riemannulus]

rn, I have some trade-offs for solving this problem.

• Rollback lazy-execute behavior. But I'm not sure this is the best solution. Also, I'm not sure if BlockChain<T>.Append() is atomic and quit-proof(?).
• Save progress in the store. and process from previous progress. But I'm sus may this makes another side-effect.

Have any ideas? @longfin /cc. @libplanet

[longfin]

Rollback lazy-execute behavior. But I'm not sure this is the best solution.

• Deferring state computation until the entire block is received, is unnecessary, at least for now. (even before PBFT)
  • This behavior was valid and actually used when there were protocols and operations that directly receive the status from other nodes, but as we moved the status DB to an MPT-based one, it was no longer supported, and related message types no longer exist.
  • Of course, this is just an explanation of the phenomenon, and the demand to receive the status directly may arise again later... But, I still think it's safe to assume at least in the scope of fixes targeting the current version of Libplanet and Nine Chronicles.

Also, I'm not sure if BlockChain<T>.Append() is atomic and quit-proof(?).

I agree that this can be difficult, but also think it's easier this way than complicating others.

[riemannulus]

So, your idea is rollback lazy-execute behavior is not the worst plan in this situation?

I have no strong opinion about this so if you think so, I'll do it like that.

[longfin]

So, your idea is rollback lazy-execute behavior is not the worst plan in this situation?

Right. 😄

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions.