near / nearcore

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

Missing, malformed or inaccessible config file(s) cause code panics #9950

Closed gmilescu closed 1 year ago

gmilescu commented 2 years ago

Describe the bug

Running `neard` commands with an absent configuration file will cause `neard` invocations to panic, rather than to print out an error message and terminate regularly.

For example:

1\. ./target/release/neard view_state peers
Nov 26 17:56:58.036 INFO neard: Version: trunk, Build: crates-0.10.0-64-gdcab6f416-modified, Latest Protocol: 49
thread 'main' panicked at 'Could not open config file: `~/.near/config.json`', nearcore/src/config.rs:469:33
stack backtrace:
0: rust_begin_unwind
at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/std/src/panicking.rs:517:5
1: core::panicking::panic_fmt
at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/panicking.rs:101:14
2: nearcore::config::Config::from_file::(file::)`closure`
3: nearcore::config::Config::from_file
4: nearcore::config::load_config_without_genesis_records
5: nearcore::config::load_config
6: state_viewer::cli::StateViewerSubCommand::run
7: neard::cli::NeardCmd::parse_and_run
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
```

Touching the config file causes a different, more opaque panic:

  1. mkdir ~/.near && touch ~/.near/config.json && ./target/release/neard view_state peers Nov 26 18:00:07.619 INFO neard: Version: trunk, Build: crates-0.10.0-64-gdcab6f416-modified, Latest Protocol: 49 thread 'main' panicked at 'Failed to deserialize config: Error("EOF while parsing a value", line: 1, column: 0)', nearcore/src/config.rs:502:39 stack backtrace: 0: rust_begin_unwind at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/std/src/panicking.rs:517:5 1: core::panicking::panic_fmt at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/panicking.rs:101:14 2: core::result::unwrap_failed at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/result.rs:1617:5 3: nearcore::config::Config::from_file 4: nearcore::config::load_config_without_genesis_records 5: nearcore::config::load_config 6: state_viewer::cli::StateViewerSubCommand::run 7: neard::cli::NeardCmd::parse_and_run note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

If the config file exists, but cannot be opened or read from for other reasons, the panics like those above will be presented.

Expected behavior

`neard` should print an error message rather than raise a panic. These are user errors, not inherent mistakes in the codebase. Additionally, my belief is that the error messages should attempt to retain the causal chain so that it is possible to present not just `failed to deserialize config` but also `could not obtain config from "~/.near/config.json"`.

Version (please complete the following information):

ND-78 created by None

gmilescu commented 2 years ago

marcelo-gonzalez commented:

didn't realize that https://github.com/near/nearcore/issues/5695 existed, but these two issues seem to be talking about pretty much the same thing? I'll just close this one in favor of tht one since there's more discussion over there.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

gmilescu commented 2 years ago

nagisa commented:

Your call. I think this issue is reasonably actionable whereas #5695 is pretty broad and its hard to tell if anybody fixing that issue will get around to fixing this one.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

gmilescu commented 2 years ago

marcelo-gonzalez commented:

> Your call. I think this issue is reasonably actionable whereas #5695 is pretty broad and its hard to tell if anybody fixing that issue will get around to fixing this one.

@nagisa hm yah that's true. I'll reopen then, since in any case I dont think there's that much left to do to resolve just this one. will send some PRs soon

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

gmilescu commented 2 years ago

stale[bot] commented:

This issue has been automatically marked as stale because it has not had recent activity in the last 2 months. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

gmilescu commented 2 years ago

bowenwang1996 commented:

@marcelo-gonzalez is this fixed in #6053 ? If so please close the issue

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

gmilescu commented 2 years ago

marcelo-gonzalez commented:

> @marcelo-gonzalez is this fixed in #6053 ? If so please close the issue

no not quite. That PR cleans up a lot of it, but it actually still panics, since there's still an unwrap(). It's just that the unwrap() is higher up the stack now. Still needs another PR or two to call this closed

by 557058:c020323a-70e4-4c07-9ccc-3ad89b1c02ec

gmilescu commented 1 year ago

Closing as duplicate of #5485.