Skip to content

Move towards compile-time type safety for arg registration #8

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.

Sign up for GitHub

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

Open
ajtowns wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Move towards compile-time type safety for arg registration #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6e7e9e3
config: separate base and non-base chainparams config options
ajtowns Aug 23, 2023
059861c
chainparams: move -fastprune definition into chainparams.cpp
ajtowns Aug 23, 2023
28ddd97
chainparams: put -vbparams in DEBUG_TEST category
ajtowns Aug 23, 2023
7cb67a6
args: add ArgsRegister
ajtowns Aug 23, 2023
26372df
chainparams: convert signet params to ArgsRegister
ajtowns Aug 23, 2023
dddfbea
chainparams: convert regtest params to ArgsRegister
ajtowns Aug 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ BITCOIN_CORE_H = \
clientversion.h \
coins.h \
common/args.h \
common/argsregister.h \
common/bloom.h \
common/init.h \
common/run_command.h \
Expand Down
179 changes: 121 additions & 58 deletions src/chainparams.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <chainparamsbase.h>
#include <common/args.h>
#include <common/argsregister.h>
#include <consensus/params.h>
#include <deploymentinfo.h>
#include <logging.h>
Expand All @@ -21,82 +22,144 @@
#include <stdexcept>
#include <vector>

void ReadSigNetArgs(const ArgsManager& args, CChainParams::SigNetOptions& options)

