Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for slashing interchange format tests #8185

Merged
merged 12 commits into from
Apr 15, 2024
Merged

Add support for slashing interchange format tests #8185

merged 12 commits into from
Apr 15, 2024

Conversation

StefanBratanov
Copy link
Contributor

@StefanBratanov StefanBratanov commented Apr 10, 2024 •
edited
Loading

PR Description

image

As per https://github.com/eth-clients/slashing-protection-interchange-tests

Things to note:

Fixed Issue(s)

fixes #7784

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@StefanBratanov StefanBratanov marked this pull request as draft April 10, 2024 16:25
@StefanBratanov StefanBratanov marked this pull request as ready for review April 11, 2024 11:30
@StefanBratanov StefanBratanov force-pushed the add_support_slashing_format_tests branch from 8102518 to 2c47da1 Compare April 11, 2024 11:30
@mehdi-aouadi
Copy link
Contributor

mehdi-aouadi commented Apr 11, 2024 •
edited
Loading

Regarding the should_succeed_complete: It looks like in Teku we've implemented a minimal strategy (we only keep track of the latest signed blocks and attestations) so we should only check the should_succeed and that's OK (the complete strategy being a complete database containing every message signed by each validator, which is not our case: definition from the EIP-3076)

@mehdi-aouadi
Copy link
Contributor

mehdi-aouadi commented Apr 11, 2024 •
edited
Loading

Not comfortable ignoring the containsSlashableData in the other hand

@StefanBratanov
Copy link
Contributor Author

Not confortable ignoring the containsSlashableData on the other hand

Yeah, I wasn't sure what needs to be done on that part.

@StefanBratanov StefanBratanov force-pushed the add_support_slashing_format_tests branch from 620a4ae to 6a2a7b6 Compare April 12, 2024 08:48
@zilm13
Copy link
Contributor

zilm13 commented Apr 14, 2024 •
edited
Loading

From description:

If contains_slashable_data is true, then implementations have the option to do one of two things:

  • Import the interchange successfully, working around the slashable data by minification or some other mechanism. If the import succeeds, all checks must pass and the test should continue to the next step.
  • Reject the interchange (or partially import it), in which case the block/attestation checks and all future steps should be ignored.

We do the 1st option so I think nothing could be added as far as we match test result

zilm13
zilm13 reviewed Apr 14, 2024
View reviewed changes
eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java
@@ -47,7 +47,10 @@ public static Stream<TestDefinition> findReferenceTests() throws IOException {
private static Stream<TestDefinition> findTestTypes(final Path specDirectory) throws IOException {
final String spec = specDirectory.getFileName().toString();
if (spec.equals("bls")) {
return new BlsRefTestFinder().findTests(TestFork.PHASE0, spec, specDirectory);
return new BlsRefTestFinder().findTests("", spec, specDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this findTests doesn't use fork at all, maybe remove parameter?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is an interface method though, not sure how best to refactor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh sorry, haven't checked this
ok, it's a tests

eth-tests/src/main/java/tech/pegasys/teku/ethtests/finder/ReferenceTestFinder.java
return new BlsRefTestFinder().findTests("", spec, specDirectory);
}
if (spec.equals("slashing-protection-interchange")) {
return new SlashingProtectionInterchangeRefTestFinder().findTests("", spec, specDirectory);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this findTestsuses fork in creating TestDefinition, but inits it with bad fork actually. if createSpec() is called, it will throw, confusing too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how I can change this without a bigger refactor. If we wrongly use Spec when we don't need to, we will fail anyways.

...erence/phase0/slashing_protection_interchange/SlashingProtectionInterchangeTestExecutor.java
TestDataUtils.loadJson(testDefinition, testDefinition.getTestName(), TestData.class);

// our implementation fails when importing one of the keys in an interchange, which is already
// in our slashprotection directory with a different genesis validators root. However, the test
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the name of the test which covers this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually there is no test for this, will add it.

@StefanBratanov StefanBratanov force-pushed the add_support_slashing_format_tests branch from da5389e to 7882df3 Compare April 15, 2024 10:12
zilm13
zilm13 approved these changes Apr 15, 2024
View reviewed changes
Copy link
Contributor

@zilm13 zilm13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@StefanBratanov StefanBratanov enabled auto-merge (squash) April 15, 2024 10:35
@StefanBratanov StefanBratanov merged commit 6a84100 into Consensys:master Apr 15, 2024
15 of 16 checks passed
@StefanBratanov StefanBratanov deleted the add_support_slashing_format_tests branch April 15, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zilm13 zilm13 zilm13 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement slashing interchange format tests for teku
3 participants
@StefanBratanov @mehdi-aouadi @zilm13