Cleanup and improvements for ControlledValidatorIndices#8896
Cleanup and improvements for ControlledValidatorIndices#8896
ControlledValidatorIndices#8896Conversation
tdimitrov
added
T0-node
| } | ||
|
|
||
| controlled | ||
| validators |
There was a problem hiding this comment.
bikeshedding/unpopular opinion :D, this is not more readable to me :D.
polkadot/node/subsystem-util/src/controlled_validator_indices.rs
Outdated
Show resolved
Hide resolved
|
|
||
| /// Find indices controlled by this validator. | ||
| /// | ||
| /// That is all `ValidatorIndex`es we have private keys for. Usually this will only be one. |
There was a problem hiding this comment.
I doubt our code would work if the validator controlled multiple validator keys. We have many subsystems which terminate the search once they find the first one (pvf-checker, statement-distribution, collator-protocol (all which use signing_key_and_index helper fn.
We could modify that to log a warn if it finds multiple keys
There was a problem hiding this comment.
signing_key_and_index returns an Option so if we stick to it we don't need to check if more than one index is returned.
We should update dispute-coordinator to use Option for the controlled validator index instead of a HashSet here but I prefer to do this in a separate PR.
There was a problem hiding this comment.
I think we should actually modify the inner workings of signing_key_and_index to log an error if it finds multiple validator keys. Maybe we should even fail to start the validator if we detect this (but still log on this function since the keystore probably can change at runtime). But of course, it can be a follow-up.
There was a problem hiding this comment.
What about session changes and key rotations? In this case (especially relevant for disputes), we might indeed have two still relevant keys available at a given point in time: One for the old and one for the most recent/current session. For a single session, I would also not see a reason why there should be more than one key.
There was a problem hiding this comment.
What about session changes and key rotations? In this case (especially relevant for disputes), we might indeed have two still relevant keys available at a given point in time: One for the old and one for the most recent/current session.
So this is the reason to keep more than one key in dispute-coordinator.
I think we should revert all optimizations and simplifications and just keep this:
https://github.com/paritytech/polkadot-sdk/pull/8896/files#diff-40d27ae9fcbe1c9f5075913a3b52571f075eddb0bd79807b34b34501bda881d1L35
Before we break something :)
There was a problem hiding this comment.
all of this key iteration is done per-session. we couldn't have more than one key per session
There was a problem hiding this comment.
I think @eskimor was referring to my comment about dispute-coordinator:
We should update dispute-coordinator to use Option for the controlled validator index instead of a HashSet here but I prefer to do this in a separate PR.
On second thought, CandiateEnvironment is also per session:
There was a problem hiding this comment.
As discussed offline with @eskimor, this is per session so it is okay.
…alidator_indices`
* master: (91 commits) Add extra information to the harmless error logs during validate_transaction (#9047) `sp-tracing`: Remove `test-utils` feature (#9063) add try-state check for staking roles -- staker cannot be nominator a… (#9034) net/discovery: File persistence for `AddrCache` (#8839) dispute-coordinator: handle race with offchain disabling (#9050) Align parameters for `EventEmitter::emit_sent_event` (#9057) Fetch parent block `api_version` (#9059) [XCM Precompile] Rename functions and improve docs in the Solidity interface (#9023) Cleanup and improvements for `ControlledValidatorIndices` (#8896) reenable 0001-parachains-pvf (#9046) Add optional auto-rebag within on-idle (#8684) Fix flaxy 0003-block-building-warp-sync test - one more approach (#8974) [Staking] [AHM] Fixes insufficient slashing of nominators (and some other small issues). (#8937) chore: Bump bounded-collections dep (#9004) XCMP and DMP improvements (#8860) EPMB/unsigned: fixed multi-page winner computation (#8987) Always send full parent header, not only hash, part of collation response (#8939) revive: Precompiles should return dummy code when queried (#9001) Fix confusing log messages in network protocol behaviour (#8819) Fix pallet_migrations benchmark when FailedMigrationHandler emits events (#8694) ...
Improvements for `ControlledValidatorIndices` from #8837: - remove unneeded dependency - more readable implementations for `get` and `find_controlled_validator_indices`
6 participants
Improvements for
ControlledValidatorIndicesfrom #8837:- remove unneeded dependency
- more readable implementations for
getandfind_controlled_validator_indices