Page MenuHomePhabricator

[avalanche] move proof verification code to processor.cpp
AbandonedPublic

Authored by PiRK on Apr 5 2021, 09:07.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Summary

This moves the verification of the proof at node startup from init.cpp to processor.cpp.

A factory method is used to get the error message for InitError.

This introduces a minor change from the previous behavior, which I consider to be an improvement: we used to check the proof only if -enableavalanche was set and -avaproof was specified, but now the proof is checked if specified even in the absence of -enableavalanche.

Test Plan

ninja all check-all

Event Timeline

PiRK requested review of this revision.Apr 5 2021, 09:07
deadalnix requested changes to this revision.Apr 6 2021, 15:26
deadalnix added a subscriber: deadalnix.

likestamp

Some open question, but this is better.

src/init.cpp
2437 ↗(On Diff #28072)

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.

2444 ↗(On Diff #28072)

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.

This revision now requires changes to proceed.Apr 6 2021, 15:26

address review

I will do getProof in another diff, I still need to figure out how to not return a copy of the proof.

remove unecessary import and else clause (the if ends with a throw)

deadalnix requested changes to this revision.Apr 7 2021, 17:04
deadalnix added inline comments.
src/avalanche/processor.cpp
216 ↗(On Diff #28079)

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

477–480 ↗(On Diff #28079)

That control flow is redundant.

test/functional/abc_rpc_avalancheproof.py
68 ↗(On Diff #28079)

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.

This revision now requires changes to proceed.Apr 7 2021, 17:04

revert unrelated style changes in the test, use ternary operator in hasLocalProof instead of control flow

Regarding mustRegisterProof, it scan be improved in another diff. Currently we check at each tip change if the node is now out of IBD and if this flag is set. When the proof is registered, the flag is reset to false. I'm not sure how to detect a "node just finished IBD" event without this flag. Anyway, this is unrelated to this diff, I just moved the line.

I noticed that I removed a block scope that used to contain only the Proof deserialization. I'm not sure why it was there. Should I restore it?

deadalnix requested changes to this revision.Apr 8 2021, 13:16

You need to read your code, it is telling you things and you are ignoring these things.

src/avalanche/processor.cpp
210 ↗(On Diff #28082)

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.

477 ↗(On Diff #28082)

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

This revision now requires changes to proceed.Apr 8 2021, 13:16

rebase onto D9394. This solves the issue of getLocalProof making an unnecessary copy, or hasLocalProof returning a boolean.

@deadalnix In an earlier review you suggested returning a std::optional<Proof &>, but as far as I can tell C++ does not allow optional references.

Changes still planned: figure out a way to raise the initError with a specific message, insted of using LogPrintf followed by a generic message.

PiRK planned changes to this revision.Apr 11 2021, 08:17

remove the need to check avaproof in init.cpp by moving the logging into the Processor's constructor.

PiRK planned changes to this revision.Apr 12 2021, 07:29
PiRK edited the summary of this revision. (Show Details)

use a factory method that return possible error, restore the previous behavior of InitError instead of LogPrintf

Undo some unecessary changes to make this diff as minimalistic as possible

deadalnix requested changes to this revision.Apr 12 2021, 14:45
deadalnix added inline comments.
src/avalanche/processor.cpp
164 ↗(On Diff #28139)

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.

167 ↗(On Diff #28139)

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.

229 ↗(On Diff #28139)

This assumes intent rather than be informative.

src/init.cpp
2438 ↗(On Diff #28139)

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).

2443 ↗(On Diff #28139)

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.

This revision now requires changes to proceed.Apr 12 2021, 14:45

address review: check the proof before initializing a Processor, rename Processor::Init -> Processor::MakeProcessor, make it return a Processor * or a nullptr (if the proof verification failed), call InitError direcly in processor.cpp to avoid error messages as out parameters,

deadalnix requested changes to this revision.Apr 12 2021, 16:46

You'll keep shuffling things around until you ask yourself the questions "What does the initialization?" "What does fetch the config?" "What does verify the validity of the different parameters?" and so on. Once you got the answers to these question, you need to apply them consistently.

This is the essence of software design, and this is how one can grow an application to large sizes while keeping it manageable.

src/avalanche/processor.cpp
167

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

177

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

196

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.

205

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.

src/init.cpp
2435

This temporary doesn't seems necessary.

This revision now requires changes to proceed.Apr 12 2021, 16:46