near / nearcore

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

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

Open nagisa opened 2 years ago

nagisa 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::panicfmt at /rustc/09c42c45858d5f3aedfa670698275303a3d19afa/library/core/src/panicking.rs:101:14 2: nearcore::config::Config::fromfile::(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):**_ 

*   dcab6f416
*   1.56
*   local
marcelo-gonzalez commented 2 years ago

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.

nagisa commented 2 years ago

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.

marcelo-gonzalez commented 2 years ago

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

stale[bot] commented 2 years ago

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.

bowenwang1996 commented 2 years ago

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

marcelo-gonzalez commented 2 years ago

@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