[Docs] Document consensus orchestration by red-0ne · Pull Request #951 · pokt-network/pocket

red-0ne · 2023-07-30T20:33:36Z

Description

Document consensus orchestration from the POV of a validator node

Summary generated by Reviewpad on 04 Aug 23 10:35 UTC

This pull request includes the following changes:

Updated the functions in file1 to improve performance.
Refactored code in file2 for better readability.
Fixed a bug in file3 that was causing incorrect output.
Added new feature in file4 to enhance functionality.

These are the main changes made in the diff. Let me know if you need further details on any of these changes.

Issue

Fixes #919

Type of change

Please mark the relevant option(s):

Testing

make develop_test; if any code changes were made
make test_e2e on k8s LocalNet; if any code changes were made
e2e-devnet-test passes tests on DevNet; if any code was changed
Docker Compose LocalNet; if any major functionality was changed or introduced
k8s LocalNet; if any infrastructure or configuration changes were made

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 added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
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)

Olshansk

@red-0ne I reviewed this up to the "### Apply routine" section and will review the second half tomorrow. So far this looks amazing!

consensus/README.md

Olshansk

@red-0ne I reviewed this up until ### PaceMaker block proposal delaying and will pick it up again tomorrow.

I believe some of the statements here (e.g. regarding signature swapping, time, etc..) are not completely correct and worded too strongly (without the appropriate qualifiers) that could lead people in the wrong direction. For example, see discussion in #953.

I explicitly avoided diving into a deep discussion/edits since I'm not 100% sure how familiar you are with the technical concepts.

My request:

Can you do a self-review and update some of the wording appropriately?
If I'm being too vague and you need me to provide a detailed explanation, please let me know and I will happily do so.

consensus/README.md

Olshansk · 2023-08-08T19:19:24Z

@RossiNYC I believe you were added to this PR by accident. Please ignore. I've removed you from the list of reviewers.

Olshansk · 2023-08-08T19:38:10Z

consensus/README.md

+
+### PaceMaker block proposal delaying
+
+The pace maker ensures minimum block production time with the aim to have a constant production pace.


pacemaker is one word

Olshansk

Great addition to the documentation! If anything is unclear in the future, we'll iterate on it 💯

0xRampey

Super helpful documentation! Just added some grammatical changes

0xRampey · 2023-08-09T15:11:08Z

consensus/README.md

+* With this chain of trust form a total of `3t+1` validators, where at least `2t+1` validators are honest and live. A synching node systematically detects invalid blocks
+  * No malicious or faulty node could inject an alternative block without making at least `2t+1` validators sign it
+  * The persistence layer is used as a resume point for the block application, so a node won't restart block application from genesis each time it's rebooted
+* When the routine applies `NetworkCurrentHeight`, it signals it so the node could switch to `consensus` mode. Meanwhile, it waits to apply a new downloaded block


Suggested change

* When the routine applies `NetworkCurrentHeight`, it signals it so the node could switch to `consensus` mode. Meanwhile, it waits to apply a new downloaded block

* When the routine applies a block with height that matches `NetworkCurrentHeight`, it triggers a signal, prompting the node to switch to `consensus` mode. Meanwhile, it waits to apply a new downloaded block.

0xRampey · 2023-08-09T15:11:29Z

consensus/README.md

+* The `apply` mechanism needs to maintain a chain of trust while applying blocks by performing the following:
+  * Before applying block at height `h`, verify that it is signed by a quorum from the validator set at height `h-1`; note that the genesis validator set is used for block `1`
+  * By applying each block, the validator set is updated (validators joining or leaving), starting from genesis validator set for any new node
+* With this chain of trust form a total of `3t+1` validators, where at least `2t+1` validators are honest and live. A synching node systematically detects invalid blocks


Suggested change

* With this chain of trust form a total of `3t+1` validators, where at least `2t+1` validators are honest and live. A synching node systematically detects invalid blocks

* With this chain of trust form a total of `3t+1` validators, where at least `2t+1` validators are honest and live. A syncing node systematically detects invalid blocks

0xRampey · 2023-08-09T15:26:13Z

consensus/README.md

+It has a bootstrapping state where it:
+  * Initializes connections to the network through a bootstrap node
+  * Keeps an updated current height (the greatest block height seen on the network)
+  * Compares network and local current heights, before switching to one of two mutually exclusive modes: `sync` or `consensus`


There's another doc which goes over StateSync in more detail over here https://github.com/pokt-network/pocket/blob/consensus-orchestration-doc/consensus/doc/PROTOCOL_STATE_SYNC.md#state-sync-lifecycle.

The names used here are different from the above doc such as the types of mode.
I'd expect a dev would want to go here for more in-depth info about state sync, so do we

Leave a link to the doc here? or maybe

Reconcile the terms used between both docs?

I think the idea is to use terminology that is as close as possible to the variable names used in the codebase.

cc: @Olshansk for your thoughts

[Docs] Document consensus orchestration

193f7eb

red-0ne added documentation Improvements or additions to documentation consensus Consensus specific changes labels Jul 30, 2023

red-0ne added this to the M1: Pocket PoS (Proof of Stake) milestone Jul 30, 2023

red-0ne requested a review from Olshansk July 30, 2023 20:33

red-0ne self-assigned this Jul 30, 2023

reviewpad bot added medium Pull request is medium docs labels Jul 30, 2023

Olshansk requested a review from 0xRampey July 31, 2023 00:31

small addition on pace maker and global pace

af87895

red-0ne marked this pull request as ready for review July 31, 2023 03:25

reviewpad bot added the waiting-for-review label Jul 31, 2023

[Docs] Document PaceMaker behavior

30d1115

Olshansk requested changes Aug 1, 2023

View reviewed changes

address review changes

242362b

Olshansk requested changes Aug 1, 2023

View reviewed changes

red-0ne added 2 commits August 4, 2023 11:22

address change requests

164ef29

document pacemaker disabling when manual mode is on

9fa6d51

red-0ne requested review from Olshansk and RossiNYC August 4, 2023 10:38

Olshansk removed the request for review from RossiNYC August 8, 2023 19:18

Olshansk reviewed Aug 8, 2023

View reviewed changes

Olshansk approved these changes Aug 8, 2023

View reviewed changes

0xRampey reviewed Aug 9, 2023

View reviewed changes


		### PaceMaker block proposal delaying

		The pace maker ensures minimum block production time with the aim to have a constant production pace.

	* When the routine applies `NetworkCurrentHeight`, it signals it so the node could switch to `consensus` mode. Meanwhile, it waits to apply a new downloaded block
	* When the routine applies a block with height that matches `NetworkCurrentHeight`, it triggers a signal, prompting the node to switch to `consensus` mode. Meanwhile, it waits to apply a new downloaded block.

	* With this chain of trust form a total of `3t+1` validators, where at least `2t+1` validators are honest and live. A synching node systematically detects invalid blocks
	* With this chain of trust form a total of `3t+1` validators, where at least `2t+1` validators are honest and live. A syncing node systematically detects invalid blocks

Comments

Conversation

red-0ne commented Jul 30, 2023 • edited by reviewpad bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Summary generated by Reviewpad on 04 Aug 23 10:35 UTC

Issue

Type of change

Testing

Required Checklist

If Applicable Checklist

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Olshansk commented Aug 8, 2023

Uh oh!

Olshansk Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

Olshansk left a comment

Choose a reason for hiding this comment

Uh oh!

0xRampey left a comment

Choose a reason for hiding this comment

Uh oh!

0xRampey Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xRampey Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

0xRampey Aug 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

red-0ne commented Jul 30, 2023 •

edited by reviewpad bot

Loading

0xRampey Aug 9, 2023 •

edited

Loading

0xRampey Aug 9, 2023 •

edited

Loading