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

abci: implement vote extensions #9620

Closed
wants to merge 57 commits into from

Conversation

cmwaters
Copy link
Contributor

This builds on top of #9532 and is part of feature/abci++vef

This PR traces through the following PRs trying to make the appropriate changes on top of main

ref: #9396


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@cmwaters cmwaters requested a review from a team October 24, 2022 12:28
@cmwaters cmwaters added the S:wip Work in progress (prevents stalebot from automatically closing) label Oct 24, 2022
@cmwaters cmwaters self-assigned this Oct 24, 2022
@cmwaters cmwaters marked this pull request as draft November 9, 2022 15:11
@thanethomson thanethomson added the C:abci Component: Application Blockchain Interface label Nov 28, 2022
@sergio-mena sergio-mena assigned sergio-mena and unassigned cmwaters Nov 29, 2022
@sergio-mena sergio-mena changed the base branch from cal/vote-extensions-1 to feature/abci++vef November 30, 2022 12:20
@sergio-mena
Copy link
Contributor

I just finished doing the diff-of-diffs comparison between the branch with the cherry-picked commits and this PR, which has been rebased to the tip of the feature branch, and now also contains #9532.

This experience was much better than FinalizeBlock. I am glad I could find problems on both sides:

  • On this PR, there are some gaps in the production logic, in particular in what has to do with the newly introduced "enable extensions" ConsensusParam. That logic is partially missing, and we would only have found out probably through --painful-- extensive testing, or worse, by our users. So the diff-of-diffs comparison (and therefore this PR) has been instrumental in avoiding that.
  • The cherry-pick branch is code complete (no hunks left behind by the very method used) but by comparing it with this PR I found some differences, and even a few bugs!; caused either by the (manual) conflict resolution process during the cherry-picking, or even in the v0.36.x implementation.

The way I am planning to move forward is the following:

  • I have extracted all relevant divergences between the two diffs, and added them in a commit here at the tip of the cherry-pick branch including:
    • all bugs, and parts I am not sure about at this time can all be seen in that commit
    • all other relevant divergences (not necessarily bugs), are also there, also useful comments added in this PR
    • all of the above will have their own dedicated PR where we will be able to better examine them
  • As it is now up to me to drive this effort, I will go for posting smaller, well defined chunks of this PR for others to review, rather than having someone else review them in one shot as I've done so far.

sergio-mena added a commit that referenced this pull request Dec 16, 2022
* Obvious changes (including bugs)

* Update privval/file.go

Co-authored-by: Thane Thomson <connect@thanethomson.com>

Co-authored-by: Thane Thomson <connect@thanethomson.com>
sergio-mena added a commit that referenced this pull request Dec 22, 2022
…locksync reactor (#9926)

* Fix problems in blocksync reactor logic & test the fixes

* diagnose messages on TestCheckSwitchToConsensusLastHeightZero

* Fixed test logic

* Apply suggestions from code review

Co-authored-by: Lasaro <lasaro@informal.systems>

* Addressed @lasarojc's comments

Co-authored-by: Lasaro <lasaro@informal.systems>
sergio-mena added a commit that referenced this pull request Dec 22, 2022
…9927)

* Make mempool v1 UTs more predictable

* Simple changes

* Reuse new signVote tests in production code

* Fix `IsNil` problem from cherry-pick: should be `IsZero`

* Fix linter issue

* Apply suggestions from code review

Co-authored-by: Lasaro <lasaro@informal.systems>

* Addressed @lasarojc's comment

* Addressed @jmalicevic's comment

Co-authored-by: Lasaro <lasaro@informal.systems>
@sergio-mena
Copy link
Contributor

All the differences between this PR and branch feature/abci++vef are now integrated as tracked by #9887

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:abci Component: Application Blockchain Interface S:wip Work in progress (prevents stalebot from automatically closing)
Projects
Status: Done/Merged
Development

Successfully merging this pull request may close these issues.

None yet

3 participants