From 4abace34adcb175a18c05cd20571c8e48d7d1be6 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 13 Apr 2020 11:39:41 -0400 Subject: [PATCH 1/9] Merge #18493: rpc: Remove deprecated "size" from mempool txs 0753efd9dc8f2e756955a726afbb602d904e1e92 rpc: Remove deprecated "size" from mempool txs (Vasil Dimov) Pull request description: Remove the "size" property of a mempool transaction from RPC replies. Deprecated in e16b6a718 in 0.19, about 1 year ago. ACKs for top commit: kristapsk: ACK 0753efd9dc8f2e756955a726afbb602d904e1e92 Tree-SHA512: 392ced6764dd6a1d47c6d1dc9de78990cf3384910d801253f8f620bd1751b2676a6b52bee8a30835d28e845d84bfb37431c1d2370f48c9d4dc8e6a48a5ae8b9e --- src/rpc/blockchain.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 86e476d846..a5748f9cef 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -451,8 +451,6 @@ static UniValue getdifficulty(const JSONRPCRequest& request) static std::vector MempoolEntryDescription() { return { RPCResult{RPCResult::Type::NUM, "vsize", "virtual transaction size. This can be different from actual serialized size for high-sigop transactions."}, - RPCResult{RPCResult::Type::NUM, "size", "(DEPRECATED) same as vsize. Only returned if dashd is started with -deprecatedrpc=size. " - "size will be completely removed in v0.20."}, RPCResult{RPCResult::Type::STR_AMOUNT, "fee", "transaction fee in " + CURRENCY_UNIT + " (DEPRECATED)"}, RPCResult{RPCResult::Type::STR_AMOUNT, "modifiedfee", "transaction fee with fee deltas used for mining priority (DEPRECATED)"}, RPCResult{RPCResult::Type::NUM_TIME, "time", "local time transaction entered pool in " + UNIX_EPOCH_TIME}, @@ -489,7 +487,6 @@ static void entryToJSON(const CTxMemPool& pool, UniValue& info, const CTxMemPool info.pushKV("fees", fees); info.pushKV("vsize", (int)e.GetTxSize()); - if (IsDeprecatedRPCEnabled("size")) info.pushKV("size", (int)e.GetTxSize()); info.pushKV("fee", ValueFromAmount(e.GetFee())); info.pushKV("modifiedfee", ValueFromAmount(e.GetModifiedFee())); info.pushKV("time", e.GetTime()); From 9867af5500085c239d7f248cf79c954bb0e9db0c Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 17 Apr 2020 07:55:36 -0400 Subject: [PATCH 2/9] Merge #18467: rpc: Improve documentation and return value of settxfee MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 38677274f931088218eeb1f258077d3387f39c89 rpc: settxfee respects -maxtxfee wallet setting (Fabian Jahr) bda84a08a0ac92dff6cadc99cf9bb8c3fadd7e13 rpc: Add documentation for deactivating settxfee (Fabian Jahr) Pull request description: ~~Closes 18315~~ `settxfee` can be deactivated by passing 0 as the fee. That does not seem to be documented for the user so this PR adds it in the description. The return value of a simple boolean seems also too simplified given the multiple dimensions that this deactivation feature enables. I.e. it does not seem intuitive if the returned boolean shows that the call succeeded or if means that `settxfee` is active. My suggested solution is to change the return value to a JSON object that included the "active" state and the currently set fee rate. Examples: ``` $ src/bitcoin-cli settxfee 0.0000000 { "active": false, "fee_rate": "0.00000000 BTC/kB" } $ src/bitcoin-cli settxfee 0.0001 { "active": true, "fee_rate": "0.00010000 BTC/kB" } ``` ACKs for top commit: MarcoFalke: ACK 38677274f931088218eeb1f258077d3387f39c89, seems useful to error out early instead of later #16257 🕍 jonatack: ACK 38677274f931088218eeb meshcollider: LGTM, utACK 38677274f931088218eeb1f258077d3387f39c89 Tree-SHA512: 642813b5cf6612abb4b6cb63728081a6bd1659d809e0149c8f56060b6da7253fee989b3b202854f3051df3773c966799af30b612648c466b099f00590f356548 --- src/wallet/rpcwallet.cpp | 6 +++++- test/functional/wallet_multiwallet.py | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 717377e10f..368dfe1ec5 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -2288,7 +2288,8 @@ static UniValue listlockunspent(const JSONRPCRequest& request) static UniValue settxfee(const JSONRPCRequest& request) { RPCHelpMan{"settxfee", - "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n", + "\nSet the transaction fee per kB for this wallet. Overrides the global -paytxfee command line parameter.\n" + "Can be deactivated by passing 0 as the fee. In that case automatic fee selection will be used by default.\n", { {"amount", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "The transaction fee in " + CURRENCY_UNIT + "/kB"}, }, @@ -2309,12 +2310,15 @@ static UniValue settxfee(const JSONRPCRequest& request) CAmount nAmount = AmountFromValue(request.params[0]); CFeeRate tx_fee_rate(nAmount, 1000); + CFeeRate max_tx_fee_rate(pwallet->m_default_max_tx_fee, 1000); if (tx_fee_rate == 0) { // automatic selection } else if (tx_fee_rate < pwallet->chain().relayMinFee()) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than min relay tx fee (%s)", pwallet->chain().relayMinFee().ToString())); } else if (tx_fee_rate < pwallet->m_min_fee) { throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be less than wallet min fee (%s)", pwallet->m_min_fee.ToString())); + } else if (tx_fee_rate > max_tx_fee_rate) { + throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("txfee cannot be more than wallet max tx fee (%s)", max_tx_fee_rate.ToString())); } pwallet->m_pay_tx_fee = tx_fee_rate; diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 66e0f1f32b..64db9b363c 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -7,6 +7,7 @@ Verify that a dashd node can load multiple wallet files """ from threading import Thread +from decimal import Decimal import os import shutil import time @@ -204,9 +205,9 @@ class MultiWalletTest(BitcoinTestFramework): self.log.info('Check for per-wallet settxfee call') assert_equal(w1.getwalletinfo()['paytxfee'], 0) assert_equal(w2.getwalletinfo()['paytxfee'], 0) - w2.settxfee(4.0) + w2.settxfee(0.001) assert_equal(w1.getwalletinfo()['paytxfee'], 0) - assert_equal(w2.getwalletinfo()['paytxfee'], 4.0) + assert_equal(w2.getwalletinfo()['paytxfee'], Decimal('0.00100000')) self.log.info("Test dynamic wallet loading") From 2e28e6eee386b8a419c910f04b854ac0118ed27f Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sun, 19 Apr 2020 06:10:04 -0400 Subject: [PATCH 3/9] Merge #18633: test: Properly raise FailedToStartError when rpc shutdown before warmup finished (take 2) fa03713e133e3017112fdd5c278e0c8643054578 test: Properly raise FailedToStartError when rpc shutdown before warmup finished (take 2) (MarcoFalke) Pull request description: actually (?) fix #18561 See most recent traceback https://travis-ci.org/github/bitcoin/bitcoin/jobs/674668692#L7062 I believe the reason the error is still there is that ConnectionResetError is derived from OSError: ConnectionResetError(ConnectionError(OSError)) And IOError is an alias for OSError since python 3.3, see https://docs.python.org/3/library/exceptions.html#IOError So fix that by renaming IOError to the alias OSError and move the less specific catch clause down a few lines. ACKs for top commit: jonatack: ACK fa03713e133e3017112fdd5c278e0c8643054578 Tree-SHA512: 6e5b214ed9101bf8ebe7472dcc1f9e9d128e2575c93ec00c8d0774ae1a9b52a8c2a653a45a0eab8d881570b08dd5ffeddf5aca88a10438c366e1f633253cb0b5 --- test/functional/test_framework/test_node.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index dddc8ac96c..5c26df7974 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -262,14 +262,14 @@ class TestNode(): self.rpc_connected = True self.url = self.rpc.url return - except IOError as e: - if e.errno != errno.ECONNREFUSED: # Port not yet open? - raise # unknown IO error except JSONRPCException as e: # Initialization phase # -28 RPC in warmup # -342 Service unavailable, RPC server started but is shutting down due to error if e.error['code'] != -28 and e.error['code'] != -342: raise # unknown JSON RPC exception + except OSError as e: + if e.errno != errno.ECONNREFUSED: # Port not yet open? + raise # unknown OS error except ValueError as e: # cookie file not found and no rpcuser or rpcassword. dashd still starting if "No RPC credentials" not in str(e): raise From 3dde9ab5c3395c9481b1ad41d98ea6a9ee351004 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 24 Apr 2020 14:22:10 -0400 Subject: [PATCH 4/9] Merge #18157: doc: fixing init.md documentation to not require rpcpassword MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit a2a03c3ca94b1cdd279ac09f2a81e04d262586fd fixing documentation to not require rpcpassword (“jkcd”) Pull request description: Configuration section in [doc/init.md](https://github.com/bitcoin/bitcoin/blob/master/doc/init.md) says user must set rpcpassword in order to run bitcoind. Since [71cbea](https://github.com/bitcoin/bitcoin/commit/71cbeaad9a929ba6a7b62d9b37a09b214ae00c1a) fixed the code to use a cookie for authentication, it is not mandatory to set rpcpassword in the configuration. Fixes #16346 ACKs for top commit: hebasto: ACK a2a03c3ca94b1cdd279ac09f2a81e04d262586fd, modulo nit Tree-SHA512: a62816fef78bed32200bb278cfc7aacf6ea154a60fdf5181927e48b806a1bd694bdf3ccec8362f5e58aad694d636c63f540323d54d85b61deaa417b95b8b56eb --- doc/init.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/init.md b/doc/init.md index 36d27cccea..4b4c5da565 100644 --- a/doc/init.md +++ b/doc/init.md @@ -20,9 +20,9 @@ The macOS configuration assumes dashd will be set up for the current user. Configuration --------------------------------- -At a bare minimum, dashd requires that the rpcpassword setting be set -when running as a daemon. If the configuration file does not exist or this -setting is not set, dashd will shut down promptly after startup. +Running dashd as a daemon does not require any manual configuration. You may +set the `rpcauth` setting in the `dash.conf` configuration file to override +the default behaviour of using a special cookie for authentication. This password does not have to be remembered or typed as it is mostly used as a fixed token that dashd and client programs read from the configuration From ed096a5d286fc43261a24b21230921a8852c62f2 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 29 Apr 2020 11:08:32 -0400 Subject: [PATCH 5/9] Merge #18774: test: added test for upgradewallet RPC 66fe7b1a98c03f690dcf60d359baac124658aeae test: added test for upgradewallet RPC (Harris) Pull request description: This PR adds tests for the newly merged *upgradewallet* RPC. Additionally, it expands `test_framework/util.py` by adding the function `adjust_bitcoin_conf_for_pre_17` to support nodes that don't parse configuration sections. This test uses two older node versions, v0.15.2 and v0.16.3, to create older wallet versions to be used by `upgradewallet`. Fixes https://github.com/bitcoin/bitcoin/issues/18767 Top commit has no ACKs. Tree-SHA512: bb72ff1e829e2c3954386cc308842820ef0828a4fbb754202b225a8748f92d4dcc5ec77fb146bfd5484a5c2f29ce95adf9f3fb4483437088ff3ea4a8d2c442c1 --- test/functional/test_framework/util.py | 7 ++ test/functional/test_runner.py | 1 + test/functional/wallet_upgradewallet.py | 156 ++++++++++++++++++++++++ 3 files changed, 164 insertions(+) create mode 100755 test/functional/wallet_upgradewallet.py diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py index 7f03ea9550..befe077073 100644 --- a/test/functional/test_framework/util.py +++ b/test/functional/test_framework/util.py @@ -359,6 +359,13 @@ def initialize_datadir(dirname, n, chain): os.makedirs(os.path.join(datadir, 'stdout'), exist_ok=True) return datadir +def adjust_bitcoin_conf_for_pre_17(conf_file): + with open(conf_file,'r', encoding='utf8') as conf: + conf_data = conf.read() + with open(conf_file, 'w', encoding='utf8') as conf: + conf_data_changed = conf_data.replace('[regtest]', '') + conf.write(conf_data_changed) + def get_datadir_path(dirname, n): return os.path.join(dirname, "node" + str(n)) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index b13bcbe2de..5e477b3edc 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -202,6 +202,7 @@ BASE_SCRIPTS = [ 'mempool_expiry.py', 'wallet_import_rescan.py', 'wallet_import_with_label.py', + 'wallet_upgradewallet.py', 'rpc_bind.py --ipv4', 'rpc_bind.py --ipv6', 'rpc_bind.py --nonloopback', diff --git a/test/functional/wallet_upgradewallet.py b/test/functional/wallet_upgradewallet.py new file mode 100755 index 0000000000..d04bc4ce44 --- /dev/null +++ b/test/functional/wallet_upgradewallet.py @@ -0,0 +1,156 @@ +#!/usr/bin/env python3 +# Copyright (c) 2018-2020 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. +"""upgradewallet RPC functional test + +Test upgradewallet RPC. Download v0.15.2 v0.16.3 node binaries: + +contrib/devtools/previous_release.sh -b v0.15.2 v0.16.3 +""" + +import os +import shutil + +from test_framework.test_framework import BitcoinTestFramework, SkipTest +from test_framework.util import ( + adjust_bitcoin_conf_for_pre_17, + assert_equal, + assert_greater_than, + assert_is_hex_string +) + +class UpgradeWalletTest(BitcoinTestFramework): + def set_test_params(self): + self.setup_clean_chain = True + self.num_nodes = 3 + self.extra_args = [ + ["-addresstype=bech32"], # current wallet version + ["-usehd=1"], # v0.16.3 wallet + ["-usehd=0"] # v0.15.2 wallet + ] + + def skip_test_if_missing_module(self): + self.skip_if_no_wallet() + + def setup_network(self): + self.setup_nodes() + + def setup_nodes(self): + if os.getenv("TEST_PREVIOUS_RELEASES") == "false": + raise SkipTest("upgradewallet RPC tests") + + releases_path = os.getenv("PREVIOUS_RELEASES_DIR") or os.getcwd() + "/releases" + if not os.path.isdir(releases_path): + if os.getenv("TEST_PREVIOUS_RELEASES") == "true": + raise AssertionError("TEST_PREVIOUS_RELEASES=1 but releases missing: " + releases_path) + raise SkipTest("This test requires binaries for previous releases") + + self.add_nodes(self.num_nodes, extra_args=self.extra_args, versions=[ + None, + 160300, + 150200 + ], binary=[ + self.options.bitcoind, + releases_path + "/v0.16.3/bin/bitcoind", + releases_path + "/v0.15.2/bin/bitcoind", + ], binary_cli=[ + self.options.bitcoincli, + releases_path + "/v0.16.3/bin/bitcoin-cli", + releases_path + "/v0.15.2/bin/bitcoin-cli", + ]) + # adapt bitcoin.conf, because older bitcoind's don't recognize config sections + adjust_bitcoin_conf_for_pre_17(self.nodes[1].bitcoinconf) + adjust_bitcoin_conf_for_pre_17(self.nodes[2].bitcoinconf) + self.start_nodes() + + def dumb_sync_blocks(self): + """ + Little helper to sync older wallets. + Notice that v0.15.2's regtest is hardforked, so there is + no sync for it. + v0.15.2 is only being used to test for version upgrade + and master hash key presence. + v0.16.3 is being used to test for version upgrade and balances. + Further info: https://github.com/bitcoin/bitcoin/pull/18774#discussion_r416967844 + """ + node_from = self.nodes[0] + v16_3_node = self.nodes[1] + to_height = node_from.getblockcount() + height = self.nodes[1].getblockcount() + for i in range(height, to_height+1): + b = node_from.getblock(blockhash=node_from.getblockhash(i), verbose=0) + v16_3_node.submitblock(b) + assert_equal(v16_3_node.getblockcount(), to_height) + + def run_test(self): + self.nodes[0].generatetoaddress(101, self.nodes[0].getnewaddress()) + self.dumb_sync_blocks() + # # Sanity check the test framework: + res = self.nodes[0].getblockchaininfo() + assert_equal(res['blocks'], 101) + node_master = self.nodes[0] + v16_3_node = self.nodes[1] + v15_2_node = self.nodes[2] + + # Send coins to old wallets for later conversion checks. + v16_3_wallet = v16_3_node.get_wallet_rpc('wallet.dat') + v16_3_address = v16_3_wallet.getnewaddress() + node_master.generatetoaddress(101, v16_3_address) + self.dumb_sync_blocks() + v16_3_balance = v16_3_wallet.getbalance() + + self.log.info("Test upgradewallet RPC...") + # Prepare for copying of the older wallet + node_master_wallet_dir = os.path.join(node_master.datadir, "regtest/wallets") + v16_3_wallet = os.path.join(v16_3_node.datadir, "regtest/wallets/wallet.dat") + v15_2_wallet = os.path.join(v15_2_node.datadir, "regtest/wallet.dat") + self.stop_nodes() + + # Copy the 0.16.3 wallet to the last Bitcoin Core version and open it: + shutil.rmtree(node_master_wallet_dir) + os.mkdir(node_master_wallet_dir) + shutil.copy( + v16_3_wallet, + node_master_wallet_dir + ) + self.restart_node(0, ['-nowallet']) + node_master.loadwallet('') + + wallet = node_master.get_wallet_rpc('') + old_version = wallet.getwalletinfo()["walletversion"] + + # calling upgradewallet without version arguments + # should return nothing if successful + assert_equal(wallet.upgradewallet(), "") + new_version = wallet.getwalletinfo()["walletversion"] + # upgraded wallet version should be greater than older one + assert_greater_than(new_version, old_version) + # wallet should still contain the same balance + assert_equal(wallet.getbalance(), v16_3_balance) + + self.stop_node(0) + # Copy the 0.15.2 wallet to the last Bitcoin Core version and open it: + shutil.rmtree(node_master_wallet_dir) + os.mkdir(node_master_wallet_dir) + shutil.copy( + v15_2_wallet, + node_master_wallet_dir + ) + self.restart_node(0, ['-nowallet']) + node_master.loadwallet('') + + wallet = node_master.get_wallet_rpc('') + # should have no master key hash before conversion + assert_equal('hdseedid' in wallet.getwalletinfo(), False) + # calling upgradewallet with explicit version number + # should return nothing if successful + assert_equal(wallet.upgradewallet(169900), "") + new_version = wallet.getwalletinfo()["walletversion"] + # upgraded wallet should have version 169900 + assert_equal(new_version, 169900) + # after conversion master key hash should be present + assert_is_hex_string(wallet.getwalletinfo()['hdseedid']) + +if __name__ == '__main__': + UpgradeWalletTest().main() From 58ba7a3d88494912063ba8caa4e4992ee43549d4 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 20 May 2020 07:24:56 -0400 Subject: [PATCH 6/9] Merge #19006: rpc: Avoid crash when g_thread_http was never started faf45d1f1f997c316fc4c611a23c4456533eefe9 http: Avoid crash when g_thread_http was never started (MarcoFalke) fa12a37b27f0570a551b8c103ea6537ee4a8e399 test: Replace inline-comments with logs, pep8 formatting (MarcoFalke) fa83b39ff3ae3fbad93df002915c0e5f99c104a9 init: Remove confusing and redundant InitError (MarcoFalke) Pull request description: Avoid a crash during shutdown when the init sequence failed for some reason ACKs for top commit: promag: Tested ACK faf45d1f1f997c316fc4c611a23c4456533eefe9. ryanofsky: Code review ACK faf45d1f1f997c316fc4c611a23c4456533eefe9. Thanks for updates, this is much easier to parse for me now. Since previous reviews: split out and reverted some cleanups & replaced chmod with mkdir in test hebasto: ACK faf45d1f1f997c316fc4c611a23c4456533eefe9, tested on Linux Mint 19.3 with the following patch: Tree-SHA512: 59632bf01c999e65c724e2728ac103250ccd8b0b16fac19d3a2a82639ab73e4f2efb86c78e63c588a5954625d8d0cf9545e2a7e070e6e15d2a54beeb50e00b61 --- src/httprpc.cpp | 4 ---- src/httpserver.cpp | 6 +++--- test/functional/rpc_users.py | 29 ++++++++++++++++------------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/src/httprpc.cpp b/src/httprpc.cpp index bb975a4ea1..3fcf4101f2 100644 --- a/src/httprpc.cpp +++ b/src/httprpc.cpp @@ -9,7 +9,6 @@ #include #include #include -#include #include #include #include @@ -253,9 +252,6 @@ static bool InitRPCAuthentication() { LogPrintf("Using random cookie authentication.\n"); if (!GenerateAuthCookie(&strRPCUserColonPass)) { - uiInterface.ThreadSafeMessageBox( - _("Error: A fatal internal error occurred, see debug.log for details"), // Same message as AbortNode - "", CClientUIInterface::MSG_ERROR); return false; } } else { diff --git a/src/httpserver.cpp b/src/httpserver.cpp index d001a9690c..110ff95939 100644 --- a/src/httpserver.cpp +++ b/src/httpserver.cpp @@ -423,7 +423,7 @@ bool UpdateHTTPServerLogging(bool enable) { #endif } -static std::thread threadHTTP; +static std::thread g_thread_http; static std::vector g_thread_http_workers; void StartHTTPServer() @@ -431,7 +431,7 @@ void StartHTTPServer() LogPrint(BCLog::HTTP, "Starting HTTP server\n"); int rpcThreads = std::max((long)gArgs.GetArg("-rpcthreads", DEFAULT_HTTP_THREADS), 1L); LogPrintf("HTTP: starting %d worker threads\n", rpcThreads); - threadHTTP = std::thread(ThreadHTTP, eventBase); + g_thread_http = std::thread(ThreadHTTP, eventBase); for (int i = 0; i < rpcThreads; i++) { g_thread_http_workers.emplace_back(HTTPWorkQueueRun, g_work_queue.get(), i); @@ -468,7 +468,7 @@ void StopHTTPServer() boundSockets.clear(); if (eventBase) { LogPrint(BCLog::HTTP, "Waiting for HTTP event thread to exit\n"); - threadHTTP.join(); + if (g_thread_http.joinable()) g_thread_http.join(); } if (eventHTTP) { evhttp_free(eventHTTP); diff --git a/test/functional/rpc_users.py b/test/functional/rpc_users.py index fcaa3b0526..de4169134e 100755 --- a/test/functional/rpc_users.py +++ b/test/functional/rpc_users.py @@ -44,18 +44,15 @@ class HTTPBasicsTest(BitcoinTestFramework): self.password = lines[3] with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "dash.conf"), 'a', encoding='utf8') as f: - f.write(rpcauth+"\n") - f.write(rpcauth2+"\n") - f.write(rpcauth3+"\n") + f.write(rpcauth + "\n") + f.write(rpcauth2 + "\n") + f.write(rpcauth3 + "\n") with open(os.path.join(get_datadir_path(self.options.tmpdir, 1), "dash.conf"), 'a', encoding='utf8') as f: - f.write(rpcuser+"\n") - f.write(rpcpassword+"\n") + f.write(rpcuser + "\n") + f.write(rpcpassword + "\n") def run_test(self): - - ################################################## - # Check correctness of the rpcauth config option # - ################################################## + self.log.info('Check correctness of the rpcauth config option') url = urllib.parse.urlparse(self.nodes[0].url) #Old authpair @@ -161,9 +158,7 @@ class HTTPBasicsTest(BitcoinTestFramework): assert_equal(resp.status, 401) conn.close() - ############################################################### - # Check correctness of the rpcuser/rpcpassword config options # - ############################################################### + self.log.info('Check correctness of the rpcuser/rpcpassword config options') url = urllib.parse.urlparse(self.nodes[1].url) # rpcuser and rpcpassword authpair @@ -203,5 +198,13 @@ class HTTPBasicsTest(BitcoinTestFramework): conn.close() + self.log.info('Check that failure to write cookie file will abort the node gracefully') + self.stop_node(0) + cookie_file = os.path.join(get_datadir_path(self.options.tmpdir, 0), self.chain, '.cookie.tmp') + os.mkdir(cookie_file) + init_error = 'Error: Unable to start HTTP server. See debug log for details.' + self.nodes[0].assert_start_raises_init_error(expected_msg=init_error) + + if __name__ == '__main__': - HTTPBasicsTest ().main () + HTTPBasicsTest().main() From f87b37e07b4729a03ae030f0a5605e7e5be1750b Mon Sep 17 00:00:00 2001 From: "Wladimir J. van der Laan" Date: Wed, 22 Apr 2020 13:59:11 +0200 Subject: [PATCH 7/9] Merge #18612: script: Remove undocumented and unused operator+ ccccd5190898ece3ac17aa3178f320d091f221df script: Remove undocumented and unused operator+ (MarcoFalke) Pull request description: This operator has no documented use case and is also unused outside of test code. The test code and all other (imaginary) code that might use this operator is written more clear and concise by the existing CScript push operators for opcodes and data. Removing the operator is also going to protect against accidentally reintroducing bugs like this https://github.com/bitcoin/bitcoin/commit/6ff5f718b6a67797b2b3bab8905d607ad216ee21#diff-8458adcedc17d046942185cb709ff5c3L1135 (last time it was used). ACKs for top commit: laanwj: ACK ccccd5190898ece3ac17aa3178f320d091f221df Tree-SHA512: 43898ac77e4d9643d9f8ac6f8f65497a4f0bbb1fb5dcaecc839c3719aa36181ba77befb213e59a9f33a20a29e0173a0e9c4763b1930940b32c3d1598b3e39af9 --- src/script/script.h | 27 ++---------------------- src/test/fuzz/script_ops.cpp | 12 +++++++---- src/test/script_tests.cpp | 41 ++++++++++-------------------------- 3 files changed, 21 insertions(+), 59 deletions(-) diff --git a/src/script/script.h b/src/script/script.h index c1514e1aa0..ed2dd833b0 100644 --- a/src/script/script.h +++ b/src/script/script.h @@ -416,28 +416,15 @@ public: SERIALIZE_METHODS(CScript, obj) { READWRITEAS(CScriptBase, obj); } - CScript& operator+=(const CScript& b) - { - reserve(size() + b.size()); - insert(end(), b.begin(), b.end()); - return *this; - } - - friend CScript operator+(const CScript& a, const CScript& b) - { - CScript ret = a; - ret += b; - return ret; - } - explicit CScript(int64_t b) { operator<<(b); } - explicit CScript(opcodetype b) { operator<<(b); } explicit CScript(const CScriptNum& b) { operator<<(b); } // delete non-existent constructor to defend against future introduction // e.g. via prevector explicit CScript(const std::vector& b) = delete; + /** Delete non-existent operator to defend against future introduction */ + CScript& operator<<(const CScript& b) = delete; CScript& operator<<(int64_t b) { return push_int64(b); } @@ -484,15 +471,6 @@ public: return *this; } - CScript& operator<<(const CScript& b) - { - // I'm not sure if this should push the script or concatenate scripts. - // If there's ever a use for pushing a script onto a script, delete this member fn - assert(!"Warning: Pushing a CScript onto a CScript with << is probably not intended, use + to concatenate!"); - return *this; - } - - bool GetOp(const_iterator& pc, opcodetype& opcodeRet, std::vector& vchRet) const { return GetScriptOp(pc, end(), opcodeRet, &vchRet); @@ -503,7 +481,6 @@ public: return GetScriptOp(pc, end(), opcodeRet, nullptr); } - /** Encode/decode small integers: */ static int DecodeOP_N(opcodetype opcode) { diff --git a/src/test/fuzz/script_ops.cpp b/src/test/fuzz/script_ops.cpp index 556d6ca1e3..1a86db20bc 100644 --- a/src/test/fuzz/script_ops.cpp +++ b/src/test/fuzz/script_ops.cpp @@ -17,12 +17,16 @@ void test_one_input(const std::vector& buffer) CScript script = ConsumeScript(fuzzed_data_provider); while (fuzzed_data_provider.remaining_bytes() > 0) { switch (fuzzed_data_provider.ConsumeIntegralInRange(0, 7)) { - case 0: - script += ConsumeScript(fuzzed_data_provider); + case 0: { + CScript s = ConsumeScript(fuzzed_data_provider); + script = std::move(s); break; - case 1: - script = script + ConsumeScript(fuzzed_data_provider); + } + case 1: { + const CScript& s = ConsumeScript(fuzzed_data_provider); + script = s; break; + } case 2: script << fuzzed_data_provider.ConsumeIntegral(); break; diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp index dad3d99f5f..f729f0c4cc 100644 --- a/src/test/script_tests.cpp +++ b/src/test/script_tests.cpp @@ -207,7 +207,6 @@ struct KeyData KeyData() { - key0.Set(vchKey0, vchKey0 + 32, false); key0C.Set(vchKey0, vchKey0 + 32, true); pubkey0 = key0.GetPubKey(); @@ -250,9 +249,9 @@ private: void DoPush(const std::vector& data) { - DoPush(); - push = data; - havePush = true; + DoPush(); + push = data; + havePush = true; } public: @@ -272,10 +271,10 @@ public: return *this; } - TestBuilder& Add(const CScript& _script) + TestBuilder& Opcode(const opcodetype& _op) { DoPush(); - spendTx.vin[0].scriptSig += _script; + spendTx.vin[0].scriptSig << _op; return *this; } @@ -644,22 +643,22 @@ BOOST_AUTO_TEST_CASE(script_build) tests.push_back(TestBuilder(CScript() << OP_2 << ToByteVector(keys.pubkey1C) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG, "2-of-2 with two identical keys and sigs pushed using OP_DUP but no SIGPUSHONLY", 0 - ).Num(0).PushSig(keys.key1).Add(CScript() << OP_DUP)); + ).Num(0).PushSig(keys.key1).Opcode(OP_DUP)); tests.push_back(TestBuilder(CScript() << OP_2 << ToByteVector(keys.pubkey1C) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG, "2-of-2 with two identical keys and sigs pushed using OP_DUP", SCRIPT_VERIFY_SIGPUSHONLY - ).Num(0).PushSig(keys.key1).Add(CScript() << OP_DUP).ScriptError(SCRIPT_ERR_SIG_PUSHONLY)); + ).Num(0).PushSig(keys.key1).Opcode(OP_DUP).ScriptError(SCRIPT_ERR_SIG_PUSHONLY)); tests.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey2C) << OP_CHECKSIG, "P2SH(P2PK) with non-push scriptSig but no P2SH or SIGPUSHONLY", 0, true - ).PushSig(keys.key2).Add(CScript() << OP_NOP8).PushRedeem()); + ).PushSig(keys.key2).Opcode(OP_NOP8).PushRedeem()); tests.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey2C) << OP_CHECKSIG, "P2PK with non-push scriptSig but with P2SH validation", 0 - ).PushSig(keys.key2).Add(CScript() << OP_NOP8)); + ).PushSig(keys.key2).Opcode(OP_NOP8)); tests.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey2C) << OP_CHECKSIG, "P2SH(P2PK) with non-push scriptSig but no SIGPUSHONLY", SCRIPT_VERIFY_P2SH, true - ).PushSig(keys.key2).Add(CScript() << OP_NOP8).PushRedeem().ScriptError(SCRIPT_ERR_SIG_PUSHONLY)); + ).PushSig(keys.key2).Opcode(OP_NOP8).PushRedeem().ScriptError(SCRIPT_ERR_SIG_PUSHONLY)); tests.push_back(TestBuilder(CScript() << ToByteVector(keys.pubkey2C) << OP_CHECKSIG, "P2SH(P2PK) with non-push scriptSig but not P2SH", SCRIPT_VERIFY_SIGPUSHONLY, true - ).PushSig(keys.key2).Add(CScript() << OP_NOP8).PushRedeem().ScriptError(SCRIPT_ERR_SIG_PUSHONLY)); + ).PushSig(keys.key2).Opcode(OP_NOP8).PushRedeem().ScriptError(SCRIPT_ERR_SIG_PUSHONLY)); tests.push_back(TestBuilder(CScript() << OP_2 << ToByteVector(keys.pubkey1C) << ToByteVector(keys.pubkey1C) << OP_2 << OP_CHECKMULTISIG, "2-of-2 with two identical keys and sigs pushed", SCRIPT_VERIFY_SIGPUSHONLY ).Num(0).PushSig(keys.key1).PushSig(keys.key1)); @@ -1403,24 +1402,6 @@ BOOST_AUTO_TEST_CASE(script_FindAndDelete) BOOST_CHECK(s == expect); } -BOOST_AUTO_TEST_CASE(script_can_append_self) -{ - CScript s, d; - - s = ScriptFromHex("00"); - s += s; - d = ScriptFromHex("0000"); - BOOST_CHECK(s == d); - - // check doubling a script that's large enough to require reallocation - static const char hex[] = "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38c4f35504e51ec112de5c384df7ba0b8d578a4c702b6bf11d5f"; - s = CScript() << ParseHex(hex) << OP_CHECKSIG; - d = CScript() << ParseHex(hex) << OP_CHECKSIG << ParseHex(hex) << OP_CHECKSIG; - s += s; - BOOST_CHECK(s == d); -} - - #if defined(HAVE_CONSENSUS_LIB) /* Test simple (successful) usage of dashconsensus_verify_script */ From fce00a8118a9cfcbcf70a3d585b829037664bf03 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 16 Nov 2020 07:57:30 +0100 Subject: [PATCH 8/9] Merge #20033: refactor: minor whitespace fixups, s/const/constexpr/ and remove template (followup to #19845) 89836a82eec63f93bbe6c3bd6a52be26e71ab54d style: minor improvements as a followup to #19845 (Vasil Dimov) Pull request description: Address suggestions: https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495486760 https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495488051 https://github.com/bitcoin/bitcoin/pull/19845#discussion_r495730125 ACKs for top commit: jonatack: re-ACK 89836a8 change since previous review is replacing std::runtime_error with std::exception, built/ran unit tests with gcc debian 10.2.0-15, then broke a few v3 net_tests involving `BOOST_CHECK_EXCEPTION`, rebuilt, ran `src/test/test_bitcoin -t net_tests -l all` and checked the error reporting. hebasto: re-ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d theStack: ACK 89836a82eec63f93bbe6c3bd6a52be26e71ab54d Tree-SHA512: 36477fdccabe5a8ad91fbabb4655cc363a3a7ca237a98ae6dd4a9fae4a4113762040f864d4ca13a47d081f7d16e5bd487edbfb61ab50a37e4a0424e9bec30b24 --- src/netaddress.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/netaddress.h b/src/netaddress.h index 903cc2a1bd..c6eba8d5c9 100644 --- a/src/netaddress.h +++ b/src/netaddress.h @@ -30,7 +30,7 @@ extern bool fAllowPrivateNet; * should be serialized in (unserialized from) v2 format (BIP155). * Make sure that this does not collide with any of the values in `version.h` */ -static const int ADDRV2_FORMAT = 0x20000000; +static constexpr int ADDRV2_FORMAT = 0x20000000; /** * A network type. From 01226e7830a57814147ddfee89fedba21106c860 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Tue, 17 Mar 2020 14:27:37 -0400 Subject: [PATCH 9/9] Merge #15283: log: Fix UB with bench on genesis block ec30a79f1c430cc7fbda37e5d747b0b31b262fa5 Fix UB with bench on genesis block (Gregory Sanders) Pull request description: During the loading of the genesis block, the bench print lines in ConnectTip divide by zero due to early return in ConnectBlock. ACKs for top commit: practicalswift: ACK ec30a79f1c430cc7fbda37e5d747b0b31b262fa5 sipa: utACK ec30a79f1c430cc7fbda37e5d747b0b31b262fa5 promag: ACK ec30a79, `nBlocksTotal` is only used in logging. Tree-SHA512: b3bdbb58d10d002a2293d7f99196b227ed9f4ca8c6cd08981e95cc964be47efed98b91fad276ee6da5cf7e6684610998ace7ce9bace172dd6c51c386d985b83c --- src/validation.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 2c7e9dc39b..19c9489345 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2011,6 +2011,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl return AbortNode(state, "Found EvoDB inconsistency, you must reindex to continue"); } } + nBlocksTotal++; // Special case for the genesis block, skipping connection of its transactions // (its coinbase is unspendable) @@ -2020,8 +2021,6 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl return true; } - nBlocksTotal++; - bool fScriptChecks = true; if (!hashAssumeValid.IsNull()) { // We've been configured with the hash of a block which has been externally verified to have a valid history. @@ -2833,6 +2832,7 @@ bool CChainState::ConnectTip(CValidationState& state, const CChainParams& chainp return error("%s: ConnectBlock %s failed, %s", __func__, pindexNew->GetBlockHash().ToString(), FormatStateMessage(state)); } nTime3 = GetTimeMicros(); nTimeConnectTotal += nTime3 - nTime2; + assert(nBlocksTotal > 0); LogPrint(BCLog::BENCHMARK, " - Connect total: %.2fms [%.2fs (%.2fms/blk)]\n", (nTime3 - nTime2) * MILLI, nTimeConnectTotal * MICRO, nTimeConnectTotal * MILLI / nBlocksTotal); bool flushed = view.Flush(); assert(flushed);