Implement Doppelganger Check #9120

nisdas · 2021-06-29T15:27:06Z

What type of PR is this?

Feature Implementation

What does this PR do? Why is it needed?

Checks for validator balances in the last N epochs, and determines if there is a positive change.
Add tests for the feature.
Gate it behind a flag.

Which issues(s) does this PR fix?

Fixes #7985

Other notes for review

potuz

Haven't checked the syntax of the PR, but the design of the protection looks good to me, simple to read and effective.

beacon-chain/rpc/validator/status.go

Co-authored-by: Potuz <potuz@potuz.net>

codecov · 2021-06-30T08:15:42Z

Codecov Report

Merging #9120 (4b74efa) into develop (2df024a) will decrease coverage by 0.25%.
The diff coverage is 54.54%.

@@             Coverage Diff             @@
##           develop    #9120      +/-   ##
===========================================
- Coverage    60.36%   60.10%   -0.26%     
===========================================
  Files          550      550              
  Lines        39123    39142      +19     
===========================================
- Hits         23616    23526      -90     
- Misses       12141    12221      +80     
- Partials      3366     3395      +29

…abs/geth-sharding into implementDoppleganger

jmozah · 2021-06-30T13:52:49Z

beacon-chain/rpc/validator/status.go

+	}
+	olderState, err := vs.StateGen.StateBySlot(ctx, params.BeaconConfig().SlotsPerEpoch.Mul(uint64(olderEpoch)))
+	if err != nil {
+		return nil, status.Error(codes.Internal, "Could not get previous state")


"Could not get older state" would be appropriate.

ahadda5 · 2021-06-30T14:00:38Z

beacon-chain/rpc/validator/server.go

@@ -62,7 +62,7 @@ type Server struct {
 	Eth1BlockFetcher       powchain.POWBlockFetcher
 	PendingDepositsFetcher depositcache.PendingDepositsFetcher
 	OperationNotifier      opfeed.Notifier
-	StateGen               *stategen.State
+	StateGen               stategen.StateManager


For my benefit. why this change?

Simply changes it from a struct to an interface, this allows us to mock it correctly for tests. *stategen.State fulfills the state manager interface.

yorickdowne · 2021-06-30T22:56:13Z

config.go and flags.go misspell "doppelganger" as "doppleganger". Correct spelling is "doppelganger" (well, anglicized, never mind the lack of umlauts :)).

prestonvanloon

A few comments. There are a few typos with Doppelganger vs Doppleganger. Correct is Doppelganger.

I have some concerns about the implied safety of this feature. Please correct me if these assumptions are wrong.

It seems that if a validator was online and attesting, but did not increase in balance, then this implementation could return a false negative.
A quick restart, the check is ignored.

Why does a validator need to provide the last known attested epoch?
Is it so that restarts would be quick?
If this is the case and that's the trade off we want to make to prevent a 2 epoch mandatory delay on every restart, then we ought to log to the user "WARNING: Validator was running within the last 2 epochs and doppelganger detection will be skipped."

prestonvanloon · 2021-06-30T14:07:39Z

proto/eth/v1alpha1/validator.proto

+    bytes signed_root = 2 [(ethereum.eth.ext.ssz_size) = "32"];
+}
+
+repeated ValidatorRequest validator_requests = 1;


prestonvanloon · 2021-07-01T00:00:21Z

beacon-chain/rpc/validator/status.go

+		// If the validator's last recorded epoch was
+		// less than 2 epoch ago, this method will not
+		// be able to catch duplicates.
+		if v.Epoch+2 >= currEpoch {
+			resp.Responses = append(resp.Responses,
+				&ethpb.DoppelGangerResponse_ValidatorResponse{
+					PublicKey:       v.PublicKey,
+					DuplicateExists: false,
+				})
+			continue
+		}


Why is this the case? It isn't clear to the reader why a request within the last 2 epochs will be marked as false or safe to proceed without actually checking.

beacon-chain/rpc/validator/status.go

prestonvanloon · 2021-07-01T00:08:08Z

shared/featureconfig/flags.go

@@ -122,6 +122,10 @@ var (
 		Name:  "enable-optimized-balance-update",
 		Usage: "Enables the optimized method of updating validator balances.",
 	}
