From 147d391c74c6ec15ab8b3f50cc6b6bae34638cc3 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 4 Dec 2020 15:18:44 +0100 Subject: [PATCH 01/11] Merge #20566: refactor: Use C++17 std::array where possible fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 refactor: Use C++17 std::array where possible (MarcoFalke) Pull request description: Using the C++11 std::array with explicit template parameters is problematic because overshooting the size will fill the memory with default constructed types. For example, ```cpp #include #include int main() { std::array a{1, 2}; for (const auto& i : a) { std::cout << i << std::endl; // prints "1 2 0" } } ``` ACKs for top commit: jonasschnelli: Code Review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 practicalswift: cr ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2 vasild: ACK fac7ab1d promag: Code review ACK fac7ab1d5b58fb9cfd80d5cf74ac4d2e5cb8eff2. Tree-SHA512: ef7e872340226e0d6160e6fd66c6ca78b2ef9c245fa0ab27fe4777aac9fba8d5aaa154da3d27b65dec39a6a63d07f1063c3a8ffb667a98ab137756a1a0af2656 --- src/wallet/walletutil.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/walletutil.cpp b/src/wallet/walletutil.cpp index 68bae737a8..862794c62c 100644 --- a/src/wallet/walletutil.cpp +++ b/src/wallet/walletutil.cpp @@ -36,7 +36,7 @@ bool IsFeatureSupported(int wallet_version, int feature_version) WalletFeature GetClosestWalletFeature(int version) { - const std::array wallet_features{{FEATURE_LATEST, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}}; + static constexpr std::array wallet_features{FEATURE_LATEST, FEATURE_HD, FEATURE_COMPRPUBKEY, FEATURE_WALLETCRYPT, FEATURE_BASE}; for (const WalletFeature& wf : wallet_features) { if (version >= wf) return wf; } From 785f7310ed4f7d5ce4c1963284b7f0566b7a8a32 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Sat, 12 Dec 2020 12:31:26 +0100 Subject: [PATCH 02/11] Merge #20079: p2p: Treat handshake misbehavior like unknown message faaad1bbac46cfeb22654b4c59f0aac7a680c03a p2p: Ignore version msgs after initial version msg (MarcoFalke) fad68afcff731153d1c83f7f56c91ecbb264b59a p2p: Ignore non-version msgs before version msg (MarcoFalke) Pull request description: Handshake misbehaviour doesn't cost us more than any other unknown message, so it seems odd to treat it differently ACKs for top commit: jnewbery: utACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a practicalswift: ACK faaad1bbac46cfeb22654b4c59f0aac7a680c03a: patch looks correct Tree-SHA512: 9f30c3b5c1f6604fd02cff878f10999956152419a3dd9825f8267cbdeff7d06787418b41c7fde8a00a5e557fe89204546e05d5689042dbf7b07fbb7eb95cddff --- src/net_processing.cpp | 8 +++----- test/functional/p2p_invalid_messages.py | 9 +++++++++ test/functional/p2p_leak.py | 19 +------------------ test/functional/p2p_timeouts.py | 6 ++++-- 4 files changed, 17 insertions(+), 25 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 77ed5bb9b2..a6c9502706 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3274,10 +3274,8 @@ void PeerManagerImpl::ProcessMessage( if (peer == nullptr) return; if (msg_type == NetMsgType::VERSION) { - // Each connection can only send one version message - if (pfrom.nVersion != 0) - { - Misbehaving(pfrom.GetId(), 1, "redundant version message"); + if (pfrom.nVersion != 0) { + LogPrint(BCLog::NET, "redundant version message from peer=%d\n", pfrom.GetId()); return; } @@ -3507,7 +3505,7 @@ void PeerManagerImpl::ProcessMessage( if (pfrom.nVersion == 0) { // Must have a version message before anything else - Misbehaving(pfrom.GetId(), 1, "non-version message before version handshake"); + LogPrint(BCLog::NET, "non-version message before version handshake. Message \"%s\" from peer=%d\n", SanitizeString(msg_type), pfrom.GetId()); return; } diff --git a/test/functional/p2p_invalid_messages.py b/test/functional/p2p_invalid_messages.py index 48a6cebc29..616d8b8935 100755 --- a/test/functional/p2p_invalid_messages.py +++ b/test/functional/p2p_invalid_messages.py @@ -17,6 +17,7 @@ from test_framework.messages import ( msg_headers, msg_inv, MSG_TX, + msg_version, ) from test_framework.p2p import ( P2PDataStore, P2PInterface @@ -50,6 +51,7 @@ class InvalidMessagesTest(BitcoinTestFramework): def run_test(self): self.test_buffer() + self.test_duplicate_version_msg() self.test_magic_bytes() self.test_checksum() self.test_size() @@ -78,6 +80,13 @@ class InvalidMessagesTest(BitcoinTestFramework): conn.sync_with_ping(timeout=1) self.nodes[0].disconnect_p2ps() + def test_duplicate_version_msg(self): + self.log.info("Test duplicate version message is ignored") + conn = self.nodes[0].add_p2p_connection(P2PDataStore()) + with self.nodes[0].assert_debug_log(['redundant version message from peer']): + conn.send_and_ping(msg_version()) + self.nodes[0].disconnect_p2ps() + def test_magic_bytes(self): self.log.info("Test message with invalid magic bytes disconnects peer") conn = self.nodes[0].add_p2p_connection(P2PDataStore()) diff --git a/test/functional/p2p_leak.py b/test/functional/p2p_leak.py index de4188174c..e1f4965d90 100755 --- a/test/functional/p2p_leak.py +++ b/test/functional/p2p_leak.py @@ -25,8 +25,6 @@ from test_framework.util import ( assert_greater_than_or_equal, ) -DISCOURAGEMENT_THRESHOLD = 100 - class LazyPeer(P2PInterface): def __init__(self): @@ -97,27 +95,16 @@ class P2PLeakTest(BitcoinTestFramework): self.setup_nodes() def run_test(self): - # Peer that never sends a version. We will send a bunch of messages - # from this peer anyway and verify eventual disconnection. - no_version_disconnect_peer = self.nodes[0].add_p2p_connection( - LazyPeer(), send_version=False, wait_for_verack=False) - # Another peer that never sends a version, nor any other messages. It shouldn't receive anything from the node. no_version_idle_peer = self.nodes[0].add_p2p_connection(LazyPeer(), send_version=False, wait_for_verack=False) # Peer that sends a version but not a verack. no_verack_idle_peer = self.nodes[0].add_p2p_connection(NoVerackIdlePeer(), wait_for_verack=False) - # Send enough ping messages (any non-version message will do) prior to sending - # version to reach the peer discouragement threshold. This should get us disconnected. - for _ in range(DISCOURAGEMENT_THRESHOLD): - no_version_disconnect_peer.send_message(msg_ping()) - # Wait until we got the verack in response to the version. Though, don't wait for the node to receive the # verack, since we never sent one no_verack_idle_peer.wait_for_verack() - no_version_disconnect_peer.wait_until(lambda: no_version_disconnect_peer.ever_connected, check_connected=False) no_version_idle_peer.wait_until(lambda: no_version_idle_peer.ever_connected) no_verack_idle_peer.wait_until(lambda: no_verack_idle_peer.version_received) @@ -127,13 +114,9 @@ class P2PLeakTest(BitcoinTestFramework): #Give the node enough time to possibly leak out a message time.sleep(5) - #This peer should have been banned - assert not no_version_disconnect_peer.is_connected - self.nodes[0].disconnect_p2ps() # Make sure no unexpected messages came in - assert no_version_disconnect_peer.unexpected_msg == False assert no_version_idle_peer.unexpected_msg == False assert no_verack_idle_peer.unexpected_msg == False @@ -152,7 +135,7 @@ class P2PLeakTest(BitcoinTestFramework): p2p_old_peer = self.nodes[0].add_p2p_connection(P2PInterface(), send_version=False, wait_for_verack=False) old_version_msg = msg_version() old_version_msg.nVersion = 31799 - with self.nodes[0].assert_debug_log(['peer=4 using obsolete version 31799; disconnecting']): + with self.nodes[0].assert_debug_log(['peer=3 using obsolete version 31799; disconnecting']): p2p_old_peer.send_message(old_version_msg) p2p_old_peer.wait_for_disconnect() diff --git a/test/functional/p2p_timeouts.py b/test/functional/p2p_timeouts.py index 704324aac8..7a13fc83c1 100755 --- a/test/functional/p2p_timeouts.py +++ b/test/functional/p2p_timeouts.py @@ -66,8 +66,10 @@ class TimeoutsTest(BitcoinTestFramework): assert no_version_node.is_connected assert no_send_node.is_connected - no_verack_node.send_message(msg_ping()) - no_version_node.send_message(msg_ping()) + with self.nodes[0].assert_debug_log(['Unsupported message "ping" prior to verack from peer=0']): + no_verack_node.send_message(msg_ping()) + with self.nodes[0].assert_debug_log(['non-version message before version handshake. Message "ping" from peer=1']): + no_version_node.send_message(msg_ping()) self.mock_forward(1) From e5249fb307875c376e04f763971c15c07f1beced Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 29 Jan 2021 07:43:05 +0100 Subject: [PATCH 03/11] Merge #21012: ci: Fuzz with integer sanitizer faff3991a9be0ea7be31685fb46d94c212c5da34 ci: Fuzz with integer sanitizer (MarcoFalke) Pull request description: Otherwise the suppressions file will go out of sync ACKs for top commit: practicalswift: cr ACK faff3991a9be0ea7be31685fb46d94c212c5da34: patch looks correct Tree-SHA512: 349216d071a2c5ccf24565fe0c52d7a570ec148d515d085616a284f1ab9992ce10ff82eb17962dddbcda765bbd3a9b15e8b25f34bdbed99fc36922d4161d307c --- .cirrus.yml | 2 +- Makefile.am | 4 ++++ ci/test/00_setup_env_native_fuzz.sh | 2 +- test/fuzz/test_runner.py | 20 +++++++++++++------- test/sanitizer_suppressions/ubsan | 1 + 5 files changed, 20 insertions(+), 9 deletions(-) diff --git a/.cirrus.yml b/.cirrus.yml index ab5893f054..04af90a764 100644 --- a/.cirrus.yml +++ b/.cirrus.yml @@ -124,7 +124,7 @@ task: FILE_ENV: "./ci/test/00_setup_env_native_asan.sh" task: - name: '[no depends, sanitizers: fuzzer,address,undefined] [focal]' + name: '[no depends, sanitizers: fuzzer,address,undefined,integer] [focal]' << : *GLOBAL_TASK_TEMPLATE container: image: ubuntu:focal diff --git a/Makefile.am b/Makefile.am index 2bc205a973..887f9ad4ec 100644 --- a/Makefile.am +++ b/Makefile.am @@ -45,8 +45,12 @@ OSX_PLIST=$(top_builddir)/share/qt/Info.plist #not installed DIST_CONTRIB = \ $(top_srcdir)/contrib/debian/copyright \ $(top_srcdir)/contrib/install_db4.sh \ + $(top_srcdir)/test/sanitizer_suppressions/lsan \ + $(top_srcdir)/test/sanitizer_suppressions/tsan \ + $(top_srcdir)/test/sanitizer_suppressions/ubsan \ $(top_srcdir)/contrib/linearize/linearize-data.py \ $(top_srcdir)/contrib/linearize/linearize-hashes.py + DIST_SHARE = \ $(top_srcdir)/share/genbuild.sh \ $(top_srcdir)/share/rpcauth diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_fuzz.sh index a6f7f629a8..ee87aed0c7 100755 --- a/ci/test/00_setup_env_native_fuzz.sh +++ b/ci/test/00_setup_env_native_fuzz.sh @@ -16,4 +16,4 @@ export RUN_UNIT_TESTS=false export RUN_FUNCTIONAL_TESTS=false export RUN_FUZZ_TESTS=true export GOAL="install" -export BITCOIN_CONFIG="--enable-zmq --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined --enable-suppress-external-warnings CC=clang-16 CXX=clang++-16 --with-boost-process" +export BITCOIN_CONFIG="--enable-zmq --disable-ccache --enable-fuzz --with-sanitizers=fuzzer,address,undefined,integer --enable-suppress-external-warnings CC=clang-16 CXX=clang++-16 --with-boost-process" diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index 61559c4cb4..581fb36c53 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -13,9 +13,12 @@ import os import subprocess import sys -def get_fuzz_env(*, target): + +def get_fuzz_env(*, target, source_dir): return { 'FUZZ': target, + 'UBSAN_OPTIONS': + f'suppressions={source_dir}/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1', 'ASAN_OPTIONS': # symbolizer disabled due to https://github.com/google/sanitizers/issues/1364#issuecomment-761072085 'symbolize=0:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1', } @@ -136,7 +139,7 @@ def main(): os.path.join(config["environment"]["BUILDDIR"], 'src', 'test', 'fuzz', 'fuzz'), '-help=1', ], - env=get_fuzz_env(target=test_list_selection[0]), + env=get_fuzz_env(target=test_list_selection[0], source_dir=config['environment']['SRCDIR']), timeout=20, check=True, stderr=subprocess.PIPE, @@ -153,6 +156,7 @@ def main(): if args.generate: return generate_corpus( fuzz_pool=fuzz_pool, + src_dir=config['environment']['SRCDIR'], build_dir=config["environment"]["BUILDDIR"], corpus_dir=args.corpus_dir, targets=test_list_selection, @@ -163,6 +167,7 @@ def main(): fuzz_pool=fuzz_pool, corpus=args.corpus_dir, test_list=test_list_selection, + src_dir=config['environment']['SRCDIR'], build_dir=config["environment"]["BUILDDIR"], merge_dir=args.m_dir, ) @@ -172,6 +177,7 @@ def main(): fuzz_pool=fuzz_pool, corpus=args.corpus_dir, test_list=test_list_selection, + src_dir=config['environment']['SRCDIR'], build_dir=config["environment"]["BUILDDIR"], use_valgrind=args.valgrind, ) @@ -191,7 +197,7 @@ def generate_corpus(*, fuzz_pool, src_dir, build_dir, corpus_dir, targets): ' '.join(command), subprocess.run( command, - env=get_fuzz_env(target=t), + env=get_fuzz_env(target=t, source_dir=src_dir), check=True, stderr=subprocess.PIPE, universal_newlines=True, @@ -212,7 +218,7 @@ def generate_corpus(*, fuzz_pool, src_dir, build_dir, corpus_dir, targets): future.result() -def merge_inputs(*, fuzz_pool, corpus, test_list, build_dir, merge_dir): +def merge_inputs(*, fuzz_pool, corpus, test_list, src_dir, build_dir, merge_dir): logging.info("Merge the inputs from the passed dir into the corpus_dir. Passed dir {}".format(merge_dir)) jobs = [] for t in test_list: @@ -232,7 +238,7 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, build_dir, merge_dir): output = 'Run {} with args {}\n'.format(t, " ".join(args)) output += subprocess.run( args, - env=get_fuzz_env(target=t), + env=get_fuzz_env(target=t, source_dir=src_dir), check=True, stderr=subprocess.PIPE, universal_newlines=True, @@ -245,7 +251,7 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, build_dir, merge_dir): future.result() -def run_once(*, fuzz_pool, corpus, test_list, build_dir, use_valgrind): +def run_once(*, fuzz_pool, corpus, test_list, src_dir, build_dir, use_valgrind): jobs = [] for t in test_list: corpus_path = os.path.join(corpus, t) @@ -262,7 +268,7 @@ def run_once(*, fuzz_pool, corpus, test_list, build_dir, use_valgrind): output = 'Run {} with args {}'.format(t, args) result = subprocess.run( args, - env=get_fuzz_env(target=t), + env=get_fuzz_env(target=t, source_dir=src_dir), stderr=subprocess.PIPE, universal_newlines=True, ) diff --git a/test/sanitizer_suppressions/ubsan b/test/sanitizer_suppressions/ubsan index a306ca62a6..e6ae66d275 100644 --- a/test/sanitizer_suppressions/ubsan +++ b/test/sanitizer_suppressions/ubsan @@ -89,6 +89,7 @@ implicit-signed-integer-truncation:streams.h implicit-signed-integer-truncation:test/arith_uint256_tests.cpp implicit-signed-integer-truncation:test/skiplist_tests.cpp implicit-signed-integer-truncation:torcontrol.cpp +implicit-unsigned-integer-truncation:*/include/c++/ implicit-unsigned-integer-truncation:crypto/ implicit-unsigned-integer-truncation:leveldb/ implicit-unsigned-integer-truncation:test/fuzz/crypto_diff_fuzz_chacha20.cpp From 292861a9bcc089a5f4e3e44fdd90cc0ba9b7a466 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 1 Feb 2021 10:56:19 +0100 Subject: [PATCH 04/11] Merge #20868: validation: remove redundant check on pindex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit c943282b5e6312537f885c811d43120ce2f5b766 validation: remove redundant check on pindex (jarolrod) Pull request description: This removes a redundant check on `pindex` being a `nullptr`. By the time we get to this step `pindex` is always a `nullptr` as the branch where it has been set would have already returned. Closes #19223 ACKs for top commit: Zero-1729: re-ACK c943282 ajtowns: ACK c943282b5e6312537f885c811d43120ce2f5b766 - code review only MarcoFalke: review ACK c943282b5e6312537f885c811d43120ce2f5b766 📨 theStack: re-ACK c943282b5e6312537f885c811d43120ce2f5b766 Tree-SHA512: d2dc58206be61d2897b0703ee93af67abed492a80e84ea03dcfbf7116833173da3bdafa18ff80422d5296729d5254d57cc1db03fdaf817c8f57c62c3abef673c --- src/validation.cpp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index a5faa07ef4..909752877b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -4044,13 +4044,12 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS // Check for duplicate uint256 hash = block.GetHash(); BlockMap::iterator miSelf = m_block_index.find(hash); - CBlockIndex *pindex = nullptr; // TODO : ENABLE BLOCK CACHE IN SPECIFIC CASES if (hash != chainparams.GetConsensus().hashGenesisBlock) { if (miSelf != m_block_index.end()) { // Block header is already known. - pindex = miSelf->second; + CBlockIndex* pindex = miSelf->second; if (ppindex) *ppindex = pindex; if (pindex->nStatus & BLOCK_FAILED_MASK) { @@ -4133,15 +4132,14 @@ bool BlockManager::AcceptBlockHeader(const CBlockHeader& block, BlockValidationS } if (llmq::chainLocksHandler->HasConflictingChainLock(pindexPrev->nHeight + 1, hash)) { - if (pindex == nullptr) { + if (miSelf == m_block_index.end()) { AddToBlockIndex(block, hash, BLOCK_CONFLICT_CHAINLOCK); } LogPrintf("ERROR: %s: header %s conflicts with chainlock\n", __func__, hash.ToString()); return state.Invalid(BlockValidationResult::BLOCK_CHAINLOCK, "bad-chainlock"); } } - if (pindex == nullptr) - pindex = AddToBlockIndex(block, hash); + CBlockIndex* pindex = AddToBlockIndex(block, hash); if (ppindex) *ppindex = pindex; From 5ec99ff3b424c208db9bf00889683315751335de Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Thu, 4 Feb 2021 09:11:54 +0100 Subject: [PATCH 05/11] Merge #20715: util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet fa61b9d1a68820758f9540653920deaeae6abe79 util: Add ArgsManager::GetCommand() and use it in bitcoin-wallet (MarcoFalke) 7777105a24a36b62df35d12ecf6c6370671568c8 refactor: Move all command dependend checks to ExecuteWalletToolFunc (MarcoFalke) fa06bce4ac17f93decd4ee38c956e7aa55983f0d test: Add tests (MarcoFalke) fac05ccdade8b34c969b9cd9b37b355bc0aabf9c wallet: [refactor] Pass ArgsManager to WalletAppInit (MarcoFalke) Pull request description: This not only moves the parsing responsibility out from the wallet tool, but it also makes it easier to implement bitcoin-util #19937 Fixes: #20902 ACKs for top commit: ajtowns: ACK fa61b9d1a68820758f9540653920deaeae6abe79 fjahr: Code review ACK fa61b9d1a68820758f9540653920deaeae6abe79 Tree-SHA512: 79622b806e8bf9dcd0dc24a8a6687345710df57720992e83a41cd8d6762a6dc112044ebc58fcf6e8fbf45de29a79b04873c5b8c2494a1eaaf902a2884703e47b --- src/bitcoin-wallet.cpp | 63 +++++++++++++--------------------- src/test/fuzz/system.cpp | 2 +- src/util/system.cpp | 52 +++++++++++++++++++++++++++- src/util/system.h | 22 ++++++++++++ src/wallet/wallettool.cpp | 10 ++++-- src/wallet/wallettool.h | 2 +- test/functional/tool_wallet.py | 8 +++-- 7 files changed, 111 insertions(+), 48 deletions(-) diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp index e4e9247a78..176b455ce9 100644 --- a/src/bitcoin-wallet.cpp +++ b/src/bitcoin-wallet.cpp @@ -34,54 +34,53 @@ static void SetupWalletToolArgs(ArgsManager& argsman) argsman.AddArg("-format=", "The format of the wallet file to create. Either \"bdb\" or \"sqlite\". Only used with 'createfromdump'", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -debug is true, 0 otherwise).", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); - argsman.AddArg("info", "Get wallet info", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); - argsman.AddArg("create", "Create new wallet file", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); - - // Hidden - argsman.AddArg("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); - argsman.AddArg("wipetxes", "Wipe all transactions from a wallet", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); - argsman.AddArg("dump", "Print out all of the wallet key-value records", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); - argsman.AddArg("createfromdump", "Create new wallet file from dumped records", ArgsManager::ALLOW_ANY, OptionsCategory::COMMANDS); + argsman.AddCommand("info", "Get wallet info", OptionsCategory::COMMANDS); + argsman.AddCommand("create", "Create new wallet file", OptionsCategory::COMMANDS); + argsman.AddCommand("salvage", "Attempt to recover private keys from a corrupt wallet. Warning: 'salvage' is experimental.", OptionsCategory::COMMANDS); + argsman.AddCommand("wipetxes", "Wipe all transactions from a wallet", OptionsCategory::COMMANDS); + argsman.AddCommand("dump", "Print out all of the wallet key-value records", OptionsCategory::COMMANDS); + argsman.AddCommand("createfromdump", "Create new wallet file from dumped records", OptionsCategory::COMMANDS); } -static bool WalletAppInit(int argc, char* argv[]) +static bool WalletAppInit(ArgsManager& args, int argc, char* argv[]) { - SetupWalletToolArgs(gArgs); + SetupWalletToolArgs(args); std::string error_message; - if (!gArgs.ParseParameters(argc, argv, error_message)) { + if (!args.ParseParameters(argc, argv, error_message)) { tfm::format(std::cerr, "Error parsing command line arguments: %s\n", error_message); return false; } - if (argc < 2 || HelpRequested(gArgs) || gArgs.IsArgSet("-version")) { + if (argc < 2 || HelpRequested(args) || args.IsArgSet("-version")) { std::string strUsage = strprintf("%s dash-wallet version", PACKAGE_NAME) + " " + FormatFullVersion() + "\n"; - if (!gArgs.IsArgSet("-version")) { - strUsage += "\n" + if (!args.IsArgSet("-version")) { + strUsage += "\n" "dash-wallet is an offline tool for creating and interacting with " PACKAGE_NAME " wallet files.\n" "By default dash-wallet will act on wallets in the default mainnet wallet directory in the datadir.\n" "To change the target wallet, use the -datadir, -wallet and -testnet/-regtest arguments.\n\n" "Usage:\n" " dash-wallet [options] \n"; - strUsage += "\n" + gArgs.GetHelpMessage(); - } + strUsage += "\n" + args.GetHelpMessage(); + } tfm::format(std::cout, "%s", strUsage); return false; } // check for printtoconsole, allow -debug - LogInstance().m_print_to_console = gArgs.GetBoolArg("-printtoconsole", gArgs.GetBoolArg("-debug", false)); + LogInstance().m_print_to_console = args.GetBoolArg("-printtoconsole", args.GetBoolArg("-debug", false)); if (!CheckDataDirOption()) { - tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", gArgs.GetArg("-datadir", "")); + tfm::format(std::cerr, "Error: Specified data directory \"%s\" does not exist.\n", args.GetArg("-datadir", "")); return false; } // Check for -testnet or -regtest parameter (Params() calls are only valid after this clause) - SelectParams(gArgs.GetChainName()); + SelectParams(args.GetChainName()); return true; } MAIN_FUNCTION { + ArgsManager& args = gArgs; #ifdef WIN32 util::WinCmdLineArgs winArgs; std::tie(argc, argv) = winArgs.get(); @@ -89,38 +88,24 @@ MAIN_FUNCTION SetupEnvironment(); RandomInit(); try { - if (!WalletAppInit(argc, argv)) return EXIT_FAILURE; + if (!WalletAppInit(args, argc, argv)) return EXIT_FAILURE; } catch (...) { PrintExceptionContinue(std::current_exception(), "WalletAppInit()"); return EXIT_FAILURE; } - std::string method {}; - for(int i = 1; i < argc; ++i) { - if (!IsSwitchChar(argv[i][0])) { - if (!method.empty()) { - tfm::format(std::cerr, "Error: two methods provided (%s and %s). Only one method should be provided.\n", method, argv[i]); - return EXIT_FAILURE; - } - method = argv[i]; - } - } - - if (method.empty()) { + const auto command = args.GetCommand(); + if (!command) { tfm::format(std::cerr, "No method provided. Run `dash-wallet -help` for valid methods.\n"); return EXIT_FAILURE; } - - // A name must be provided when creating a file - if (method == "create" && !gArgs.IsArgSet("-wallet")) { - tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); + if (command->args.size() != 0) { + tfm::format(std::cerr, "Error: Additional arguments provided (%s). Methods do not take arguments. Please refer to `-help`.\n", Join(command->args, ", ")); return EXIT_FAILURE; } - std::string name = gArgs.GetArg("-wallet", ""); - ECC_Start(); - if (!WalletTool::ExecuteWalletToolFunc(gArgs, method, name)) { + if (!WalletTool::ExecuteWalletToolFunc(args, command->command)) { return EXIT_FAILURE; } ECC_Stop(); diff --git a/src/test/fuzz/system.cpp b/src/test/fuzz/system.cpp index 219175ed66..8a77e46d4f 100644 --- a/src/test/fuzz/system.cpp +++ b/src/test/fuzz/system.cpp @@ -55,7 +55,7 @@ FUZZ_TARGET(system) if (args_manager.GetArgFlags(argument_name) != std::nullopt) { return; } - args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeIntegral(), options_category); + args_manager.AddArg(argument_name, fuzzed_data_provider.ConsumeRandomLengthString(16), fuzzed_data_provider.ConsumeIntegral() & ~ArgsManager::COMMAND, options_category); }, [&] { // Avoid hitting: diff --git a/src/util/system.cpp b/src/util/system.cpp index 00b2f6cf59..fdb2124382 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -15,6 +15,8 @@ #include #include #include +#include +#include #include #include #include @@ -347,8 +349,22 @@ bool ArgsManager::ParseParameters(int argc, const char* const argv[], std::strin key[0] = '-'; #endif - if (key[0] != '-') + if (key[0] != '-') { + if (!m_accept_any_command && m_command.empty()) { + // The first non-dash arg is a registered command + std::optional flags = GetArgFlags(key); + if (!flags || !(*flags & ArgsManager::COMMAND)) { + error = strprintf("Invalid command '%s'", argv[i]); + return false; + } + } + m_command.push_back(key); + while (++i < argc) { + // The remaining args are command args + m_command.push_back(argv[i]); + } break; + } // Transform --foo to -foo if (key.length() > 1 && key[1] == '-') @@ -471,6 +487,26 @@ void ArgsManager::ClearPathCache() m_cached_blocks_path = fs::path(); } +std::optional ArgsManager::GetCommand() const +{ + Command ret; + LOCK(cs_args); + auto it = m_command.begin(); + if (it == m_command.end()) { + // No command was passed + return std::nullopt; + } + if (!m_accept_any_command) { + // The registered command + ret.command = *(it++); + } + while (it != m_command.end()) { + // The unregistered command and args (if any) + ret.args.push_back(*(it++)); + } + return ret; +} + std::vector ArgsManager::GetArgs(const std::string& strArg) const { std::vector result; @@ -616,8 +652,22 @@ void ArgsManager::ForceSetArg(const std::string& strArg, const std::string& strV m_settings.forced_settings[SettingName(strArg)] = strValue; } +void ArgsManager::AddCommand(const std::string& cmd, const std::string& help, const OptionsCategory& cat) +{ + Assert(cmd.find('=') == std::string::npos); + Assert(cmd.at(0) != '-'); + + LOCK(cs_args); + m_accept_any_command = false; // latch to false + std::map& arg_map = m_available_args[cat]; + auto ret = arg_map.emplace(cmd, Arg{"", help, ArgsManager::COMMAND}); + Assert(ret.second); // Fail on duplicate commands +} + void ArgsManager::AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat) { + Assert((flags & ArgsManager::COMMAND) == 0); // use AddCommand + // Split arg name from its help param size_t eq_index = name.find('='); if (eq_index == std::string::npos) { diff --git a/src/util/system.h b/src/util/system.h index 1ca9e3d727..d01810ed9f 100644 --- a/src/util/system.h +++ b/src/util/system.h @@ -188,6 +188,7 @@ public: NETWORK_ONLY = 0x200, // This argument's value is sensitive (such as a password). SENSITIVE = 0x400, + COMMAND = 0x800, }; protected: @@ -200,9 +201,11 @@ protected: mutable RecursiveMutex cs_args; util::Settings m_settings GUARDED_BY(cs_args); + std::vector m_command GUARDED_BY(cs_args); std::string m_network GUARDED_BY(cs_args); std::set m_network_only_args GUARDED_BY(cs_args); std::map> m_available_args GUARDED_BY(cs_args); + bool m_accept_any_command GUARDED_BY(cs_args){true}; std::list m_config_sections GUARDED_BY(cs_args); fs::path m_cached_blocks_path GUARDED_BY(cs_args); mutable fs::path m_cached_datadir_path GUARDED_BY(cs_args); @@ -256,6 +259,20 @@ public: */ const std::list GetUnrecognizedSections() const; + struct Command { + /** The command (if one has been registered with AddCommand), or empty */ + std::string command; + /** + * If command is non-empty: Any args that followed it + * If command is empty: The unregistered command and any args that followed it + */ + std::vector args; + }; + /** + * Get the command and command args (returns std::nullopt if no command provided) + */ + std::optional GetCommand() const; + /** * Return the map of all the args passed via the command line */ @@ -377,6 +394,11 @@ public: */ void AddArg(const std::string& name, const std::string& help, unsigned int flags, const OptionsCategory& cat); + /** + * Add subcommand + */ + void AddCommand(const std::string& cmd, const std::string& help, const OptionsCategory& cat); + /** * Add many hidden arguments */ diff --git a/src/wallet/wallettool.cpp b/src/wallet/wallettool.cpp index e3f3fe59b4..82f417dba9 100644 --- a/src/wallet/wallettool.cpp +++ b/src/wallet/wallettool.cpp @@ -115,10 +115,8 @@ static void WalletShowInfo(CWallet* wallet_instance) tfm::format(std::cout, "Address Book: %zu\n", wallet_instance->m_address_book.size()); } -bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, const std::string& name) +bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command) { - const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name); - if (args.IsArgSet("-format") && command != "createfromdump") { tfm::format(std::cerr, "The -format option can only be used with the \"createfromdump\" command.\n"); return false; @@ -131,6 +129,12 @@ bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, tfm::format(std::cerr, "The -descriptors option can only be used with the 'create' command.\n"); return false; } + if (command == "create" && !args.IsArgSet("-wallet")) { + tfm::format(std::cerr, "Wallet name must be provided when creating a new wallet.\n"); + return false; + } + const std::string name = args.GetArg("-wallet", ""); + const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name); if (command == "create") { DatabaseOptions options; diff --git a/src/wallet/wallettool.h b/src/wallet/wallettool.h index f544a6f727..f4516bb5bc 100644 --- a/src/wallet/wallettool.h +++ b/src/wallet/wallettool.h @@ -10,7 +10,7 @@ namespace WalletTool { void WalletShowInfo(CWallet* wallet_instance); -bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, const std::string& file); +bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command); } // namespace WalletTool diff --git a/test/functional/tool_wallet.py b/test/functional/tool_wallet.py index a35686bc53..1e1b773c44 100755 --- a/test/functional/tool_wallet.py +++ b/test/functional/tool_wallet.py @@ -182,11 +182,13 @@ class ToolWalletTest(BitcoinTestFramework): def test_invalid_tool_commands_and_args(self): self.log.info('Testing that various invalid commands raise with specific error messages') - self.assert_raises_tool_error('Invalid command: foo', 'foo') + self.assert_raises_tool_error("Error parsing command line arguments: Invalid command 'foo'", 'foo') # `dash-wallet help` raises an error. Use `dash-wallet -help`. - self.assert_raises_tool_error('Invalid command: help', 'help') - self.assert_raises_tool_error('Error: two methods provided (info and create). Only one method should be provided.', 'info', 'create') + self.assert_raises_tool_error("Error parsing command line arguments: Invalid command 'help'", 'help') + self.assert_raises_tool_error('Error: Additional arguments provided (create). Methods do not take arguments. Please refer to `-help`.', 'info', 'create') self.assert_raises_tool_error('Error parsing command line arguments: Invalid parameter -foo', '-foo') + self.assert_raises_tool_error('No method provided. Run `dash-wallet -help` for valid methods.') + self.assert_raises_tool_error('Wallet name must be provided when creating a new wallet.', 'create') locked_dir = os.path.join(self.options.tmpdir, "node0", self.chain, "wallets") error = 'Error initializing wallet database environment "{}"!'.format(locked_dir) if self.options.descriptors: From e3e69290a20c784880a6be69f8060ed9302c0b48 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Wed, 17 Feb 2021 09:53:14 +0100 Subject: [PATCH 06/11] Merge #21188: scripted-diff: Remove redundant lock annotations in net processing fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 scripted-diff: Remove shadowing lock annotations (MarcoFalke) Pull request description: Would be good to not redundantly copy the lock annotation from the class declaration to the member implementation. Otherwise it may not result in a compile failure if a new lock requirement is added to the member implementation, but not the class declaration. ACKs for top commit: amitiuttarwar: ACK `fafddfadda`, confirmed that the annotations removed were all redundant. confirmed the claim of potential issue :) hebasto: ACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 jonatack: Light utACK fafddfadda0c77876ba764c5b65ee5fa8e53a5e0 verified that the removed annotations in the definitions correspond to those in their respective declarations Tree-SHA512: ea095c6d4e0bedd70d4e2d8a42b06cfd90c161ebfcaac13558c5dc065601a732e5f812f332104b7daa087aa57b8b0242b177799d22eef7628d77d4d87f443bf2 --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a6c9502706..a55e8494bf 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2500,7 +2500,7 @@ void PeerManagerImpl::ProcessGetBlockData(CNode& pfrom, Peer& peer, const CInv& } //! Determine whether or not a peer can request a transaction, and return it (or nullptr if not found or not allowed). -CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main) +CTransactionRef PeerManagerImpl::FindTxForGetData(const CNode* peer, const uint256& txid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) { auto txinfo = m_mempool.info(txid); if (txinfo.tx) { From 91e0359df447c6f86e47496749f111abae85baf1 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 19 Feb 2021 16:47:54 +0800 Subject: [PATCH 07/11] Merge #21205: build: actually fail when Boost is missing c5da2749e2f7375e292fb0982e8e252ae1adbce3 build: actually stop configure if Boost isn't available (fanquake) cad8b527eaf7a93877e2249960866fd4db2d1c14 build: explicitly install libboost-dev package (fanquake) Pull request description: If Boost is not found via AX_BOOST_BASE, we don't actually stop configuring, only a warning is emitted: ```bash checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version MINIMUM_REQUIRED_BOOST or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in . See http://randspringer.de/boost for more documentation. ``` Instead we usually fail when one of the other AX_BOOST_* macros fails to find a library. These macros are slowly being removed, and in any case, it makes more sense to fail earlier if Boost is missing. If Boost is unavailable, the failure now looks like: ```bash checking for boostlib >= 1.58.0 (105800)... configure: We could not detect the boost libraries (version 1.58.0 or higher). If you have a staged boost library (still not installed) please specify $BOOST_ROOT in your environment and do not give a PATH to --with-boost option. If you are sure you have boost installed, then check your version number looking in . See http://randspringer.de/boost for more documentation. configure: error: Boost is not available! ``` Note that we now just pass the version into AX_BOOST_BASE, which fixes it's display in the output (rather than showing `MINIMUM_REQUIRED_BOOST`). This PR also has a commit that adds `libboost-dev` to our install instructions and CI. This package is currently installed as a side-effect of installing our other libboost-*-dev packages. However as those continue to disappear, it makes sense to install boost-dev explicitly. ACKs for top commit: laanwj: Code review ACK c5da2749e2f7375e292fb0982e8e252ae1adbce3 MarcoFalke: Concept ACK c5da2749e2f7375e292fb0982e8e252ae1adbce3 Tree-SHA512: f866062f9d7d3a2316b6c887f17c664b9cfff41fdc0cb99ca79d641240fb01a5ae0d34140e515bc465219e1b43d5ca84f7c55f48b9c5b45a80ff2795dafd072b --- ci/test/00_setup_env_native_asan.sh | 2 +- ci/test/00_setup_env_native_fuzz.sh | 2 +- ci/test/00_setup_env_native_fuzz_with_valgrind.sh | 2 +- ci/test/00_setup_env_native_valgrind.sh | 2 +- doc/build-unix.md | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ci/test/00_setup_env_native_asan.sh b/ci/test/00_setup_env_native_asan.sh index cd5f9c67df..fc143b7e0f 100755 --- a/ci/test/00_setup_env_native_asan.sh +++ b/ci/test/00_setup_env_native_asan.sh @@ -6,7 +6,7 @@ export LC_ALL=C.UTF-8 -export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-filesystem-dev libboost-test-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev" +export PACKAGES="clang llvm python3-zmq qtbase5-dev qttools5-dev-tools libevent-dev bsdmainutils libboost-dev libboost-filesystem-dev libboost-test-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev libqrencode-dev" export NO_DEPENDS=1 export TEST_RUNNER_EXTRA="--timeout-factor=4" # Increase timeout because sanitizers slow down export FUNCTIONAL_TESTS_CONFIG="--exclude wallet_multiwallet.py" # Temporarily suppress ASan heap-use-after-free (see issue #14163) diff --git a/ci/test/00_setup_env_native_fuzz.sh b/ci/test/00_setup_env_native_fuzz.sh index ee87aed0c7..ae27e61a0b 100755 --- a/ci/test/00_setup_env_native_fuzz.sh +++ b/ci/test/00_setup_env_native_fuzz.sh @@ -7,7 +7,7 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_fuzz -export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-filesystem-dev libboost-test-dev" +export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-dev libboost-filesystem-dev libboost-test-dev" export DEP_OPTS="NO_UPNP=1 DEBUG=1" export CPPFLAGS="-DDEBUG_LOCKORDER -DARENA_DEBUG" export CXXFLAGS="-Werror -Wno-unused-command-line-argument -Wno-unused-value -Wno-deprecated-builtins" diff --git a/ci/test/00_setup_env_native_fuzz_with_valgrind.sh b/ci/test/00_setup_env_native_fuzz_with_valgrind.sh index 468235deba..4ba4feea49 100755 --- a/ci/test/00_setup_env_native_fuzz_with_valgrind.sh +++ b/ci/test/00_setup_env_native_fuzz_with_valgrind.sh @@ -7,7 +7,7 @@ export LC_ALL=C.UTF-8 export CONTAINER_NAME=ci_native_fuzz_valgrind -export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev valgrind" +export PACKAGES="clang llvm python3 libevent-dev bsdmainutils libboost-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev valgrind" export NO_DEPENDS=1 export RUN_UNIT_TESTS=false export RUN_FUNCTIONAL_TESTS=false diff --git a/ci/test/00_setup_env_native_valgrind.sh b/ci/test/00_setup_env_native_valgrind.sh index 5a843b3f9b..64dfd64d22 100755 --- a/ci/test/00_setup_env_native_valgrind.sh +++ b/ci/test/00_setup_env_native_valgrind.sh @@ -6,7 +6,7 @@ export LC_ALL=C.UTF-8 -export PACKAGES="valgrind clang llvm python3-zmq libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-test-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev" +export PACKAGES="valgrind clang llvm python3-zmq libevent-dev bsdmainutils libboost-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev libdb5.3++-dev libminiupnpc-dev libzmq3-dev" export USE_VALGRIND=1 export NO_DEPENDS=1 export TEST_RUNNER_EXTRA="--exclude rpc_bind --timeout-factor=4" # Excluded for now, see https://github.com/bitcoin/bitcoin/issues/17765#issuecomment-602068547 diff --git a/doc/build-unix.md b/doc/build-unix.md index 624c3a7d45..eb37c6744c 100644 --- a/doc/build-unix.md +++ b/doc/build-unix.md @@ -74,7 +74,7 @@ Build requirements: Now, you can either build from self-compiled [depends](/depends/README.md) or install the required dependencies: - sudo apt-get libevent-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev + sudo apt-get libevent-dev libboost-dev libboost-system-dev libboost-filesystem-dev libboost-test-dev Berkeley DB is required for the wallet. From 259a767a38ba5b7930b087ce0feb592d546486fd Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Fri, 19 Feb 2021 13:14:15 +0100 Subject: [PATCH 08/11] Merge #21211: test: Move P2WSH_OP_TRUE to shared test library 22220ef6d5f331c9e1f3e9487eaf07ab13693921 test: Move P2WSH_OP_TRUE to shared test library (MarcoFalke) Pull request description: Otherwise it can't be used in other tests (unit, fuzz, bench, ...) ACKs for top commit: darosior: ACK 22220ef6d5f331c9e1f3e9487eaf07ab13693921 Tree-SHA512: 1b636e751281291f7c21ac51c3d014f6a565144c9482974391c516228e756442b077655eda970eb8bdb12974b97855a909b2b60d518026a8d5f41aa15ec7cbc8 --- src/test/util/script.h | 11 +++++++++++ src/test/validation_block_tests.cpp | 15 +++------------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/src/test/util/script.h b/src/test/util/script.h index 215a943b1a..81f94afec0 100644 --- a/src/test/util/script.h +++ b/src/test/util/script.h @@ -5,6 +5,17 @@ #ifndef BITCOIN_TEST_UTIL_SCRIPT_H #define BITCOIN_TEST_UTIL_SCRIPT_H +#include +#include