Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

go/consensus/tendermint: Implement {Prepare,Process}Proposal #5285

Merged
merged 8 commits into from Jun 23, 2023

Conversation

kostko
Copy link
Member

@kostko kostko commented Jun 16, 2023

This also makes the nodes execute the proposal in the prepare/process phase such that advanced modification (e.g. including meta transactions based on results) and validation (e.g. rejecting blocks with invalid transactions) becomes possible (although these are not implemented here).

@kostko kostko force-pushed the kostko/feature/prepare-proposal branch from f773431 to 0b287be Compare June 16, 2023 10:12
go/consensus/tendermint/apps/staking/proposing_rewards.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/state.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/storage/mkvs/overlay.go Outdated Show resolved Hide resolved
go/consensus/tendermint/api/block.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
@peternose
Copy link
Contributor

// Update cache of consensus parameters (the only places where consensus parameters can be
// changed are InitChain and EndBlock, so we can safely update the cache here).

Off-topic: The upper comment and the code beneath are incorrect as I think that consensus parameters can also be changed in the BeginBlock, if we are doing an upgrade.

@kostko kostko force-pushed the kostko/feature/prepare-proposal branch 13 times, most recently from dc262fc to 9176a65 Compare June 21, 2023 12:53
@kostko kostko marked this pull request as ready for review June 21, 2023 13:18
Copy link
Contributor

@abukosek abukosek left a comment

Choose a reason for hiding this comment

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

Looks good to me!
The fixes made to the e2e tests should also hopefully reduce the flakiness.

I think Peter's extra logging idea in PrepareProposal in go/consensus/tendermint/abci/mux.go is good, but it might spam the logs too much, so it might be better to add it as a metric instead (but this can be done in a separate PR with perhaps a few other new metrics as well).

@kostko
Copy link
Member Author

kostko commented Jun 22, 2023

Ugh, fuzzing E2E tests indicate this stuff is somewhat racy as this situation is possible after node kill/restart:

  • PrepareProposal is called for a height where the node was a proposer and it successfully executes generating proposal A.
  • The node receives a previous proposal B via P2P for the same height (also proposed by the same node but during a previous run).
  • ProcessProposal is not called for the newly prepared proposal A but is instead called for proposal B which results in corruption.

This seems to contradict the CometBFT spec which says:

For every round, if the local process is the proposer of the current round, CometBFT calls PrepareProposal, followed by ProcessProposal. These two always come together because they reflect the same proposal that the process also delivers to itself.

@peternose
Copy link
Contributor

  • PrepareProposal is called for a height where the node was a proposer and it successfully executes generating proposal A.

Are both proposals (A and B) for the same round?

@kostko
Copy link
Member Author

kostko commented Jun 22, 2023

Yes, both are for the same round, but B is a replay from the old node process (this happens when a node crashes or is killed and an old proposal is replayed). I believe it is as such because of how replay is implemented which first pushes a replay of the timeout event, triggering a new round and a new proposal being generated which fails on the signing step (as this would trigger double signing, but during replay the error is ignored), but then pushes the old proposal which is passed to ProcessProposal.

@kostko kostko force-pushed the kostko/feature/prepare-proposal branch 2 times, most recently from 74d37ac to 398ecd7 Compare June 23, 2023 05:33
@kostko
Copy link
Member Author

kostko commented Jun 23, 2023

Ok, seems to be fixed now, continuing to run the e2e fuzzing tests though.

Also filed an issue upstream (cometbft/cometbft#1018).

txs [][]byte,
lastCommit types.CommitInfo,
misbehavior []types.Misbehavior,
) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
) error {
) {

go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/upgrade.go Outdated Show resolved Hide resolved
go/consensus/tendermint/abci/mux.go Outdated Show resolved Hide resolved
@kostko kostko force-pushed the kostko/feature/prepare-proposal branch from 398ecd7 to b386216 Compare June 23, 2023 09:07
This also makes the nodes execute the proposal in the prepare/process
phase such that advanced modification (e.g. including meta transactions
based on results) and validation (e.g. rejecting blocks with invalid
transactions) becomes possible.
Previously the raw RequestBeginBlock request was distributed to the
various consensus services. This could result in these services using
invalid state (e.g. using the req.Hash in the proposal preparation phase
when this data is not yet available).

These requests are no longer propagated and any relevant state is made
available through the block context.
@kostko kostko force-pushed the kostko/feature/prepare-proposal branch from b386216 to 732f12f Compare June 23, 2023 09:14
@kostko kostko merged commit 7c9fd0f into master Jun 23, 2023
3 checks passed
@kostko kostko deleted the kostko/feature/prepare-proposal branch June 23, 2023 11:34
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.

None yet

3 participants