From b814336aec55b187097089d88c202170ed2fe9a0 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 10 Feb 2023 22:13:36 +0000 Subject: [PATCH 1/3] Load `-allowdeprecated` settings after reading the config file We need to load these early so that it's possible for other initialization steps to respect them. However, we were loading them slightly too early, before the config file had been read, which meant that only CLI arguments were being used. We now load the `-allowdeprecated` settings just after the config file is parsed and the chain parameters are prepared; neither of these are features we would ever consider deprecating (at least while `zcashd` exists in its Bitcoin Core-derived form). Closes zcash/zcash#6420. --- src/bitcoind.cpp | 16 ++++++++-------- src/deprecation.cpp | 2 +- src/deprecation.h | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp index 2c9c0e14a..152044ac7 100644 --- a/src/bitcoind.cpp +++ b/src/bitcoind.cpp @@ -94,14 +94,6 @@ bool AppInit(int argc, char* argv[]) return true; } - // Handle setting of allowed-deprecated features as early as possible - // so that it's possible for other initialization steps to respect them. - auto deprecationError = SetAllowedDeprecatedFeaturesFromCLIArgs(); - if (deprecationError.has_value()) { - fprintf(stderr, "%s", deprecationError.value().c_str()); - return false; - } - try { if (!fs::is_directory(GetDataDir(false))) @@ -143,6 +135,14 @@ bool AppInit(int argc, char* argv[]) return false; } + // Handle setting of allowed-deprecated features as early as possible + // so that it's possible for other initialization steps to respect them. + auto deprecationError = LoadAllowedDeprecatedFeatures(); + if (deprecationError.has_value()) { + fprintf(stderr, "%s", deprecationError.value().c_str()); + return false; + } + // Command-line RPC bool fCommandLine = false; for (int i = 1; i < argc; i++) diff --git a/src/deprecation.cpp b/src/deprecation.cpp index 47569752c..cb9ad1e26 100644 --- a/src/deprecation.cpp +++ b/src/deprecation.cpp @@ -61,7 +61,7 @@ void EnforceNodeDeprecation(int nHeight, bool forceLogging, bool fThread) { } } -std::optional SetAllowedDeprecatedFeaturesFromCLIArgs() { +std::optional LoadAllowedDeprecatedFeatures() { auto args = GetMultiArg("-allowdeprecated"); std::set allowdeprecated(args.begin(), args.end()); diff --git a/src/deprecation.h b/src/deprecation.h index 565cca66c..e8b816e60 100644 --- a/src/deprecation.h +++ b/src/deprecation.h @@ -71,13 +71,13 @@ extern bool fEnableWalletTxVJoinSplit; void EnforceNodeDeprecation(int nHeight, bool forceLogging=false, bool fThread=true); /** - * Checks command-line arguments for enabling and/or disabling of deprecated + * Checks config options for enabling and/or disabling of deprecated * features and sets flags that enable deprecated features accordingly. * * @return std::nullopt if successful, or an error message indicating what * values are permitted for `-allowdeprecated`. */ -std::optional SetAllowedDeprecatedFeaturesFromCLIArgs(); +std::optional LoadAllowedDeprecatedFeatures(); /** * Returns a comma-separated list of the valid arguments to the -allowdeprecated From 8dd3e6135df212a89879ddfc857baa4abbfbe6fb Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 10 Feb 2023 23:39:19 +0000 Subject: [PATCH 2/3] qa: Refactor `wallet_deprecation` test to extract common logic Co-authored-by: Daira Hopwood --- qa/rpc-tests/wallet_deprecation.py | 57 +++++++++++++++--------------- 1 file changed, 28 insertions(+), 29 deletions(-) diff --git a/qa/rpc-tests/wallet_deprecation.py b/qa/rpc-tests/wallet_deprecation.py index e735470d0..f1fed7209 100755 --- a/qa/rpc-tests/wallet_deprecation.py +++ b/qa/rpc-tests/wallet_deprecation.py @@ -12,6 +12,17 @@ from test_framework.util import ( ) from test_framework.authproxy import JSONRPCException +# Pick a subset of the deprecated RPC methods to test with. This test assumes that +# the deprecation feature name is the same as the RPC method name, and that the RPC +# method works without any arguments. +DEFAULT_ENABLED = [ + "z_gettotalbalance", +] +DEFAULT_DISABLED = [ + "getnewaddress", + "z_getnewaddress", +] + # Test wallet address behaviour across network upgrades class WalletDeprecationTest(BitcoinTestFramework): def __init__(self): @@ -19,9 +30,9 @@ class WalletDeprecationTest(BitcoinTestFramework): self.num_nodes = 1 def setup_network(self): - self.setup_network_internal([]) + self.setup_network_with_args([]) - def setup_network_internal(self, allowed_deprecated = []): + def setup_network_with_args(self, allowed_deprecated): dep_args = ["-allowdeprecated=" + v for v in allowed_deprecated] self.nodes = start_nodes( @@ -50,16 +61,17 @@ class WalletDeprecationTest(BitcoinTestFramework): "failed with '%s'" % errorString if len(errorString) > 0 else "succeeded", )) - def run_test(self): - # Pick a subset of the deprecated RPC methods to test with. This test assumes that - # the deprecation feature name is the same as the RPC method name. - DEFAULT_ENABLED = [ - ] - DEFAULT_DISABLED = [ - "getnewaddress", - "z_getnewaddress", - ] + def test_case(self, start_mode, features_to_allow, expected_state): + stop_nodes(self.nodes) + wait_bitcoinds() + start_mode(features_to_allow) + for function in DEFAULT_ENABLED: + expected_state(function) + for function in DEFAULT_DISABLED: + expected_state(function) + + def run_test(self): # RPC methods that are deprecated but enabled by default should succeed for function in DEFAULT_ENABLED: self.verify_enabled(function) @@ -68,25 +80,12 @@ class WalletDeprecationTest(BitcoinTestFramework): for function in DEFAULT_DISABLED: self.verify_disabled(function) - # restart with a specific selection of deprecated methods enabled - stop_nodes(self.nodes) - wait_bitcoinds() - self.setup_network_internal(DEFAULT_DISABLED) + for start_mode in (self.setup_network_with_args,): + # restart with a specific selection of deprecated methods enabled + self.test_case(start_mode, DEFAULT_DISABLED, self.verify_enabled) - for function in DEFAULT_ENABLED: - self.verify_enabled(function) - for function in DEFAULT_DISABLED: - self.verify_enabled(function) - - # restart with no deprecated methods enabled - stop_nodes(self.nodes) - wait_bitcoinds() - self.setup_network_internal(["none"]) - - for function in DEFAULT_ENABLED: - self.verify_disabled(function) - for function in DEFAULT_DISABLED: - self.verify_disabled(function) + # restart with no deprecated methods enabled + self.test_case(start_mode, ["none"], self.verify_disabled) if __name__ == '__main__': WalletDeprecationTest().main() From 1ceb4c19cce09e6792f2607c1a505184aa807845 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 10 Feb 2023 23:51:56 +0000 Subject: [PATCH 3/3] qa: Extend `wallet_deprecation` to test `allowdeprecated` in config file Co-authored-by: Daira Hopwood --- qa/rpc-tests/wallet_deprecation.py | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/qa/rpc-tests/wallet_deprecation.py b/qa/rpc-tests/wallet_deprecation.py index f1fed7209..ddbfc8a55 100755 --- a/qa/rpc-tests/wallet_deprecation.py +++ b/qa/rpc-tests/wallet_deprecation.py @@ -12,6 +12,8 @@ from test_framework.util import ( ) from test_framework.authproxy import JSONRPCException +import os.path + # Pick a subset of the deprecated RPC methods to test with. This test assumes that # the deprecation feature name is the same as the RPC method name, and that the RPC # method works without any arguments. @@ -29,6 +31,12 @@ class WalletDeprecationTest(BitcoinTestFramework): super().__init__() self.num_nodes = 1 + def setup_chain(self): + super().setup_chain() + # Save a copy of node 0's zcash.conf + with open(os.path.join(self.options.tmpdir, "node0", "zcash.conf"), 'r', encoding='utf8') as f: + self.conf_lines = f.readlines() + def setup_network(self): self.setup_network_with_args([]) @@ -39,6 +47,13 @@ class WalletDeprecationTest(BitcoinTestFramework): self.num_nodes, self.options.tmpdir, extra_args=[dep_args] * self.num_nodes) + def setup_network_with_config(self, allowed_deprecated): + conf_lines = self.conf_lines + ["allowdeprecated={}\n".format(v) for v in allowed_deprecated] + with open(os.path.join(self.options.tmpdir, "node0", "zcash.conf"), 'w', encoding='utf8') as f: + f.writelines(conf_lines) + + self.nodes = start_nodes(self.num_nodes, self.options.tmpdir) + def verify_enabled(self, function): try: getattr(self.nodes[0], function)() @@ -80,7 +95,7 @@ class WalletDeprecationTest(BitcoinTestFramework): for function in DEFAULT_DISABLED: self.verify_disabled(function) - for start_mode in (self.setup_network_with_args,): + for start_mode in (self.setup_network_with_args, self.setup_network_with_config): # restart with a specific selection of deprecated methods enabled self.test_case(start_mode, DEFAULT_DISABLED, self.verify_enabled)