proptest_state_machine over PeerMessage

[skaunov]

It's rough yet, and I'd appreciate review of the main approaches in it while I'm dealing with TODO and groom the whole contribution.

I understood @aszepieniec that the interactions are subject to change, so currently it's only outlined how an interaction outcome actually asserted (messages the peer-loop sends to the main, and checking standing). I can only repeat current behavior there, and if there's ideas to tune/amend that I guess it would be right place and time for that.

Another decision pending is weighting PeerMessage variants. I guess it's important to add random "bad" once a while, but uniform distribution washes away valid interactions to the margin. I can only roughly guess the distribution you might see fit, so it's the other open question.

[aszepieniec]

I can only repeat current behavior there, and if there's ideas to tune/amend that I guess it would be right place and time for that.

I wonder if the state machine can be factored out such that:

• the actual peer message handlers are a thin wrapper for dealing with network transmissions, but on the inside invoke the state machine;
• the state machine can be tested in isolation of network effects (no-io).

If that's where this is headed, please point me to the location in the code where I can read it.

Another decision pending is weighting PeerMessage variants. I guess it's important to add random "bad" once a while, but uniform distribution washes away valid interactions to the margin. I can only roughly guess the distribution you might see fit, so it's the other open question.

I don't know off the top of my head what the right weights are. Consider writing a patch for master that will record and report relative frequencies of PeerMessage types. I'll be happy to run that to find out what the frequencies are in practice.

[skaunov]

@aszepieniec ,

I can only repeat current behavior there, and if there's ideas to tune/amend that I guess it would be right place and time for that.

I wonder if the state machine can be factored out such that:

* the actual peer message handlers are a thin wrapper for dealing with network transmissions, but on the inside invoke the state machine;

* the state machine can be tested in isolation of network effects (no-io).

If that's where this is headed, please point me to the location in the code where I can read it.

The state machine you're talking about should then be an implementation under testing, not the reference one. I'd be happy to convert something to this approach, but PeerMessage has quite a variability. Do you have a smaller one object to start with on your mind?

Current implementation indeed heading the way you described. It process the PeerMessage at https://github.com/skaunov/neptune-core/blob/fa768a9db2f891b4531a7973c8982cf100a3171a/src/models/peer/tests/automaton.rs#L166 and then checks the main loop channel and standing. (But is checking for nothing yet: I draft where and how it should be done, and am asking for what to actually check.)

What do you think about not creating proptest-state-machine for PeerMessage at all and instead implement Strategy for [Mock]

neptune-core/src/tests/shared.rs

Line 365 in 73ec193

pub struct Mock<Item> {

and some #[test] inside a proptest!? That feels more elegant since state machine brings benefits when there are actual state changes and we're checking it can't find itself in a wrong one. And as with PeerMessage it's by design looping in the single state that makes inevitable proptest-state-machine managing code just a boilerplate. =(

Another decision pending is weighting PeerMessage variants. I guess it's important to add random "bad" once a while, but uniform distribution washes away valid interactions to the margin. I can only roughly guess the distribution you might see fit, so it's the other open question.

I don't know off the top of my head what the right weights are. Consider writing a patch for master that will record and report relative frequencies of PeerMessage types. I'll be happy to run that to find out what the frequencies are in practice.

I don't see the point repeating the real freqs in Strategy<PeerMessage> since the whole idea is to give attention to those rare cases which aren't happen there. I would propose then just depress the bad messages to happen once in a 100(?) messages on average. And those bad messages are complete gibberish so I guess those are not very useful, but to have few of those is good for testing.

[skaunov]

@aszepieniec
So, the state machine crash the node as was the aim of the task initially. On the other hand, due to the nature of proptest it does this in quite a trivial way just hitting some of handful unwrap at like <src/models/state/archival_state.rs>. Though unwrap in production code is still a bad pattern, and a right design choice for this is beyond my current knowledge of the system: expect alone won't save it obviously; should that return Result to be able to punish the peer; or should such thing be caught on deserialization and those Option should be processed earlier so PeerMessage would always have the complete info.

[aszepieniec]

We have to analyze these unwraps on a case-by-case basis. There is no one-size-fits-all solution. It could be that

• a) the unwrap is benign given the context and the test should be modified to take that context into account; or
• b) the unwrap is a vulnerability that should be refactored away -- somehow.
• c) (I might be missing options.)

Can you point us to a few unwraps that are being triggered by the new tests, so that we can start that discussion? Or point us to the tests that do the triggering.

[skaunov]

I need to think about "c)"; the first two seems quite extremes of the spectrum yet. 😵‍💫 And I really like the approach with examples.

For a quick peek I hit

neptune-core/src/models/state/archival_state.rs

Line 589 in e78ccf5

.unwrap()

. Isn't it better to communicate with an Err that the record for tip_digest isn't there. Currently I can imagine how match will be replaced with ? at some point, and then if .unwrap() will be replaced with another ? the behavior will be changed to a wrong one.

[aszepieniec]

This unwrap() is triggered if

• a) the block record's key is present in the database; but
• b) the block record's value (i.e., the block record itself) is not.

It is fair to assume that the database is not corrupt. If the database is corrupt so as trigger this scenario, there is no way to avoid an application crash. Passing around Err variants stays execution temporarily but ultimately moves it to a place that is further from the root cause. So I would argue against passing around Results.

Rather than uninformatively using unwrap(), I would prefer to make the assumption about databases not being corrupt explicit using an expect(..).

Flagging @Sword-Smith for double-checking but in my estimation and based on what I think I don't know there is only a 10% probability that he disagrees with this recommendation.

[skaunov]

@aszepieniec , I'd create a new issue from the previous comment since changing to expect is needed anyway. Tell me if that's a bad idea, pls.

Let's look at another example. I dug into, but not exhaustively. The test hits https://github.com/skaunov/neptune-core/blob/8698d30980b1bafafc58622fba915df07e890fb5/src/models/state/archival_state.rs#L503; it's called in the pub function https://github.com/skaunov/neptune-core/blob/8698d30980b1bafafc58622fba915df07e890fb5/src/models/state/mod.rs#L254 (guarded by ...Lock but it's already sounds like a borderline in this context) which doesn't check new_block height. My point is that it seems to possible to crash a node with this expect, and an Error for the wrong height would communicate to check this when calling the function.

[Sword-Smith]

@aszepieniec , I'd create a new issue from the previous comment since changing to expect is needed anyway. Tell me if that's a bad idea, pls.

Let's look at another example. I dug into, but not exhaustively. The test hits https://github.com/skaunov/neptune-core/blob/8698d30980b1bafafc58622fba915df07e890fb5/src/models/state/archival_state.rs#L503; it's called in the pub function https://github.com/skaunov/neptune-core/blob/8698d30980b1bafafc58622fba915df07e890fb5/src/models/state/mod.rs#L254 (guarded by ...Lock but it's already sounds like a borderline in this context) which doesn't check new_block height. My point is that it seems to possible to crash a node with this expect, and an Error for the wrong height would communicate to check this when calling the function.

Wouldn't this expect only be hit if someone tried to set a block with height 0 as the new tip? Block height 0 is by definition the genesis block, and the genesis block is considered code, not data, so it can never be changed while the node is running.

[skaunov]

@Sword-Smith , oh I misinterpreted pruning there - thanks! So is it possible to crash a node with the genesis for the tip? 🤔

[Sword-Smith]

@Sword-Smith , oh I misinterpreted pruning there - thanks! So is it possible to crash a node with the genesis for the tip? 🤔

A peer shouldn't be able to do that. If a peer disagrees on the Genesis block, that should trigger an automatic permaban of that peer.