Chain reorganization discussion

[Aprogiena]

tbd

i want to talk about reorgs and current code - ConcurrentChain and discuss a wild idea of getting rid of it (or upgrading it). i will provide more info later, no time now

[Aprogiena]

There has been quite a lot of problems detected recently caused by various use of ConcurrentChain. The problem is that things like this.chain.Tip provide value that is not guaranteed to be valid just after it has been received. Similarly, any use of this.chain.GetBlock(hash) can return null at any time even if we just checked that the hash is on the chain. The problem is that the chain can be changed at any time, just because a new peer appears and tells us about its new chain (which can even be invalid).

In general, I'd suggest to create a better implementation of the chain in a way that each code has to open a session, in which its copy of the chain is guaranteed to be immutable. Therefore all such problems would disappear. The session should also provide an easy method on how to check if its copy of the chain is subchain of the best chain. Maybe the caller could open a session with some flags, such as - to guard against chain advancing (i.e. protect the tip), or to guard against reorgs (local copy is subchain of best chain) - and if any such thing would be violated, exception would be thrown. And methods to check for those things should be implemented.

[Aprogiena]

Little simpler concept would be to introduce a public lock into concurrent chain, which would allowed you to prevent changes in the chain for a short period of time. For example #22 could be implemented like this:

if (this.chain.Contains(assumeValidBlock.Hash) && this.chain.Contains(newBlockToValidate.Hash))
{
    skipNewBlockValidation = true;
}

however, currently this is not correct as the chain can change between the first and the second call to chain.Contains. If we did have such a lock, we could do:

using (this.chain.Lock())
{
    if (this.chain.Contains(assumeValidBlock.Hash) && this.chain.Contains(newBlockToValidate.Hash))
    {
        skipNewBlockValidation = true;
    }
}

and it would be correct because the lock would guarantee the chain won't change.

Such a lock is not as good solution as having the chain session completely immutable as suggested in previous comment, but at least it would allow us to write correct code, which is now rather impossible.

The current solution for #22 would probably use an approach like this:

ChainedBlock avb = this.chain.GetBlock(assumeValidBlock.Hash);
if (avb != null)
{
    ChainedBlock newBlockOnAvbChain = avb.FindAncestorOrSelf(avb);
    if (newBlockOnAvbChain != null)
    {
        skipNewBlockValidation = true;
    }
}

which is highly inefficient due to poor implementation of FindAncestorOrSelf.

[dangershony]

using (this.chain.Lock()) very interesting idea.
I would say though that in your example if any of the conditions is false would indicate a reorg and the processes should not continue, so this may not be a such a good example, but it surly looks like a good idea to expose a public lock, however it will increase the ability to use the lock in a bad way, it might be better to expose such functionality inside ConcurrenctChain where the lock can be held under control.
chain.ContainsAll(assumeValidBlock.Hash, newBlockToValidate.Hash)

As I specified in a previous comment the initial plan is to bringing ConcurrentChain in to the fullnode and making modifications to it that better fit the node requirements.

[Aprogiena]

I agree, there are several ways and introducing such functions like ContainsAll is another one. There is a whole spectrum of solutions, starting from the public lock (super easy to implement, quick fix, but not elegant) and ending with the immutable session chain system as described earlier (most complicated and probably most elegant). And there are these ContainsAll solutions somewhere in the middle.

And yes, bring it in, so we can touch it conveniently.

[bokobza]

@Aprogiena @dangershony found the thread discussing the chain redesign!

[Aprogiena]

i know about this one, it is on our list

[noescape00]

Fixed with activation merge.