From ea1c3e6708cc9f9813482b1befb6a4fde0425c8d Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 13 Oct 2023 10:16:16 +0200 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#28644: test: Fuzz merge with -use_value_profile=0 for now faa190b1efbdfdb9b12a7bfa7f732b5471a02e64 test: Fuzz merge with -use_value_profile=0 for now (MarcoFalke) Pull request description: Seems odd that this has to be done, but for now there are (unknown) size limits on the qa-assets repo. Also, a larger size means that cloning and iterating over the files takes a longer time. Not sure how to measure the net impact of this, but with some backups reverting this commit, it can be limited on the downside? ACKs for top commit: dergoegge: ACK faa190b1efbdfdb9b12a7bfa7f732b5471a02e64 Tree-SHA512: 9f8b3f4526f60e4ff6fca97859a725d145a8339c216bd15c92fad7e53f84308745fee47727527de459c0245ef9d474a9dc836fee599ab2b556b519bd900b9a33 --- test/fuzz/test_runner.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index 399d9209e280..f5dd73565078 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -# Copyright (c) 2019-2020 The Bitcoin Core developers +# Copyright (c) 2019-present The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Run fuzz test targets. @@ -249,7 +249,11 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, src_dir, fuzz_bin, merge_dir): # [0] https://github.com/bitcoin-core/qa-assets/issues/130#issuecomment-1761760866 '-shuffle=0', '-prefer_small=1', - '-use_value_profile=1', # Also done by oss-fuzz https://github.com/google/oss-fuzz/issues/1406#issuecomment-387790487 + '-use_value_profile=0', + # use_value_profile is enabled by oss-fuzz [0], but disabled for + # now to avoid bloating the qa-assets git repository [1]. + # [0] https://github.com/google/oss-fuzz/issues/1406#issuecomment-387790487 + # [1] https://github.com/bitcoin-core/qa-assets/issues/130#issuecomment-1749075891 os.path.join(corpus, t), os.path.join(merge_dir, t), ] From dc5cc10e55279c0310f97dc82998bd6caf0f8108 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Mon, 16 Oct 2023 13:29:06 +0100 Subject: [PATCH 2/4] Merge bitcoin-core/gui#765: Fix wallet list hover crash on shutdown 8b6470a90652fcffc45b8d7998af7c8ad6251332 gui: disable top bar menu actions during shutdown (furszy) 7066e8996d0ac090535cc97cdcb54a219986460f gui: provide wallet controller context to wallet actions (furszy) Pull request description: Small follow-up to #751. Fixes another crash cause during shutdown. Which occurs when the user hovers over the wallets list. Future Note: This surely happen in other places as well, we should re-work the way we connect signals. Register lambas without any precaution can leave dangling pointers. ACKs for top commit: hebasto: ACK 8b6470a90652fcffc45b8d7998af7c8ad6251332, I've tested each commit separately on macOS Sonoma 14.0 (Apple M1). Tree-SHA512: 6fbd1bcd6717a8c1633beb9371463ed22422f929cccf9b791ee292c5364134c501e099329cf77a06b74a84c64c1c3d22539199ec49ccd74b3950036316c0dab3 --- src/qt/bitcoingui.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/qt/bitcoingui.cpp b/src/qt/bitcoingui.cpp index 249e4d25d751..99a60a2bee0a 100644 --- a/src/qt/bitcoingui.cpp +++ b/src/qt/bitcoingui.cpp @@ -517,7 +517,7 @@ void BitcoinGUI::createActions() connect(usedSendingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedSendingAddresses); connect(usedReceivingAddressesAction, &QAction::triggered, walletFrame, &WalletFrame::usedReceivingAddresses); connect(openAction, &QAction::triggered, this, &BitcoinGUI::openClicked); - connect(m_open_wallet_menu, &QMenu::aboutToShow, [this] { + connect(m_open_wallet_menu, &QMenu::aboutToShow, m_wallet_controller, [this] { m_open_wallet_menu->clear(); for (const std::pair& i : m_wallet_controller->listWalletDir()) { const std::string& path = i.first; @@ -533,7 +533,7 @@ void BitcoinGUI::createActions() continue; } - connect(action, &QAction::triggered, [this, path] { + connect(action, &QAction::triggered, m_wallet_controller, [this, path] { auto activity = new OpenWalletActivity(m_wallet_controller, this); connect(activity, &OpenWalletActivity::opened, this, &BitcoinGUI::setCurrentWallet, Qt::QueuedConnection); connect(activity, &OpenWalletActivity::opened, rpcConsole, &RPCConsole::setCurrentWallet, Qt::QueuedConnection); @@ -545,7 +545,7 @@ void BitcoinGUI::createActions() action->setEnabled(false); } }); - connect(m_restore_wallet_action, &QAction::triggered, [this] { + connect(m_restore_wallet_action, &QAction::triggered, m_wallet_controller, [this] { //: Name of the wallet data file format. QString name_data_file = tr("Wallet Data"); @@ -571,16 +571,16 @@ void BitcoinGUI::createActions() auto backup_file_path = fs::PathFromString(backup_file.toStdString()); activity->restore(backup_file_path, wallet_name.toStdString()); }); - connect(m_close_wallet_action, &QAction::triggered, [this] { + connect(m_close_wallet_action, &QAction::triggered, m_wallet_controller, [this] { m_wallet_controller->closeWallet(walletFrame->currentWalletModel(), this); }); - connect(m_create_wallet_action, &QAction::triggered, [this] { + connect(m_create_wallet_action, &QAction::triggered, m_wallet_controller, [this] { auto activity = new CreateWalletActivity(m_wallet_controller, this); connect(activity, &CreateWalletActivity::created, this, &BitcoinGUI::setCurrentWallet); connect(activity, &CreateWalletActivity::created, rpcConsole, &RPCConsole::setCurrentWallet); activity->create(); }); - connect(m_close_all_wallets_action, &QAction::triggered, [this] { + connect(m_close_all_wallets_action, &QAction::triggered, m_wallet_controller, [this] { m_wallet_controller->closeAllWallets(this); }); @@ -892,7 +892,8 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH m_mask_values_action->setChecked(_clientModel->getOptionsModel()->getOption(OptionsModel::OptionID::MaskValues).toBool()); } else { - if(trayIconMenu) + // Shutdown requested, disable menus + if (trayIconMenu) { // Disable context menu on tray icon trayIconMenu->clear(); @@ -906,6 +907,8 @@ void BitcoinGUI::setClientModel(ClientModel *_clientModel, interfaces::BlockAndH } #endif // ENABLE_WALLET unitDisplayControl->setOptionsModel(nullptr); + // Disable top bar menu actions + appMenuBar->clear(); #ifdef Q_OS_MACOS if(dockIconMenu) From 78967aa9543e45baa708b7f7fb80668812cd18b1 Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 2 Oct 2023 11:18:50 +0200 Subject: [PATCH 3/4] Merge bitcoin/bitcoin#28184: lint: fix custom mypy cache dir setting f9047771d642c5887c752872b6ffbbd974603b35 lint: fix custom mypy cache dir setting (Fabian Jahr) Pull request description: fixes #28183 The custom cache dir for `mypy` can only be set via an environment variable, setting the `MYPY_CACHE_DIR` variable in the program is not sufficient. This error was introduced while translating the shell script to python. See also the mypy documentation: https://mypy.readthedocs.io/en/stable/config_file.html#confval-cache_dir ACKs for top commit: MarcoFalke: lgtm ACK f9047771d642c5887c752872b6ffbbd974603b35 Tree-SHA512: 7e8fb0cd06688129bd46d1afb8647262eb53d0f60b1ef6f288fedaa122d906fb62c9855e8bb0d6c6297d41a87a47d3cec7a00df55a7d033947937dfe23d07ba7 --- test/lint/lint-python.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/test/lint/lint-python.py b/test/lint/lint-python.py index e94c05c7235b..d4c750147e7a 100755 --- a/test/lint/lint-python.py +++ b/test/lint/lint-python.py @@ -9,14 +9,17 @@ """ import os +from pathlib import Path import subprocess import sys from importlib.metadata import metadata, PackageNotFoundError +# Customize mypy cache dir via environment variable +cache_dir = Path(__file__).parent.parent / ".mypy_cache" +os.environ["MYPY_CACHE_DIR"] = str(cache_dir) DEPS = ['flake8', 'lief', 'mypy', 'pyzmq'] -MYPY_CACHE_DIR = f"{os.getenv('BASE_ROOT_DIR', '')}/test/.mypy_cache" FILES_ARGS = ['git', 'ls-files', '--','test/functional/*.py', 'contrib/devtools/*.py', ':(exclude)contrib/devtools/github-merge.py'] EXCLUDE_DIRS = ['src/dashbls/', 'src/immer/'] From 2aa6e19985e035d787af263c726c8cc525b2ea96 Mon Sep 17 00:00:00 2001 From: fanquake Date: Sun, 10 Sep 2023 14:06:17 +0100 Subject: [PATCH 4/4] Merge bitcoin/bitcoin#27944: test: various USDT functional test cleanups (27831 follow-ups) 9f55773a370a0d039e727445ccee6b84e05f562a test: refactor: usdt_mempool: store all events (stickies-v) bc432704505eb165dd86de39ea3434c6fb7a2514 test: refactor: remove unnecessary nonlocal (stickies-v) 326db63a6819813db55ba0d01ab4fe80f7a0d818 test: log sanity check assertion failures (stickies-v) f5525ad6808df6afc38e5c6e4767ab577e30629c test: store utxocache events (stickies-v) f1b99ac94fb77340c4d3a5b4bbc3df28009bc773 test: refactor: deduplicate handle_utxocache_* logic (stickies-v) ad90ba36bd930f00753643cd1fe0af72d1c828c2 test: refactor: rename inbound to is_inbound (stickies-v) afc0224cdbe73e326addf5fb98a3e95d941f2104 test: refactor: remove unnecessary blocks_checked counter (stickies-v) Pull request description: Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to https://github.com/bitcoin/bitcoin/pull/27831#pullrequestreview-1491438045. Except for slightly different logging behaviour in "test: store utxocache events" and "test: log sanity check assertion failures", this is a refactor PR, removing unnecessary code and (imo) making it more readable and maintainable. The rationale for each change is in the corresponding commit message. Note: except for "test: store utxocache events" (which relies on its parent, and I separated into two commits because we may want the parent but not the child), all commits are stand-alone and I'm okay with dropping one/multiple commits if they turn out to be controversial or undesired. ACKs for top commit: 0xB10C: ACK 9f55773a370a0d039e727445ccee6b84e05f562a. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable. Tree-SHA512: 6c37a0265b6c26d4f9552a056a690b8f86f7304bd33b4419febd8b17369cf6af799cb87c16df35d0c2a1b839ad31de24661d4384eafa88816c2051c522fd3bf5 --- test/functional/interface_usdt_net.py | 11 ++-- test/functional/interface_usdt_utxocache.py | 68 +++++++++----------- test/functional/interface_usdt_validation.py | 5 +- 3 files changed, 35 insertions(+), 49 deletions(-) diff --git a/test/functional/interface_usdt_net.py b/test/functional/interface_usdt_net.py index d446c0e5b7a1..d7f6613802de 100755 --- a/test/functional/interface_usdt_net.py +++ b/test/functional/interface_usdt_net.py @@ -121,11 +121,11 @@ def __repr__(self): checked_outbound_version_msg = 0 events = [] - def check_p2p_message(event, inbound): + def check_p2p_message(event, is_inbound): nonlocal checked_inbound_version_msg, checked_outbound_version_msg if event.msg_type.decode("utf-8") == "version": self.log.info( - f"check_p2p_message(): {'inbound' if inbound else 'outbound'} {event}") + f"check_p2p_message(): {'inbound' if is_inbound else 'outbound'} {event}") peer = self.nodes[0].getpeerinfo()[0] msg = msg_version() msg.deserialize(BytesIO(bytes(event.msg[:event.msg_size]))) @@ -133,13 +133,12 @@ def check_p2p_message(event, inbound): assert_equal(peer["addr"], event.peer_addr.decode("utf-8")) assert_equal(peer["connection_type"], event.peer_conn_type.decode("utf-8")) - if inbound: + if is_inbound: checked_inbound_version_msg += 1 else: checked_outbound_version_msg += 1 def handle_inbound(_, data, __): - nonlocal events event = ctypes.cast(data, ctypes.POINTER(P2PMessage)).contents events.append((event, True)) @@ -157,8 +156,8 @@ def handle_outbound(_, data, __): self.log.info( "check receipt and content of in- and outbound version messages") - for event, inbound in events: - check_p2p_message(event, inbound) + for event, is_inbound in events: + check_p2p_message(event, is_inbound) assert_equal(EXPECTED_INOUTBOUND_VERSION_MSG, checked_inbound_version_msg) assert_equal(EXPECTED_INOUTBOUND_VERSION_MSG, diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index 76bddc392ac9..5cc30e990d21 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -254,43 +254,30 @@ def test_add_spent(self): # that the handle_* functions succeeded. EXPECTED_HANDLE_ADD_SUCCESS = 2 EXPECTED_HANDLE_SPENT_SUCCESS = 1 - handle_add_succeeds = 0 - handle_spent_succeeds = 0 - expected_utxocache_spents = [] expected_utxocache_adds = [] + expected_utxocache_spents = [] + + actual_utxocache_adds = [] + actual_utxocache_spents = [] + + def compare_utxo_with_event(utxo, event): + """Compare a utxo dict to the event produced by BPF""" + assert_equal(utxo["txid"], bytes(event.txid[::-1]).hex()) + assert_equal(utxo["index"], event.index) + assert_equal(utxo["height"], event.height) + assert_equal(utxo["value"], event.value) + assert_equal(utxo["is_coinbase"], event.is_coinbase) def handle_utxocache_add(_, data, __): - nonlocal handle_add_succeeds event = ctypes.cast(data, ctypes.POINTER(UTXOCacheChange)).contents self.log.info(f"handle_utxocache_add(): {event}") - add = expected_utxocache_adds.pop(0) - try: - assert_equal(add["txid"], bytes(event.txid[::-1]).hex()) - assert_equal(add["index"], event.index) - assert_equal(add["height"], event.height) - assert_equal(add["value"], event.value) - assert_equal(add["is_coinbase"], event.is_coinbase) - except AssertionError: - self.log.exception("Assertion failed") - else: - handle_add_succeeds += 1 + actual_utxocache_adds.append(event) def handle_utxocache_spent(_, data, __): - nonlocal handle_spent_succeeds event = ctypes.cast(data, ctypes.POINTER(UTXOCacheChange)).contents self.log.info(f"handle_utxocache_spent(): {event}") - spent = expected_utxocache_spents.pop(0) - try: - assert_equal(spent["txid"], bytes(event.txid[::-1]).hex()) - assert_equal(spent["index"], event.index) - assert_equal(spent["height"], event.height) - assert_equal(spent["value"], event.value) - assert_equal(spent["is_coinbase"], event.is_coinbase) - except AssertionError: - self.log.exception("Assertion failed") - else: - handle_spent_succeeds += 1 + actual_utxocache_spents.append(event) bpf["utxocache_add"].open_perf_buffer(handle_utxocache_add) bpf["utxocache_spent"].open_perf_buffer(handle_utxocache_spent) @@ -326,19 +313,18 @@ def handle_utxocache_spent(_, data, __): "is_coinbase": block_index == 0, }) - assert_equal(EXPECTED_HANDLE_ADD_SUCCESS, len(expected_utxocache_adds)) - assert_equal(EXPECTED_HANDLE_SPENT_SUCCESS, - len(expected_utxocache_spents)) - bpf.perf_buffer_poll(timeout=200) - bpf.cleanup() + + assert_equal(EXPECTED_HANDLE_ADD_SUCCESS, len(expected_utxocache_adds), len(actual_utxocache_adds)) + assert_equal(EXPECTED_HANDLE_SPENT_SUCCESS, len(expected_utxocache_spents), len(actual_utxocache_spents)) self.log.info( f"check that we successfully traced {EXPECTED_HANDLE_ADD_SUCCESS} adds and {EXPECTED_HANDLE_SPENT_SUCCESS} spent") - assert_equal(0, len(expected_utxocache_adds)) - assert_equal(0, len(expected_utxocache_spents)) - assert_equal(EXPECTED_HANDLE_ADD_SUCCESS, handle_add_succeeds) - assert_equal(EXPECTED_HANDLE_SPENT_SUCCESS, handle_spent_succeeds) + for expected_utxo, actual_event in zip(expected_utxocache_adds + expected_utxocache_spents, + actual_utxocache_adds + actual_utxocache_spents): + compare_utxo_with_event(expected_utxo, actual_event) + + bpf.cleanup() def test_flush(self): """ Tests the utxocache:flush tracepoint API. @@ -368,9 +354,13 @@ def handle_utxocache_flush(_, data, __): assert_equal(expected["mode"], FLUSHMODE_NAME[event.mode]) possible_cache_sizes.remove(event.size) # fails if size not in set # sanity checks only - assert(event.memory > 0) - assert(event.duration > 0) - handle_flush_succeeds += 1 + try: + assert event.memory > 0 + assert event.duration > 0 + except AssertionError: + self.log.exception("Assertion error") + else: + handle_flush_succeeds += 1 bpf["utxocache_flush"].open_perf_buffer(handle_utxocache_flush) diff --git a/test/functional/interface_usdt_validation.py b/test/functional/interface_usdt_validation.py index 0fe56abe0d51..9e4f7c617965 100755 --- a/test/functional/interface_usdt_validation.py +++ b/test/functional/interface_usdt_validation.py @@ -86,7 +86,6 @@ def __repr__(self): self.duration) BLOCKS_EXPECTED = 2 - blocks_checked = 0 expected_blocks = dict() events = [] @@ -98,7 +97,6 @@ def __repr__(self): usdt_contexts=[ctx], debug=0, cflags=["-Wno-error=implicit-function-declaration"]) def handle_blockconnected(_, data, __): - nonlocal events, blocks_checked event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") block_hash = bytes(event.hash[::-1]).hex() @@ -111,7 +109,6 @@ def handle_blockconnected(_, data, __): # only plausibility checks assert(event.duration > 0) del expected_blocks[block_hash] - blocks_checked += 1 bpf["block_connected"].open_perf_buffer( handle_blockconnected) @@ -136,7 +133,7 @@ def handle_blockconnected(_, data, __): # only plausibility checks assert event.duration > 0 del expected_blocks[block_hash] - assert_equal(BLOCKS_EXPECTED, blocks_checked) + assert_equal(BLOCKS_EXPECTED, len(events)) assert_equal(0, len(expected_blocks)) bpf.cleanup()