Allow validators to reset to the slot which matches their last voted slot #28172

jstarry · 2022-10-01T09:22:32Z

Problem

Validators are unable to reset back to the heaviest bank on the fork that includes their last vote if the heaviest bank's slot matches their last voted slot. If there is a heavier bank on the last voted fork than the last voted bank itself, validators can erroneously reset to an invalid slot.

Summary of Changes

Allow resetting to a slot which matches the last vote slot
Disallow resetting to a slot which is part of an invalid fork
Fix tests

Fixes #

jstarry · 2022-10-01T10:37:59Z

Turns out we already had a test for a very similar case: test_unconfirmed_duplicate_slots_and_lockouts. This PR still needs to modify or copy that test with a condition that makes the non voted fork the heavier fork and ensure that we can reset to the heaviest voted fork when the dup is confirmed but the switch threshold fails.

jstarry · 2022-10-01T15:07:15Z

The latest commit has a test that fails on latest master but is fixed in this PR. This is now ready for review!

jstarry · 2022-10-01T16:01:23Z

Just pushed a small patch to the failing test

core/src/replay_stage.rs

jstarry · 2022-10-01T16:08:52Z

core/src/replay_stage.rs

+        );
+        // Should now pick 5 the heaviest fork from last vote again.
+        assert!(vote_fork.is_none());
+        assert_eq!(reset_fork.unwrap(), 5);


It also would fail here if it didn't get stuck on the previous failure.

nice gud test I like

carllin · 2022-10-01T18:39:58Z

core/src/heaviest_subtree_fork_choice.rs

