near / nearcore

Reference client for NEAR Protocol
https://near.org
GNU General Public License v3.0
2.31k stars 615 forks source link

feat: Update neard localnet command to support archival and rpc nodes in post-stateless-validation #11797

Closed tayfunelmas closed 1 month ago

tayfunelmas commented 2 months ago

(This change is to test archival nodes in a localnet for any archival node improvements.)

The neard localnet command does not have flexibility to configure archival and RPC nodes separately from the other validator and non-validator nodes. For example today --archival-nodes is a boolean flag and makes all nodes archival nodes. Similarly --tracked-shards specify the shards tracked by all the nodes. Thus, there is no way to have some validator nodes tracking no shards (single-shard tracking) and separate archival/RPC nodes tracking all shards. Especially after stateless validation gets effective, validators will start tracking single/some shards, while we can archival and RPC nodes to track all shards.

To support this for localnet setup, we add flags to the neard localnet command to create non-validator nodes configured as archival and RPC nodes. These nodes track all shards even if the validators run with single-shard tracking enabled. This allows to run a neard localnet command, for example, to have 4 validator nodes to track single-shard, 1 archival node to track all shards, and 1 RPC node to track all shards as follows:

neard localnet \
  --validators 4 \               # node0..node3 will be validator nodes.
  --non-validators-archival 1 \  # node4 will be archival non-validator node.
  --non-validators-rpc 1 \       # node5 will be RPC non-validator node.
  --non-validators 2 \           # node6..node7 will non-validators that can be custom-configured .
  --shards 4 \
  --tracked-shards "none"

Other changes:

wacban commented 2 months ago

Pre-review - this has the potential to break a few nayduck tests since the localnet subcommand API is changing. Can you check and fix any breakages there?

wacban commented 2 months ago

Do you need the flexibility to configure each node separately as archival or rpc? If not I think the interface would be cleaner with just providing the number of each type of nodes e.g. --validators-num 4 --rpc-num 1 --archival-num 1. This would make the first 4 nodes validators, the next one rpc and the last one archival. WDYT?

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 94.18182% with 16 lines in your changes missing coverage. Please review.

Project coverage is 71.76%. Comparing base (8fee337) to head (491aaf4).

Files Patch % Lines
integration-tests/src/node/mod.rs 0.00% 10 Missing :warning:
neard/src/cli.rs 42.85% 4 Missing :warning:
nearcore/src/config.rs 99.22% 1 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #11797 +/- ## ========================================== + Coverage 71.74% 71.76% +0.01% ========================================== Files 796 796 Lines 162984 163169 +185 Branches 162984 163169 +185 ========================================== + Hits 116937 117101 +164 - Misses 41004 41021 +17 - Partials 5043 5047 +4 ``` | [Flag](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | Coverage Δ | | |---|---|---| | [backward-compatibility](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <0.00%> (-0.01%)` | :arrow_down: | | [db-migration](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.23% <0.00%> (-0.01%)` | :arrow_down: | | [genesis-check](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.35% <0.00%> (-0.01%)` | :arrow_down: | | [integration-tests](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `37.78% <0.00%> (-0.08%)` | :arrow_down: | | [linux](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.40% <94.18%> (+0.03%)` | :arrow_up: | | [linux-nightly](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `71.35% <85.81%> (+0.01%)` | :arrow_up: | | [macos](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `53.36% <89.05%> (-1.21%)` | :arrow_down: | | [pytests](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.62% <63.80%> (+0.03%)` | :arrow_up: | | [sanity-checks](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `1.42% <63.80%> (+0.03%)` | :arrow_up: | | [unittests](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `66.21% <89.05%> (+0.02%)` | :arrow_up: | | [upgradability](https://app.codecov.io/gh/near/nearcore/pull/11797/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near) | `0.28% <0.00%> (-0.01%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=near#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tayfunelmas commented 2 months ago

Pre-review - this has the potential to break a few nayduck tests since the localnet subcommand API is changing. Can you check and fix any breakages there?

This will likely break the existing users of nearup tool (frozen repo), as it uses the flag as a boolean. I in fact changed the tool in my fork.

Regarding nayduck: I see that nayduck tests do not use the --archival-nodes flag but they update the config after init_cluster function to explicitly set archive to true before starting the nodes (example test).

I will run the nayduck anyways, thanks for the notice.

tayfunelmas commented 2 months ago

Do you need the flexibility to configure each node separately as archival or rpc? If not I think the interface would be cleaner with just providing the number of each type of nodes e.g. --validators-num 4 --rpc-num 1 --archival-num 1. This would make the first 4 nodes validators, the next one rpc and the last one archival. WDYT?

Good point. Right, I do not need to specifically point to certain nodes and in most cases the archival and rpc nodes would be separate from the validator nodes.

But (differently from the archival-nodes flag), the flag for specifying the number of non-validator nodes is used extensively in the code/tests, so replacing or complementing it with num-archival/rpc-nodes kind of flags would be a larger change.

wacban commented 1 month ago

Just in case can you schedule a nayduck run on this? ./scripts/nayduck.py should do the trick.