Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange #8085

rauljordan · 2020-12-09T20:23:01Z

Part of #7813, this PR makes sure we check the following when importing proposals via slashing interchange:

Given signing roots are optional in the EIP standard, we behave as follows:

For a given block:
If we have a previous block with the same slot in our history:
If signing root is empty, we consider that proposer public key as slashable
If signing root is not empty , then we compare signing roots. If they are different,
then we consider that proposer public key as slashable.

prestonvanloon

Can you update the title? There are changes in the proposal logic rather than changes only in the import logic.

validator/client/validator.go

validator/client/propose_protect.go

prestonvanloon · 2020-12-10T22:24:17Z

validator/client/propose_protect.go

+	// If the signing root is nil, then we consider it slashable. If signing root is not nil,
+	// we check if it is different than the incoming block's signing root. If that is the case,
+	// we consider that proposal slashable.
+	if exists && (prevSigningRoot == params.BeaconConfig().ZeroHash || prevSigningRoot != signingRoot) {


Shouldn't we stop a proposal if a block proposal was already submitted?
Why allow another block to be proposed, even if it has exactly the same information?

Whatever benefit there is, I dont know if it's worth the risk of proposing a block when the validator has already proposed one.

This is totally allowed. The only slashable offense is if the signing root does not match but the slot matches

Also this is a test case for the EIP spec tests. We expect a block with same slot and same signing root to go through ok

…t-checks

rauljordan · 2020-12-11T21:02:54Z

beacon-chain/state/stateutil/attestations.go

@@ -84,66 +84,6 @@ func marshalAttestationData(data *ethpb.AttestationData) []byte {
 	return enc
 }

-func attestationRoot(hasher htrutils.HashFn, att *ethpb.Attestation) ([32]byte, error) {


Deep source was complaining of unused code

prestonvanloon · 2020-12-11T21:37:06Z

validator/client/propose.go

+	if err := v.preBlockSignValidations(ctx, pubKey, b, signingRoot); err != nil {
+		log.WithFields(
+			blockLogFields(pubKey, b, nil),
+		).WithError(err).Error("Failed block slashing protection check")
+		return
+	}
+
+	if err := v.postBlockSignUpdate(ctx, pubKey, blk, signingRoot); err != nil {


Maybe something for another PR, but these should just be merged into 1 method.

Agreed, but out of scope. Will open an issue

prestonvanloon · 2020-12-11T21:38:05Z

validator/client/propose_protect.go

 	"github.com/sirupsen/logrus"
 )

 var failedPreBlockSignLocalErr = "attempted to sign a double proposal, block rejected by local protection"
 var failedPreBlockSignExternalErr = "attempted a double proposal, block rejected by remote slashing protection"
 var failedPostBlockSignErr = "made a double proposal, considered slashable by remote slashing protection"

-func (v *validator) preBlockSignValidations(ctx context.Context, pubKey [48]byte, block *ethpb.BeaconBlock) error {
+func (v *validator) preBlockSignValidations(


Again, maybe for another PR. Would like some documentation on what this method does here.

* Change LowestSignedProposal to Also Return a Boolean for Slashing Protection (#8020) * amend to use bools * ineff assign * comment * Update `LowestSignedTargetEpoch` to include exists (#8004) * Replace highest with lowerest * Update validator/db/kv/attestation_history_v2.go Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Update validator/db/kv/attestation_history_v2.go Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Invert equality for saveLowestSourceTargetToDB * Add eip checks to ensure epochs cant be lower than db ones * Should be less than equal to * Check if epoch exists in DB getters * Revert run time checks Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * Export Attesting History for Slashing Interchange Standard (#8027) * added in att history checks * logic for export * export return nil * test for export atts * round trip passes first try! * rem println * fix up tests * pass test * Validate Proposers Are Not Slashable With Regard to Data Within Slasher Interchange JSON (#8031) * filter slashable blocks and atts in same json stub * add filter blocks func * add test for filtering out the bad public keys * Export Slashing Protection History Via CLI (#8040) * include cli entrypoint for history exports * builds properly * test to confirm we export the data as expected * abstract helpers properly * full test suite * gaz * better errors * marshal ident * Add the additional eip-3076 attestation checks (#7966) * Replace highest with lowerest * Update validator/db/kv/attestation_history_v2.go Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Update validator/db/kv/attestation_history_v2.go Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Invert equality for saveLowestSourceTargetToDB * Add eip checks to ensure epochs cant be lower than db ones * Should be less than equal to * Check if epoch exists in DB getters Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Add EIP-3076 Invariants for Proposer Slashing Protection (#8067) * add invariant for proposer protection * write different test cases * pass tests * Add EIP-3076 Interchange JSON CLI command to validator (#7880) * Import JSON CLI * CLI impotr * f * Begin adding new commands in slashing protection * Move testing helpers to separate packae * Add command for importing slashing protection JSONs * fix import cycle * fix test * Undo cleaning changes * Improvements * Add better prompts * Fix prompt * Fix * Fix * Fix * Fix conflict * Fix * Fixes * Fixes * Fix exported func * test func * Fixes * fix test * simplify import and standardize with export * add round trip test * true integration test works * fix up comments * logrus * better error * fix build * build fix * Update validator/slashing-protection/cli_export.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Update validator/slashing-protection/cli_import.go Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * fmt Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> * Filter Slashable Attester Public Keys in Slashing Interchange Import (#8051) * filter slashable attesters from the same JSON * builds * fix up initially broken test * circular dep * import fix * giz * added in attesting history package * add test for filter slashable attester keys * pass tests * Save Slashable Keys to Disk in the Validator Client (#8082) * begin db funcs * add in test and bucket * gaz * rem changes to import * ineff assign * add godoc * Properly Handle Duplicate Public Key Entries in Slashing Interchange Imports (#8089) * Prevent Blacklisted Public Keys from Slashing Protection Imports from Having Duties at Runtime (#8084) * tests on update duties * ensure the slashable public keys are filtered out from update duties via test * begin test * attempt test * rename for better context * pass tests * deep source * ensure tests pass * Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange (#8085) * flexible signing root * add test * add tests * fix test * Preston's comments * res tests * ensure we consider the case for minimum proposals * pass test * tests passing * rem unused code * Set Empty Epochs in Between Attestations as FAR_FUTURE_EPOCH in Attesting History (#8113) * set target data * all tests passing * ineff assign * signing root * Add Slashing Interchange, EIP-3076, Spec Tests to Prysm (#7858) * Add interchange test framework * add checks for attestations * Import genesis root if necessary * flexible signing root * add test * Sync * fix up test build * only 3 failing tests now * two failing * attempting to debug problems in conformity tests * include latest changes * protect test in validator/client passing * pass tests * imports * spec tests passing with bazel * gh archive link to spectests using tar.gz suffix * rev * rev more comment changes * fix sha * godoc * add back save Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * Implement Migration for Unattested Epochs in Attesting History Database (#8121) * migrate attesting history backbone done * begin migration logic * implement migration logic * migration test * add test * migration logic * bazel * migration to its own file * Handle empty blocks and attestations in interchange json and sort interchange json by public key (#8132) * Handle empty blocks and attestations in interchange json * add test * sort json * easier empty arrays * pass test Co-authored-by: Raul Jordan <raul@prysmaticlabs.com> * builds * more tests finally build * Align Slashing Interchange With Optimized Slashing Protection (#8268) * attestation history should account for multiple targets per source * attempt at some fixes * attempt some test fixes * experimenting with sorting * only one more failing test * tests now pass * slash protect tests passing * only few tests now failing * only spec tests failing now * spec tests passing * all tests passing * helper function for verifying double votes * use helper * gaz * deep source * tests fixed * expect specific number of times for domain data calls * final comments * Batch Save Imported EIP-3076 Attestations (#8304) * optimize save * test added * add test for sad path Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> * revert bad find replace * add comment to db func Co-authored-by: terence tsao <terence@prysmaticlabs.com> Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Ivan Martinez <ivanthegreatdev@gmail.com> Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com> Co-authored-by: Shay Zluf <thezluf@gmail.com>

rauljordan added 2 commits December 9, 2020 14:09

flexible signing root

558edab

add test

7fdeb78

rauljordan requested a review from a team as a code owner December 9, 2020 20:23

rauljordan requested review from 0xKiwi, prestonvanloon and terencechain December 9, 2020 20:23

rauljordan self-assigned this Dec 9, 2020

rauljordan added the Ready For Review A pull request ready for code review label Dec 9, 2020

rauljordan added 2 commits December 9, 2020 15:01

add tests

ae8f83b

fix test

67eaa89

rauljordan mentioned this pull request Dec 10, 2020

Allow resubmission of blocks with the same signing root #7866

Closed

prestonvanloon requested changes Dec 10, 2020

View reviewed changes

rauljordan changed the title ~~Check for Signing Root When Importing Proposals in Slashing Interchange~~ Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange Dec 11, 2020

rauljordan added 9 commits December 11, 2020 10:12

Preston's comments

b46d746

Merge branch 'feature/slashing-interchange' into flexible-signing-roo…

6e5e900

…t-checks

res tests

210d362

ensure we consider the case for minimum proposals

0df245d

Merge branch 'feature/slashing-interchange' into flexible-signing-roo…

e44c708

…t-checks

pass test

ee3273d

tests passing

8cd31eb

merge

d797154

rem unused code

1449a0a

rauljordan commented Dec 11, 2020

View reviewed changes

prestonvanloon approved these changes Dec 11, 2020

View reviewed changes

rauljordan merged commit 2276a85 into feature/slashing-interchange Dec 11, 2020

delete-merged-branch bot deleted the flexible-signing-root-checks branch December 11, 2020 22:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange #8085

Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange #8085

rauljordan commented Dec 9, 2020 •

edited

prestonvanloon left a comment

prestonvanloon Dec 10, 2020

rauljordan Dec 11, 2020

rauljordan Dec 11, 2020

rauljordan Dec 11, 2020

prestonvanloon Dec 11, 2020

rauljordan Dec 11, 2020

prestonvanloon Dec 11, 2020

Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange #8085

Check for Signing Root Mismatch When Submitting Proposals and Importing Proposals in Slashing Interchange #8085

Conversation

rauljordan commented Dec 9, 2020 • edited

prestonvanloon left a comment

Choose a reason for hiding this comment

prestonvanloon Dec 10, 2020

Choose a reason for hiding this comment

rauljordan Dec 11, 2020

Choose a reason for hiding this comment

rauljordan Dec 11, 2020

Choose a reason for hiding this comment

rauljordan Dec 11, 2020

Choose a reason for hiding this comment

prestonvanloon Dec 11, 2020

Choose a reason for hiding this comment

rauljordan Dec 11, 2020

Choose a reason for hiding this comment

prestonvanloon Dec 11, 2020

Choose a reason for hiding this comment

rauljordan commented Dec 9, 2020 •

edited