[Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) #520

deblasis · 2023-02-16T20:28:34Z

Description

This PR has been extracted from #491 and is, hopefully, more digestible from a code-review and scope point of view.

In a nutshell:

It introduces the a Finite State Machine that is meant to govern the internal state, transitions and events
It includes a refactoring of our module initialization pattern using functional options (https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)
It improves the module registration with a first class... you might have guessed it: ModulesRegistry
It reduces boilerplate code making our modules more DRY with the introduction of base_modules that provide basic functionality (that can still be customized/extended when needed)

Issue

Fixes #499

Type of change

Please mark the relevant option(s):

List of changes

In a nutshell:

It introduces the a Finite State Machine that is meant to govern the internal state, transitions and events
It includes a refactoring of our module initialization pattern using functional options (https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)
It improves the module registration with a first class... you might have guessed it: ModulesRegistry
It reduces boilerplate code making our modules more DRY with the introduction of base_modules that provide basic functionality (that can still be customized/extended when needed)

Testing

make develop_test
LocalNet w/ all of the steps outlined in the README

Required Checklist

I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have tested my changes using the available tooling
I have updated the corresponding CHANGELOG

If Applicable Checklist

I have updated the corresponding README(s); local and/or global
I have added tests that prove my fix is effective or that my feature works
I have added, or updated, mermaid.js diagrams in the corresponding README(s)
I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

shared/modules/module.go

state_machine/docs/state-machine.diagram.md

Olshansk

Left a few comments/suggestions but nothing blocking. Approving so we can get the ⛵ moving.

This was VERY easy to read and review. I know it was painful, but truly appreciate it!!

consensus/e2e_tests/utils_test.go

Olshansk · 2023-02-17T03:54:00Z

