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

feat: Move shard-assignment for chunk producers to nightly #11203

Merged
merged 15 commits into from
May 9, 2024

Conversation

tayfunelmas
Copy link
Contributor

@tayfunelmas tayfunelmas commented May 1, 2024

Per #11190, move the shard-assignment shuffling feature for chunk producers to nightly.

Also enable this feature for mocknet together with stateless validation to exercise the state sync code path more often. Note that the guarding for the feature in nightly/prod is different from mocknet, as we use mocknet to exercise (stateless validation) features in between prod and nightly.

After this change, shard shuffling will be activated in the following cases:

  1. In "nightly", it will be active by default, since we move the feature among the set of nightly features and update the protocol version accordingly (143).
  2. In "mocknet", it will be active by default, as we enable it directly when chain id == "mocknet".
  3. In all other networks (including "mainnet", "testnet", and "statelessnet"), it will be disabled by default and will only be enabled (later) explicitly by moving the feature from nightly to stable protocol version.

Compiling the binary with or without "statelessnet_protocol" will not have any effect on the enablement of the feature (instead need to compile with "nightly").

@tayfunelmas tayfunelmas marked this pull request as ready for review May 1, 2024 21:18
@tayfunelmas tayfunelmas requested a review from a team as a code owner May 1, 2024 21:18
@tayfunelmas tayfunelmas requested review from wacban and robin-near May 1, 2024 21:18
Copy link

codecov bot commented May 1, 2024

Codecov Report

Attention: Patch coverage is 96.96970% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 70.97%. Comparing base (92ee5d2) to head (b43485d).

Files Patch % Lines
core/primitives/src/epoch_manager.rs 96.87% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #11203   +/-   ##
=======================================
  Coverage   70.96%   70.97%           
=======================================
  Files         781      781           
  Lines      155420   155434   +14     
  Branches   155420   155434   +14     
=======================================
+ Hits       110301   110322   +21     
+ Misses      40349    40340    -9     
- Partials     4770     4772    +2     
Flag Coverage Δ
backward-compatibility 0.24% <0.00%> (-0.01%) ⬇️
db-migration 0.24% <0.00%> (-0.01%) ⬇️
genesis-check 1.40% <0.00%> (-0.01%) ⬇️
integration-tests 37.14% <90.90%> (+<0.01%) ⬆️
linux 68.88% <45.45%> (+<0.01%) ⬆️
linux-nightly 70.44% <96.96%> (+0.02%) ⬆️
macos 52.50% <45.45%> (+0.04%) ⬆️
pytests 1.63% <0.00%> (-0.01%) ⬇️
sanity-checks 1.42% <0.00%> (-0.01%) ⬇️
unittests 65.42% <51.51%> (+<0.01%) ⬆️
upgradability 0.29% <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.

Copy link
Contributor

@wacban wacban left a comment

Choose a reason for hiding this comment

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

I think the plan should be somewhat different.

  • We want to keep it enabled in forknet for as long as possible maybe with a small exception to run it once without it at the end. I would leave it as is in the StatelessNet collection of protocol features.
  • We want to have it enabled in mocknet / forknet pre-release testing. In order to do that we can enable it based on the chain id but without checking if the feature is enabled.
  • Taking a step back we actually not only want shard shuffling but also single shard tracking enabled in pre-release testing. That's more of a configuration change and I lack expertise to say how best to go about it.

cc some node team experts to share their opinions
@marcelo-gonzalez @posvyatokum @VanBarbascu

core/primitives-core/src/version.rs Show resolved Hide resolved
core/primitives-core/src/version.rs Outdated Show resolved Hide resolved
core/primitives/src/epoch_manager.rs Outdated Show resolved Hide resolved
Comment on lines 166 to 167
// (see config_validator_selection function). For pre-release environment such as mocknet,
// we enable it by default with stateless validation to exercise the codepaths for state sync more often.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not how it'll work currently. Right now you check both chain id and the if the feature is enabled. This means it will not be enabled in forknet and mocknet by default because neither has nightly protocol version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about enabling it with stateless validation only, not always (even in existing stateful validation). This is why I check the stateless validation feature explicitly. But we can also do that independently because without single-shard tracking (which comes with stateless validation) it should not have any effect. what do you think?

