refactor: Make validator_signer mutable #11400

staffik · 2024-05-27T15:46:05Z

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.

codecov · 2024-05-27T16:07:00Z

Codecov Report

Attention: Patch coverage is 84.83548% with 106 lines in your changes missing coverage. Please review.

Project coverage is 71.64%. Comparing base (57c9796) to head (18b9705).

Files	Patch %	Lines
chain/chunks/src/shards_manager_actor.rs	88.88%	9 Missing and 12 partials ⚠️
chain/client/src/client.rs	86.48%	3 Missing and 12 partials ⚠️
nearcore/src/dyn_config.rs	0.00%	11 Missing ⚠️
...alidation/partial_witness/partial_witness_actor.rs	65.38%	5 Missing and 4 partials ⚠️
chain/client/src/client_actor.rs	90.80%	4 Missing and 4 partials ⚠️
nearcore/src/config.rs	69.23%	8 Missing ⚠️
integration-tests/src/node/mod.rs	0.00%	4 Missing ⚠️
chain/chain-primitives/src/error.rs	0.00%	3 Missing ⚠️
...nt/src/stateless_validation/chunk_validator/mod.rs	86.36%	3 Missing ⚠️
...hain/network/src/peer_manager/network_state/mod.rs	0.00%	3 Missing ⚠️
... and 14 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11400      +/-   ##
==========================================
+ Coverage   71.57%   71.64%   +0.06%     
==========================================
  Files         787      787              
  Lines      160728   160969     +241     
  Branches   160728   160969     +241     
==========================================
+ Hits       115039   115319     +280     
+ Misses      40648    40614      -34     
+ Partials     5041     5036       -5

Flag	Coverage Δ
backward-compatibility	`0.23% <0.00%> (-0.01%)`	⬇️
db-migration	`0.23% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.36% <0.00%> (-0.01%)`	⬇️
integration-tests	`37.77% <60.08%> (+0.03%)`	⬆️
linux	`69.04% <78.90%> (+0.04%)`	⬆️
linux-nightly	`71.13% <84.69%> (+0.05%)`	⬆️
macos	`51.36% <71.47%> (+0.38%)`	⬆️
pytests	`1.59% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.39% <0.00%> (-0.01%)`	⬇️
unittests	`66.21% <80.23%> (+0.07%)`	⬆️
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.

chain/client/src/client.rs

chain/chunks/src/shards_manager_actor.rs

wacban · 2024-06-12T12:55:33Z

chain/network/src/peer_manager/network_state/tier1.rs