namespace {
class SigNetArgsRegister
{
if (args.IsArgSet("-signetseednode")) {
options.seeds.emplace(args.GetArgs("-signetseednode"));
}
if (args.IsArgSet("-signetchallenge")) {
const auto signet_challenge = args.GetArgs("-signetchallenge");
if (signet_challenge.size() != 1) {
public:
using T = CChainParams::SigNetOptions;

static inline void GetChallenge(std::optional<std::vector<uint8_t>>& challenge, const std::vector<std::string>& arg_challenges)
{
if (arg_challenges.size() != 1) {
throw std::runtime_error("-signetchallenge cannot be multiple values.");
}
const auto val{TryParseHex<uint8_t>(signet_challenge[0])};

const auto val{TryParseHex<uint8_t>(arg_challenges[0])};
if (!val) {
throw std::runtime_error(strprintf("-signetchallenge must be hex, not '%s'.", signet_challenge[0]));
throw std::runtime_error(strprintf("-signetchallenge must be hex, not '%s'.", arg_challenges[0]));
}
options.challenge.emplace(*val);
challenge.emplace(*val);
}
}

void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& options)
template<typename C, typename Op>
static inline void Register(Op& op)
{
return C::Do(op,
C::Defn(&T::challenge, "-signetchallenge", "", GetChallenge,
"Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)",
ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION,
OptionsCategory::CHAINPARAMS),
C::Defn(&T::seeds, "-signetseednode", "",
"Specify a seed node for the signet network, in the hostname[:port] format, e.g. sig.net:1234 (may be used multiple times to specify multiple seed nodes; defaults to the global default signet test network seed node(s))",
ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION,
OptionsCategory::CHAINPARAMS)
);
}
};

class RegTestArgsRegister
Copy link
Owner Author

@ajtowns ajtowns Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should have a namespace, eg:

template<typename T>
class ArgsRegisterImpl<T>;

template<>
class ArgsRegisterImpl<CChainParams::RegTestOptions>
{
public:
    ...
}

Then you can just say ArgsRegister<CChainParams::RegTestOptions> later.

{
if (auto value = args.GetBoolArg("-fastprune")) options.fastprune = *value;
public:
using T = CChainParams::RegTestOptions;

for (const std::string& arg : args.GetArgs("-testactivationheight")) {
const auto found{arg.find('@')};
if (found == std::string::npos) {
throw std::runtime_error(strprintf("Invalid format (%s) for -testactivationheight=name@height.", arg));
}

const auto value{arg.substr(found + 1)};
int32_t height;
if (!ParseInt32(value, &height) || height < 0 || height >= std::numeric_limits<int>::max()) {
throw std::runtime_error(strprintf("Invalid height value (%s) for -testactivationheight=name@height.", arg));
}
static inline void GetActivationHeights(std::unordered_map<Consensus::BuriedDeployment, int>& activation_heights, const std::vector<std::string>& args)
Copy link
Owner Author

@ajtowns ajtowns Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These could be outside the class, just regular static module level functions.

{
for (const std::string& arg : args) {
const auto found{arg.find('@')};
if (found == std::string::npos) {
throw std::runtime_error(strprintf("Invalid format (%s) for -testactivationheight=name@height.", arg));
}

const auto deployment_name{arg.substr(0, found)};
if (const auto buried_deployment = GetBuriedDeployment(deployment_name)) {
options.activation_heights[*buried_deployment] = height;
} else {
throw std::runtime_error(strprintf("Invalid name (%s) for -testactivationheight=name@height.", arg));
const auto value{arg.substr(found + 1)};
int32_t height;
if (!ParseInt32(value, &height) || height < 0 || height >= std::numeric_limits<int>::max()) {
throw std::runtime_error(strprintf("Invalid height value (%s) for -testactivationheight=name@height.", arg));
}

const auto deployment_name{arg.substr(0, found)};
if (const auto buried_deployment = GetBuriedDeployment(deployment_name)) {
activation_heights[*buried_deployment] = height;
} else {
throw std::runtime_error(strprintf("Invalid name (%s) for -testactivationheight=name@height.", arg));
}
}
}

if (!args.IsArgSet("-vbparams")) return;

for (const std::string& strDeployment : args.GetArgs("-vbparams")) {
std::vector<std::string> vDeploymentParams = SplitString(strDeployment, ':');
if (vDeploymentParams.size() < 3 || 4 < vDeploymentParams.size()) {
throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end[:min_activation_height]");
}
CChainParams::VersionBitsParameters vbparams{};
if (!ParseInt64(vDeploymentParams[1], &vbparams.start_time)) {
throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
}
if (!ParseInt64(vDeploymentParams[2], &vbparams.timeout)) {
throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
}
if (vDeploymentParams.size() >= 4) {
if (!ParseInt32(vDeploymentParams[3], &vbparams.min_activation_height)) {
throw std::runtime_error(strprintf("Invalid min_activation_height (%s)", vDeploymentParams[3]));
static inline void GetVBParams(std::unordered_map<Consensus::DeploymentPos, CChainParams::VersionBitsParameters>& version_bits_parameters, const std::vector<std::string>& args)
{
for (const std::string& deployment : args) {
std::vector<std::string> vDeploymentParams = SplitString(deployment, ':');
if (vDeploymentParams.size() < 3 || 4 < vDeploymentParams.size()) {
throw std::runtime_error("Version bits parameters malformed, expecting deployment:start:end[:min_activation_height]");
}
} else {
vbparams.min_activation_height = 0;
}
bool found = false;
for (int j=0; j < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {
if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) {
options.version_bits_parameters[Consensus::DeploymentPos(j)] = vbparams;
found = true;
LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld, min_activation_height=%d\n", vDeploymentParams[0], vbparams.start_time, vbparams.timeout, vbparams.min_activation_height);
break;
CChainParams::VersionBitsParameters vbparams{};
if (!ParseInt64(vDeploymentParams[1], &vbparams.start_time)) {
throw std::runtime_error(strprintf("Invalid nStartTime (%s)", vDeploymentParams[1]));
}
if (!ParseInt64(vDeploymentParams[2], &vbparams.timeout)) {
throw std::runtime_error(strprintf("Invalid nTimeout (%s)", vDeploymentParams[2]));
}
if (vDeploymentParams.size() >= 4) {
if (!ParseInt32(vDeploymentParams[3], &vbparams.min_activation_height)) {
throw std::runtime_error(strprintf("Invalid min_activation_height (%s)", vDeploymentParams[3]));
}
} else {
vbparams.min_activation_height = 0;
}
bool found = false;
for (int j=0; j < (int)Consensus::MAX_VERSION_BITS_DEPLOYMENTS; ++j) {
if (vDeploymentParams[0] == VersionBitsDeploymentInfo[j].name) {
version_bits_parameters[Consensus::DeploymentPos(j)] = vbparams;
found = true;
LogPrintf("Setting version bits activation parameters for %s to start=%ld, timeout=%ld, min_activation_height=%d\n", vDeploymentParams[0], vbparams.start_time, vbparams.timeout, vbparams.min_activation_height);
break;
}
}
if (!found) {
throw std::runtime_error(strprintf("Invalid deployment (%s)", vDeploymentParams[0]));
}
}
if (!found) {
throw std::runtime_error(strprintf("Invalid deployment (%s)", vDeploymentParams[0]));
}
}

template<typename C, typename Op>
static inline void Register(Op& op)
{
return C::Do(op,
C::Defn(&T::version_bits_parameters, "-vbparams", "=deployment:start:end[:min_activation_height]", GetVBParams,
Copy link
Owner Author

@ajtowns ajtowns Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe C::Register(op, C::Arg(..)) would be better names?

"Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)",
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY,
OptionsCategory::DEBUG_TEST),
C::Defn(&T::activation_heights, "-testactivationheight", "=name@height.", GetActivationHeights,
"Set the activation height of 'name' (segwit, bip34, dersig, cltv, csv). (regtest-only)",
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY,
OptionsCategory::DEBUG_TEST),
C::Defn(&T::fastprune, "-fastprune", "",
"Use smaller block files and lower minimum prune height for testing purposes",
ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY,
OptionsCategory::DEBUG_TEST)
);
}
};

} // anon namespace

void SetupChainParamsOptions(ArgsManager& argsman)
{
ArgsRegister<RegTestArgsRegister>::Register(argsman);
ArgsRegister<SigNetArgsRegister>::Register(argsman);
}


void ReadSigNetArgs(const ArgsManager& args, CChainParams::SigNetOptions& options)
{
ArgsRegister<SigNetArgsRegister>::Update(args, options);
}

void ReadRegTestArgs(const ArgsManager& args, CChainParams::RegTestOptions& options)
{
ArgsRegister<RegTestArgsRegister>::Update(args, options);
Copy link
Owner Author

@ajtowns ajtowns Aug 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe ArgsRegister<..>::ApplyArgs would be better naming? (cf node/mempool_args.h)

}

static std::unique_ptr<const CChainParams> globalChainParams;
Expand Down
5 changes: 5 additions & 0 deletions src/chainparams.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@

class ArgsManager;

/**
* Setup configuration options
*/
void SetupChainParamsOptions(ArgsManager& argsman);

/**
* Creates and returns a std::unique_ptr<CChainParams> of the chosen chain.
*/
Expand Down
4 changes: 0 additions & 4 deletions src/chainparamsbase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,8 @@ void SetupChainParamsBaseOptions(ArgsManager& argsman)
argsman.AddArg("-chain=<chain>", "Use the chain <chain> (default: main). Allowed values: main, test, signet, regtest", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-regtest", "Enter regression test mode, which uses a special chain in which blocks can be solved instantly. "
"This is intended for regression testing tools and app development. Equivalent to -chain=regtest.", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-testactivationheight=name@height.", "Set the activation height of 'name' (segwit, bip34, dersig, cltv, csv). (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
argsman.AddArg("-testnet", "Use the test chain. Equivalent to -chain=test.", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-vbparams=deployment:start:end[:min_activation_height]", "Use given start/end times and min_activation_height for specified version bits deployment (regtest-only)", ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-signet", "Use the signet chain. Equivalent to -chain=signet. Note that the network is defined by the -signetchallenge parameter", ArgsManager::ALLOW_ANY, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-signetchallenge", "Blocks must satisfy the given script to be considered valid (only for signet networks; defaults to the global default signet test network challenge)", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS);
argsman.AddArg("-signetseednode", "Specify a seed node for the signet network, in the hostname[:port] format, e.g. sig.net:1234 (may be used multiple times to specify multiple seed nodes; defaults to the global default signet test network seed node(s))", ArgsManager::ALLOW_ANY | ArgsManager::DISALLOW_NEGATION, OptionsCategory::CHAINPARAMS);
}

static std::unique_ptr<CBaseChainParams> globalChainBaseParams;
Expand Down
22 changes: 22 additions & 0 deletions src/common/args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,6 +489,28 @@ std::optional<int64_t> ArgsManager::GetIntArg(const std::string& strArg) const
return SettingToInt(value);
}

template<>
std::optional<std::string> ArgsManager::Get<std::string>(const std::string& arg) const
{
return GetArg(arg);
}

template<>
std::optional<bool> ArgsManager::Get<bool>(const std::string& arg) const
{
return GetBoolArg(arg);
}

template<>
std::optional<std::vector<std::string>> ArgsManager::Get<std::vector<std::string>>(const std::string& arg) const
{
std::optional<std::vector<std::string>> result{GetArgs(arg)};
if (result.has_value() && result.value().empty()) {
result.reset();
}
return result;
}

std::optional<int64_t> SettingToInt(const common::SettingsValue& value)
{
if (value.isNull()) return std::nullopt;
Expand Down
20 changes: 20 additions & 0 deletions src/common/args.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,15 @@ class ArgsManager
*/
bool IsArgNegated(const std::string& strArg) const;

/**
* Return typed argument if present
*
* @param arg Argument to get (e.g. "-foo")
* @return command-line argument or nullopt
*/
template<typename T>
std::optional<T> Get(const std::string& arg) const;

/**
* Return string argument or default value
*
Expand Down Expand Up @@ -343,6 +352,17 @@ class ArgsManager
*/
void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat);

/**
* Add typed argument
*/
template<typename T>
void AddTypedArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat)
{
// able to asssociate type information with argument directly in future,
// eg specialise AddTypedArg<std::vector<std::string>> to call AddArg(name, help, flags|ALLOW_LIST, cat).
AddArg(name, help, flags, cat);
}

/**
* Add subcommand
*/
Expand Down
Loading