@tayfunelmas
Copy link
Contributor Author

I think the plan should be somewhat different.

  • We want to keep it enabled in forknet for as long as possible maybe with a small exception to run it once without it at the end. I would leave it as is in the StatelessNet collection of protocol features.

The reason I moved it our of the statenessnet collection is I assumed that when launching stateless validation, we would bump the protocol version above 86 or so. Thus to avoid production getting shard shuffling at that point.

  • We want to have it enabled in mocknet / forknet pre-release testing. In order to do that we can enable it based on the chain id but without checking if the feature is enabled.

The change does this currently, but it still checks stateless validation feature. I think that can be removed (as it will only take effect with single-shard tracking).

  • Taking a step back we actually not only want shard shuffling but also single shard tracking enabled in pre-release testing. That's more of a configuration change and I lack expertise to say how best to go about it.

Is this also for pre-release testing of stateless validation as well as any previous release before stateless? Single shard tracking is only for stateless right? I am assuming that forknet/mocknet testing for stateless should be somewhere above 86 and the mainnet release before stateless will remain somewhere above 66.

cc some node team experts to share their opinions @marcelo-gonzalez @posvyatokum @VanBarbascu

@tayfunelmas tayfunelmas requested a review from VanBarbascu May 2, 2024 17:44
@robin-near
Copy link
Contributor

Why does this need to be only active for mocknet?

Also, we've been compiling our nodes with the flag statelessnet_protocol. Are we going to stop using that and instead use "nightly"? But nightly includes more features than statelessnet_protocol. So I'm a bit confused here about the exact strategy.

@tayfunelmas
Copy link
Contributor Author

tayfunelmas commented May 2, 2024

Why does this need to be only active for mocknet?

Also, we've been compiling our nodes with the flag statelessnet_protocol. Are we going to stop using that and instead use "nightly"? But nightly includes more features than statelessnet_protocol. So I'm a bit confused here about the exact strategy.

Updated the description to clarify, adding it here as well. You are right that compiling the binary with statelessnet_protocol will not enable shard shuffling and I am actually not sure we should do that. Instead we can add a feature to enable it explicitly. In this setup, only "nightly" and "mocknet" will have shard-shuffling enabled by default.


After this change, shard shuffling will be activated in the following cases:

  1. In "nightly", it will be active by default, since we move the feature among the set of nightly features and update the protocol version accordingly (143).
  2. In "mocknet", it will be active by default, as we enable it directly when chain id == "mocknet".
  3. In all other networks (including "mainnet", "testnet", and "statelessnet"), it will be disabled by default and will only be enabled (later) explicitly by moving the feature from nightly to stable protocol version.

Compiling the binary with or without "statelessnet_protocol" will not have any effect on the enablement of the feature (instead need to compile with "nightly").

core/primitives/src/epoch_manager.rs Show resolved Hide resolved
@robin-near
Copy link
Contributor

In "mocknet", it will be active by default, as we enable it directly when chain id == "mocknet".

Ah, sorry I missed that this is unconditionally applied if the network is mocknet.

@tayfunelmas tayfunelmas enabled auto-merge May 3, 2024 23:40
@tayfunelmas
Copy link
Contributor Author