@@ -291,6 +299,7 @@ impl super::NetworkState {
            }
        }
        if let Some(vc) = validator_cfg {
+            let validator_signer = if let Some(v) = vc.signer.get() { v } else { return };


Question about tier1 - when the node becomes a validator it should join tier1. Would that happen automatically or do we need to handle it?

nearcore/src/config.rs

nearcore/src/lib.rs

nearcore/src/state_sync.rs

wacban

We're getting closer :)

wacban · 2024-06-20T09:54:08Z

chain/chain/src/chain.rs

@@ -539,7 +540,7 @@ impl Chain {
            .iter()
            .filter(|shard_uid| {
                shard_tracker.care_about_shard(
-                    validator_account_id,
+                    validator.get().map(|v| v.validator_id().clone()).as_ref(),


Can you remind me what is the plan regarding loading memtries when using the hot swap?

This is the place where we load memtries on startup. We do not have shadow tracking implemented yet, so currently the new node would have to load memtries for all shards

chain/chunks/src/shards_manager_actor.rs

chain/client/src/client.rs

wacban · 2024-06-20T10:43:33Z

chain/client/src/stateless_validation/chunk_validator/mod.rs

+        // Chunk producers should not receive state witness from themselves.
+        log_assert!(
+            signer.is_some(),
+            "Received a chunk state witness but this is not a validator node. Witness={:?}",
+            witness
+        );
+        let signer = signer.unwrap();
+


not your change but this is iffy

The comment doesn't match the code.

Can an attacker send state witness to an RPC node and crash it?

Best to handle it outside of this PR though.
cc @pugachAG

Can an attacker send state witness to an RPC node and crash it?

I don't this so, this code should only be executed after the witness was validated as part of partial witness actor, so this is safe

wacban · 2024-06-20T10:50:15Z

neard/src/cli.rs

-                    .validator
-                    .as_ref()
-                    .map(|validator| validator.account_id()),
+                near_config.network_config.validator.account_id(),


Should this be mutable as well?

I've added a TODO here

nearcore/chain/network/src/peer/peer_actor.rs

Lines 310 to 311 in b053500

// TODO(validator-key-hot-swap) Consider using mutable validator signer instead of PeerInfo.account_id ?

// That likely requires bigger changes and account_id here is later used for debug / logging purposes only.

This account_id is consumed here: https://github.com/near/nearcore/blob/master/core/o11y/src/opentelemetry.rs#L45-L49
to create a name for opentelemetry channel. Changing it would require bigger changes and changing the name would make logs from single node are not visible together.

chain/client/src/stateless_validation/partial_witness/partial_witness_actor.rs

chain/network/src/config.rs

wacban

LGTM, thanks, that was quite a bit of work!

chain/chain-primitives/src/error.rs

andrei-near · 2024-06-21T12:22:36Z

Non validator logs are spammed with:
Cannot process ready orphan witnesses - not a validator

@staffik

See #11400 (comment) It seems non-validator nodes still call `process_ready_orphan_witnesses_and_clean_old` and logging an error in such case is spammy.

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 <you@example.com>

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 <you@example.com>

See #11400 (comment) It seems non-validator nodes still call `process_ready_orphan_witnesses_and_clean_old` and logging an error in such case is spammy.

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 <you@example.com>

staffik and others added 8 commits May 24, 2024 17:10

Refactor Signer, ValidatorSigner

4c23cfa

Refactor continued

47d9d26

refactor

5ca2fd3

formatting

b60dde1

Pytest

edc3486

sighup

3a8b7fd

Dynamic updateable validator key

8d887a5

hotswap

d82dc0b

staffik added 16 commits June 3, 2024 16:53

Dynamic validator key everywhere

6657b3f

Dynamic validator_id for shards_manager_actor

a335707

Advertise Tier1 proxies

760f8b3

Adjust ViewClientActorInner

78fe1a8

Adjust StateSyncDumper

07ec14a

formatting

718942a

clippy

ca88247

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

96bef88

clippy

960ca21

Merge branch 'refactor-signer' into validator-key-hot-swap

bc47972

update tests

a1ee361

add comments

f0e444d

Merge branch 'refactor-signer' into validator-key-hot-swap

0b77278

compiler errors

22b1af1

style fix

ff4d55d

fix

1866a90

Base automatically changed from refactor-signer to master June 10, 2024 14:26

staffik added 3 commits June 10, 2024 16:37

nit

c3ce95d

Merge remote-tracking branch 'origin/master' into validator-key-hot-swap

637d384

Turn back into noop

a153c73

staffik changed the title ~~Validator key hot swap~~ refactor: Make validator_signer mutable Jun 10, 2024

staffik added 2 commits June 11, 2024 16:33

fix

054c742

Merge remote-tracking branch 'origin/master' into validator-key-hot-swap

70cc0ed

staffik requested a review from wacban June 11, 2024 17:32

wacban reviewed Jun 12, 2024

View reviewed changes

staffik added 4 commits June 19, 2024 11:46

Refactor client validator_signer

044e772

Merge remote-tracking branch 'origin/master' into validator-key-hot-swap

10c1699

cargo fmt

dc264e8

FrozenValidatorConfig

909e03d

wacban reviewed Jun 20, 2024

View reviewed changes

comments

b2646ba

staffik force-pushed the validator-key-hot-swap branch from 295add3 to b2646ba Compare June 20, 2024 11:49

staffik added 2 commits June 20, 2024 14:20

Reduce me.as_ref() and prefer &Arc<ValidatorSigner>

b053500

Enrich NotAValidator error

2b04477

staffik force-pushed the validator-key-hot-swap branch from 3a71acf to 2b04477 Compare June 20, 2024 13:03

comment

a465905

wacban approved these changes Jun 20, 2024

View reviewed changes

chain/chain-primitives/src/error.rs Outdated Show resolved Hide resolved

staffik added 2 commits June 21, 2024 10:08

enrich error

9b6e83c

Merge remote-tracking branch 'origin/master' into validator-key-hot-swap

18b9705

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

Merged via the queue into master with commit e95c123 Jun 21, 2024
30 checks passed

staffik deleted the validator-key-hot-swap branch June 21, 2024 08:54

staffik mentioned this pull request Jun 21, 2024

fix: spammy log - not a validator #11647

Merged

github-merge-queue bot pushed a commit that referenced this pull request Jun 21, 2024

fix: spammy log - not a validator (#11647)

5ef1ac3

See #11400 (comment) It seems non-validator nodes still call `process_ready_orphan_witnesses_and_clean_old` and logging an error in such case is spammy.

staffik added a commit that referenced this pull request Jun 25, 2024

fix: spammy log - not a validator (#11647)

de83cf0

See #11400 (comment) It seems non-validator nodes still call `process_ready_orphan_witnesses_and_clean_old` and logging an error in such case is spammy.

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: Make validator_signer mutable #11400

refactor: Make validator_signer mutable #11400

staffik commented May 27, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

wacban Jun 12, 2024

wacban left a comment

wacban Jun 20, 2024

staffik Jun 20, 2024

wacban Jun 20, 2024

pugachAG Jun 20, 2024

wacban Jun 20, 2024

staffik Jun 20, 2024

wacban left a comment

andrei-near commented Jun 21, 2024

	// TODO(validator-key-hot-swap) Consider using mutable validator signer instead of PeerInfo.account_id ?
	// That likely requires bigger changes and account_id here is later used for debug / logging purposes only.

refactor: Make validator_signer mutable #11400

refactor: Make validator_signer mutable #11400

Conversation

staffik commented May 27, 2024 • edited Loading

Summary

Extra changes

codecov bot commented May 27, 2024 • edited Loading

Codecov Report

wacban Jun 12, 2024

Choose a reason for hiding this comment

wacban left a comment

Choose a reason for hiding this comment

wacban Jun 20, 2024

Choose a reason for hiding this comment

staffik Jun 20, 2024

Choose a reason for hiding this comment

wacban Jun 20, 2024

Choose a reason for hiding this comment

pugachAG Jun 20, 2024

Choose a reason for hiding this comment

wacban Jun 20, 2024

Choose a reason for hiding this comment

staffik Jun 20, 2024

Choose a reason for hiding this comment

wacban left a comment

Choose a reason for hiding this comment

andrei-near commented Jun 21, 2024

staffik commented May 27, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading