fix: prevent panic on chain replay #1197
Conversation
karlem
force-pushed
the
fix-replay-panic
branch
from
f05ad1d
to
c181afa
Compare
|
@LePremierHomme could you have another look please? |
There was a problem hiding this comment.
@karlem A quick question, how will the cache be recovered after crashes? Seems not queried when booting?
| let keys = self.public_keys.read().unwrap(); | ||
| keys.get(id).copied() | ||
| } | ||
| fn fendermint_pub_key_from_tendermint_pub_key( |
There was a problem hiding this comment.
I think there might have a function like this somewhere in the codebase.
| if *id == validator.address { | ||
| pubkey = Some(key); | ||
| for (id, key) in public_keys { | ||
| if let Ok(fendermint_key) = fendermint_pub_key_from_tendermint_pub_key(&key) { |
There was a problem hiding this comment.
maybe worth warn or error for the else, it should always be Ok, but if not, then something is generally wrong.
| Some(consensus_params) | ||
| // Updates the validator cache with the new validator set. | ||
| let validators_ids_with_keys: Vec<(TendermintAccountId, TendermintPublicKey)> = | ||
| validator_updates |
There was a problem hiding this comment.
If i rmb correctly, validator_updates is the power change, so it's about changes, not setting validators.
There was a problem hiding this comment.
Hmm. In that case where would be a good place to get the validator changes?
There was a problem hiding this comment.
So we just need to check if the power is 0, that means the validator is removed, we should purge instead. See:
. Else it's just anupsert.
Yeah good point. We would loose it after the crash. Do you think maybe saving it to the app state might be a way to go? |
|
@karlem The rabbit hole gets deeper. But I think it's possible to read the state during boot up. The validators and gas limits are all stored in the contract or actor, so technically we can call the getters from the store directly. |
|
@cryptoAtwill that's a good point; it would also take us one tiny step closer to our desired end state of having as much logic as possible in on-chain actors. |
|
@cryptoAtwill Yeah, I think working with the contract makes more sense than this. I would ditch this in favor of using the contracts, as they are part of the app state—much more solid than this. |
Close #1196
Removing the dependency on the CometBFT client in favor of caching. When CometBFT is catching up—replaying from the beginning of the chain to synchronize with the ABCI app—it does not start the RPC API. Unfortunately, our ABCI app relied on the API during consensus events, which made it impossible to replay the chain.
THIS PR is a hot fix and it is not ready to be merged!