Update to trie and the skipblock does not happen atomically

[kc1212]

In the StoreBlocks function, the skipblock update happens in one boltdb transaction and then it makes a callback to updateTrieCallback which makes another transaction to update the trie. These should happen atomically otherwise we may get into an inconsistent state when the conode crashes in between these two transactions.

[kc1212]

Making the trie storage and block storage is tricky with the current API because the primary way we access the trie does not allow us to specify a boltdb tx. Each function call to the trie APIs, for example stateTrie.GetValues or stateTrie.VerifiedStoreAll create new transactions. There are three ways that I can think of to resolve this issue:

1. Create a new set of stateTrie APIs that operate inside a transaction by making use of trie.*WithBucket. The ReadOnlyStateTrie may need to be updated too because it also creates new transactions, if it's used in updateTrieCallback. This would be the correct way to fix this issue but it will involve a lot of work, not only on the stateTrie side but also the SkipBlockDB side because we also use it to get information on skipblocks in the callback.

2. Instead of modifying the trie directly inside updateTrieCallback, have the callback return a set of instructions that will be executed by SkipBlockDB. This approach might involve less work but it will change the order of operations which might make cothority unstable.

3. Keep the updateTrieCallback the same but add a mechanism that detects the inconsistency and tries to recover from it on startup. This approach does not address the root cause but it is probably the simplest solution.

[ineiti]

When starting to read I thought directly of 3., without reading it ;) So I vote +1 for 3....

[Gilthoniel]

I see there an occasion to fix the problem we have when a forward link level 0 is stored but the block is still processing. Now it could end up in a crazy design idea but I think it might be worth investigate.

It would great to have a transaction running from the beginning of the block creation to the point it's actually stored. For the moment it's only a bunch of DB calls there and there.

That means the transaction would need to be transmitted from the forwardLinkLevel0 function through the callback and plus the hard work you said about the trie but the good side is that everything is private so there should not be backwards compatibility troubles.

So I kinda vote for 1.

[kc1212]

I analyzed the complexity of doing option 1. It's big but not unmanageable. I've grouped the API changes into 5 sections.

1. We need to create new versions of byzcoin.{LoadBlockInfo, LoadConfig, computeInitialDuration}, which are basically wrappers around stateTrie.
2. The stateTrie itself - all the function related to getting, setting and creating state tries like stateTrie.{StoreAll, GetValues, GetIndex, ...}, byzcoin.getStateTrie.
3. byzcoin.stateChangeStorage.append because it stores stuff in the database.
4. The skipblock function like GetByID.
5. context.GetAdditionalBucket from onet - this function needs to work with an existing transaction because we might create a new trie inside updateTrieCallback.

Except the skipblock and onet change, the rest can be private methods.

The question is whether adding this extra set of function is too much of a trade-off (adding extra complexity and increasing maintenance overhead) for fixing the issue at hand. If we do implement these, will it have other benefits?

[kc1212]

After discussing with @jeffallen, we decided to go forward with option 1 because we are committed to improving the stability of byzcoin.

[kc1212]

roadblock (maybe): createStateChanges uses a stagingStateTrie so we need to make sure all of its methods work inside one transaction. One of the method it uses is GetRoot which needs to create a new transaction on every call. That's because under the hood it applies the temporary key/value pairs to a transaction, computes the root, then rolls back. boltdb doesn't have an API to rollback x number of operations or to some marker.

In summary: atomic storage -> everything happen in one transaction -> cannot do GetRoot because it relies on rollbacks. We might need to implement a different GetRoot.