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

[stateless_validation] test_chunk_forwarding_optimization fails with --features statelessnet_protocol #10600

Closed
jancionear opened this issue Feb 12, 2024 · 2 comments · Fixed by #10601
Assignees
Labels
A-stateless-validation Area: stateless validation

Comments

@jancionear
Copy link
Contributor

jancionear commented Feb 12, 2024

Runing the integration test test_chunk_forwarding_optimization with --features statelessnet_protocol causes the test to fail:

$ cargo nextest run --package integration-tests --features statelessnet_protocol test_chunk_forwarding_optimization
...
test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 367 filtered out; finished in 2.66s


--- STDERR:              integration-tests tests::client::features::access_key_nonce_for_implicit_accounts::test_chunk_forwarding_optimization ---
thread 'tests::client::features::access_key_nonce_for_implicit_accounts::test_chunk_forwarding_optimization' panicked at integration-tests/src/tests/client/features/access_key_nonce_for_implicit_accounts.rs:779:9:
assertion failed: PARTIAL_ENCODED_CHUNK_FORWARD_CACHED_WITHOUT_HEADER.get() > 0.0

It looks like the PARTIAL_ENCODED_CHUNK_FORWARD_CACHED_WITHOUT_HEADER isn't increased as it should.

The test passes with --features nightly and no special features (stable).

The test was modified in #10527 to make it work with stateless validation. It started passing with --features nightly and the fix was merged. Now it turns out that it doesn't work with --features stateless_protocol, so it'll need to be adjusted further.

It should be fixed quickly because it causes CI runs to fail: (e.g https://github.com/near/nearcore/actions/runs/7872120842/job/21476689267)

Refs: #10506, zulip conversation

@jancionear jancionear added the A-stateless-validation Area: stateless validation label Feb 12, 2024
@jancionear jancionear self-assigned this Feb 12, 2024
@jancionear
Copy link
Contributor Author

Ah, the check for increased metric is disabled on nightly beacause "we're so efficient with part forwarding":

   // With very high probability we should've encountered some cases where forwarded parts
   // could not be applied because the chunk header is not available. Assert this did indeed
   // happen.
   // Note: For nightly, which includes SingleShardTracking, this check is disabled because
   // we're so efficient with part forwarding now that we don't seem to be forwarding more
   // than it is necessary.
   if !cfg!(feature = "nightly") {
       assert!(PARTIAL_ENCODED_CHUNK_FORWARD_CACHED_WITHOUT_HEADER.get() > 0.0);
   }

That sounds weird. But I guess we can also disable the check for feature = "statelessnet_protocol" as a quick fix 0_o
/cc @Longarithm

@jancionear
Copy link
Contributor Author

The check was disabled for "nightly" in #10582 by @robin-near, 4 days ago.
Can I assume that the same reasoning can be applied to statelessnet_protocol, which also uses SingleShardTracking?
It sounds reasonable to me, I'll open a PR.

jancionear added a commit to jancionear/nearcore that referenced this issue Feb 12, 2024
…ature

The main assertion in the integration test `test_chunk_forwarding_optimization`
is disabled on `--features nightly`, which is a recent change that was introduced
due to single shard tracking in near#10582.

Let's also disable it on `--features statelessnet_protocol`, the reasoning is similar.

Fixes: near#10600
github-merge-queue bot pushed a commit that referenced this issue Feb 12, 2024
…ature (#10601)

The main assertion in the integration test
`test_chunk_forwarding_optimization` is disabled on `--features
nightly`, which is a recent change that was introduced due to single
shard tracking in #10582.

Let's also disable it on `--features statelessnet_protocol`, the
reasoning is similar.

Fixes: #10600
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stateless-validation Area: stateless validation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant