Recursively Find Validator Database File In Slashing Protection Commands #8518

ahadda5 · 2021-02-25T16:26:18Z

What type of PR is this?

Feature

What does this PR do? Why is it needed?
It ensures that the validator.db is found in the specified datadir or its subdirectories before the slashing export commences. Want to avoid going through the process if validator.db is not there.

Which issues(s) does this PR fix?
Fixes #8500

ahadda5 · 2021-02-25T16:35:22Z

I need some pointers before i proceed to make further changes ExportSlashingProtectionJSONCli /ImportSlashingProtectionJSONCli

A util func was added.

It returns a bool if a file is found in a dir or its sub and the path of that file. While we do not need the path for our issue 8500, i kept it for future usage?!
When unit testing , the found flag (bool) works however it is worth noting that once creating tmp dir the full path cannot be pre - determined so i commented that assert. See below.
If that is fair, i can proceed to make the change to ExportSlashingProtectionJSONCli /ImportSlashingProtectionJSONCli ensuring that validator.db exists in the dir/sub_dir and stop with an error if that is not the case.

//assert.DeepEqual(t, tt.path, fpath)

FAIL: TestRecursiveFileFind (0.00s)
    --- FAIL: TestRecursiveFileFind/file1 (0.00s)
        assertions.go:48: fileutil_test.go:289 Values are not equal, want: "subfolder1/subfolder11/file1", got: "/tmp/TestRecursiveFileFind269727708/001/subfolder1/subfolder11/file1", diff: modified:  = "/tmp/TestRecursiveFileFind269727708/001/subfolder1/subfolder11/file1"
    --- FAIL: TestRecursiveFileFind/file2 (0.00s)
        assertions.go:48: fileutil_test.go:289 Values are not equal, want: "subfolder2/file2", got: "/tmp/TestRecursiveFileFind269727708/001/subfolder2/file2", diff: modified:  = "/tmp/TestRecursiveFileFind269727708/001/subfolder2/file2"
    --- FAIL: TestRecursiveFileFind/file1#01 (0.00s)
        assertions.go:48: fileutil_test.go:289 Values are not equal, want: "subfolder11/file1", got: "/tmp/TestRecursiveFileFind269727708/001/subfolder1/subfolder11/file1", diff: modified:  = "/tmp/TestRecursiveFileFind269727708/001/subfolder1/subfolder11/file1"
    --- FAIL: TestRecursiveFileFind/file3 (0.00s)
        assertions.go:48: fileutil_test.go:289 Values are not equal, want: "file3", got: "/tmp/TestRecursiveFileFind269727708/001/file3", diff: modified:  = "/tmp/TestRecursiveFileFind269727708/001/file3"
FAIL

codecov · 2021-02-25T16:47:52Z

Codecov Report

Merging #8518 (c9f91bd) into develop (878bc15) will decrease coverage by 0.03%.
The diff coverage is 73.33%.

@@             Coverage Diff             @@
##           develop    #8518      +/-   ##
===========================================
- Coverage    58.15%   58.12%   -0.04%     
===========================================
  Files          459      459              
  Lines        32918    32905      -13     
===========================================
- Hits         19144    19126      -18     
- Misses       10872    10883      +11     
+ Partials      2902     2896       -6

rauljordan

Looks good so far!

rauljordan · 2021-02-25T17:12:54Z

shared/fileutil/fileutil_test.go

+			require.NoError(t, err)
+
+			assert.DeepEqual(t, tt.found, found)
+			//assert.DeepEqual(t, tt.path, fpath)


Remove commented code

ok will do.

rauljordan · 2021-02-25T20:00:23Z

@ahadda5 we should make the title share more context. Something like: "Recursively Find Validator Database File In Slashing Protection Commands". Additionally, this PR just adds a utility function but does not integrate it where it is needed. Since this PR is quite small, it is OK to do both things in this single PR. Please revise at your own convenience and then we can review again

ahadda5 · 2021-02-26T12:17:40Z

Sorry what title are you referring to? code commentary and/or the func name ?

@ahadda5 we should make the title share more context. Something like: "Recursively Find Validator Database File In Slashing Protection Commands". Additionally, this PR just adds a utility function but does not integrate it where it is needed. Since this PR is quite small, it is OK to do both things in this single PR. Please revise at your own convenience and then we can review again

ahadda5 · 2021-02-26T14:15:57Z

unit tested the slashing

bazel test //validator/slashing-protection:cli_import_export_test.go --test_output=streamed

However i need to test the actual export/import , how can i make sure the shell script is calling my build (read somewhere that it downloads the latest and greatest build).

prysm.sh validator slashing-protection export --datadir=/home/ahaddad/eth2validators --slashing-protection-export-dir=/home/ahaddad

ahadda5 · 2021-02-26T14:16:55Z

shared/fileutil/fileutil.go

