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

Neard doesn't warn about unrecognised options in config.json #10309

Closed
jancionear opened this issue Dec 7, 2023 · 4 comments
Closed

Neard doesn't warn about unrecognised options in config.json #10309

jancionear opened this issue Dec 7, 2023 · 4 comments

Comments

@jancionear
Copy link
Contributor

jancionear commented Dec 7, 2023

Describe the bug

When neard reads config.json, it should validate that all options in the config are correct. It shouldn't ignore options that are unrecognised, because then there's a risk that the operator will make a typo in the config and they will unknowingly run a misconfigured node.

There was another issue about it (#6436), and it looks like the feature has been implemented in #6490 (which should've closed #6436), but it doesn't work.
This issue is about a bug in the implementation of #6436. The logic is there, but it doesn't produce any warnings: https://github.com/near/nearcore/blob/abf6e99bad479a216a7f6e8a465a3eaf0523659e/nearcore/src/config.rs#L496C17-L496C27

To Reproduce

  1. In ~/.near/config.json add a parameter that doesn't exist. For example:
{
  "anotheroptionthatdoesntexist": "whatisthat",
  "genesis_file": "genesis.json",
  "genesis_records_file": null,
  1. Run neard:
cargo run --bin neard -- run
  1. There's no warning about the nonexistent option:
2023-12-07T10:29:10.084808Z  INFO config: Validating Config, extracted from config.json...
2023-12-07T10:29:10.084964Z  INFO near_o11y::reload: Updated the logging layer according to `log_config.json`
2023-12-07T10:29:10.085140Z  INFO db_opener: Opening NodeStorage path="/Users/jancio/.near/data" cold_path="none"
2023-12-07T10:29:10.085290Z  INFO db: Opened a new RocksDB instance. num_instances=1
2023-12-07T10:29:10.090302Z  INFO db: Closed a RocksDB instance. num_instances=0
2023-12-07T10:29:10.090315Z  INFO db_opener: The database exists. path=/Users/jancio/.near/data
2023-12-07T10:29:10.090334Z  INFO db: Opened a new RocksDB instance. num_instances=1
2023-12-07T10:29:10.126215Z  INFO db: Closed a RocksDB instance. num_instances=0
2023-12-07T10:29:10.126256Z  INFO db: Opened a new RocksDB instance. num_instances=1
2023-12-07T10:29:10.127329Z  INFO db: Closed a RocksDB instance. num_instances=0
2023-12-07T10:29:10.127351Z  INFO db: Opened a new RocksDB instance. num_instances=1
...

Expected behavior
I expected to see a warning that there's an unrecognised option in config.json.

Version (please complete the following information):

  • nearcore commit/branch: 8985a96 (master)
  • rust version (if local): cargo 1.74.0 (ecb9851af 2023-10-18)
  • docker (if using docker): no
  • mainnet/testnet/betanet/local: local
@jancionear
Copy link
Contributor Author

I added a bit of debug information (the code is on this branch: https://github.com/jancionear/nearcore/tree/config-debug).
Output:

2023-12-07T10:31:58.340878Z  INFO neard: version="trunk" build="1.1.0-4648-g5b5634bf7-modified" latest_protocol=64
Reading config from file: "/Users/jancio/.near/config.json"
Config starts with: {
  "anotheroptionthatdoesntexist": "whatisthat",
  "genesis_file": "genesis.json",
  "genesis_records_file": null,
  "validator_key_file": "validator_key.json",
  "node_key_file": "node_key.json",
  
[nearcore/src/config.rs:510] unrecognised_fields.len() = 0
2023-12-07T10:31:58.345258Z  INFO config: Validating Config, extracted from config.json...
2023-12-07T10:31:58.347211Z  WARN genesis: Skipped genesis validation
2023-12-07T10:31:58.347301Z  INFO config: Validating Genesis config and records. This could take a few minutes...
2023-12-07T10:31:58.347713Z  INFO config: All validations have passed!
2023-12-07T10:31:58.352408Z  INFO neard: Reset the config "/Users/jancio/.near/log_config.json" because the config file doesn't exist. err=Os { code: 2, kind: NotFound, message: "No such file or directory" }
Reading config from file: "/Users/jancio/.near/config.json"
Config starts with: {
  "anotheroptionthatdoesntexist": "whatisthat",
  "genesis_file": "genesis.json",
  "genesis_records_file": null,
  "validator_key_file": "validator_key.json",
  "node_key_file": "node_key.json",
  
[nearcore/src/config.rs:510] unrecognised_fields.len() = 0
2023-12-07T10:31:58.352763Z  INFO config: Validating Config, extracted from config.json...
2023-12-07T10:31:58.352974Z  INFO near_o11y::reload: Updated the logging layer according to `log_config.json`
2023-12-07T10:31:58.353240Z  INFO db_opener: Opening NodeStorage path="/Users/jancio/.near/data" cold_path="none"
2023-12-07T10:31:58.353433Z  INFO db: Opened a new RocksDB instance. num_instances=1
2023-12-07T10:31:58.360813Z  INFO db: Closed a RocksDB instance. num_instances=0

It looks like the unrecognised_fields variable is always empty, despite having an invalid option in the config. This makes me suspicious of serde_ignored::deserialize, maybe it's doing a different thing from what the authors expected it to do?
(code for reference: https://github.com/near/nearcore/blob/abf6e99bad479a216a7f6e8a465a3eaf0523659e/nearcore/src/config.rs#L496C17-L496C27)

@nagisa
Copy link
Collaborator

nagisa commented Dec 7, 2023

Duplicate of #9073. Since you're looking into this, up to you which issue you want to keep.

@jancionear
Copy link
Contributor Author

Duplicate of #9073. Since you're looking into this, up to you which issue you want to keep.

Thanks, I didn't see it. I searched for unrecongised, but not for unrecognized x.x
It's a shame that github's search isn't more fuzzy.

#9073 seems like a better issue, it actually explains why things are wrong and links to the serde_ignore bug. I will close this one.

@jancionear
Copy link
Contributor Author

Closing as a duplicate of #9073

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

No branches or pull requests

2 participants