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.

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

Improve the memtrie integration test to include a state sync base case. #10592

Merged
merged 7 commits into from
Feb 15, 2024

Conversation

robin-near
Copy link
Contributor

It turns out that even setting up a e2e test case for single-shard tracking is tricky, so this PR does that. It makes a few fixes to the test framework as well as the memtrie test code, so that

  • In the track-all-shards + single-shard-tracking case (i.e. single shard tracking is enabled but nodes still track all shards), allow test code a chance to request partial encoded chunks.
  • Query account balance from a client that tracks the shard (otherwise we would get trie node missing errors).
  • Send txns to every client instead of client 0, because not everyone produces chunks for every shard now.
  • Check that every block contains all new chunks, to self-verify that the partial encoded chunk propagation is correct. Also check that the total number of transactions included is correct in the end.

@robin-near robin-near requested a review from a team as a code owner February 9, 2024 02:24
_ => panic!("Wrong return value"),

for i in 0..self.clients.len() {
let tracks_shard = client
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be self.clients[i] not client

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 doesn't actually matter, because I'm querying using epoch_manager which would give the same result on every client. If I had used the ShardTracker then it would take local tracking config into account.

@@ -251,6 +280,20 @@ fn get_block_producer(env: &TestEnv, head: &Tip, height_offset: u64) -> AccountI
block_producer
}

fn check_block_does_not_have_missing_chunks(env: &TestEnv, block: &Block) {
if block.header().prev_hash() != env.clients[0].chain.genesis_block().hash() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we use chunk.height_included and block.height here? Why do we need to fetch the prev_block?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also nit: can pass just client into function instead of TestEnv and can incorporate early return for block after genesis.

Copy link
Member

Choose a reason for hiding this comment

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

+1, we recently introduced ShardChunkHeader::is_new_chunk for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha I was still not confident in my ability to correctly use the heights to compare, that was why :P

}

trait TestEnvBuilderExt {
fn maybe_track_all_shards(self, track_all_shards: bool) -> Self;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this only for the in_memory_tries test? Eventually single shard tracking related tests should hopefully be more common?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yeah, I guess I should move that in there.

@akhi3030 akhi3030 removed their request for review February 9, 2024 11:50
Copy link

codecov bot commented Feb 13, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (8512574) 72.19% compared to head (23a4422) 72.16%.

Files Patch % Lines
chain/client/src/test_utils/test_env.rs 86.66% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10592      +/-   ##
==========================================
- Coverage   72.19%   72.16%   -0.03%     
==========================================
  Files         726      726              
  Lines      147700   147719      +19     
  Branches   147700   147719      +19     
==========================================
- Hits       106630   106608      -22     
- Misses      36273    36301      +28     
- Partials     4797     4810      +13     
Flag Coverage Δ
backward-compatibility 0.08% <0.00%> (-0.01%) ⬇️
db-migration 0.08% <0.00%> (-0.01%) ⬇️
genesis-check 1.24% <0.00%> (-0.01%) ⬇️
integration-tests 37.05% <88.57%> (-0.02%) ⬇️
linux 71.18% <88.57%> (-0.01%) ⬇️
linux-nightly 71.61% <88.57%> (-0.03%) ⬇️
macos 55.10% <71.42%> (-0.01%) ⬇️
pytests 1.46% <0.00%> (-0.01%) ⬇️
sanity-checks 1.26% <0.00%> (-0.01%) ⬇️
unittests 68.08% <71.42%> (-0.02%) ⬇️
upgradability 0.13% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@robin-near robin-near added this pull request to the merge queue Feb 15, 2024
Merged via the queue into near:master with commit 69c3923 Feb 15, 2024
28 of 29 checks passed
@robin-near robin-near deleted the snet10 branch February 15, 2024 02:27
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.

3 participants