cometbft / cometbft

CometBFT (fork of Tendermint Core): A distributed, Byzantine fault-tolerant, deterministic state machine replication engine
https://docs.cometbft.com
Apache License 2.0
592 stars 415 forks source link

Conform consensus state to Tendermint technical specification #1171

Closed BrendanChou closed 11 months ago

BrendanChou commented 1 year ago

Bug Report

Setup

CometBFT version: v0.37.2 and before

Have you tried the latest version: Yes

ABCI app: N/A

Environment: N/A

node command runtime flags: N/A

Config

N/A

What happened?

Discrepancies between the Tendermint implementation code and whitepaper were found https://github.com/tendermint/tendermint/issues/6849 https://github.com/tendermint/tendermint/issues/6850

What did you expect to happen?

These were fixed in Tendermint core in 2021, these changes did not seem to make it to CometBFT somehow, perhaps due to a code rollback of some sort

How to reproduce it

N/A

Anything else we need to know

These fixes are considered to block larger, previously-accepted consensus changes such as Proposer-Based Timestamps https://github.com/tendermint/tendermint/issues/6942

sergio-mena commented 12 months ago

Taking a close look at tendermint/tendermint#6849 and tendermint/tendermint#6850 before forward-porting them to main (as a first step... then back to the relevant branches: possibly v0.38.x and v0.37.x)

BrendanChou commented 12 months ago

The main difference for https://github.com/tendermint/tendermint/issues/6850 is that it was written before ProcessProposal existed. Therefore, the fall-through-case (the case when reaching the end of the function) is now different from when it was written, requiring a rewrite.

https://github.com/tendermint/tendermint/issues/6849 can be ported largely unchanged, but also includes some minor cleanup which I consider unnecessary to include in the same PR. I have ported it here https://github.com/cometbft/cometbft/pull/1175

sergio-mena commented 11 months ago

Closed by #1175, #1207, and #1203