refactor: don't throw in GetChainName() #21039

fanquake commented at 9:58 AM on January 31, 2021: member

Currently if you start bitcoind with multiple chain options, we will fail with an assertion rather than exit gracefully. i.e:

src/bitcoind -testnet -regtest

************************
EXCEPTION: St13runtime_error
Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.
bitcoin in AppInit()

Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 36.

Rather than throw in GetChainName(), just return the string and throw in CreateBaseChainParams() which will fail more gracefully. i.e:

src/bitcoind -testnet -regtest
Error: CreateBaseChainParams: Unknown chain: Invalid combination of -regtest, -signet, -testnet or -chain.

src/bitcoind -chain=not_a_chain
Error: CreateBaseChainParams: Unknown chain: not_a_chain.

One of the followups from #15934.

fanquake added the label Refactoring on Jan 31, 2021

fanquake added the label Waiting for author on Jan 31, 2021

fanquake force-pushed on Jan 31, 2021

refactor: don't throw in GetChainName()

Currently if you start bitcoind with multiple chain options, bitcoind
will fail an assertion rather than exit gracefully. i.e:
```bash
src/bitcoind -testnet -regtest

************************
EXCEPTION: St13runtime_error
Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.
bitcoin in AppInit()

Assertion failed: (globalChainBaseParams), function BaseParams, file chainparamsbase.cpp, line 36.
```

Rather than throw in GetChainName(), return the error and throw in
CreateBaseChainParams() which will fail grafully. i.e:
```bash
src/bitcoind -testnet -regtest
Error: CreateBaseChainParams: Unknown chain: Invalid combination of -regtest, -signet, -testnet or -chain.
```

3b9227567f

fanquake force-pushed on Jan 31, 2021

in src/chainparamsbase.cpp:55 in 3b9227567f

  51 | @@ -52,7 +52,7 @@ std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain
  52 |      } else if (chain == CBaseChainParams::REGTEST) {
  53 |          return MakeUnique<CBaseChainParams>("regtest", 18443, 18445);
  54 |      }
  55 | -    throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
  56 | +    throw std::runtime_error(strprintf("%s: Unknown chain: %s.", __func__, chain));

jonatack commented at 6:26 PM on January 31, 2021:

maybe suggest valid values to the user

    throw std::runtime_error(strprintf("%s: Unknown chain: %s. Please provide one of: main, test, signet, regtest", __func__, chain));

jonatack commented at 6:36 PM on January 31, 2021: member

Tested ACK 3b9227567fafe9c4a9f165d1fd8f0eaa8ebe476a

practicalswift commented at 9:18 PM on January 31, 2021: contributor

Concept ACK: +5 −26 and a better user experience. Nice!

maflcko removed the label Waiting for author on Feb 1, 2021

maflcko added the label Waiting for author on Feb 1, 2021

maflcko commented at 8:22 AM on February 1, 2021: member

Btw, this was my attempt at this #14309

