Skip to content

Test era producer consumer#65

Open
dapplion wants to merge 48 commits intounstablefrom
test-era-producer-consumer
Open

Test era producer consumer#65
dapplion wants to merge 48 commits intounstablefrom
test-era-producer-consumer

Conversation

@dapplion
Copy link
Owner

Issue Addressed

Which issue # does this PR address?

Proposed Changes

Please list or describe the changes introduced by this PR.

Additional Info

Please provide any additional information. For example, future considerations
or information useful for reviewers.

Tests verify:
1. All ERA files are parseable
2. All blocks have correct tree-hash roots
3. ERA boundary states are available at correct slots
4. Import works and block root index is populated
5. All imported blocks are fetchable and have correct roots
6. Final head matches expected value
7. Genesis state integrity (1024 validators, slot 0)

Test vectors: 13 ERA files, 767 blocks, 832 slots, minimal preset
Comment on lines 178 to 182
Copy link
Owner Author

Choose a reason for hiding this comment

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

What's the point of this test? If you load all era files and get to a node that has the same head as the metadata -> Then call read all blocks from DB and check their hash chain you are good.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Redundant, collapse with test mentioned above that loads all era files and checks the blocks hash chains in the DB post import

Copy link
Owner Author

Choose a reason for hiding this comment

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

Just do this checks in the same test mentioned above. In 1 test:

  • import all era files
  • check blocks hash chains
  • check states have the expected roots

Comment on lines 504 to 521
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why so much code? The files should have the same name, just use that to compare byte for byte

Copy link
Owner Author

Choose a reason for hiding this comment

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

Why? Just do basic equality of the bytes

Copy link
Owner Author

Choose a reason for hiding this comment

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

I don't like this tests, they should cover a block having the wrong block root.

err.contains("slot mismatch"),
"expected slot mismatch error: {err}"
);
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

For the corrupt ones you need to test for this errors specifically:

era root mismatch for era {era_number}: expected {expected_root:?}, got {actual_root:?}
era state slot mismatch: expected {end_slot}, got {}
block summary root post-capella mismatch for era
block summary root pre-capella mismatch for era {}: {:?} != {:?}
block root mismatch at slot {slot}: expected {expected_block_root:?}, got {block_root:?}

Plus implement verification that the state_root matches from the initial trusted state root supplied by the user. For that mutate a state by changing a single state.balances[i] value

- 3 new corrupt ERA files: era0-wrong-root, era8-corrupt-block-summary, era2-wrong-block-root
- 3 new test functions: rejects_wrong_era_root, rejects_corrupt_block_summary, rejects_wrong_block_root
- Python generator for reproducible corruption (create_corrupt_files.py)
- All 8 corruption tests passing
era_dir
.import_era_file(&store, era, &spec, None, None)
.unwrap_or_else(|e| panic!("import ERA {era}: {e}"));
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Use the builder code from beacon_node/beacon_chain/src/builder.rs don't re-implement it in the test

Copy link
Owner Author

Choose a reason for hiding this comment

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

WTF is this verification? You need to check that the chain of blocks is correct, read all blocks in DB and check that they form hash chain. Check that the stored states are the expected states, and that the head matches the test metadata

Copy link
Owner Author

Choose a reason for hiding this comment

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

WTF is verifying?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Why?

/// The trusted state root feature catches it.
#[test]
fn era_rejects_mutated_state_with_trusted_root() {
let tmp = era_dir_with_corrupt("era3-wrong-state-root.era", "-00003-");
Copy link
Owner Author

Choose a reason for hiding this comment

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

How did you generate this file?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

Comments