Warn validators about stake deactivation #5955

cyberbono3 · 2019-09-18T18:46:46Z

|Fixes] #5773

Summary of Changes

validators_current_stakes
ctrlc::set_handler

Questions:

Should i match vote_state.node_pubkey and validator public key here ?
if we vote_state.node_pubkey == validator.publickey so i can warn this validator in ctrlc::set_handler right?

I need some guidance to proceed further.

mvines

Should i match vote_state.node_pubkey and validator public key here ?

Yep, that looks right to me!

core/src/validator.rs

cyberbono3 · 2019-09-19T23:19:05Z

@mvines could you leave a feedback, please?

core/src/validator.rs

cyberbono3 · 2019-09-20T22:11:20Z

@mvines I added a counter and put logic inside set_handler. Does it look good?

mvines

Looking great so far, I have some comments but 📈

Can you remove the changes to programs/bpf/Cargo.lock in this PR too please, they don't seem related

core/src/validator.rs

mvines · 2019-09-21T04:19:48Z

core/src/validator.rs

+                .map(|accounts| {
+                    accounts
+                        .iter()
+                        .filter_map(|(pubkey, (stake, account))| {


nit: I think we can make this block smaller, like this:

.filter_map(|(pubkey, (stake, account))| { if let Ok(vote_state) = VoteState::deserialize(&account.data) && vote_state.node_pubkey == validator_pubkey { return Some((*pubkey, *stake)); } None })

Does this approach not work well?

What do you mean here?

I'm just suggesting the filter_map() implementation be restructured to avoid the double else { None } at the end, with the sample code I wrote

mvines · 2019-09-21T04:21:48Z

core/src/validator.rs

+                        .collect()
+                })
+                .unwrap();
+                if current_stakes.len() > 0 {


Hmm, I think we could just move the warn!() into the .filter_map() and then remove this entire block.

core/src/validator.rs

codecov · 2019-09-24T18:33:14Z

Codecov Report

Merging #5955 into master will decrease coverage by 4.8%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #5955     +/-   ##
========================================
- Coverage    79.2%   74.3%   -4.9%     
========================================
  Files         219     238     +19     
  Lines       41905   42312    +407     
========================================
- Hits        33200   31464   -1736     
- Misses       8705   10848   +2143

cyberbono3 · 2019-09-24T18:39:02Z

@mvines ready for review

mvines

I had a couple more comments, this is progressing nicely though!

validator/src/main.rs

core/src/validator.rs

cyberbono3 · 2019-09-25T17:27:22Z

@mvines, thanks for your guidance

cyberbono3 · 2019-09-25T17:42:18Z

I just copied programs/bpf/Cargo.lock from solana master branch. I hope it works.

mvines

Also remove the changes to programs/bpf/Cargo.lock please.

mvines · 2019-09-30T14:12:42Z

core/src/validator.rs

+                .map(|accounts| {
+                    accounts
+                        .iter()
+                        .filter_map(|(pubkey, (stake, account))| {


Does this approach not work well?

mvines · 2019-09-30T14:14:35Z

core/src/validator.rs

@@ -168,7 +173,36 @@ impl Validator {
            config.storage_slots_per_turn,
            bank.slots_per_segment(),
        );
-
+        let bf = bank_forks.clone();


Can you wrap this entire new block in:

if config. enable_ctrl_c_handler { .. }

since we only want to execute it when the handler is enabled.

Once that's done within the new if block we can do this, which will avoid the need to use a short less readable bf variable name :

- let bf = bank_forks.clone(); + let bank_forks = bank_forks.clone();

mvines · 2019-09-30T14:15:03Z

validator/src/main.rs


    validator_config.rpc_config.drone_addr = matches.value_of("rpc_drone_addr").map(|address| {
-        solana_netutil::parse_host_port(address).expect("failed to parse drone address")
+


can you please avoid making this change

If you take a look at line 442, I added this line "solana_netutil::parse_host_port(address).expect("failed to parse drone address")"

mvines · 2019-09-30T14:16:12Z

Please feel free to promote this PR from "Draft" to a normal PR. I think this is almost ready to land!

cyberbono3

Also remove the changes to programs/bpf/Cargo.lock please.

I have reviewed programs/bpf/Cargo.lock.
There is only one line added to programs/bpf/Cargo.lock to enable CTRL+C catch functionality.

"ctrlc 3.1.3 (registry+https://github.com/rust-lang/crates.io-index)"

cyberbono3 · 2019-09-24T18:37:40Z

core/src/validator.rs

                    warn!("Validator exits..");
                    // run vvalidator exit 
                }
-                *counter_ref += 1;
+                *c += 1;


it should work , there is test here. don't u think we have to test set_handler()?

Sorry I'm not quite sure what you mean here?

I think this comment has been made last week. It is no longer relevant

mvines · 2019-09-30T23:03:50Z

Also remove the changes to programs/bpf/Cargo.lock please.

I have reviewed programs/bpf/Cargo.lock.
There is only one line added to programs/bpf/Cargo.lock to enable CTRL+C catch functionality.

"ctrlc 3.1.3 (registry+https://github.com/rust-lang/crates.io-index)"

Ah, got it. Yep that's fine thanks :)

cyberbono3 · 2019-10-01T03:04:30Z

I ran cargo test in solana/core and got a following error

cyberbono3 · 2019-10-01T03:07:49Z

@mvines, you wrote :
.map(|accounts| { accounts .iter() .filter_map(|(pubkey, (stake, account))| {

Does this approach not work well?

What do you mean by that?

mvines · 2019-10-01T19:35:56Z

core/Cargo.toml

@@ -76,7 +76,8 @@ tokio-codec = "0.1"
 tokio-fs = "0.1"
 tokio-io = "0.1"
 untrusted = "0.7.0"
-solana-rayon-threadlimit = { path = "../rayon-threadlimit", version = "0.20.0" }
+solana-rayon-threadlimit = { path = "../rayon-threadlimit", version = "0.19.0-pre0" }


oops, this change shouldn't have snuck in

This change is still unneeded, version should remain at 0.20.0 please

mvines · 2019-10-01T19:37:34Z

core/src/validator.rs

+                .map(|accounts| {
+                    accounts
+                        .iter()
+                        .filter_map(|(pubkey, (stake, account))| {


I'm just suggesting the filter_map() implementation be restructured to avoid the double else { None } at the end, with the sample code I wrote

mvines · 2019-10-01T19:39:38Z

core/src/validator.rs

+        let bank_forks = bank_forks.clone();
+        let counter = Arc::new(AtomicUsize::new(0));


These two lines can move after if config. enable_ctrl_c_handler { to keep their scope as limited as possible

mvines · 2019-10-01T19:40:15Z

validator/src/main.rs

@@ -1,3 +1,3 @@
 fn main() {
    solana_validator::main()
-}


Oh where did the change to validator/src/ go that sets enable_ctrl_c_handler = true

cyberbono3 · 2019-10-03T03:18:06Z

i hope no more errors left. @mvines, could you review it, please?

mvines · 2019-10-03T04:06:09Z

Looks super close, thanks for your patience. I see two issues:

Your changes to validator/src/main.rs should go into validator/src/lib.rs instead
Can you please run cargo fmt, which will format the new code you're adding into the standard style

cyberbono3 · 2019-10-03T15:40:53Z

@mvines, I applied changes to validator/src/lib.rs and ran cargo fmt.

mvines · 2019-10-03T15:47:14Z

validator/src/lib.rs

-        .send()
-        .and_then(|response| response.error_for_status())
-        .map_err(|err| format!("Unable to get: {:?}", err))?;
+    let client = ureq::agent();


It looks like you're working off an older version of the code instead of the HEAD of the master branch. We switched from ureq to reqwest maybe a week ago now, so all these changes shouldn't be present.

it seems i just have to copy validator/src/lib.rs of master branch to my branch right?

I'd get the latest lib.rs from the master branch, then only add in your change (which if I recall is just a single line)

mvines · 2019-10-03T15:49:01Z

core/src/validator.rs

+                    std::process::exit(1);
+                }
+                let bank = bank_forks.read().unwrap().working_bank();
+                let _: Vec<_> = bank


The point was that if the node currently has no stake it's ok to exit on the first ^C though. I don't see that code anymore here. It looks like we now collect all the stake but never check it for 0, and always exit on the second ^C

cyberbono3 · 2019-10-04T22:51:31Z

@mvines, i have updated lib.rs ( copied lib.rs from master branch and added one new line) and added few lines that a validator exits on first ^C if he has no stake. Please let me know what I have to do to proceed further.

mvines · 2019-10-04T22:53:25Z

core/Cargo.toml

@@ -76,7 +76,8 @@ tokio-codec = "0.1"
 tokio-fs = "0.1"
 tokio-io = "0.1"
 untrusted = "0.7.0"
-solana-rayon-threadlimit = { path = "../rayon-threadlimit", version = "0.20.0" }
+solana-rayon-threadlimit = { path = "../rayon-threadlimit", version = "0.19.0-pre0" }


This change is still unneeded, version should remain at 0.20.0 please

core/src/validator.rs

cyberbono3 · 2019-10-05T22:55:10Z

core/src/validator.rs

-                                        warn!("Validator with no stake exiting on first ^C");
+                                if *pubkey == keypair.pubkey() && *stake > 0 {
+                                    if let Ok(vote_state) = VoteState::deserialize(&account.data) {
+                                        warn!("Validator with stake is exiting on first ^C");
                                        std::process::exit(1);


@mvines , do u think i should return Some(*pubkey, *stake) somewhere here?

I'd just return Some(*stake) here. Then turn epoch_vote_accounts(..).map(|accounts| ...) into epoch_vote_accounts(..).filter_map(|accounts| ...). This way once we arrive at line 206 we have either

value None if the node is not stake, in which can we can std::process::exit(1)

Some(stake) representing the current stake of the node, in which can we can warn! the user

cyberbono3 · 2019-10-05T22:56:28Z

in /core ran cargo test , it raised the following error

mvines · 2019-10-16T16:18:07Z

Hey looks like there's some compile errors with the latest commits

cyberbono3 · 2019-10-23T19:02:07Z

core/src/validator.rs

-                    .filter_map(|account| match account {
-                        pubkey, (stake, accoount) => {
+                    .filter_map(|account| match (account) {
+                        Some(pubkey, (stake, account)) => {


@mvines, I guess filter_map unwraps an option automatically, so it is not necessarily to add Some here right?

Sorry for the review delay here @cyberbono3! I just realized that we can use .find() instead of .filter_map(). This way let stakes: Vec<Option<u64>> ... turns into just let stake: Option<u64> ..., we can remove the for stake in stakes.iter() loop entirely and just match stake { ... } directly.

@mvines thanks for your feedback. I used find instead of filter_map and then matched stake directly. I tried to run cargo test from solana/core to make sure my code has no compile errors, got this error. It seems, that we use outdated librocksdb-sys v5.18.3, the current version is librocksdb-sys = "6.1.3"

cyberbono3 · 2019-10-23T19:06:30Z

@mvines, trying to complete this PR i was running cargo test in core/ to find out if my code is correct. I encountered multiple compile errors . i have tried to fix them by updeting the version of some libraries in Cargo.toml. After I fixed these compile errors, other errors have been popped up (e.g. updated library has no such method etc). i would appreciate your guidance to resolve this issue.

mvines

Thanks, getting there.

I also see CI is complaining about cargo fmt issues, a quick run of cargo fmt should fix those up though!

mvines · 2019-10-31T03:20:50Z

core/src/validator.rs

-                    .filter_map(|account| match account {
-                        pubkey, (stake, accoount) => {
+                    .filter_map(|account| match (account) {
+                        Some(pubkey, (stake, account)) => {


Sorry for the review delay here @cyberbono3! I just realized that we can use .find() instead of .filter_map(). This way let stakes: Vec<Option<u64>> ... turns into just let stake: Option<u64> ..., we can remove the for stake in stakes.iter() loop entirely and just match stake { ... } directly.

mvines · 2019-10-31T03:21:50Z

validator/src/lib.rs

@@ -457,8 +457,9 @@ pub fn main() {

    validator_config.voting_disabled = matches.is_present("no_voting");

-    validator_config.rpc_config.enable_validator_exit = matches.is_present("enable_rpc_exit");
-
+    validator_config.rpc_config.enable_fullnode_exit = matches.is_present("enable_rpc_exit");


This line looks like a minor rebase error, it shouldn't be modified.

cyberbono3 · 2019-11-03T23:40:09Z

validator/src/lib.rs

@@ -457,7 +457,7 @@ pub fn main() {

    validator_config.voting_disabled = matches.is_present("no_voting");

-    validator_config.rpc_config.enable_fullnode_exit = matches.is_present("enable_rpc_exit");
+    validator_config.rpc_config.enable_validator_exit = matches.is_present("enable_rpc_exit");


fixed rebase error here

cyberbono3 · 2019-11-04T21:51:50Z

core/src/validator.rs

+                        account.iter().find(|(pubkey, (stake, account))| {
+                            *pubkey == &keypair.pubkey()
+                                && *stake > 0
+                                && Ok(vote_state) == VoteState::deserialize(&account.data)


@mvines I got this error here Ok(vote_state) == VoteState::deserialize(&account.data) ) | ^^^^^^^^^^ not found in this scope How can I fix it?

You'll need to bring it into scope with use solana_vote_api::vote_state::VoteState;

@mvines it is already here

Is it maybe the vote_state variable that's underlined in the error message?
I think this line can be instead:

&& VoteState::deserialize(&account.data).is_ok()

@CriesofCarrots thank you

mvines · 2019-11-05T20:28:28Z

It would be good to squash the 27 commits here into 1 single commit eventually

cyberbono3 · 2019-11-07T20:38:39Z

@mvines i have squashed multiple commits in one

mvines · 2019-11-09T01:53:40Z

Hey looks like the rebase isn't quite right. Look at the diff in the PR, there's a ton of changes that essentially roll back numerous recent commits. I can't really review this unfortunately. Please ping me here once you get the rebase right. Thanks!

mvines · 2019-11-14T18:15:25Z

Obsoleted by #6874

sakridge changed the title ~~Warn validators aboiut stake deactivation~~ Warn validators about stake deactivation Sep 18, 2019

mvines reviewed Sep 19, 2019

View reviewed changes

core/src/validator.rs Outdated Show resolved Hide resolved

mvines reviewed Sep 19, 2019

View reviewed changes

core/src/validator.rs Outdated Show resolved Hide resolved

mvines reviewed Sep 19, 2019

View reviewed changes

core/src/validator.rs Outdated Show resolved Hide resolved

mvines reviewed Sep 21, 2019

View reviewed changes

cyberbono3 commented Sep 23, 2019

View reviewed changes

core/src/validator.rs Outdated Show resolved Hide resolved

mvines reviewed Sep 24, 2019

View reviewed changes

validator/src/main.rs Outdated Show resolved Hide resolved

core/src/validator.rs Outdated Show resolved Hide resolved

core/src/validator.rs Outdated Show resolved Hide resolved

mvines reviewed Sep 30, 2019

View reviewed changes

mvines added the CI Pull Request is ready to enter CI label Sep 30, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Sep 30, 2019

cyberbono3 commented Sep 30, 2019

View reviewed changes

cyberbono3 marked this pull request as ready for review October 1, 2019 03:16

mvines reviewed Oct 1, 2019

View reviewed changes

mvines reviewed Oct 3, 2019

View reviewed changes

mvines reviewed Oct 4, 2019

View reviewed changes

cyberbono3 commented Oct 5, 2019

View reviewed changes

mvines added the CI Pull Request is ready to enter CI label Oct 16, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 16, 2019

cyberbono3 commented Oct 23, 2019

View reviewed changes

mvines added the CI Pull Request is ready to enter CI label Oct 31, 2019

solana-grimes removed the CI Pull Request is ready to enter CI label Oct 31, 2019

mvines reviewed Oct 31, 2019

View reviewed changes

cyberbono3 commented Nov 3, 2019

View reviewed changes

cyberbono3 commented Nov 4, 2019

View reviewed changes

final state

4803d7d

cyberbono3 mentioned this pull request Nov 11, 2019

Warn validators about stake deactivation (rebased) #6874

Closed

mvines closed this Nov 14, 2019


		validator_config.rpc_config.drone_addr = matches.value_of("rpc_drone_addr").map(\|address\| {
		solana_netutil::parse_host_port(address).expect("failed to parse drone address")

		let bank_forks = bank_forks.clone();
		let counter = Arc::new(AtomicUsize::new(0));

Warn validators about stake deactivation #5955

Warn validators about stake deactivation #5955

Conversation

cyberbono3 commented Sep 18, 2019

Summary of Changes

mvines left a comment

Choose a reason for hiding this comment

cyberbono3 commented Sep 19, 2019

cyberbono3 commented Sep 20, 2019

mvines 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

codecov bot commented Sep 24, 2019 • edited Loading

Codecov Report

cyberbono3 commented Sep 24, 2019

mvines left a comment

Choose a reason for hiding this comment

cyberbono3 commented Sep 25, 2019

cyberbono3 commented Sep 25, 2019

mvines 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

mvines commented Sep 30, 2019

cyberbono3 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

mvines commented Sep 30, 2019

cyberbono3 commented Oct 1, 2019

cyberbono3 commented Oct 1, 2019

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

cyberbono3 commented Oct 3, 2019

mvines commented Oct 3, 2019 • edited Loading

cyberbono3 commented Oct 3, 2019

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

cyberbono3 commented Oct 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberbono3 commented Oct 5, 2019

mvines commented Oct 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyberbono3 commented Oct 23, 2019

mvines 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

mvines commented Nov 5, 2019

cyberbono3 commented Nov 7, 2019

mvines commented Nov 9, 2019

mvines commented Nov 14, 2019

codecov bot commented Sep 24, 2019 •

edited

Loading

mvines commented Oct 3, 2019 •

edited

Loading