refactor: Wrap validator_signer with MutableConfigValue #11372

staffik · 2024-05-21T16:55:21Z

Part of: #11264
This PR should be no-op:

convert Signer and ValidatorSigner traits into an enum
wrap ValidatorSigner with MutableConfigValue

MutableConfigValue requires to implement PartialEq and Clone traits. These traits are not object safe, thus cannot be derived for ValidatorSigner trait. We need to convert ValidatorSigner trait into an enum, similarly Signer trait.
That's also the solution recommended by Rust: rust-lang/rust#80194

Unfortunately, this change requires a change in enormous number of places, because the existing code mostly used InMemory(Validator)Signer directly instead of dyn (Validator)Signer.
To minimize changes, I added traits like
From<InMemoryValidatorSigner> for ValidatorSigner so that it suffices to add .into() in most cases.

tayfunelmas · 2024-05-21T23:05:47Z

Can you give a bit more context for this change? What is the benefit we get to wrap them in MutableConfigValue? it this prep for another change? thanks.

staffik · 2024-05-22T07:48:50Z

Can you give a bit more context for this change? What is the benefit we get to wrap them in MutableConfigValue? it this prep for another change? thanks.

Yes, it is prep for another change. Part of #11264.
We need to wrap validator key in MutableConfigValue, so that it can be updated dynamically, without restarting neard.

tayfunelmas · 2024-05-22T15:46:32Z

Can you give a bit more context for this change? What is the benefit we get to wrap them in MutableConfigValue? it this prep for another change? thanks.

Yes, it is prep for another change. Part of #11264. We need to wrap validator key in MutableConfigValue, so that it can be updated dynamically, without restarting neard.

Ah ok, makes sense, thanks.

core/primitives/src/validator_signer.rs

core/crypto/src/signer.rs

codecov · 2024-05-26T17:51:39Z

Codecov Report

Attention: Patch coverage is 83.87097% with 70 lines in your changes missing coverage. Please review.

Project coverage is 71.38%. Comparing base (fe5a71c) to head (f0e444d).
Report is 2 commits behind head on master.

Files	Patch %	Lines
core/primitives/src/validator_signer.rs	79.78%	19 Missing ⚠️
integration-tests/src/node/process_node.rs	0.00%	13 Missing ⚠️
chain/client/src/client.rs	67.64%	5 Missing and 6 partials ⚠️
chain/client/src/client_actor.rs	63.63%	4 Missing ⚠️
chain/client/src/debug.rs	0.00%	3 Missing ⚠️
core/crypto/src/signer.rs	93.33%	3 Missing ⚠️
chain/chain/src/runtime/tests.rs	92.85%	2 Missing ⚠️
...nt/src/stateless_validation/chunk_validator/mod.rs	60.00%	2 Missing ⚠️
chain/client/src/test_utils/test_env.rs	77.77%	2 Missing ⚠️
core/chain-configs/src/updateable_config.rs	50.00%	2 Missing ⚠️
... and 6 more

Additional details and impacted files

@@           Coverage Diff            @@
##           master   #11372    +/-   ##
========================================
  Coverage   71.37%   71.38%            
========================================
  Files         790      790            
  Lines      160086   160243   +157     
  Branches   160086   160243   +157     
========================================
+ Hits       114256   114384   +128     
- Misses      40857    40886    +29     
  Partials     4973     4973

