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

fix!: new validator creation not persisting to cometbft #149

Closed
wants to merge 11 commits into from
Closed

fix!: new validator creation not persisting to cometbft #149

wants to merge 11 commits into from

Conversation

Reecepbcups
Copy link
Member

@Reecepbcups Reecepbcups commented Apr 3, 2024 •
edited
Loading

ref #156

ref: found by ethos

Summary

Despite PoA depending on x/staking for it's EndBlock actions, it does not seem it is persisted when changing Power amounts for new validators -> CometBFT consensus

Issues

  • If PoA uses ABCIEndBlock, Comet does not like this due to staking also using it.

@Reecepbcups Reecepbcups changed the title use ABCI EndBlocker fix!: use ABCI EndBlocker Apr 3, 2024
@Reecepbcups Reecepbcups changed the title fix!: use ABCI EndBlocker fix!: use abci endblock instead of depending on staking Apr 3, 2024
@Reecepbcups
Copy link
Member Author

Reecepbcups commented Apr 4, 2024 •
edited
Loading

current issue: 81166e2 fixes the issue (new validators now get pushed into cometbft consensus) with test_node.sh (still need to fix in the new add val test)

However: when you set the power of a validator, it now persist in the valUpdates forever. We ONLY want to update if it is a new validator. Since staking handles it usually

chain launch
set val1 power to 2000000
set val1 power 3000000
panic for duplicate

Working on a cache mechanism so abci.ValUpdates are only submitted on new vals vs existing updates

@Reecepbcups
Copy link
Member Author

32e6d77 looks to fix this in manual testing (start network, set val[0] to 18, add pending val, set pending val to 2, wait, query comet consensus, remove pending val, query comet consensus just has val[0] at 18).

Reecepbcups and others added 2 commits April 5, 2024 18:55
@Reecepbcups
fix(test): poa add val logic
64770ae
@Reecepbcups
fix(->abci-patch): abci removing logic (#152)
f3ebd75 
* fix jail test (unbonding or unbond)

* use idx 0 since it fails anyways

* bump ictest to latest main

* rm remove val test from monolith tester

* test: `TestPOARemoval`

* fix test

* touchups

* `ictest-gov`

* abic refactor

* working local: val-add and jail
@Reecepbcups Reecepbcups changed the title fix!: use abci endblock instead of depending on staking fix!: new valiadtor creation not persisting to cometbft Apr 13, 2024
Reecepbcups
Reecepbcups commented Apr 13, 2024
View reviewed changes
simapp/app.go Show resolved Hide resolved
module/abci.go
return err
}

if val.Status == stakingtypes.Bonded {
Copy link
Member Author

@Reecepbcups Reecepbcups Apr 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

TODO: add note this is to make ABCI events happy so we always control the key (which changes over blocks) & persist adding new validators to the network

@Reecepbcups Reecepbcups changed the title fix!: new valiadtor creation not persisting to cometbft fix!: new validator creation not persisting to cometbft Apr 13, 2024
@Reecepbcups Reecepbcups mentioned this pull request Apr 14, 2024
Merged
3 tasks
@Reecepbcups
Copy link
Member Author

moved to #160

@Reecepbcups Reecepbcups mentioned this pull request Apr 15, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Reecepbcups