Skip to content

abci: implement vote extensions#9620

Closed
cmwaters wants to merge 57 commits intofeature/abci++veffrom
cal/vote-extensions-2
Closed

abci: implement vote extensions#9620
cmwaters wants to merge 57 commits intofeature/abci++veffrom
cal/vote-extensions-2

Conversation

@cmwaters
Copy link
Contributor

@cmwaters cmwaters commented Oct 24, 2022

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 sergio-mena mentioned this pull request Nov 30, 2022
Closed
sergio-mena added a commit that referenced this pull request Nov 30, 2022
@sergio-mena
Result to diff-of-diffs comparison between the cherry-picked commits …
f0be166 
…and #9620
@sergio-mena
Copy link
Contributor

sergio-mena commented Nov 30, 2022

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.

This was referenced Dec 15, 2022
Closed
Closed
Merged
sergio-mena added a commit that referenced this pull request Dec 16, 2022
@sergio-mena @thanethomson
Divergences in comparison with #9620. Part 1: easy/obvious (#9888)
a749afe 
* Obvious changes (including bugs)

* Update privval/file.go

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

Co-authored-by: Thane Thomson <connect@thanethomson.com>
This was referenced Dec 20, 2022
Merged
Merged
sergio-mena added a commit that referenced this pull request Dec 22, 2022
@sergio-mena @lasarojc
Divergences in comparison with #9620. Part 3: Fix problems found in b…
1332be0 
…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
@sergio-mena @lasarojc
Divergences in comparison with #9620. Part 4: Other changes spotted (#…
4255d5d 
…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

sergio-mena commented Dec 22, 2022

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

Reviewers

No reviews

Assignees

@sergio-mena sergio-mena

Labels

C:abci Component: Application Blockchain Interface S:wip Work in progress (prevents stalebot from automatically closing)

Projects

Tendermint
Status: Done/Merged

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cmwaters @sergio-mena @thanethomson