ABCI++ Prepare/Process Proposal

[thanethomson]

In this work, we effectively need to rebuild ABCI++ (currently on the v0.36.x branch) on a branch off of v0.34.x, starting just with ABCI++ Prepare/Process Proposal.

The remainder of ABCI++ will be released in subsequent releases.

Proposed Path to Done. Summary

The main idea behind this plan is to proceed in the same way we did for v0.36.x, represented in ABCI++ project board.
The main differences will be:

• We will only be considering PrepareProposal/ProcessProposal related tasks
• We will "follow" our work on v0.36.x. By "following a PR", I mean looking at that PR (similar work done for v0.36.x) and copying over only what makes sense.

The work is structured into two main parts (further described below):

• (1) Preliminary work [Estimation of critical path: 9 days (with 1 thread)]
• (2) Core feature work [Estimation of critical path: 14.5 days (with 5 threads)]

(1) and (2) can mostly proceed in parallel.

N.B.: Let’s manage the protobufs the right way this time 😊 → They should evolve with the code

Proposed Path to Done. Details

(1) Preliminary Work

We could have two threads here, but as all these tasks serialized are shorter than the critical path in "Core Feature Work", there's no point

• Fix UTs on v0.34.x branch (or on new main) [Estimation: 1 day]
• Create feature branch: feature/abci++ppp [Estimation: 0 days]
• Restore CI on feature/abci++ppp branch [Estimation: 0.5 days]
• proto generation uses buf (cherry-pick or follow #7975) [Timebox: 1 day]
• #9271

(2) Core feature work

We have three threads that can proceed in parallel:

• PrepareProposal (*) [Estimation: 9.5 days]
• ProcessProposal (*) [Estimation: 8 days]
• Spec and doc work [Estimation: 8 days]

(*) Only when PrepareProposal and ProcessProposal are done, can we proceed with the fourth part:

• Final adjustments [Estimation: 5 days, assuming no parallel work done]
  • tasks here could be further parallelized

PrepareProposal

• #9104 [Estimation: 0.5 days, ready for PR]
  • Description: Cherry-pick #6544 (Done via #7746 – as a “dry-run” for ABCI++ Option 2 in planning doc)
• #9108 [Estimation: 2 days]
  • Description: Cherry-pick #6915 (very few conflicts expected)
  • careful about protobuf bug we solved on v0.36.x (showed up as a crash in e2e tests, compare with #7821)
• #9118 [Estimation: 7 days]
  • Main part: follow #8094
  • Minor: remove vote extensions from proto: follow #8128
  • Panic on badly prepared proposal #8145
  • CheckTx out of the way: follow #8176
  • Fix Max-size: follow #8242, #8210

ProcessProposal

• Old ProcessProposal [Estimation: 3 days, ready for PR]
  • Cherry-pick #7091
  • Follow #7752
• Sync ProcessProposal with spec [5 days]
  • Follow #7961

Final adjustments

Most of these tasks can be done in parallel, but they are quite short.

• Move app_hash from Commit to EndBlock: follow #8664
• Protobuf: responses should be enums: see what makes sense out of #8158 [Estimation: 0.5 days]
• #9211: follow #8216 and follow or cherry-pick the PrepareProposal and ProcessProposal parts [Estimation: 1 day]
• (#9221) Compare the status of abci's types.proto in feature/abci++ppp and v0.36.x and make sure no changes are related to PrepareProposal/ProcessProposal

At this point, external teams should be able to start integration of PrepareProposal/ ProcessProposal

• Add metrics: see what makes sense out of #8480 [Estimation: 1 day]
• Remove old methods. See what makes sense out of #8633
• (#9254) Add sleep in e2e app: follow or cherry-pick #8638 [Estimation: 1.5 days]
• (#9270) abci-cli: PrepareProposal (follow #8656), ProcessProposal (follow #8901) [Estimation: 1.5 days]
• (#9287)

Spec and doc work

• [Overall estimation: 9 days] This is low risk, and can proceed completely in parallel with the rest
• (#9201 ) Copy current state of ABCI++ spec on v0.36.x onto feature/abci++ppp branch and remove references to unused features
  - Vote extensions, FinalizeBlock (type of work similar to #7745)
• Remove global client mutex (#7073) won’t be picked up for risk mitigation → “revert” the spec accordingly
• Follow code-related hunks of (#8796)
  • Restore data field in Commit
  • Rename Misbehaviour to ByzantineValidators
  • Rename Evidence to Misbheaviour
• Final read of spec for overall coherence
• Review ADRs and RFCs that make sense on feature/abci++ppp branch
• #9272

[hdevalence]

What's the new (?) / current (?) plan around vote extensions? We were planning around the previous (?) plan to have 0.36 include all of ABCI++, including vote extensions -- is that still happening? There are references to vote extensions in some of the component issues but I'm not sure what the plan is.

[cmwaters]

Move app_hash from Commit to EndBlock: follow 8664 [Estimation: 0.5 days]

I think this should be part of the Finalize Block work

[williambanfield]

Hey @hdevalence the engineering team is no longer in charge of this decision, I think someone form the Tendermint Council should respond. I think they have a plan for the release. cc'ing the council members: @alexanderbez @ValarDragon @zmanian @milosevic @xla @melekes @liamsi

[sergio-mena]

I think this should be part of the Finalize Block work

We can do it both ways (moving the app_hash to EndBlock, or moving it to FinalizeBlock when the time comes).

Thinking about your comment, I realize that, if it can be done as part of Finalizeblock, as you suggest, we shouldn't let it stand on PrepareProposal/ProcessProposal's critical path. So, yes, I think I agree with your suggestion.

[williambanfield]

Looking at the issue here, I'm seeing the Oldxxx Integration tasks, and I'm not sure I understand what purpose they serve here. We have well implemented versions that match the latest spec and I don't see a need to resurrect the versions that don't match the spec and that have not benefited from running in e2e tests and receiving fixups over time but maybe there's a purpose to that exercise that I don't get? @sergio-mena @thanethomson

[sergio-mena]

@williambanfield the purpose is to methodically reuse the work we did for v0.36.x. I'm not willing to re-implement ABCI++ from scratch again. And the fact is that the diffs we worked on in v0.36.x (the famous "synchronize" tasks that you and @thanethomson worked on), which are the backbone of the v0.36.x implementation, do depend on these earlier diffs. So bringing these first looks to me like the path of least resistance. And we can leverage (with limitations) git's 3-way merge to make sure we don't forget anything, we leverage all the problems we found/fixed, all the reviews we carried out, etc, on v0.36.x.

[williambanfield]

@sergio-mena Should we include the subset of changes from #8216 that affect ProcessProposal and PrepareProposal ? That change updated fields within particular methods and updated the documentation to reflect the renamed set of fields.

[sergio-mena]

@williambanfield you're totally right. I initially left #8216 out of this tracking branch since I only remembered the changes in FinalizeBlock, but as you rightly point out, we applied the same changes on PrepareProposal and ProcessProposal.
I will add it in the Final adjustments section

[williambanfield]

The work to update to Enums ended up being a no-op at this point in time. The original PR added the following Enums:

• ModifiedTxStatus as part of ResponsePrepareProposal
• ProposalStatus as part of ResponseProcessProposal
• VerifyStatus as part of ResponseVerifyVoteExtension

No implementation was needed for any of these for the following reasons:

• ModifiedTxStatus was dropped in favor of a simpler design during development in 8df38db.
• ProposalStatus was already implemented by @cmwaters in f861062
• VerifyStatus is not used and not relevant for the calls being implemented here.

[sergio-mena]

Closing as all tasks are marked as done now