That is better but still not quite right. The best option here is to throw from the constructor.

In any case, making a bool public this way is not ideal. Also, this is very specific to what the error is, which breaks abstraction layers.

Ideally, you'd what to just just check if t-he object is in a valid state, and have it return whatever error it encountered if it did encounter one.

But before that, make sure that it actually make sense to have the object in an invalid state to begin with. This introduce temporal coupling, so if this is not needed, just throw from the constructor, or have a factory method that does the right thing.

You can keep getProof maybe? Getting the proof is probably going to be useful at some point. returning an std::optional<const proof&> for instance? Not a blocker as this is not needed for that patch.

If you have a proof, you must register it, right? This is not needed.

That control flow is redundant.

These changes are unrelated to the diff. This makes it hard to review the test portion of the patch. try to make these kind of changes in their own diffs.

I missed the fact that you changed this from initerror to logprintf. This is definitively an init error, and needs to be handled as such.

It seems like what you really need is a factory method that return possible error as out parameters or something along these lines.

Better, but the condition is already a bool, isn't it?

The rest of the codebase returns a bool and pass the error as an out parameter. But in this case, is it even necessary? Why can't this call InitError directly? This is clear meant to initialize things, because if it isn't, then the function needs to be renamed. To me, it seems more like this function is there to ensure that the processor is in some valid state. It seems that initializing g_avalancheis therefore out of scope (after all, the validation logic is not specific to g_avalanche, that same logic would be just as valid for any other instance) and not really about initialization either (which, to begin with belong in init.cpp, but second, this logic remains valid whether it is run during the node startup or at another point in time).

What you want is a factory method.

If you extract part of the logic of the constructor in there, You won't need to twiddle with the internal state of the processor after the fact. Inf act, do you want to build a processor in an invalid state? that doesn't seems like the most solid design.

This assumes intent rather than be informative.

When you reach that point, you cannot know if or how the string needs to be translated. For instance, it may have dynamic parameters in it, and not be translatable anymore as a result at this stage.

You want to bubble up a bilingual_str here.

It is also poor design to assign a specific meaning to an empty string. The rest of the codebase returns a bool of a nullable pointer, and you get the errors and warning as out parameters (see LoadWallet for instance).

You did not introduce that problem, but this is actually incorrect. It is not because avalanche is enabled on the node that the node is providing a service to the network. For instance, if the node doesn't have a proof attached, then it provide no service to the network.

There is no reason to not return a unique pointer here.

This and the constructor are doing this. One of them shouldn't.

Now this method constructs a Processor. It has nothing to do with initialization anymore. if it did, it would be setting g_avalanche and doing other avalanche related initialization procedures, which it doesn't.

You are not using the proof you just de-serialized. As it turns out it happens that the constructor will deserialize the same way right now. But this is fragile. You don't want your code to happen to work. You want your code to work by design.

This temporary doesn't seems necessary.