consensus/module.go

 func (m *consensusModule) SetBus(pocketBus modules.Bus) {
-	m.bus = pocketBus
+	m.IntegratableModule.SetBus(pocketBus)


Since we're embedding IntegradbleModule, you should be able to use pocketBus directly

Your example is not representative of the scenario.
In here we are overloading something that's handled already in the embedded struct.

If I did what you are suggesting I guess we would have this 🤔:

Let's play dumb and try it out...
Actually the behaviour is even weirder:

overloading.mp4

nothing happens!

If I try to "walk my way into the stack"... it stops there, suggesting that my solution seems to be the right approach.

overloading2.mp4

If you wanna play along:
https://go.dev/play/p/s8ncxTVMMp6

Okay, I understand what I was missing and why you're solution works.

However, now I'm trying to understand:

We're not calling the parent's foo function (bey default)

We're not entering an infinite recursion

I can understand why (1) is not happening, but without researching, this makes me feel that the playground abstracts (2) away for us.

runtime/bus.go

shared/modules/doc/README.md

shared/core/types/states.go

shared/modules/module.go

state_machine/module.go

state_machine/docs/README.md

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

…ssue/499-fsm

…429) (#521) ## Description This PR has been extracted from #491 and is, hopefully, more digestible from a code-review and scope point of view. The main goal is to remove hardcoded nodes and move towards a more dynamic environment. It's also highlighting the potential entry points for subsequent P2P work The code leverages the abstractions added recently (`currentHeightProvider` and `addressBookProvider`) to fetch the data from an RPC endpoint. ## Issue Fixes #416 Fixes #429 ## Type of change Please mark the relevant option(s): - [x] New feature, functionality or library - [ ] Bug fix - [ ] Code health or cleanup - [x] Major breaking change - [ ] Documentation - [ ] Other  ## List of changes ### CLI - Updated CLI to use to source the address book and the current height from the RPC server leveraging the `rpcAddressBookProvider` and `rpcCurrentHeightProvider` respectively and the `bus` for dependency injection ### P2P - Modules embed `base_modules.IntegratableModule` and `base_modules.InterruptableModule` for DRYness - Deprecated `debugAddressBookProvider` - Added `rpcAddressBookProvider` to source the address book from the RPC server - Leveraging `bus` for dependency injection of the `addressBookProvider` and `currentHeightProvider` - Deprecated `debugCurrentHeightProvider` - Added `rpcCurrentHeightProvider` to source the current height from the RPC server - Fixed raintree to use the `currentHeightProvider` instead of consensus (that was what we wanted to avoid in the first place) - Added `getAddrBookDelta` to calculate changes to the address book between heights and update the internal state and componentry accordingly - Reacting to `ConsensusNewHeightEventType` to update the address book - Updated tests ### RPC - Updated RPC to expose the node's address book via GET /v1/p2p/staked_actors_address_book ## Testing - [x] `make develop_test` - [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`  ## Required Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have tested my changes using the available tooling - [x] I have updated the corresponding CHANGELOG ### If Applicable Checklist - [ ] I have updated the corresponding README(s); local and/or global - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have added, or updated, [mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding README(s) - [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s) --------- Signed-off-by: Alessandro De Blasis <alex@deblasis.net> Co-authored-by: Dmitry K <okdas@pm.me> Co-authored-by: Dmitry Knyazev <okdas@users.noreply.github.com> Co-authored-by: Daniel Olshansky <olshansky@pokt.network> Co-authored-by: Daniel Olshansky <olshansky.daniel@gmail.com>

* pokt/main: [Infra] KISS 3 - Cluster Manager [Merge me after #521] - (Issues: #490) (#522) Refactor/fix state sync logs (#515) [P2P] KISS 2 - Peer discovery [Merge me after #520] - (Issues: #416, #429) (#521) [Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) (#520) [CLI] Stake command bugfix (#518) [CLI] Cannot run make localnet_client_debug: Cannot initialise the keybase with the validator keys: Unable to find YAML file (#517) Fix the link shown by `make go_doc` Fixed duplicate GITHUB_WIKI tag [Documentation] Update Devlog Formatting (#512) [Docs & Bugs] Minor fixes post keybase changes (#513) [Utility] Foundational bugs, tests, code cleanup and improvements (1 / 2) (#503) [Tooling] Integrate Keybase w/ CLI (Issue #484 ) (#501) update devlog2.md update devlog2.md Update devlog1.md

deblasis added 30 commits February 16, 2023 17:19

feat(shared): module Create with ModuleOptions

143feaa

docs(shared): godoc for ModuleOption

a046984

docs(shared): updated README

82505e4

feat(shared): base_modules

da9774b

fix(utility): logging error when ApplyTransaction fails

f784dab

refactor(utility): implementing base_modules and options

049023c

fix(utility): using logger in message_handler

a18ecf5

docs(utility): changelog

e01c75c

refactor(telemetry): implementing base_modules and options

142d53b

docs(telemetry): changelog

3c84eae

feat(shared): ModulesRegistry interface

4eed45a

refactor(shared): updated bus interface to use ModuleRegistry

b094622

refactor(persistence): implementing base_modules and options

e6e8fe5

docs(persistence): changelog

f8ed453

refactor(consensus): implementing base_modules and options

2a21542

refactor(logger): implementing base_modules and options

e70b308

docs(logger): changelog

0d20ada

refactor(rpc): implementing base_modules and options

963212a

docs(rpc): changelog

8871a3d

feat(runtime): ModulesRegistry implementation

8d50538

feat(p2p): implementing base_modules and options

5141f07

refactor(state_sync): implementing base_modules and options

1d98c9f

refactor(runtime): bus uses ModulesRegistry and logger

f5f9777

fix(runtime): StateMachine doesn't exist yet :)

4c5f50c

refactor(runtime): errors referring ModulesRegistry

7023ca0

refactor(runtime): runtimeManager implements base IntegratableModule

34f3d87

feat(state_machine): KISS FSM implementation

74d012f

feat(shared): state_machine integration with node

d697a7f

feat(shared): FSM states and events

8e89219

refactor(shared): messaging events

c66a5c8

fix(p2p): fixes from backporting legacy functionality

9bf8b81

deblasis added core Core infrastructure - protocol related p2p P2P specific changes consensus Consensus specific changes code health Nice to have code improvement labels Feb 16, 2023

deblasis added this to the M1: Pocket PoS (Proof of Stake) milestone Feb 16, 2023

deblasis self-assigned this Feb 16, 2023

deblasis marked this pull request as ready for review February 16, 2023 20:28

docs(shared): changelogs

724a326

deblasis commented Feb 16, 2023

View reviewed changes

shared/modules/module.go Show resolved Hide resolved

deblasis commented Feb 16, 2023

View reviewed changes

state_machine/docs/state-machine.diagram.md Show resolved Hide resolved

deblasis requested a review from Olshansk February 16, 2023 20:55

deblasis added 2 commits February 16, 2023 21:10

feat(consensus): improved test resiliency

c0cdeac

chore(consensus): lint

e5c93f7

deblasis changed the title ~~[Core] KISS - Finite State Machine~~ [Core] KISS - Finite State Machine [Merge me first] - (Issue: #499) Feb 16, 2023

deblasis changed the title ~~[Core] KISS - Finite State Machine [Merge me first] - (Issue: #499)~~ [Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) Feb 17, 2023

Olshansk approved these changes Feb 17, 2023

View reviewed changes

deblasis and others added 7 commits February 17, 2023 09:31

Update state_machine/docs/README.md

7381c8b

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

Update consensus/e2e_tests/utils_test.go

51e971e

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

refactor(runtime): improved module getters via generics

43d31cd

chore(shared): renamed fsm states and events files

61bdd35

Update shared/modules/doc/README.md

65ac7ce

Co-authored-by: Daniel Olshansky <olshansky@pokt.network>

Merge branch 'issue/499-fsm' of github.com:pokt-network/pocket into i…

8c5ad05

…ssue/499-fsm

docs(shared): changelogs dates

9fe0dc6

deblasis merged commit d7060b1 into main Feb 17, 2023

deblasis deleted the issue/499-fsm branch February 17, 2023 10:20

deblasis mentioned this pull request Feb 17, 2023

KISS Peer discovery, KISS Bootstrapping, Nodes auto-stake/unstake via K8S cluster-manager, Node Finite state machine - Issues #416, #490, #429, #498, #499 #491

Closed

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) #520

[Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) #520

deblasis commented Feb 16, 2023 •

edited

Loading

Olshansk left a comment

Olshansk Feb 17, 2023

deblasis Feb 17, 2023

Olshansk Feb 17, 2023

[Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) #520

[Core] KISS 1 - Finite State Machine [Merge me first] - (Issue: #499) #520

Conversation

deblasis commented Feb 16, 2023 • edited Loading

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk left a comment

Choose a reason for hiding this comment

Olshansk Feb 17, 2023

Choose a reason for hiding this comment

deblasis Feb 17, 2023

Choose a reason for hiding this comment

Olshansk Feb 17, 2023

Choose a reason for hiding this comment

deblasis commented Feb 16, 2023 •

edited

Loading