in src/util/system.cpp:948 in 3b9227567f

 944 | @@ -945,7 +945,7 @@ std::string ArgsManager::GetChainName() const
 945 |      const bool is_chain_arg_set = IsArgSet("-chain");
 946 |  
 947 |      if ((int)is_chain_arg_set + (int)fRegTest + (int)fSigNet + (int)fTestNet > 1) {
 948 | -        throw std::runtime_error("Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.");
 949 | +        return "Invalid combination of -regtest, -signet, -testnet or -chain";

maflcko commented at 8:24 AM on February 1, 2021:

I am not convinced that an error should be encoded as special chain name of "Invalid...". Even if this compiles right now, this is asking for issues down the road where the caller doesn't sanitize the chain name.

laanwj commented at 1:20 PM on February 1, 2021:

Agree, special-case values are prone to breakage. This is what std::optional is for! Let's use it.

ajtowns commented at 2:18 PM on February 4, 2021:

Doing this with std::optional everywhere seems pretty ugly, here's an alternative approach: https://github.com/ajtowns/bitcoin/commits/202102-getchainname . Probably better to have string GetChainName() and string GetChainNameCheck() and a private bool GetChainName(string&) that they both call than moving the exception message to the various callers though.

ryanofsky approved

ryanofsky commented at 10:58 PM on February 1, 2021: contributor

Code review ACK 3b9227567fafe9c4a9f165d1fd8f0eaa8ebe476a. Nice to see this followup! Other review comments seem reasonable but more minor compared to the existing improvement.

re: #21039#issue-564626324

One of the followups from #15934.

This seems to be linking to the wrong comment. #15934 (review). It looks like the right comment is jnewbery's #15934 (comment)

Verified diff with

git checkout origin/pull/21039/head
make -C src test/test_bitcoin && CHAIN_MERGE_TEST_OUT=b.txt src/test/test_bitcoin --run_test=util_tests/util_ChainMerge
git checkout HEAD^
make -C src test/test_bitcoin && CHAIN_MERGE_TEST_OUT=a.txt src/test/test_bitcoin --run_test=util_tests/util_ChainMerge
diff -u a.txt b.txt
sed 's/error: Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one./Invalid combination of -regtest, -signet, -testnet or -chain/' < a.txt > c.txt
diff -u b.txt c.txt

The diffs all look like

--- a.txt       2021-02-01 17:49:53.513092936 -0500
+++ b.txt       2021-02-01 17:49:43.785054447 -0500
@@ -7,7 +7,7 @@
  noregtest=1 || main
  testnet=0 testnet=1 || test
  notestnet=1 testnet=1 || test
- regtest=1 testnet=1 || error: Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.
+ regtest=1 testnet=1 || Invalid combination of -regtest, -signet, -testnet or -chain
  regtest=0 testnet=1 || test
  noregtest=1 testnet=1 || test
  testnet=1 testnet=0 || main
@@ -20,7 +20,7 @@
  regtest=1 notestnet=1 || regtest
  regtest=0 notestnet=1 || main
  noregtest=1 notestnet=1 || main
- testnet=1 regtest=1 || error: Invalid combination of -regtest, -signet, -testnet and -chain. Can use at most one.
+ testnet=1 regtest=1 || Invalid combination of -regtest, -signet, -testnet or -chain
  testnet=0 regtest=1 || regtest
  notestnet=1 regtest=1 || regtest
  regtest=0 regtest=1 || regtest
...

michaelfolkson commented at 11:17 PM on February 1, 2021: contributor

Concept ACK

At least in theory (if not in practice) src/bitcoind -testnet -regtest could be handled as src/bitcoind -testnet first and then src/bitcoind -regtest right? You can run two or more chains in parallel on the same machine. Not suggesting this as viable alternative though, just asking for own understanding.

sipa commented at 11:24 PM on February 1, 2021: member

@michaelfolkson You can run multiple bitcoind processes on the same machine, whether they operate on the same chain or different chains. But at least for now there is no way to have one process run on multiple chains simultaneously, and I don't expect there will be, so it's unclear what specifying multiple chains would mean.

It's also unrelated to this PR.

laanwj commented at 8:35 AM on February 2, 2021: member

It would IMO be unnecessary complication to run multiple chains in one process. When you can solve something using multiple processes that tends to be (definitely from a decoupling angle) a better design.

Offtopic also: I understand the confusion though. In retrospect I think we should have bitten the bullet and switched to -chain=X, instead of an option per chain, such an option maps much better to how things are handled under the hood. But I think that ship sailed.

maflcko commented at 9:53 AM on March 13, 2021: member

Is this up for grabs?

fanquake commented at 3:17 AM on March 29, 2021: member

Is this up for grabs?

🤷 It's got 2 ACKs, 1 Concept ACK, 1 suggestion to take a different approach, and 1 alternative to the different approach. Happy for someone else to take a stab.

fanquake closed this on Mar 29, 2021

fanquake removed the label Waiting for author on Mar 29, 2021

fanquake added the label Up for grabs on Mar 29, 2021

bitcoin locked this on Aug 16, 2022

fanquake deleted the branch on Nov 9, 2022

maflcko removed the label Up for grabs on Dec 3, 2024