+	enableDoppleGangerProtection = &cli.BoolFlag{


Typo with Dopple

validator/client/validator.go

potuz · 2021-07-01T00:30:20Z

I have some concerns about the implied safety of this feature. Please correct me if these assumptions are wrong.

It seems that if a validator was online and attesting, but did not increase in balance, then this implementation could return a false negative.

This is correct. The odds of this happening are less than 10^{-5} already conditional to being running a doppelganger as I showed in the issue originally. We can do better than this by checking that the balance increased more than certain negative multiple of the base_reward but the gain is minimal (if any). Notice that attesting and getting a negative reward for 2 epochs can only happen if the validator was assigned two epochs in a row to a slot 0 and missed target and head in both votes in a row. This is incredibly rare.

A quick restart, the check is ignored.

Correct again

Why does a validator need to provide the last known attested epoch?
Is it so that restarts would be quick?
If this is the case and that's the trade off we want to make to prevent a 2 epoch mandatory delay on every restart, then we ought to log to the user "WARNING: Validator was running within the last 2 epochs and doppelganger detection will be skipped."

I agree with the warning.

But let me stress something that passes without notice in all the approaches waiting, like the one implemented in Lighthouse: the point is that the 2 epochs mandatory delay is completely useless in the case that this method fails to find a doppelganger, namely. In the rare event that the user starts two validators simultaneously, then if the mandatory two epochs delay is implemented, then both validators will wait two epochs, not find anything and then start happily validating at the same time and get slashed.

So yes, this method will not find a doppelganger if both instances are launched together, but neither will waiting 2 epochs.

into implementDoppleganger

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

…abs/geth-sharding into implementDoppleganger

prestonvanloon · 2021-07-01T13:51:08Z

beacon-chain/rpc/validator/status.go

+		// If the validator's last recorded epoch was
+		// less than or equal to 2 epochs ago, this method will not
+		// be able to catch duplicates. This is due to how attestation
+		// inclusion works, where an attestation for the current epoch
+		// is able to included in the current or next epoch. Depending
+		// on which epoch it is included the balance change will be
+		// reflected in the following epoch.


This is much more clear! 👍

prestonvanloon · 2021-07-01T13:52:33Z

shared/featureconfig/flags.go

+		Usage: "Enables the validator to perform a doppelganger check. (Warning): This is not " +
+			"a foolproof method to find duplicate instances in the network. Your validator will still be" +
+			" vulnerable if it is being run in unsafe configurations.",


Looks good!

prestonvanloon · 2021-07-01T13:58:08Z

validator/client/validator_test.go

+				}
+				client.EXPECT().CheckDoppelGanger(
+					gomock.Any(), // ctx
+					gomock.Any(), // request


Could you add the actual requests for all test cases? This will add a bit more rigidity to these tests

So this is actually not trivial, I tried to do it previously, however the mock key manager actually uses a map underneath to store all the keys. When you request for all validating pubkeys, the ordering is non-deterministic, therefore any request mocked in here will be non deterministic too and basically only succeed 1/10 times. If needed I can create a new deterministic key manager to mock this.

Added a custom matcher that will help with this.

validator/client/validator.go

…hen attestation history existed. Still some tests to fix due to another bug in attester protection AttestationHistoryForPubKey.

prestonvanloon · 2021-07-01T17:22:02Z

Blocked until #9135 merges

prestonvanloon · 2021-07-01T17:29:40Z

validator/client/attest_test.go

@@ -132,7 +133,7 @@ func TestAttestToBlockHead_AttestsCorrectly(t *testing.T) {
 	m.validatorClient.EXPECT().ProposeAttestation(
 		gomock.Any(), // ctx
 		gomock.AssignableToTypeOf(&ethpb.Attestation{}),
-	).Do(func(_ context.Context, att *ethpb.Attestation) {
+	).Do(func(_ context.Context, att *ethpb.Attestation, opts ...grpc.CallOption) {


This needed to be updated after updating gomock

prestonvanloon · 2021-07-01T17:30:26Z

validator/client/validator.go

+		r := retrieveLatestRecord(attRec)
+		if pkey != r.PubKey {
+			return errors.New("attestation record mismatched public key")
+		}


Added this as a sanity check. It may not be necessary but it was some assurance that we received the correct record.

nisdas added 2 commits June 29, 2021 23:23

checkpoint changes

fa6d7ea

Merge branch 'develop' into implementDoppleganger

3c3c39e

terencechain changed the title ~~Implement DoppleGanger Check~~ Implement Doppelganger Check Jun 29, 2021

potuz requested changes Jun 29, 2021

View reviewed changes

beacon-chain/rpc/validator/status.go Outdated Show resolved Hide resolved

beacon-chain/rpc/validator/status.go Outdated Show resolved Hide resolved

nisdas and others added 3 commits June 30, 2021 14:14

Update beacon-chain/rpc/validator/status.go

461faa7

Co-authored-by: Potuz <potuz@potuz.net>

Update beacon-chain/rpc/validator/status.go

240127d

Co-authored-by: Potuz <potuz@potuz.net>

add in client side tests

baba3ee

nisdas added 4 commits June 30, 2021 16:36

add ordering

a74b646

add all new test cases

e0c84be

gate feature

5102d5a

Merge branch 'develop' into implementDoppleganger

c53ca33

nisdas marked this pull request as ready for review June 30, 2021 11:02

nisdas requested a review from a team as a code owner June 30, 2021 11:02

nisdas requested review from kasey, jmozah and rauljordan and removed request for a team June 30, 2021 11:02

nisdas added the Ready For Review label Jun 30, 2021

nisdas added 3 commits June 30, 2021 20:41

handle edge case

382c221

Merge branch 'implementDoppleganger' of https://github.com/prysmaticl…

43101ed

…abs/geth-sharding into implementDoppleganger

add one more test case

2f23842

jmozah reviewed Jun 30, 2021

View reviewed changes

ahadda5 reviewed Jun 30, 2021

View reviewed changes

nisdas added 2 commits June 30, 2021 22:23

fatal error

db90b7a

zahoor's review

69d4c6a

ahadda5 mentioned this pull request Jun 30, 2021

Validator client can start with invalid parameters #9127

Closed

prestonvanloon requested changes Jul 1, 2021

View reviewed changes

nisdas and others added 9 commits July 1, 2021 11:15

Merge branch 'develop' of https://github.com/prysmaticlabs/geth-sharding

dfaaf4a

into implementDoppleganger

Update validator/client/validator.go

20d95bc

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

Update validator/client/validator.go

3611f5d

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

doppelganger not doppleganger

d624bbd

Merge branch 'implementDoppleganger' of https://github.com/prysmaticl…

09f260e

…abs/geth-sharding into implementDoppleganger

preston's review

0a3941a

add in comment

959bcbc

change comment

3845258

Fix e2e to only run new flags on the current version

684038c

prestonvanloon reviewed Jul 1, 2021

View reviewed changes

validator/client/validator.go Show resolved Hide resolved

Fix bug where zero byte public keys were always sent in the request w…

2e5ad7f

…hen attestation history existed. Still some tests to fix due to another bug in attester protection AttestationHistoryForPubKey.

prestonvanloon mentioned this pull request Jul 1, 2021

Return pubkey with AttestationHistoryForPubKey AttestationRecord response #9135

Merged

go mod tidy, gazelle

dac200f

prestonvanloon reviewed Jul 1, 2021

View reviewed changes

Merge branch 'develop' into implementDoppleganger

c6b6b21

prestonvanloon previously approved these changes Jul 1, 2021

View reviewed changes

Increase test size

5f3677e

prestonvanloon dismissed their stale review via 5f3677e July 1, 2021 22:33

fix timeout, change size back to small

4b74efa

prestonvanloon approved these changes Jul 1, 2021

View reviewed changes

nisdas merged commit 5d65ace into develop Jul 2, 2021

delete-merged-branch bot deleted the implementDoppleganger branch July 2, 2021 04:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Doppelganger Check #9120

Implement Doppelganger Check #9120

nisdas commented Jun 29, 2021 •

edited

Loading

potuz left a comment

codecov bot commented Jun 30, 2021 •

edited

Loading

jmozah Jun 30, 2021

ahadda5 Jun 30, 2021

nisdas Jun 30, 2021

yorickdowne commented Jun 30, 2021

prestonvanloon left a comment

prestonvanloon Jun 30, 2021

prestonvanloon Jul 1, 2021

prestonvanloon Jul 1, 2021

potuz commented Jul 1, 2021

prestonvanloon Jul 1, 2021

prestonvanloon Jul 1, 2021

prestonvanloon Jul 1, 2021

nisdas Jul 1, 2021

prestonvanloon Jul 1, 2021

prestonvanloon commented Jul 1, 2021

prestonvanloon Jul 1, 2021

prestonvanloon Jul 1, 2021

Implement Doppelganger Check #9120

Implement Doppelganger Check #9120

Conversation

nisdas commented Jun 29, 2021 • edited Loading

potuz left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 30, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorickdowne commented Jun 30, 2021

prestonvanloon 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

potuz commented Jul 1, 2021

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

prestonvanloon commented Jul 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nisdas commented Jun 29, 2021 •

edited

Loading

codecov bot commented Jun 30, 2021 •

edited

Loading