Skip to content

feat!: verify next consensus params between nodes#550

Merged
lklimek merged 5 commits intov0.10-devfrom
verify-next-consensus-params
Jan 5, 2023
Merged

feat!: verify next consensus params between nodes#550
lklimek merged 5 commits intov0.10-devfrom
verify-next-consensus-params

Conversation

@lklimek
Copy link
Collaborator

@lklimek lklimek commented Jan 4, 2023

Issue being fixed or feature implemented

Consensus params updates are not verified between nodes until they get applied.

Current process of updating consensus params is as follows:

  1. At height H, ABCI App on a node generates and returns to the Tenderdash consensus params update message (as part of ResponsePrepareProposal and ResponseProcessProposal).
  2. Updates to consensus params returned at height H are applied when generating next block (at height H+1).
  3. Updated consensus params returned at height H are reflected in Block.Header.ConsensusHash at height H+1.
  4. Potential invalid (eg. non-deterministic) consensus params update at height H will be detected by other nodes at height H+1, and result in a rejection of proposals referring to invalid consensus params update. In a worst-case scenario, this can cause a chain halt with finalized state H.

To summarize, the process described above means we delay verification consensus params update generated at height H by one block, to H+1.

However, If the state at height H caused a non-deterministic consensus params update, it should be also considered invalid, and we should not finalize it. It means that the current implementation allows the finalization of a possibly invalid state.

An alternative approach is to verify consensus params updates when they are generated - that is, at height H (instead of H+1). It means that the worst-case scenario will lead to a chain halt with finalized state H-1. Implementation of this approach involves adding the hash of NextConsensusParams into Header, increasing the size of the Header.

What was done?

  1. Added NextConsensusHash to Header
  2. Added NextConsensusHash verification

How Has This Been Tested?

Breaking Changes

  • changed the way header hash is calculated - added new field; affects block ID generation

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 relevant unit/integration/functional/e2e tests
  • I have made corresponding changes to the documentation

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

@lklimek lklimek force-pushed the verify-next-consensus-params branch from 067e30f to 1e95c2c Compare January 4, 2023 12:42
@lklimek lklimek changed the title feat: verify next consensus params feat: verify next consensus params between nodes Jan 4, 2023
@lklimek lklimek changed the title feat: verify next consensus params between nodes feat!: verify next consensus params between nodes Jan 5, 2023
Copy link
Collaborator

@shotonoff shotonoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
left one minor question

@lklimek lklimek merged commit cc2090c into v0.10-dev Jan 5, 2023
@lklimek lklimek deleted the verify-next-consensus-params branch January 5, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants