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.

Sign up for GitHub

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

Jump to bottom

ABCI++ Prepare/Process Proposal #9053

Closed
35 tasks done
Tracked by #9
, #9091 ,  #9886 ,  #41
...
thanethomson opened this issue Jul 20, 2022 · 10 comments
Closed
35 tasks done
Tracked by #9
, #9091 ,  #9886 ,  #41
...

ABCI++ Prepare/Process Proposal #9053

thanethomson opened this issue Jul 20, 2022 · 10 comments
Assignees
@williambanfield @sergio-mena @cmwaters
Labels
C:abci Component: Application Blockchain Interface E:medium Estimated to take up to a month or two feature Feature work that definitely changes system behavior

Comments

@thanethomson
Copy link
Contributor

thanethomson commented Jul 20, 2022 •
edited by sergio-mena

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

(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

ProcessProposal

Final adjustments

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

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

Spec and doc work
@thanethomson thanethomson added feature Feature work that definitely changes system behavior prioritized E:medium Estimated to take up to a month or two T:tracking Tracking issue for other issues and removed prioritized labels Jul 20, 2022
@thanethomson thanethomson changed the title ABCI++ WIP: ABCI++ Jul 20, 2022
@thanethomson thanethomson changed the title WIP: ABCI++ WIP: ABCI++ Prepare/Process Proposal Jul 22, 2022
@thanethomson thanethomson changed the title WIP: ABCI++ Prepare/Process Proposal ABCI++ Prepare/Process Proposal Jul 22, 2022
@hdevalence
Copy link

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.

@thanethomson thanethomson added the C:abci Component: Application Blockchain Interface label Jul 23, 2022
@hdevalence hdevalence mentioned this issue Jul 24, 2022
Merged
@thanethomson thanethomson mentioned this issue Jul 25, 2022
Closed
39 tasks
@cmwaters cmwaters pinned this issue Jul 26, 2022
@cmwaters
Copy link
Contributor

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
Copy link
Contributor

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

@cmwaters cmwaters mentioned this issue Jul 26, 2022
Closed
This was referenced Jul 27, 2022
Closed
Closed
@sergio-mena
Copy link
Contributor

sergio-mena commented Jul 27, 2022 •
edited

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.

@thanethomson thanethomson added priority A high-priority, high-level item to be shown on the priorities project board and removed T:tracking Tracking issue for other issues labels Jul 27, 2022
@williambanfield
Copy link
Contributor

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
Copy link
Contributor

sergio-mena commented Jul 27, 2022 •
edited

@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.

@sergio-mena sergio-mena mentioned this issue Jul 28, 2022
Closed
@cmwaters cmwaters mentioned this issue Jul 28, 2022
Merged
@williambanfield
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

williambanfield commented Aug 9, 2022 •
edited

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

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.

This was referenced Aug 10, 2022
Closed
Closed
Closed
Merged
@jmalicevic jmalicevic mentioned this issue Aug 12, 2022
Closed
9 tasks
This was referenced Aug 12, 2022
Merged
Closed
@thanethomson thanethomson mentioned this issue Aug 16, 2022
Closed
6 tasks
@williambanfield williambanfield mentioned this issue Aug 16, 2022
Merged
3 tasks
This was referenced Aug 17, 2022
Closed
Merged
Merged
Merged
This was referenced Sep 6, 2022
Merged
Merged
Closed
@sergio-mena
Copy link
Contributor

Closing as all tasks are marked as done now

@thanethomson thanethomson mentioned this issue Dec 15, 2022
Closed
15 tasks
@thanethomson thanethomson removed the priority A high-priority, high-level item to be shown on the priorities project board label Dec 22, 2022
This was referenced Dec 23, 2022
Open
Open
@thanethomson thanethomson mentioned this issue Dec 23, 2022
Closed
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@williambanfield williambanfield

@sergio-mena sergio-mena

@cmwaters cmwaters

Labels
C:abci Component: Application Blockchain Interface E:medium Estimated to take up to a month or two feature Feature work that definitely changes system behavior
Projects
Tendermint
Status: Done/Merged
Milestone
No milestone
Development

No branches or pull requests
6 participants
@hdevalence @thanethomson @williambanfield @sergio-mena @cmwaters and others