openmina / openmina

Mina Rust Node
Apache License 2.0
78 stars 19 forks source link

As a Developer, I need statemachine architecture suitable for development, so I can easily understand and modify the node's implementation #187

Open akoptelov opened 9 months ago

akoptelov commented 9 months ago

Definition of DONE:

Currently there are lots of complaints about the way statemachine is implemented currently (should be sorted by severity, then subitem created for each item):

Here is the document about new/experimental statemachine implementation by @dkuehr: https://github.com/openmina/state_machine_exp/blob/main/node/README.md

And Here is a doc from @tizoc with comparison and analysis of the current and experimental approaches that is extremely relevant: https://gist.github.com/tizoc/c359f77769b61147060aa1ba31aeca49

tizoc commented 9 months ago

Enabling condition semantics should be more strict

@akoptelov can you expand on this? My issue with enabling conditions is different, I just don't find think that the current approach of having effect handlers dispatching all potentially useful actions that then get filtered by enabling conditions a good idea (to me, they make the code much harder to follow and reason about)

akoptelov commented 9 months ago

can you expand on this?

Well, this is mostly what I meant. If we have a violated enabling condition in runtime, this means program error that should be reported and fixed eventually. For testing/fuzzing this might be different though.

binier commented 9 months ago

If we have a violated enabling condition in runtime, this means program error that should be reported and fixed eventually. For testing/fuzzing this might be different though.

@akoptelov We could add dispatch_opt for optional dispatch (to use enabling condition for filtering). Otherwise by default dispatch would cause error log, or whatever the new error handler passed to the redux::Store does. Any thoughts?

akoptelov commented 9 months ago

@akoptelov We could add dispatch_opt for optional dispatch (to use enabling condition for filtering). Otherwise by default dispatch would cause error log, or whatever the new error handler passed to the redux::Store does. Any thoughts?

Yes, that might be an option. But I think this should be done case-by-case: sometimes we should be more explicit about effects.

E.g. for long-short forks, would it be better to have that dispatch_opt() || dispatch() expression, or explicit if is_short_range() ...?

But yeah, this solution firstly makes it obvious that the action might not be dispatched, and otherwise logs an error (and possibly panic in debug).

tizoc commented 8 months ago

E.g. for long-short forks, would it be better to have that dispatch_opt() || dispatch() expression, or explicit if is_short_range() ...?

Personally, for situations where concurrency is not much of an issue and clear flow is more important (case in point, the transition frontier) I would very much rather have the later.

tizoc commented 8 months ago

This is preliminar and not complete, but here is @dkuehr's doc on his implementation: https://gist.github.com/dkuehr/6a0292036becdddd3a5853a18ebf08c2

akoptelov commented 8 months ago

This is preliminar and not complete, but here is @dkuehr's doc on his implementation: https://gist.github.com/dkuehr/6a0292036becdddd3a5853a18ebf08c2

Added that to the top comment as well.