fix(validator_store): filter inactive validators by petarjuki7 · Pull Request #576 · sigp/anchor

petarjuki7 · 2025-09-03T12:58:36Z

Issue Addressed

#558
Makes sure if a cluster is liquidated that it doesn't get processed.

petarjuki7 · 2025-09-03T12:59:49Z

anchor/validator_store/src/lib.rs

        // First, attempt to get the cluster normally
        if let Some(cluster) = state.clusters().get_by(&validator.cluster_id) {
+            if cluster.liquidated {
+                return Err(Error::SpecificError(SpecificError::Unsupported));


Not sure if this is the appropriate error to throw

Should probably just add a new variant in, something like ClusterLiquidated

Copilot

Pull Request Overview

This PR fixes a critical issue where liquidated validators were still being processed by adding a check to filter out inactive validators from cluster operations. The change ensures that validators belonging to liquidated clusters are properly rejected to prevent unintended processing.

Adds a liquidation status check for validator clusters
Returns an error when attempting to process validators from liquidated clusters

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

anchor/validator_store/src/lib.rs

diegomrsantos · 2025-09-03T15:54:57Z

Can we add a test to avoid this regression from being added?

dknopik · 2025-09-04T10:26:52Z

Nice! We also should filter in voting_pubkeys, so that the validator services stop attempting duties for those validators.

petarjuki7 · 2025-09-04T12:04:24Z

Can we add a test to avoid this regression from being added?

Not sure if I understand, where to add a test?

dknopik · 2025-09-04T12:57:53Z

Can we add a test to avoid this regression from being added?

While I agree, right now it is really hard to test the validator store due to it's tightly coupled nature. I think it might be better to keep it in mind for when we add more tests.

Alternatively, our integration test approach currently is based on local Testnets. We could add a check there where we assert that a liquidated validator does not attest

dknopik · 2025-09-04T13:00:46Z

anchor/validator_store/src/lib.rs

            .shares()
            .values()
            .filter_map(|v| filter_func(DoppelgangerStatus::SigningEnabled(v.validator_pubkey)))
+            .filter(|public_key| self.get_validator_and_cluster(*public_key).is_ok())


This approach has a problem:

We try to lock (within get_validator_and_cluster) while we already hold a lock (here by calling state)

This can cause a deadlock, as the second lock might wait for a pending writer, which waits for the lock we're holding here first.

Instead, get the cluster here and check for liquidation.

Ah okay, I understand, I meant to save from some duplication of code.

diegomrsantos · 2025-09-04T13:01:55Z

Can we add a test to avoid this regression from being added?

While I agree, right now it is really hard to test the validator store due to it's tightly coupled nature. I think it might be better to keep it in mind for when we add more tests.

Alternatively, our integration test approach currently is based on local Testnets. We could add a check there where we assert that a liquidated validator does not attest

Could you explain in more detail how our integration test approach works? Where are the tests located?

dknopik · 2025-09-04T13:24:02Z

@diegomrsantos I am talking about #562. It runs a Testnet and then checks if the expected duties are performed. It seems feasible to add a liquidated cluster and check that it does not perform duties.

diegomrsantos · 2025-09-04T13:26:21Z

@diegomrsantos I am talking about #562. It runs a Testnet and then checks if the expected duties are performed. It seems feasible to add a liquidated cluster and check that it does not perform duties.

But where are the tests exactly? I see only a yaml file there

dknopik · 2025-09-05T11:36:19Z

anchor/validator_store/src/lib.rs

+    fn is_cluster_active(&self, validator_pubkey: &PublicKeyBytes) -> bool {
+        let state = self.database.state();
+
+        if let Some(validator) = state.metadata().get_by(validator_pubkey)
+            && let Some(cluster) = state.clusters().get_by(&validator.cluster_id)
+        {
+            return !cluster.liquidated;
+        }
+
+        // We did not manage to fetch the cluster
+        false
+    }


Unfortunately, same issue as before:

By trying to borrow here (in line 594) while we already holding the lock through the .state() call in voting_pubkeys, we are at risk of a deadlock if a writer tries to acquire the lock between the two calls.

Furthermore, you can call get_by directly with the validator pubkey - no need to go via the metadata, as the multi index map allows access via the validator_pubkey.

dknopik

LGTM, thanks! I've got one nitpick, but that one is a matter of taste. Feel free to address or merge (by adding the ready-to-merge label)

dknopik · 2025-09-05T15:09:33Z

anchor/validator_store/src/lib.rs

+                if let Some(clusters) = clusters.get_by(public_key) {
+                    return !clusters.liquidated;
+                }
+                false


nice, exactly :)

still got one nitpick in me :P

I really like Option::is_some_and, but that is a matter of taste

The base branch was changed.

dknopik · 2025-09-05T15:14:02Z

Sorry, I just noticed that this was based on unstable. Most bugfixes should be based on the releases-v0.3.0 branch, in order to separate experimental work (on unstable) from our fixes ahead of the big release. Can you please rebase your commits on top of release-v0.3.0?

petarjuki7 · 2025-09-08T07:42:39Z

Closing this in favour of #589 to keep a cleaner commit history

#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes #576

sigp#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes sigp#576

petarjuki7 changed the title ~~fix(validator_store):filter inactive falidators~~ fix(validator_store):filter inactive validators Sep 3, 2025

petarjuki7 changed the title ~~fix(validator_store):filter inactive validators~~ fix(validator_store): filter inactive validators Sep 3, 2025

petarjuki7 commented Sep 3, 2025

View reviewed changes

diegomrsantos requested a review from Copilot September 3, 2025 14:53

Copilot AI reviewed Sep 3, 2025

View reviewed changes

anchor/validator_store/src/lib.rs Show resolved Hide resolved

petarjuki7 self-assigned this Sep 3, 2025

dknopik reviewed Sep 4, 2025

View reviewed changes

dknopik reviewed Sep 5, 2025

View reviewed changes

dknopik previously approved these changes Sep 5, 2025

View reviewed changes

dknopik added the v1.0.0 First Mainnet-release label Sep 5, 2025

dknopik changed the base branch from unstable to release-v0.3.0 September 5, 2025 15:12

chong-he and others added 9 commits September 6, 2025 16:58

docs: small revision to the documentation (sigp#512)

887ce52

chore: Add PartialEq and Eq to KeyId (sigp#520)

640a14b

fix(validator_store):Filter inactive falidators

12b5ad5

Error variant added

8ebe015

Add filter on voting_pubkeys

7c7c321

Add is_cluster_active function

480c819

Code style fix

cb5d029

State access refactor

c7b4289

Small style fix

d2d1e46

petarjuki7 force-pushed the petarjuki7/filter_inactive_validators branch from 2fac0bf to d2d1e46 Compare September 6, 2025 14:58

petarjuki7 mentioned this pull request Sep 8, 2025

fix(validator_store): filter inactive validators #589

Merged

petarjuki7 closed this Sep 8, 2025

mergify bot pushed a commit that referenced this pull request Sep 8, 2025

fix(validator_store): filter inactive validators (#589)

1c036c8

#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes #576

petarjuki7 added a commit to petarjuki7/anchor that referenced this pull request Sep 16, 2025

fix(validator_store): filter inactive validators (sigp#589)

7abb18d

sigp#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes sigp#576

diegomrsantos pushed a commit to diegomrsantos/anchor that referenced this pull request Sep 17, 2025

fix(validator_store): filter inactive validators (sigp#589)

9401fba

sigp#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes sigp#576

jking-aus pushed a commit to jking-aus/anchor that referenced this pull request Oct 8, 2025

fix(validator_store): filter inactive validators (sigp#589)

43aa06a

sigp#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes sigp#576

petarjuki7 added a commit to petarjuki7/anchor that referenced this pull request Oct 16, 2025

fix(validator_store): filter inactive validators (sigp#589)

db602e6

sigp#558 Makes sure if a cluster is liquidated that it doesn't get processed. Closes sigp#576

Comments

Conversation

petarjuki7 commented Sep 3, 2025

Issue Addressed

Uh oh!

petarjuki7 Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Zacholme7 Sep 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

diegomrsantos commented Sep 3, 2025

Uh oh!

dknopik commented Sep 4, 2025

Uh oh!

petarjuki7 commented Sep 4, 2025

Uh oh!

dknopik commented Sep 4, 2025

Uh oh!

dknopik Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

petarjuki7 Sep 4, 2025

Choose a reason for hiding this comment

Uh oh!

diegomrsantos commented Sep 4, 2025

Uh oh!

dknopik commented Sep 4, 2025

Uh oh!

diegomrsantos commented Sep 4, 2025

Uh oh!

dknopik Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik left a comment

Choose a reason for hiding this comment

Uh oh!

dknopik Sep 5, 2025

Choose a reason for hiding this comment

Uh oh!

dknopik commented Sep 5, 2025

Uh oh!

petarjuki7 commented Sep 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants