feat: declare validators in Config - #81
Conversation
ca54d96 to
52c6603
Compare
|
|
||
| #[serde_as] | ||
| #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)] | ||
| pub struct Validator { |
There was a problem hiding this comment.
now the Config takes a vec of these Validator entries
1d44583 to
21df87a
Compare
b55a388 to
015f813
Compare
015f813 to
ca42a77
Compare
ca42a77 to
0788db2
Compare
|
Rebased on top of latest main to include #130 ; still need to update the integration tests here to use that join logic, though. |
0788db2 to
e77e0bd
Compare
I've removed the validator-specific integration tests from this PR, and will submit them as a separate PR, as with #135. |
This needs more testing (integration tests TK), but the rationale is that sync_validators_from_config sets removed validators to power=0 in the store, but active_validators() filters out power=0 entries. CometBFT requires an explicit power=0 update to remove a validator from the active set. So we should return the changes from sync_validators_from_config and merge them into validator_updates in FinalizeBlock. This is super relevant for the recent 0.38 changes.
The validator set in the config is technically optional; if it's missing, no changes should be made to the validator set, otherwise all validators will be removed from consensus, which is a chain halt. This is a judgment call: it's better to drop changes silently than apply unintended changes and break the chain.
Comet doesn't want the full set, just changes to it. Let's compute the changes in the app code and pass only the changes over the ABCI boundary to comet.
e77e0bd to
c507d89
Compare
|
Rebased on top of latest main, i.e. 1a6baa0 |
redshiftzero
changed the title
ConfigConfig
| pub struct Validator { | ||
| #[serde_as(as = "Hex")] | ||
| pub public_key: Bytes, | ||
| pub power: u64, |
Add new integration tests exercising the validator lifecycle changes that can be triggered by admin config updates. Uses the join_network logic from #130. The tests include: * onboarding/offboarding: seed genesis validators into config, add a 4th validator, remove it, and verify chain liveness at each phase * setting num_vals to zero is impossible: regression test ensuring an empty validators field in a Config submission doesn't wipe the active set, which would be a catastrophic liveness failure * end-to-end joined-node promotion: a real node joins via join_network, syncs, is promoted to validator via admin reconfig, and both nodes converge on identical state Also adds a few helpers to make working with the tests more ergonomic. Notably there's a change from using cometbft's `broadcast_tx_commit` to `broadcast_tx_sync`, which requires polling for tx inclusion, but avoids a common timeout condition where the HTTP timeout on the `tx_commit` call was being reached, causing the tests to fail. This failure occurred sporadically, mostly under load.
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The check_config function doesn't ensure that the public keys used for validator identifies are valid. If a config.json contains malformed or outright junk data in a pubkey field, it's possible to vote that config into passing, at which point it'll halt the chain.
Such a failure would require multiple admins to vote it in, but still, the app logic should prevent invalid state from being approved for inclusion by the chain. If a validatory pubkey is tombstoned, the reconfigure request should be explicitly rejected, rather than merely logging a warning.
There are surely other places in the codebase where we're handling consensus-critical values as bytes or strings, when we should in fact be parsing as strong types. I'm not yet certain whether we should just tackle all those updates as part of review here, slotting the changes in via a separate PR, or try to be comprehensive about the change. My bias is toward a comprehensive solution now, while we're thinking about it.
Encountered this logic bug while reviewing #135, so I'm writing here.
2 participants
Closes #73, I've tested block production but haven't tried adding another validator yet.
I'm also scoping out updating power for existing validators. We currently have equal weights for all validators, we could change this. For now, it's the declared power in the config or 0, which might be worth hardcoding to 1 or 0, or we can leave it flexible as is.