Note: I had to move the call point of config_validator_selection below the check for use_production_config because it was failing some tests and a better fix (eg. overriding the test epoch config) also had other complications (test overrides are only effective when use_production_config is true and when I set it to true it affects other configs and so on.

Instead I will let this PR go this way and in a separate PR I will try to remove use_production_config option and see what kinds of tests it breaks.

@tayfunelmas tayfunelmas added this pull request to the merge queue May 9, 2024
Merged via the queue into near:master with commit d4eb66f May 9, 2024
28 of 29 checks passed
@tayfunelmas tayfunelmas deleted the shard-shuffling branch May 9, 2024 19:38
jancionear pushed a commit to jancionear/nearcore that referenced this pull request May 13, 2024
<p>This PR was automatically created by Snyk using the credentials of a
real user.</p><br /><h3>Snyk has created this PR to upgrade react-router
from 6.21.1 to 6.21.3.</h3>

:information_source: Keep your dependencies up-to-date. This makes it
easier to fix existing vulnerabilities and to more quickly identify and
fix newly disclosed vulnerabilities when they affect your project.
<hr/>

- The recommended version is **4 versions** ahead of your current
version.
- The recommended version was released **a month ago**, on 2024-01-18.


<details>
<summary><b>Release notes</b></summary>
<br/>
  <details>
    <summary>Package name: <b>react-router</b></summary>
    <ul>
      <li>
<b>6.21.3</b> - <a
href="https://snyk.io/redirect/github/remix-run/react-router/releases/tag/react-router-native%406.21.3">2024-01-18</a></br><p>[email protected]</p>
      </li>
      <li>
<b>6.21.3-pre.0</b> - <a
href="https://snyk.io/redirect/github/remix-run/react-router/releases/tag/react-router-native%406.21.3-pre.0">2024-01-16</a></br><p>[email protected]</p>
      </li>
      <li>
        <b>6.21.2</b> - 2024-01-11
      </li>
      <li>
        <b>6.21.2-pre.0</b> - 2024-01-09
      </li>
      <li>
        <b>6.21.1</b> - 2023-12-21
      </li>
    </ul>
from <a
href="https://snyk.io/redirect/github/remix-run/react-router/releases">react-router
GitHub release notes</a>
  </details>
</details>


<details>
  <summary><b>Commit messages</b></summary>
  </br>
  <details>
    <summary>Package name: <b>react-router</b></summary>
    <ul>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/44b391ad27dacc07bb1298bd79315c00845d5a2c">44b391a</a>
chore: Update version for release (near#11203)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/82c48435350d239b376e435e8693ea51296c5848">82c4843</a>
Exit prerelease mode</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/1551285c2e821994d01b0a86cd2839c63898dabc">1551285</a>
Change release date</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/8fc701a910788d69c358565c974665384ca462fb">8fc701a</a>
Prep release notes</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/b4bf9271bc3e2a5a1fec73485392563cf3c8a7e2">b4bf927</a>
chore: Update version for release (pre) (near#11196)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/84e5fb9f6df09e8324e43541d2ab660c383a025e">84e5fb9</a>
Enter prerelease mode</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/858f6f125d4dad94483f87fdfb8cc23cbbe68d59">858f6f1</a>
Merge branch &#x27;main&#x27; into release-next</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/b541b8dd5a582b1e95a2fd0111eeac2ab92f738b">b541b8d</a>
Fix NavLink isPending with a basename (near#11195)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/552662ab867fd09e865adb33bb6631207a16ca53">552662a</a>
Remove leftover &#x60;unstable_&#x60; prefix from
&#x60;Blocker&#x60;/&#x60;BlockerFunction&#x60; types (near#11187)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/04d653bcdb5b641485e4b9081fe196cfeac12716">04d653b</a>
Copy remix script to remove prereleases from changelogs (near#11183)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/6837a1111a8ae1c60ab665b9ff2d9eb20b2f2f40">6837a11</a>
Fix date in changelog</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/c418410cb8b7277655d51e8ad2756059ad19fbeb">c418410</a>
Merge branch &#x27;release-next&#x27; into dev</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/274027eba64e614442106678c5a0676834202491">274027e</a>
Fix date in changelog</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/6be8e6c57d347c2dd975fa76b5aef6d39fd71d0e">6be8e6c</a>
Merge branch &#x27;release-next&#x27;</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/4e528c0bcee338c8f26d948affb3092257f6ee85">4e528c0</a>
chore: Update version for release (near#11182)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/c8742d464d539bfdad452d59b7d5ce55b3628980">c8742d4</a>
Exit prerelease mode</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/f9780210d84257c21f628c31de165e91f3474263">f978021</a>
Prep release notes</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/cfce7e61655b46f7894fa8cadd336a66d58d1a00">cfce7e6</a>
Unbold date</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/038ad94452d81d882f1042395b28b0d80c3b4f40">038ad94</a>
Add dates to prior releases in changelog</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/c2c0826c6e4a9e213b4f43bc4e95d4f85bf2d96e">c2c0826</a>
chore: Update version for release (pre) (near#11178)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/5cd3a45d625d947c795bd85aab8026a5e6b29614">5cd3a45</a>
Enter prerelease mode</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/03ce0377f5bed5fca3eb94d875c33eab9414b116">03ce037</a>
Merge branch &#x27;main&#x27; into release-next</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/0ef18963d274ea78592479cab9a48be2c5ec3c6a">0ef1896</a>
Do not attempt to deserialize empty json responses (near#11164)</li>
<li><a
href="https://snyk.io/redirect/github/remix-run/react-router/commit/e0d106bbe765ef12d3ad31e223127e2d08cab8e1">e0d106b</a>
Leverage useId for internal fetcher keys when available (near#11166)</li>
    </ul>

<a
href="https://snyk.io/redirect/github/remix-run/react-router/compare/08cda17f450ebe19481be3fc080d243ec5ef509f...44b391ad27dacc07bb1298bd79315c00845d5a2c">Compare</a>
  </details>
</details>
<hr/>

**Note:** *You are seeing this because you or someone else with access
to this repository has authorized Snyk to open upgrade PRs.*

For more information: <img
src="https://api.segment.io/v1/pixel/track?data=eyJ3cml0ZUtleSI6InJyWmxZcEdHY2RyTHZsb0lYd0dUcVg4WkFRTnNCOUEwIiwiYW5vbnltb3VzSWQiOiI1YjE0ODcwNy0yNzZhLTQ2M2EtOGU0Mi01YTQ2ZGRlOWViNWEiLCJldmVudCI6IlBSIHZpZXdlZCIsInByb3BlcnRpZXMiOnsicHJJZCI6IjViMTQ4NzA3LTI3NmEtNDYzYS04ZTQyLTVhNDZkZGU5ZWI1YSJ9fQ=="
width="0" height="0"/>

🧐 [View latest project
report](https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🛠 [Adjust upgrade PR
settings](https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763/settings/integration?utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr)

🔕 [Ignore this dependency or unsubscribe from future upgrade
PRs](https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763/settings/integration?pkg&#x3D;react-router&amp;utm_source&#x3D;github&amp;utm_medium&#x3D;referral&amp;page&#x3D;upgrade-pr#auto-dep-upgrades)

<!---
(snyk:metadata:{"prId":"5b148707-276a-463a-8e42-5a46dde9eb5a","prPublicId":"5b148707-276a-463a-8e42-5a46dde9eb5a","dependencies":[{"name":"react-router","from":"6.21.1","to":"6.21.3"}],"packageManager":"npm","type":"auto","projectUrl":"https://app.snyk.io/org/ecp88/project/98480bdc-d80b-4fd1-89d7-c4c56a706763?utm_source=github&utm_medium=referral&page=upgrade-pr","projectPublicId":"98480bdc-d80b-4fd1-89d7-c4c56a706763","env":"prod","prType":"upgrade","vulns":[],"issuesToFix":[],"upgrade":[],"upgradeInfo":{"versionsDiff":4,"publishedDate":"2024-01-18T19:49:25.374Z"},"templateVariants":[],"hasFixes":false,"isMajorUpgrade":false,"isBreakingChange":false,"priorityScoreList":[]})
--->

Co-authored-by: snyk-bot <[email protected]>
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