Needed rebase. Also replaced GetParamsHelpMessages with AppendParamsHelpMessages, which optionally takes a showDebugHelp boolean.

@theuni this may interact with your code.

Closing for now to reduce noise

Great. MAX_NETWORK_TYPES is removed in the second commit.

Ack (tested).

General approach OK. Comments:

• Still repeating a chain lookup in chainparams.cpp & chainparamsbase.cpp
• Only now it is worse because it is a more expensive (now duplicated) string comparison
• Seeing this duplication hints that a table-driven approach, with each string inside the param struct, is superior to manually administered case statements (& duplicated) of chain strings.

Seems like you want find_chain_by_string(), returns id, which can be passed around efficiently as 'network' -- which is consistent with what the rest of the code does now.

@jgarzik since each param has a strNetworkID which is exactly the same as the CBaseChainParams constants introduced, I think a set would be the simplest.

Re-introducing an intermediate type would defeat the point of these patches for me, which is to allow me to easily create a new testnet. I've been hacking bitcoind to run a megablocks testnet, for example.

But I like the simplicity of the patch's mechanical translation; easy to review. A lookup mechanism on top could work well as a new patch.

• Still repeating a chain lookup in chainparams.cpp & chainparamsbase.cpp
• Only now it is worse because it is a more expensive (now duplicated) string comparison

Yes, the string comparison is more expensive, but it's done on init or tests, so I think it's fine.
What do you mean by "now duplicated". Can you point out what exactly am I duplicating here?

-Seeing this duplication hints that a table-driven approach, with each string inside the param struct, is superior to manually administered case statements (& duplicated) of chain strings.
Seems like you want find_chain_by_string(), returns id, which can be passed around efficiently as 'network' -- which is consistent with what the rest of the code does now.

@jgarzik I'm not sure I understand your suggestion.
You mean a function like CBaseChainParams::Network networkEnumFromChainName(std::string)?
That doesn't remove the duplicated switch/elifs for chainparams and basechainparams. The problem here with that duplication is having base chainparams in the first place. This is not doing anything against or in favor to that. I believe we could move the datadir to regular chainparams and have users select the bitcoin-cli port manually instead of indirectly through -testnet and -regtest. Anyway, that would belong in another PR.
Anyway, this PR was open without replacing the enum with a string. I could reopen that again, but it's cleaner this way and there's many other things that require that replacement in #6382 .

EDIT (after reverting #6077 ):
This (and thus #6382) should fail the compilation if merged after #6077, I will rebase both: but don't merge this now.

ping @theuni . This should be useful for #6526 just like it's useful for #6382

Rebased and fixed (see #6077 (comment) ).

Untested ACK

ut ACK

Fixed @sipa 's nit.

ut ACK - though this pushes the bounds of my refactor size-o-meter for end-user utility versus source code disturbance ;p

utACK

For more context on more long-term utility of this change see #6382 (needs rebase on top of #6672 ).

ping

ut ACK. I'm not concerned about string comparisons on init.

With #6382 NACK'd there is less direct utility to this patch, but I like the deduplication of the error handling. (The whole "selecting chainparams" area of the code can be frustrating to read because it has a lot of redundancy like this.)

ut ACK

Updated without translating errors or messages that are only shown when debugHelp is true (solved @laanwj and @MarcoFalke 's nits).