Flag	Coverage Δ
backward-compatibility	`0.23% <0.00%> (-0.01%)`	⬇️
db-migration	`0.23% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.35% <0.76%> (-0.01%)`	⬇️
integration-tests	`37.69% <48.84%> (+0.02%)`	⬆️
linux	`68.82% <81.98%> (+0.03%)`	⬆️
linux-nightly	`70.90% <81.56%> (-0.01%)`	⬇️
macos	`52.44% <72.67%> (+0.01%)`	⬆️
pytests	`1.58% <8.42%> (+0.01%)`	⬆️
sanity-checks	`1.38% <8.04%> (+0.01%)`	⬆️
unittests	`66.08% <81.63%> (-0.01%)`	⬇️
upgradability	`0.28% <0.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wacban

LGTM

wacban · 2024-05-28T08:15:15Z

chain/client/src/client.rs

@@ -146,7 +148,7 @@ pub struct Client {
    /// Network adapter.
    pub network_adapter: PeerManagerAdapter,
    /// Signer for block producer (if present).
-    pub validator_signer: Option<Arc<dyn ValidatorSigner>>,
+    pub validator_signer: MutableConfigValue<Option<Arc<ValidatorSigner>>>,


@staffik Can you tell me briefly how will that work here? The MutableConfigValue today checks the contents of the config file. The validator key is stored in a different file though, will that work out of the box or do you need to make any extra changes there?

Here we just wrap with MutableConfigValue and it does not update validator key. In the follow up PR we add a logic that updates the validator key based on appropriate file.

wacban · 2024-05-28T09:06:36Z

core/chain-configs/src/updateable_config.rs

@@ -37,12 +37,12 @@ impl<T: Serialize> Serialize for MutableConfigValue<T> {
    }
 }

-impl<T: Copy + PartialEq + Debug> MutableConfigValue<T> {
+impl<T: Clone + PartialEq + Debug> MutableConfigValue<T> {


Would it make sense to make the validator key Copy-able rather than this change?

I tried but I found it better to relax the Copy requirement and use Clone instead. Reasons:

it would require making Copy-able:

AccountId - which is considered stable and would require making a new release in separate repo. Perhaps there is a reason why it is non-Copy-able and it would not be possible to make it Copy-able anyway.

PublicKey, SecretKey, ED25519PublicKey, ED25519SecretKey

and possibly more

Copy is for better perfomance but here we just reload simple structs rarely.

If we want to add non-Copy-able field in the future we would have to do this change anyway.

wacban · 2024-05-28T09:07:56Z

core/crypto/src/signer.rs

 /// Generic signer trait, that can sign with some subset of supported curves.
-pub trait Signer: Sync + Send {
-    fn public_key(&self) -> PublicKey;
-    fn sign(&self, data: &[u8]) -> Signature;
+#[derive(Debug, PartialEq)]
+pub enum Signer {


Can you update the comment please? It's no longer a trait.

wacban · 2024-05-28T09:09:14Z

core/crypto/src/signer.rs

+    Empty(EmptySigner),
+    InMemory(InMemorySigner),


Can you add short comments about those signers?

wacban · 2024-05-28T09:13:36Z

core/primitives/src/transaction.rs

+        let signer: Signer =
+            InMemorySigner::from_random("test".parse().unwrap(), KeyType::ED25519).into();


Instead of into() would it make sense to have the constructors of InMemorySigner and EmptySigner return Signer? If access to the inner enum value is needed you can expose that via new methods. Same for Signer and ValidatorSigner.

I thought about it but account_id field does not exist in EmptySigner.
We could return empty string or unimplemented for EmptySigner but this code path should never be called anyway.
I have no strong opinion, looks like doing what you said would simplify this PR, I will try it out, thanks!

I've spent a while trying to get rid of into(), but it was not straightforward. There are some places that use members like secret_key. I would rather do it as a follow up PR.

Will do it as a separate issue: #11531

wacban · 2024-05-28T09:14:10Z

core/primitives/src/validator_signer.rs

+#[derive(Clone, Debug, PartialEq)]
+pub enum ValidatorSigner {
+    Empty(EmptyValidatorSigner),
+    InMemory(InMemoryValidatorSigner),
+}
+


Please add comments for the struct and the values.

wacban · 2024-06-10T13:18:00Z

@staffik Looking at the last commits it seems like those only address nits and add comment. Please let me know if you need a fresh review and of what exactly, otherwise my old approval still stands.

Issue: #11264 This is follow-up to #11372. The actual changes (+test) for #11264 will be done in a third, final PR: #11536. ### Summary This PR should mostly be no-op. It focuses on propagating `MutableConfigValue` for `validator_signer` everywhere. All instances of mutable `validator_signer` are synchronized. In case validator_id only is needed, we propagate `validator_signer` anyway as it contains the current validator info. ### Extra changes - Remove signer as a field and pass to methods instead: `Doomslug`, `InfoHelper`, `ChunkValidator`. - Make some public methods internal where they do not need to be public. - Split `process_ready_orphan_witnesses_and_clean_old` into two functions. - Removed `block_production_started` from `ClientActorInner`. - Add `FrozenValidatorConfig` to make it possible to return a snapshot of `ValidatorConfig`. --------- Co-authored-by: Your Name <[email protected]>

Issue: #11264 This PR build on: * #11372 * #11400 and contains actual changes and test for validator key hot swap. ### Summary - Extend `UpdateableConfig` with validator key. - Update client's mutable validator key when we detect it changed in the updateable config. - Advertise our new validator key through `advertise_tier1_proxies()`. - Add integration test for the new behaviour: - We start with 2 validating nodes (`node0`, `node1`) and 1 non-validating node (`node2`). It is important that the non-validating node tracks all shards, because we do not know which shard it will track when we switch validator keys. - We copy validator key from `node0` to `node2`. - We stop `node0`, then we trigger validator key reload for `node2`. - Now `node2` is a validator, but it figures out as `node0` because it copied validator key from `node0`. - We wait for a couple of epochs and we require that both remaining nodes progress the chain. Both nodes should be synchronised after a few epochs. Test with: ``` cargo build -pneard --features test_features,rosetta_rpc && cargo build -pgenesis-populate -prestaked -pnear-test-contracts && python3 pytest/tests/sanity/validator_switch_key_quick.py ``` #### Extra changes: - Use `MutableValidatorSigner` alias instead of `MutableConfigValue<Option<Arc<ValidatorSigner>>>` - Return `ConfigUpdaterResult` from config updater. - Remove (de)serialization derives for `UpdateableConfigs`. - --------- Co-authored-by: Your Name <[email protected]>

Part of: #11264 This PR should be no-op: - convert `Signer` and `ValidatorSigner` traits into an enum - wrap `ValidatorSigner` with `MutableConfigValue` `MutableConfigValue` requires to implement `PartialEq` and `Clone` traits. These traits are not object safe, thus cannot be derived for `ValidatorSigner` trait. We need to convert `ValidatorSigner` trait into an enum, similarly `Signer` trait. That's also the solution recommended by Rust: rust-lang/rust#80194 Unfortunately, this change requires a change in enormous number of places, because the existing code mostly used `InMemory(Validator)Signer` directly instead of `dyn (Validator)Signer`. To minimize changes, I added traits like `From<InMemoryValidatorSigner> for ValidatorSigner` so that it suffices to add `.into()` in most cases.

Issue: #11264 This is follow-up to #11372. The actual changes (+test) for #11264 will be done in a third, final PR: #11536. This PR should mostly be no-op. It focuses on propagating `MutableConfigValue` for `validator_signer` everywhere. All instances of mutable `validator_signer` are synchronized. In case validator_id only is needed, we propagate `validator_signer` anyway as it contains the current validator info. - Remove signer as a field and pass to methods instead: `Doomslug`, `InfoHelper`, `ChunkValidator`. - Make some public methods internal where they do not need to be public. - Split `process_ready_orphan_witnesses_and_clean_old` into two functions. - Removed `block_production_started` from `ClientActorInner`. - Add `FrozenValidatorConfig` to make it possible to return a snapshot of `ValidatorConfig`. --------- Co-authored-by: Your Name <[email protected]>

Issue: #11264 This PR build on: * #11372 * #11400 and contains actual changes and test for validator key hot swap. ### Summary - Extend `UpdateableConfig` with validator key. - Update client's mutable validator key when we detect it changed in the updateable config. - Advertise our new validator key through `advertise_tier1_proxies()`. - Add integration test for the new behaviour: - We start with 2 validating nodes (`node0`, `node1`) and 1 non-validating node (`node2`). It is important that the non-validating node tracks all shards, because we do not know which shard it will track when we switch validator keys. - We copy validator key from `node0` to `node2`. - We stop `node0`, then we trigger validator key reload for `node2`. - Now `node2` is a validator, but it figures out as `node0` because it copied validator key from `node0`. - We wait for a couple of epochs and we require that both remaining nodes progress the chain. Both nodes should be synchronised after a few epochs. Test with: ``` cargo build -pneard --features test_features,rosetta_rpc && cargo build -pgenesis-populate -prestaked -pnear-test-contracts && python3 pytest/tests/sanity/validator_switch_key_quick.py ``` #### Extra changes: - Use `MutableValidatorSigner` alias instead of `MutableConfigValue<Option<Arc<ValidatorSigner>>>` - Return `ConfigUpdaterResult` from config updater. - Remove (de)serialization derives for `UpdateableConfigs`. - --------- Co-authored-by: Your Name <[email protected]>

staffik requested a review from a team as a code owner May 21, 2024 16:55

staffik requested a review from saketh-are May 21, 2024 16:55

staffik changed the title ~~refactor: Convert Signer and ValidatorSigner into an enum~~ refactor: Wrap validator_signer with MutableConfigValue May 21, 2024

staffik marked this pull request as draft May 21, 2024 17:06

tayfunelmas reviewed May 22, 2024

View reviewed changes

core/primitives/src/validator_signer.rs Outdated Show resolved Hide resolved

tayfunelmas reviewed May 22, 2024

View reviewed changes

core/crypto/src/signer.rs Outdated Show resolved Hide resolved

Refactor Signer, ValidatorSigner

4c23cfa

staffik force-pushed the refactor-signer branch from b9e1b1c to 0ed1529 Compare May 24, 2024 15:15

Refactor continued

47d9d26

staffik force-pushed the refactor-signer branch from 0ed1529 to 47d9d26 Compare May 24, 2024 15:29

staffik added 2 commits May 26, 2024 19:21

refactor

5ca2fd3

formatting

b60dde1

staffik marked this pull request as ready for review May 26, 2024 18:01

staffik requested review from shreyan-gupta and wacban May 26, 2024 18:01

wacban approved these changes May 28, 2024

View reviewed changes

tayfunelmas approved these changes May 28, 2024

View reviewed changes

staffik marked this pull request as draft May 29, 2024 07:08

staffik added 3 commits June 10, 2024 13:09

Merge remote-tracking branch 'origin/master' into refactor-signer

96bef88

clippy

960ca21

add comments

f0e444d

staffik marked this pull request as ready for review June 10, 2024 12:30

staffik mentioned this pull request Jun 10, 2024

Use Signer and ValidatorSigner interfaces where possible #11531

Open

staffik added this pull request to the merge queue Jun 10, 2024

Merged via the queue into master with commit b8f08d9 Jun 10, 2024
30 of 31 checks passed

staffik deleted the refactor-signer branch June 10, 2024 14:26

This was referenced Jun 10, 2024

refactor: Make validator_signer mutable #11400

Merged

Validator key hot swap #11536

Merged

staffik added the A-stateless-validation Area: stateless validation label Jul 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Wrap validator_signer with MutableConfigValue #11372

refactor: Wrap validator_signer with MutableConfigValue #11372

staffik commented May 21, 2024 •

edited

Loading

tayfunelmas commented May 21, 2024

staffik commented May 22, 2024

tayfunelmas commented May 22, 2024

codecov bot commented May 26, 2024 •

edited

Loading

wacban left a comment

wacban May 28, 2024

staffik May 28, 2024

wacban May 28, 2024

staffik May 28, 2024

wacban May 28, 2024

wacban May 28, 2024

wacban May 28, 2024

staffik May 28, 2024

staffik Jun 10, 2024

staffik Jun 10, 2024

wacban May 28, 2024

wacban commented Jun 10, 2024

		let signer: Signer =
		InMemorySigner::from_random("test".parse().unwrap(), KeyType::ED25519).into();

refactor: Wrap validator_signer with MutableConfigValue #11372

refactor: Wrap validator_signer with MutableConfigValue #11372

Conversation

staffik commented May 21, 2024 • edited Loading

tayfunelmas commented May 21, 2024

staffik commented May 22, 2024

tayfunelmas commented May 22, 2024

codecov bot commented May 26, 2024 • edited Loading

Codecov Report

wacban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wacban commented Jun 10, 2024

staffik commented May 21, 2024 •

edited

Loading

codecov bot commented May 26, 2024 •

edited

Loading