+		}
+		// checks if its a file  and has the exact name as the filename
+		// need to break the walk function by using a non-fatal error
+		if !info.IsDir() && filename == info.Name() {


is this the best way to check if its a file?

Yep this is a fine approach

rauljordan · 2021-02-26T15:44:47Z

prysm.sh validator slashing-protection export --datadir=/home/ahaddad/eth2validators --slashing-protection-export-dir=/home/ahaddad

bazel run //validator -- slashing-protection export --datadir=/home/ahaddad/eth2validators --slashing-protection-export-dir=/home/ahaddad

rauljordan

Another great contribution, thanks so much @ahadda5

ahadda5 · 2021-02-26T18:13:11Z

oops @rauljordan i didn't want you to merge it before i finished the test.

Now as expected one has to specify the full path (./ or C:) to make sure it is not read relative otherwise it errors. So
bazel run //validator -- slashing-protection export --datadir=/home/.eth2validators --slashing-protection-export-dir=/home/ahaddad
Versus
bazel run //validator -- slashing-protection export --datadir=./home/.eth2validators --slashing-protection-export-dir=./home/ahaddad

with the first resulting in "error finding validator database at path" which is ok albeit added "feature"/warning.

Secondly and more importantly, my validator.db is a juicy testnet db of 20 MB yet exporting it does not result in the slashing.json. Hmm..

I'm just afraid this was not tested thoroughly.
Cheers

rauljordan · 2021-02-27T17:08:05Z

Sorry for merging so quickly @ahadda5, we should open these concerns in your message above as an issue that should be resolved

ahadda5 added 6 commits February 25, 2021 15:34

issue/feature 8500 find validator.db automatically

9304177

gazelle build

1ec1463

remove less! why there?

1f9f63d

fixed errors import

14562b6

fixed errors import

8f37f6a

unit tested

9e682a0

ahadda5 requested a review from a team as a code owner February 25, 2021 16:26

ahadda5 requested review from shayzluf, 0xKiwi and prestonvanloon February 25, 2021 16:26

Merge branch 'develop' into findValidatorAutomatically

c9f91bd

rauljordan reviewed Feb 25, 2021

View reviewed changes

Merge branch 'develop' into findValidatorAutomatically

65be74b

rauljordan added 2 commits February 25, 2021 14:06

Merge branch 'develop' into findValidatorAutomatically

ee6c238

Merge branch 'develop' into findValidatorAutomatically

e985953

ahadda5 added 2 commits February 26, 2021 17:25

adding the find validator.db fileutil func to export and import slashing

7e796b6

deleted the comment

c98c982

ahadda5 commented Feb 26, 2021

View reviewed changes

rauljordan changed the title ~~Find validator automatically~~ Recursively Find Validator Database File In Slashing Protection Commands Feb 26, 2021

Merge branch 'develop' into findValidatorAutomatically

e9c3a41

rauljordan approved these changes Feb 26, 2021

View reviewed changes

rauljordan merged commit 29d1959 into prysmaticlabs:develop Feb 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursively Find Validator Database File In Slashing Protection Commands #8518

Recursively Find Validator Database File In Slashing Protection Commands #8518

ahadda5 commented Feb 25, 2021 •

edited by rauljordan

Loading

ahadda5 commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading

rauljordan left a comment

rauljordan Feb 25, 2021

ahadda5 Feb 26, 2021

rauljordan commented Feb 25, 2021

ahadda5 commented Feb 26, 2021

ahadda5 commented Feb 26, 2021

ahadda5 Feb 26, 2021

rauljordan Feb 26, 2021

rauljordan commented Feb 26, 2021

rauljordan left a comment

ahadda5 commented Feb 26, 2021

rauljordan commented Feb 27, 2021

Recursively Find Validator Database File In Slashing Protection Commands #8518

Recursively Find Validator Database File In Slashing Protection Commands #8518

Conversation

ahadda5 commented Feb 25, 2021 • edited by rauljordan Loading

ahadda5 commented Feb 25, 2021 • edited Loading

codecov bot commented Feb 25, 2021 • edited Loading

Codecov Report

rauljordan left a comment

Choose a reason for hiding this comment

rauljordan Feb 25, 2021

Choose a reason for hiding this comment

ahadda5 Feb 26, 2021

Choose a reason for hiding this comment

rauljordan commented Feb 25, 2021

ahadda5 commented Feb 26, 2021

ahadda5 commented Feb 26, 2021

ahadda5 Feb 26, 2021

Choose a reason for hiding this comment

rauljordan Feb 26, 2021

Choose a reason for hiding this comment

rauljordan commented Feb 26, 2021

rauljordan left a comment

Choose a reason for hiding this comment

ahadda5 commented Feb 26, 2021

rauljordan commented Feb 27, 2021

ahadda5 commented Feb 25, 2021 •

edited by rauljordan

Loading

ahadda5 commented Feb 25, 2021 •

edited

Loading

codecov bot commented Feb 25, 2021 •

edited

Loading