Proposal: make method IsInitialBlockDownload() null-safe

[soma42]

https://github.com/block-core/blockcore/blob/master/src/Blockcore/Base/InitialBlockDownloadState.cs

When a component starts up, it might call this method quite early. Then, it throws a NullReferenceException.

So, when working with the code, your debugger will always break there. And you do not want the debugger to skip NullReferenceException either.

So the best would really be to rewrite this so that it it doesn't throw any Exception. If something is still null here, it should simply return true.

[dangershony]

This method is heavily called through out the node so it needs to be dealt with carefully. I suppose an additional null check if condition is ok, but it also implies the caller is making requests when node is not init yet. Once needs to monitor the node.status fields to get idea of if the node has finished loading.

[sondreb]

Then perhaps the UI should be updated to perform a feature loaded check instead of checking the IBD. I reported this earlier as well, but it would be a better solution to check status in the UI, then the UI could render some "currently loading" status screen.

This would be extremely valuable for rollbacks of the chain state, which happens from time to time and would be nice if the UI showed users the rollback status.

[dangershony]

This would be extremely valuable for rollbacks of the chain state, which happens from time to time and would be nice if the UI showed users the rollback status.

well the load status is only for startup, we don't really have a reorg state option (but we could have one using the block disconnected event)

@MithrilMan proposed (and we discussed that in the past too) to add a reorg lock flag that can maybe help in this cases too.

[soma42]

@dangershony thanks for the tip with the node state! I have tried this out now, and you are right: in my case the NullreferenceException is thrown while fullNode.State is 'Starting'.

How do I get there? I call IsInitialBlockDownload() from a c'tor that gets executed via dependency injection. So this is basically my fault. I guess the right hook to start-up your stuff is always from the InitializeAsync method of a FullNodeFeature. Or bear the consequences, and just handle Exceptions such as I report here.

However, here is the documentation for the method:

public interface IInitialBlockDownloadState
    {
        /// <summary>
        /// Checks whether the node is currently in the process of initial block download.
        /// </summary>
        /// <returns><c>true</c> if the node is currently doing IBD, <c>false</c> otherwise.</returns>
        bool IsInitialBlockDownload();
    }

It correctly says what the return values true and false mean. But it should also be documented what Exceptions it throws. See for example the documentation of Path.GetFileName(...): https://docs.microsoft.com/en-us/dotnet/api/system.io.path.getfilename?view=netcore-3.1 It's nicely documented that it can throw an ArgumentException. That enables the developer to use a non-general catch-clause: catch(ArgumentException) { }

Following that style or documentation, it could be added that IsInitialBlockDownload() throws a NullReferenceExeption.

Then it's complete and the developer knows he has to handle three cases.

Of course documenting a NullReferenceExecption to cover such a case would be a bit odd. Instead, one could check ConsensusTip for null and throw like so:

if(this.chainState.ConsensusTip?.Header == null) 
    throw new InavlidOperationException("IFullNode.State must be FullNodeState.Started before you can call this method. It is recommended to check IBD state only after FullNodeFeature.InitializeAsync has been called.");

Other alternatives:

• The method could block until chainState.ConsensusTip?.Header is no longer null or the correct state is reached.
• A decision is made that null shall mean 'true'. That would make this method some kind of shorthand to check whether the node is initialized.
• just an inline comment in the code: // if this is null then the node isn't in State 'Started' yet.

[soma42]

Ok, I tested this now, in the FullNodeFeature I tried:

But that's not the solution. Next version:

This works! After waiting for Started state, I can call IsInitialBlockDownload() without causing a NullReferenceException.

[MithrilMan]

Imo nobody should call IsInitialBlockDownload before node has started, so an idea can be to throw an explicit exception if IsInitialBlockDownload is called too early, stating something like "IsInitialBlockDownload has to be called after node is started" or something meaningful like that.

And instead of using fields, I'd create an event message like NodeStartedEvent that's published when the node enters into "Started" state so that components that need that information before implementing their logic, could subscribe to in order to trigger what they need (a Delay is never a good solution anyway)

[soma42]

@MithrilMan all you say is correct, but I still think IsInitialBlockDownload should return TRUE, instead of throwing a custom exception or NullreferenceException. A node always starts up in IBD, if only to see there is nothing to do. So if a caller receives TRUE, the caller knows the node is still syncing or otherwise not ready, and that is all the callers wants to know. The details are not that important. The added value would be that a custom fullnode module can then simply call IsInitialBlockDownload(), and if it returns false, the fullnode module knows it can start it's work. If you throw a special Exception with a help text, that's not really helpful.

We could define, that if IsInitialBlockDownload() returns false, it also means FullNodeState.Started.

[soma42]

What I see from the code I posted above is that the NullReferenceException can happen if you call IsInitialBlockDownload() from InitializeAsync(). Since that is the correct way to enter a custom FullNodeModule, the call should really not throw. In other words, maybe the problem is that InitializeAsync() gets called before the node is in Started state. What do you think @dangershony ?

[dangershony]

I think that's a reasonable fix yes, do you ant to submit a PR?