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

test_utils::upgrade_protocol bumps to the latest protocol version instead of the provided one #8590

Open
Ekleog-NEAR opened this issue Feb 17, 2023 · 5 comments
Assignees
Labels
C-housekeeping Category: Refactoring, cleanups, code quality T-core Team: issues relevant to the core team

Comments

@Ekleog-NEAR
Copy link
Collaborator

The code at Ekleog@33bf680 fails with log lines:

Testing protocol upgrade between 47 and 48
Producing block with version 48
thread 'tests::client::features::wasmer2::test_wasmer2_upgrade' panicked at 'Expected to find 'vm_kind=Wasmer2' in logs, occurences of vm_kind are ["run code.len=99650 method_name=log_something vm_kind=NearVm current_protocol_version=58"]', integration-tests/src/tests/client/features/wasmer2.rs:84:5

Note in particular the current_protocol_version=58 here.

Analysis suggested by @jakmeier is as follows:

My theory why it doesn't work: Could it be that when we set the old version in genesis config, then block producers still set the latest protocol version to the latest version they support? That's at least what I would expect to happen. And then they already schedule a protocol upgrade right up to the latest version before the call to upgrade_protocol happens. The loop that fast-forwards through 2 epochs inside upgrade_protocol will then make it happen, regardless of what that one block that has latest version 48.

@Longarithm
Copy link
Member

We may need to disable voting for latest protocol version in tests, because we do want to check only +1 protocol version switches. With flat storage features, TTN costs of same txns will change, which may break some existing tests using upgrade_protocol which is the right thing to use.

@Ekleog-NEAR
Copy link
Collaborator Author

Alternatively, if we go forward with limited-to-three replayability for the outside world, we can probably just remove all the tests that pertain to versions older than the latest we officially support, and that’ll both simplify our code and help us work around this issue :) (though it’d still be nice to actually fix it someday)

@walnut-the-cat walnut-the-cat added C-housekeeping Category: Refactoring, cleanups, code quality T-core Team: issues relevant to the core team labels Apr 3, 2023
near-bulldozer bot pushed a commit that referenced this issue May 30, 2023
This replaces the wasmer2 test instead of adding a new one because of #8590.
nikurt pushed a commit that referenced this issue May 31, 2023
This replaces the wasmer2 test instead of adding a new one because of #8590.
nikurt pushed a commit to nikurt/nearcore that referenced this issue Jun 8, 2023
This replaces the wasmer2 test instead of adding a new one because of near#8590.
nikurt pushed a commit to nikurt/nearcore that referenced this issue Jun 8, 2023
This replaces the wasmer2 test instead of adding a new one because of near#8590.
nikurt pushed a commit that referenced this issue Jun 13, 2023
This replaces the wasmer2 test instead of adding a new one because of #8590.
@pugachAG pugachAG self-assigned this Feb 1, 2024
@staffik staffik assigned staffik and unassigned staffik Feb 2, 2024
@Longarithm
Copy link
Member

Got hit by this while adding CI for chain with custom protocol version #10589

@akhi3030
Copy link
Collaborator

akhi3030 commented Apr 8, 2024

Looks like we are hitting this issue again in #10979. If we are not planning on fixing this issue any time soon, I propose that we change

pub fn upgrade_protocol(&mut self, protocol_version: ProtocolVersion) {
to stop taking a protocol_version as an argument. We will then be forced to rewrite our tests to accept this limitation.

github-merge-queue bot pushed a commit that referenced this issue Apr 12, 2024
Renames `pub fn upgrade_protocol(&mut self, protocol_version:
ProtocolVersion)` to `pub fn upgrade_protocol_to_latest_version(&mut
self)`. The function is unable to upgrade to a specific version but
instead always upgrades to the latest version.

Also see #8590.
@wacban
Copy link
Contributor

wacban commented Jul 2, 2024

JFYI I'm going to disable the test_function_limit_change test when stabilizing the cc and sv features.

If the issue is uncontrolled protocol upgrades this can now be done more precisely with NEAR_TESTS_PROTOCOL_UPGRADE_OVERRIDE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-housekeeping Category: Refactoring, cleanups, code quality T-core Team: issues relevant to the core team
Projects
None yet
Development

No branches or pull requests

7 participants