fix(x/staking)!: continue instead of panic on validator.Jailed #20059
Conversation
WalkthroughWalkthroughThe update in the Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional Context UsedPath-based Instructions (1)
Additional comments not posted (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Reecepbcups
changed the title
|
LGTM! |
This change is consensus breaking. Adding as a discussion point for our standup. |
|
how are you hitting this code in the poa module? can you add a way to reproduce this? |
|
@tac0turtle The issue is that it continues to try and slash every single block once the validator is jailed. Running on POA main repeated: Seeing this, I attempted to have POA ABCI delete the validator index in the EndBlock (first), then have staking EndBlocker run. This would fail no matter how I deleted the power store, since the jailing was in the previous EndBlock and we were on the next. Returning In removing this panic, the staking logic can handle as expected and move the validator from a power of 2 to 0 and properly jail. (POA patch) You can run this in POA main & the patch branch. Seeing POA main continues the loop, and the patch handles the jailing as expected # POA Run a single node with a new binary instance
cd simapp && BINARY="poad" CHAIN_ID="poa-1" HOME_DIR="$HOME/.poad" TIMEOUT_COMMIT="1200ms" CLEAN=true sh test_node.sh
# new terminal window
FLAGS="--keyring-backend=test --chain-id=poa-1 --home="$HOME/.poad" --yes"
MAIN_VAL=`poad q staking validators --output=json | jq .validators[0].operator_address -r` && echo $MAIN_VAL
poad tx poa set-power $MAIN_VAL 18356789 $FLAGS --from=acc1 --unsafe
sleep 3
poad q staking validators
poad q consensus comet validator-set
poad tx poa create-validator simapp/validator_file.json $FLAGS --from acc3
sleep 3
poad q poa pending-validators --output json # 1 pending
PENIDNG_OPPERATOR_ADDR=$(poad q poa pending-validators --output=json | jq .pending[0].operator_address -r) && echo $PENIDNG_OPPERATOR_ADDR
poad tx poa set-power $PENIDNG_OPPERATOR_ADDR 2000000 $FLAGS --from=acc1
sleep 3
poad q consensus comet validator-setThe patch returns the following output which is the expected behavior w/ jailing |
|
@julienrbrt Curious how this is consensus breaking for my own knowledge. In the event the code was triggered it would already panic (which for PoS chains is bad under current circumstances). Meaning to flag this it would have to be done via a Software upgrade migration & setting a val to jailed. So while technically it could be consensus breaking in such a rare case that no mainnet has done, I don't see a case where it is unless using PoA. Where am I wrong here? 🙏 |
Right, so this is why I said that we may be able to backport it. Indeed, a chain would halt if this ever gets triggered, and the other validators with the patch would just keep producing blocks (not optimal, but acceptable). |
Reecepbcups
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
Hey we dove into your code to figure out what is going on. We feel like you are approaching using the staking module in a way that it wasnt meant to be. We havent hit this error on any mainnet since the staking module does handle things correctly. We are worried your code could also leads to other unforseen issues. My personal recommendation would be to write a POA module without the staking module being used. @alpe recognised your goals and mentioned he did something similar in mesh security via decorators. If you decide to continue with the current approach then the decorator approach may be safer. We wont accept this pr |
|
Just making future reference here: this is similar to what just happened to the hub with patch #20548. (another area of the codebase, may be due to PSS, but panic on vals off by 1 with respect to jailing) |
Reecepbcups
changed the title
|
Haha I knew this was coming when they explained the bug. The issue is in how the hub is using this function. This function works correctly. The issue is this function lacks documentation to how to use it and should be private. Like much of the staking module. Don't think this is a bug as it was built this way on purpose. |
Perhaps another "unforseen" issue? strangelove-ventures/poa#170 (comment) Sim-ed POA chain crash appears to be caused by |
Description
If approved, requesting backport to release/v0.50.x as well to support current POA users
In building the x/POA module, Ethos recently discovered some misbehavior with respect to power and then we found cases for jailing. - strangelove-ventures/poa#165. In staking
ApplyAndReturnValidatorSetUpdatesthevalidator.Jailedpanic stops us from building on top with our overrides. A continue rather than panic resolves the issue since we can't jail the validator until h+1 / h+2 anyways. As it will jail the next block afterIt is very possible I am doing something wrong, but after a week of working on this i am going insane & is my last resort
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
Summary by CodeRabbit