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(staking): Unstake and restake node. Wait for it to sync #12247

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

VanBarbascu
Copy link
Contributor

@VanBarbascu VanBarbascu commented Oct 17, 2024

Run each test with the following command replacing the test case:

rm -rf ~/.near/test* ; python3 -m unittest tests.sanity.staking_single_shard.StateSyncValidatorShardSwap.test_state_sync_with_restaking_decentralized_only

@VanBarbascu VanBarbascu requested a review from a team as a code owner October 17, 2024 17:24
Copy link

codecov bot commented Oct 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.66%. Comparing base (a743a85) to head (bdeff4d).

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #12247   +/-   ##
=======================================
  Coverage   71.65%   71.66%           
=======================================
  Files         838      838           
  Lines      167441   167441           
  Branches   167441   167441           
=======================================
+ Hits       119986   119989    +3     
+ Misses      42222    42216    -6     
- Partials     5233     5236    +3     
Flag Coverage Δ
backward-compatibility 0.16% <ø> (ø)
db-migration 0.16% <ø> (ø)
genesis-check 1.25% <ø> (ø)
integration-tests 38.90% <ø> (-0.01%) ⬇️
linux 71.31% <ø> (+<0.01%) ⬆️
linux-nightly 71.24% <ø> (+0.01%) ⬆️
macos 54.28% <ø> (+0.26%) ⬆️
pytests 1.56% <ø> (ø)
sanity-checks 1.37% <ø> (ø)
unittests 65.38% <ø> (+<0.01%) ⬆️
upgradability 0.21% <ø> (ø)

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.

Copy link
Collaborator

@akhi3030 akhi3030 left a comment

Choose a reason for hiding this comment

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

@VanBarbascu: I am not sure who would be a good reviewer for this PR. As it is adding a new test, I am approving it. But if you need a more detailed review, please suggest someone.

@VanBarbascu
Copy link
Contributor Author

@VanBarbascu: I am not sure who would be a good reviewer for this PR. As it is adding a new test, I am approving it. But if you need a more detailed review, please suggest someone.

Thanks @akhi3030! This is part of the testnet release investigation. @marcelo-gonzalez , @saketh-are and @andrei-near are the target audience.

centralized_state_sync=False,
decentralized_state_sync=False):
(node_config_dump,
node_config_sync) = state_sync_lib.get_state_sync_configs_pair(
Copy link
Collaborator

Choose a reason for hiding this comment

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

All of these tests are using the same config, one in which it is specified that an external source is available in state_part_dir. In 2.3.0-rc.4 we made it so that if an external source is configured, it will be used directly and no requests will be made to peers. Then in test_state_sync_with_restaking_decentralized_only you have the same config but you don't dump state parts in the state_part_dir. Hence the node just gets stuck looking for state parts in the external directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for clarifying this! I assumed that we fallback from external to peers, but instead we do just external.

In this case, how are we going to test the DSS? DSS still relies on the external storage for state parts, but if you set the external configs, will only do cloud sync.


self.testcase.wait_for_blocks(3)

self.testcase.create_accounts()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This part seems to be flaky, sometimes the test times out waiting for the result of the transactions which create the accounts.

github-merge-queue bot pushed a commit that referenced this pull request Oct 21, 2024
If the expected sender is not specified, we should accept a response
from any peer. We don't specify an expected sender for state parts, so
this bug meant that state sync was ignoring all parts received from
peers.

#12247 adds tests which cover this code and detect this problem.
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