test: congestion control protocol upgrade #11273
Conversation
Add 2 integration tests to check the protocol upgrade works. The first test is without any traffic at all. This already works. The second test is with congestion at the time of the upgrade. We haven't handled congestion initialization, yet, therefore the last check is disabled with a TODO comment.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11273 +/- ##
==========================================
- Coverage 70.99% 70.97% -0.03%
==========================================
Files 780 781 +1
Lines 154984 155423 +439
Branches 154984 155423 +439
==========================================
+ Hits 110037 110307 +270
- Misses 40204 40355 +151
- Partials 4743 4761 +18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // The immediate protocol upgrade needs to be set for this test to pass in | ||
| // the release branch where the protocol upgrade date is set. | ||
| std::env::set_var("NEAR_TESTS_IMMEDIATE_PROTOCOL_UPGRADE", "1"); |
There was a problem hiding this comment.
That is strange, the default should be immediate. This env variable should only be needed on the release branch where the voting date is scheduled in the future. What happens if you remove it? Are you by any chance based on a release branch or do you have the voting date set? You can grep for PROTOCOL_UPGRADE_SCHEDULE to check that.
There was a problem hiding this comment.
Ah, no I just copy-pasted this from similar tests. I thought we should set it already to not break things on the release branch later.
I think I can just remove it and CI is still green.
integration-tests/src/tests/client/features/congestion_control.rs
Outdated
Show resolved
Hide resolved
| env.upgrade_protocol_to_latest_version(); | ||
|
|
||
| // check we are in the new version | ||
| let block = env.clients[0].chain.get_head_block().unwrap(); |
There was a problem hiding this comment.
micro nit - Since you read the block here I was expecting that it'll be used for the check in the next line but actually you do that with env. Can you move this line two lines below?
There was a problem hiding this comment.
true, too much frantic moving around of code :)
integration-tests/src/tests/client/features/congestion_control.rs
Outdated
Show resolved
Hide resolved
integration-tests/src/tests/client/features/congestion_control.rs
Outdated
Show resolved
Hide resolved
| let block = env.clients[0].chain.get_head_block().unwrap(); | ||
| assert!(ProtocolFeature::CongestionControl.enabled(block.header().latest_protocol_version())); |
There was a problem hiding this comment.
funnily enough here you actually use the block :)
There was a problem hiding this comment.
Oh, yeah, good catch. This is actually wrong! We don't care about the latest known protocol version of the block producer, we want to know the epoch protocol version.
| signer, | ||
| deposit, | ||
| // easy way to burn all attached gas | ||
| "loop_forever".to_owned(), |
There was a problem hiding this comment.
We could use a host function "burn_gas" that would burn gas but not waste the time and CPU to do so ;) Absolutely no need to do it in this PR thought.
There was a problem hiding this comment.
yeah, kind of hurts to let the CPU spin round and round ^^
Good idea to add a host function! Something to maybe discuss with the runtime team how to do it best, adding it to the runtime interface with a NEP and protocol upgrade seems a bit overkill, so maybe it could be a test_features-only thing.
- use epoch protocol version rather than block field for checking that the upgrade worked - don't set NEAR_TESTS_IMMEDIATE_PROTOCOL_UPGRADE unnecessarily - typos
Add 2 integration tests to check the protocol upgrade works.
The first test is without any traffic at all. This already works.
The second test is with congestion at the time of the upgrade. We haven't handled congestion initialization, yet, therefore the last check is disabled with a TODO comment.