-                        return None;
+                match self.is_candidate(&last_voted_slot_hash) {
+                    Some(true) => self.best_slot(&last_voted_slot_hash),
+                    Some(false) => None,


This may be a fine change, but trying to minimize as many logical changes as possible.

In this new code is_candidate(&last_voted_slot_hash) == false, this returns None. This seems like it might be a deviation from the original code.

To maintain the same behavior would require that in the new code if self.is_candidate(&last_voted_slot_hash) == Some(false) then that implies inthe old code that heaviest_slot_hash_on_same_voted_fork == None in the old code, which I don't think is necessarily true?

I'm having a hard time understanding what you're suggesting that I change here

To maintain the same behavior would require that in the new code if self.is_candidate(&last_voted_slot_hash) == Some(false) then that implies in the old code that heaviest_slot_hash_on_same_voted_fork == None in the old code, which I don't think is necessarily true?

I think he's saying that this is a new condition that wasn't checked in the old code that might now return None.
I don't think this implies in the old code that heaviest_slot_hash_on_same_voted_fork needed to be None, this just requires that there is an invalid ancestor which wasn't being checked before.

agreed that we should try to reduce the change set to the bare minimum here. we need to fix the known bug now, further hardening can wait.

kinda confusing that a comment like this would result in an approved review. seems blocking

The change is already at the bare minimum in my opinion. Take note that self.is_candidate(..).is_none() and self.best_slot(..).is_none() is the same thing because each of them is only None when the queried slot is not in fork_infos.

The only logical change is switching from filtering the returned heaviest slot by fork_info.is_candidate rather than checking if it is the same slot as the last vote. Checking is_candidate is crucial here because otherwise we would build blocks from heaviest bank even if it was a duplicate.

@t-nelson @carllin if you disagree on what the minimum change, please let me know. I'm also in favor of keeping the scope of this change as low as possible.

If we remove the last vote equality check and don't add the is_candidate check, then during the mainnet-beta issue, validators would have kept building off of 221 before it was determined to be confirmed. This might be a good thing actually but just wanted to call out that the last vote equality check happened to prevent this from happening.

The bug I intended to fix was that validators should've started building off of 221 after they detected it to be a confirmed duplicate, but didn't. Without is_candidate, the bug being fixed is that validators should've continued building off of 221 as soon as they started trying to switch to separate fork (225).

After more digging and discussion with @carllin, it looks like we have to add the is_candidate check after all because otherwise we could get into situations where the heaviest_slot_on_same_voted_fork was purged (because the cluster confirmed a different version) and we try to reset to a purged bank. I think that avoiding resetting to invalid forks should be avoided at all costs.

I think that avoiding resetting to invalid forks should be avoided at all costs.

this would be separate from the condition that halted MB though, right? i'm not saying that we shouldn't harden further, just that we can run anything extra through the usual (priority) stabilization/backport process

We did agree to add the is_candidate check in the normal process but without it we expose validators to increased risk of panicking due to purged slots.

core/src/replay_stage.rs

AshwinSekar · 2022-10-01T19:48:03Z

core/src/heaviest_subtree_fork_choice.rs

-                        return None;
+                match self.is_candidate(&last_voted_slot_hash) {
+                    Some(true) => self.best_slot(&last_voted_slot_hash),
+                    Some(false) => None,


To maintain the same behavior would require that in the new code if self.is_candidate(&last_voted_slot_hash) == Some(false) then that implies in the old code that heaviest_slot_hash_on_same_voted_fork == None in the old code, which I don't think is necessarily true?

I think he's saying that this is a new condition that wasn't checked in the old code that might now return None.
I don't think this implies in the old code that heaviest_slot_hash_on_same_voted_fork needed to be None, this just requires that there is an invalid ancestor which wasn't being checked before.

core/src/heaviest_subtree_fork_choice.rs

Pull request has been modified.

t-nelson · 2022-10-01T20:10:41Z

core/src/heaviest_subtree_fork_choice.rs

-                        return None;
+                match self.is_candidate(&last_voted_slot_hash) {
+                    Some(true) => self.best_slot(&last_voted_slot_hash),
+                    Some(false) => None,


agreed that we should try to reduce the change set to the bare minimum here. we need to fix the known bug now, further hardening can wait.

kinda confusing that a comment like this would result in an approved review. seems blocking

core/src/replay_stage.rs

t-nelson · 2022-10-01T20:39:17Z

core/src/replay_stage.rs

+        assert!(vote_fork.is_none());
+        assert_eq!(reset_fork.unwrap(), 5);
+
+        // Mark 5 as duplicate, 4 should be the heaviest slot, but should not be votable


should we assert the latter two conditions as well?

They are implicitly asserted by checking that vote bank is none

comment? 🥺

core/src/replay_stage.rs

mergify · 2022-10-03T07:21:27Z

automerge label removed due to a CI failure

…slot (#28172) * Add failing test * Allow resetting to duplicate was confirmed * feedback * feedback * bump * simplify change * Revert "simplify change" This reverts commit 72e5de3. * update comment * Update core/src/replay_stage.rs (cherry picked from commit c2bb2b8)

…slot (backport #28172) (#28185) Allow validators to reset to the slot which matches their last voted slot (#28172) * Add failing test * Allow resetting to duplicate was confirmed * feedback * feedback * bump * simplify change * Revert "simplify change" This reverts commit 72e5de3. * update comment * Update core/src/replay_stage.rs (cherry picked from commit c2bb2b8) Co-authored-by: Justin Starry <[email protected]>

…slot (backport #28172) (#28186) Allow validators to reset to the slot which matches their last voted slot (#28172) * Add failing test * Allow resetting to duplicate was confirmed * feedback * feedback * bump * simplify change * Revert "simplify change" This reverts commit 72e5de3. * update comment * Update core/src/replay_stage.rs (cherry picked from commit c2bb2b8) Co-authored-by: Justin Starry <[email protected]>

…slot (backport #28172) (#28187) Allow validators to reset to the slot which matches their last voted slot (#28172) * Add failing test * Allow resetting to duplicate was confirmed * feedback * feedback * bump * simplify change * Revert "simplify change" This reverts commit 72e5de3. * update comment * Update core/src/replay_stage.rs (cherry picked from commit c2bb2b8) Co-authored-by: Justin Starry <[email protected]>

ryoqun · 2022-10-03T17:51:36Z

core/src/replay_stage.rs

+    /// If `heaviest_bank_on_same_voted_fork` is `None` due to that fork no
+    /// longer being valid to vote on, it's possible that a validator will not
+    /// be able to reset away from the invalid fork that they last voted on. To
+    /// resolve this scenario, validators need to wait until they can create a
+    /// switch proof for another fork or until the invalid fork is be marked
+    /// valid again if it was confirmed by the cluster.
+    /// Until this is resolved, leaders will build each of their
+    /// blocks from the last reset bank on the invalid fork.


ryoqun · 2022-10-03T17:52:36Z

core/src/replay_stage.rs

@@ -5605,6 +5614,139 @@ pub(crate) mod tests {
        );
    }

+    #[test]
+    fn test_unconfirmed_duplicate_slots_and_lockouts_for_non_heaviest_fork() {


❤️ textbook-level quite readable unit test.

ryoqun · 2022-10-03T17:54:59Z

core/src/replay_stage.rs

+        assert_eq!(vote_fork.unwrap(), 4);
+        assert_eq!(reset_fork.unwrap(), 4);


nit: you can just resort to () for adhoc type:

assert_eq!((vote_fork, reset_fork), (Some(4), Some(4));

this should give most helpful assert.

...in other words, rust rocks

same for below this paired asserts.

ryoqun · 2022-10-03T17:57:24Z

core/src/heaviest_subtree_fork_choice.rs

-                        // (newer snapshot, or only a saved tower is moved over to new setup?)
-                        return None;
+                match self.is_candidate(&last_voted_slot_hash) {
+                    Some(true) => self.best_slot(&last_voted_slot_hash),


nit: mentioned at the different review thread, but i'd prefer provable .unwrap() here for less conditions: Some(self.best_slot(&last_voted_slot_hash).unwrap()).

ryoqun

one more (post-merge) lgtm from me, who did independent root cause analysis. :)

…t voted slot (backport solana-labs#28172) (solana-labs#28187)" This reverts commit 0a7006a.

…heir last voted slot (backport solana-labs#28172) (solana-labs#28187)"" This reverts commit 2e25fbc.

…t voted slot (backport solana-labs#28172) (solana-labs#28187)" This reverts commit 0a7006a.

…heir last voted slot (backport solana-labs#28172) (solana-labs#28187)"" This reverts commit 2e25fbc.

…t voted slot (solana-labs#28172)" This reverts commit c2bb2b8.

jstarry added v1.13 labels Oct 1, 2022

jstarry requested review from ryoqun, t-nelson, carllin and AshwinSekar October 1, 2022 09:22

jstarry changed the title ~~Allow resetting to duplicate was confirmed~~ Allow resetting to a duplicate slot that was confirmed by the cluster Oct 1, 2022

jstarry marked this pull request as ready for review October 1, 2022 15:06

jstarry added 2 commits October 2, 2022 00:00

Add failing test

094a420

Allow resetting to duplicate was confirmed

addc85f

jstarry force-pushed the fix/reset-to-dup branch from f6be321 to addc85f Compare October 1, 2022 16:00

jstarry commented Oct 1, 2022

View reviewed changes

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

jstarry commented Oct 1, 2022

View reviewed changes

carllin previously approved these changes Oct 1, 2022

View reviewed changes

AshwinSekar previously approved these changes Oct 1, 2022

View reviewed changes

feedback

e3c9748

jstarry added the automerge Merge this Pull Request automatically once CI passes label Oct 1, 2022

t-nelson reviewed Oct 1, 2022

View reviewed changes

t-nelson removed the automerge Merge this Pull Request automatically once CI passes label Oct 1, 2022

feedback

c175173

jstarry force-pushed the fix/reset-to-dup branch from eb34319 to c175173 Compare October 2, 2022 05:23

bump

60f9251

jstarry changed the title ~~Allow resetting to a duplicate slot that was confirmed by the cluster~~ Allow resetting to a duplicate slot while waiting to switch to another fork Oct 2, 2022

jstarry changed the title ~~Allow resetting to a duplicate slot while waiting to switch to another fork~~ Allow validators to reset to the slot which matches their last voted slot Oct 2, 2022

simplify change

72e5de3

jstarry commented Oct 3, 2022

View reviewed changes

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

Update core/src/replay_stage.rs

8120aa7

carllin approved these changes Oct 3, 2022

View reviewed changes

jstarry added the automerge Merge this Pull Request automatically once CI passes label Oct 3, 2022

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Oct 3, 2022

jstarry merged commit c2bb2b8 into solana-labs:master Oct 3, 2022

jstarry deleted the fix/reset-to-dup branch October 3, 2022 08:50

mergify bot mentioned this pull request Oct 3, 2022

Allow validators to reset to the slot which matches their last voted slot (backport #28172) #28185

Merged

mergify bot mentioned this pull request Oct 3, 2022

Allow validators to reset to the slot which matches their last voted slot (backport #28172) #28186

Merged

mergify bot mentioned this pull request Oct 3, 2022

Allow validators to reset to the slot which matches their last voted slot (backport #28172) #28187

Merged

ryoqun reviewed Oct 3, 2022

View reviewed changes

carllin added a commit to carllin/solana that referenced this pull request Oct 25, 2022

Revert "Allow validators to reset to the slot which matches their las…

bcb4cbf

…t voted slot (backport solana-labs#28172) (solana-labs#28187)" This reverts commit 0a7006a.

carllin added a commit to carllin/solana that referenced this pull request Dec 3, 2022

Revert "Allow validators to reset to the slot which matches their las…

2e25fbc

…t voted slot (backport solana-labs#28172) (solana-labs#28187)" This reverts commit 0a7006a.

carllin added a commit to carllin/solana that referenced this pull request Dec 3, 2022

Revert "Revert "Allow validators to reset to the slot which matches t…

2907bee

…heir last voted slot (backport solana-labs#28172) (solana-labs#28187)"" This reverts commit 2e25fbc.

carllin added a commit to carllin/solana that referenced this pull request Dec 23, 2022

Revert "Allow validators to reset to the slot which matches their las…

988aa74

…t voted slot (backport solana-labs#28172) (solana-labs#28187)" This reverts commit 0a7006a.

carllin added a commit to carllin/solana that referenced this pull request Dec 23, 2022

Revert "Revert "Allow validators to reset to the slot which matches t…

72d03a1

…heir last voted slot (backport solana-labs#28172) (solana-labs#28187)"" This reverts commit 2e25fbc.

carllin added a commit to carllin/solana that referenced this pull request Dec 23, 2022

Revert "Allow validators to reset to the slot which matches their las…

226354f

…t voted slot (solana-labs#28172)" This reverts commit c2bb2b8.

carllin mentioned this pull request Dec 24, 2022

Local Cluster Duplicate Switch Test #29416

Closed

carllin mentioned this pull request Jul 25, 2023

Local Cluster Duplicate Switch Test #32614

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow validators to reset to the slot which matches their last voted slot #28172

Allow validators to reset to the slot which matches their last voted slot #28172

jstarry commented Oct 1, 2022 •

edited

Loading

jstarry commented Oct 1, 2022 •

edited

Loading

jstarry commented Oct 1, 2022

jstarry commented Oct 1, 2022

jstarry Oct 1, 2022

carllin Oct 3, 2022

carllin Oct 1, 2022

jstarry Oct 1, 2022

AshwinSekar Oct 1, 2022

t-nelson Oct 1, 2022

This comment was marked as outdated.

jstarry Oct 2, 2022

jstarry Oct 2, 2022 •

edited

Loading

jstarry Oct 2, 2022

t-nelson Oct 2, 2022

jstarry Oct 2, 2022

AshwinSekar Oct 1, 2022

t-nelson Oct 1, 2022

t-nelson Oct 1, 2022

jstarry Oct 2, 2022

t-nelson Oct 2, 2022

mergify bot commented Oct 3, 2022

ryoqun Oct 3, 2022

ryoqun Oct 3, 2022

ryoqun Oct 3, 2022

ryoqun Oct 3, 2022 •

edited

Loading

ryoqun left a comment •

edited

Loading

		assert_eq!(vote_fork.unwrap(), 4);
		assert_eq!(reset_fork.unwrap(), 4);

Allow validators to reset to the slot which matches their last voted slot #28172

Allow validators to reset to the slot which matches their last voted slot #28172

Conversation

jstarry commented Oct 1, 2022 • edited Loading

Problem

Summary of Changes

jstarry commented Oct 1, 2022 • edited Loading

jstarry commented Oct 1, 2022

jstarry commented Oct 1, 2022

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

This comment was marked as outdated.

Choose a reason for hiding this comment

jstarry Oct 2, 2022 • edited Loading

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

mergify bot commented Oct 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

ryoqun left a comment • edited Loading

Choose a reason for hiding this comment

jstarry commented Oct 1, 2022 •

edited

Loading

jstarry commented Oct 1, 2022 •

edited

Loading

jstarry Oct 2, 2022 •

edited

Loading

ryoqun Oct 3, 2022 •

edited

Loading

ryoqun left a comment •

edited

Loading