← Watch feed
qa: Avoid cleanup when exception is raised
Public commit record
68/100 · Adequate
What the developer wrote
Authored by Hodlinator
qa: Avoid cleanup when exception is raised
If an exception is raised before we get to cleanup, do not swallow it and attempt to reset the state. Doing so could trigger knock-on exceptions and extra tracebacks.
✓ Descriptive subject✓ Names a concrete action or component✓ Provides detailed explanatory context
What changed, and why it matters
This commit changes how several Bitcoin Core test scripts clean up after themselves. Previously, cleanup code ran even when a test failed, which could hide the real failure behind extra error messages. Now cleanup is skipped if the test raises an exception, so the original failure is reported clearly. This only affects the project's internal test suite, not the live Bitcoin network or end users.
Recommended action
No security action required. This is a test-quality improvement. Reviewers may verify that skipping cleanup on failure does not leave shared test state in a way that interferes with subsequent tests, but the commit message explains the rationale.
Security signals we found
No strong security signals were identified.
Evidence from the diff
The patch removes try/finally wrappers from six functional test decorators in test/functional/*.py. Previously these decorators ran cleanup steps in a finally block, meaning cleanup executed even when the wrapped test raised an exception. The new behavior lets the exception propagate without running cleanup, avoiding secondary exceptions and confusing tracebacks during test failures. The change is purely in QA/test infrastructure and does not modify node consensus, networking, wallet, or mempool production code.
Changed components
test/functional/mempool_cluster.pytest/functional/mempool_truc.pytest/functional/p2p_opportunistic_1p1c.pytest/functional/p2p_orphan_handling.pytest/functional/wallet_sendall.pytest/functional/wallet_v3_txs.pyInspect captured patch +64 / −70
diff --git a/test/functional/mempool_cluster.py b/test/functional/mempool_cluster.py
index 0dfa6b09..a05377ab 100755
--- a/test/functional/mempool_cluster.py
+++ b/test/functional/mempool_cluster.py
@@ -30,13 +30,12 @@ def weight_to_vsize(weight):
def cleanup(func):
def wrapper(self, *args, **kwargs):
- try:
- func(self, *args, **kwargs)
- finally:
- # Mine blocks to clear the mempool and replenish the wallet's confirmed UTXOs.
- while (len(self.nodes[0].getrawmempool()) > 0):
- self.generate(self.nodes[0], 1)
- self.wallet.rescan_utxos(include_mempool=True)
+ func(self, *args, **kwargs)
+
+ # Mine blocks to clear the mempool and replenish the wallet's confirmed UTXOs.
+ while (len(self.nodes[0].getrawmempool()) > 0):
+ self.generate(self.nodes[0], 1)
+ self.wallet.rescan_utxos(include_mempool=True)
return wrapper
class MempoolClusterTest(BitcoinTestFramework):
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index e3abe039..96de2ca9 100755
--- a/test/functional/mempool_truc.py
+++ b/test/functional/mempool_truc.py
@@ -29,15 +29,14 @@ TRUC_CHILD_MAX_VSIZE = 1000
def cleanup(extra_args=None):
def decorator(func):
def wrapper(self):
- try:
- if extra_args is not None:
- self.restart_node(0, extra_args=extra_args)
- func(self)
- finally:
- # Clear mempool again after test
- self.generate(self.nodes[0], 1)
- if extra_args is not None:
- self.restart_node(0)
+ if extra_args is not None:
+ self.restart_node(0, extra_args=extra_args)
+ func(self)
+
+ # Clear mempool again after test
+ self.generate(self.nodes[0], 1)
+ if extra_args is not None:
+ self.restart_node(0)
return wrapper
return decorator
diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py
index ef4440b9..f1d42d10 100755
--- a/test/functional/p2p_opportunistic_1p1c.py
+++ b/test/functional/p2p_opportunistic_1p1c.py
@@ -58,18 +58,17 @@ GETDATA_WAIT = 60
def cleanup(func):
def wrapper(self, *args, **kwargs):
- try:
- func(self, *args, **kwargs)
- finally:
- self.nodes[0].disconnect_p2ps()
- # Do not clear the node's mempool, as each test requires mempool min feerate > min
- # relay feerate. However, do check that this is the case.
- assert self.nodes[0].getmempoolinfo()["mempoolminfee"] > self.nodes[0].getnetworkinfo()["relayfee"]
- # Ensure we do not try to spend the same UTXOs in subsequent tests, as they will look like RBF attempts.
- self.wallet.rescan_utxos(include_mempool=True)
-
- # Resets if mocktime was used
- self.nodes[0].setmocktime(0)
+ func(self, *args, **kwargs)
+
+ self.nodes[0].disconnect_p2ps()
+ # Do not clear the node's mempool, as each test requires mempool min feerate > min
+ # relay feerate. However, do check that this is the case.
+ assert self.nodes[0].getmempoolinfo()["mempoolminfee"] > self.nodes[0].getnetworkinfo()["relayfee"]
+ # Ensure we do not try to spend the same UTXOs in subsequent tests, as they will look like RBF attempts.
+ self.wallet.rescan_utxos(include_mempool=True)
+
+ # Resets if mocktime was used
+ self.nodes[0].setmocktime(0)
return wrapper
class PackageRelayTest(BitcoinTestFramework):
diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py
index 4cd59f8f..3d94f033 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -50,20 +50,19 @@ TXREQUEST_TIME_SKIP = NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY + OVERLOADED_PEER
def cleanup(func):
def wrapper(self):
- try:
- func(self)
- finally:
- # Clear mempool
- self.generate(self.nodes[0], 1)
- self.nodes[0].disconnect_p2ps()
- # Check that mempool and orphanage have been cleared
- self.wait_until(lambda: len(self.nodes[0].getorphantxs()) == 0)
- assert_equal(0, len(self.nodes[0].getrawmempool()))
-
- self.restart_node(0, extra_args=["-persistmempool=0"])
- # Allow use of bumpmocktime again
- self.nodes[0].setmocktime(int(time.time()))
- self.wallet.rescan_utxos(include_mempool=True)
+ func(self)
+
+ # Clear mempool
+ self.generate(self.nodes[0], 1)
+ self.nodes[0].disconnect_p2ps()
+ # Check that mempool and orphanage have been cleared
+ self.wait_until(lambda: len(self.nodes[0].getorphantxs()) == 0)
+ assert_equal(0, len(self.nodes[0].getrawmempool()))
+
+ self.restart_node(0, extra_args=["-persistmempool=0"])
+ # Allow use of bumpmocktime again
+ self.nodes[0].setmocktime(int(time.time()))
+ self.wallet.rescan_utxos(include_mempool=True)
return wrapper
class PeerTxRelayer(P2PTxInvStore):
diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py
index 0524b5c8..89afae4c 100755
--- a/test/functional/wallet_sendall.py
+++ b/test/functional/wallet_sendall.py
@@ -18,12 +18,11 @@ from test_framework.util import (
# Decorator to reset activewallet to zero utxos
def cleanup(func):
def wrapper(self):
- try:
- func(self)
- finally:
- if 0 < self.wallet.getbalances()["mine"]["trusted"]:
- self.wallet.sendall([self.remainder_target])
- assert_equal(0, self.wallet.getbalances()["mine"]["trusted"]) # wallet is empty
+ func(self)
+
+ if 0 < self.wallet.getbalances()["mine"]["trusted"]:
+ self.wallet.sendall([self.remainder_target])
+ assert_equal(0, self.wallet.getbalances()["mine"]["trusted"]) # wallet is empty
return wrapper
class SendallTest(BitcoinTestFramework):
diff --git a/test/functional/wallet_v3_txs.py b/test/functional/wallet_v3_txs.py
index 6ba30de4..8ba422f2 100755
--- a/test/functional/wallet_v3_txs.py
+++ b/test/functional/wallet_v3_txs.py
@@ -38,28 +38,27 @@ from test_framework.mempool_util import (
# sweep alice and bob's wallets and clear the mempool
def cleanup(func):
def wrapper(self, *args):
- try:
- self.generate(self.nodes[0], 1)
- func(self, *args)
- finally:
- self.generate(self.nodes[0], 1)
- for wallet in [self.alice, self.bob]:
- txs = set(tx["txid"] for tx in wallet.listtransactions("*", 1000) if tx["confirmations"] == 0 and not tx["abandoned"])
- for tx in txs:
- wallet.abandontransaction(tx)
- try:
- wallet.sendall([self.charlie.getnewaddress()])
- except JSONRPCException as e:
- assert "Total value of UTXO pool too low to pay for transaction" in e.error['message']
- self.generate(self.nodes[0], 1)
-
- for wallet in [self.alice, self.bob]:
- balance = wallet.getbalances()["mine"]
- for balance_type in ["untrusted_pending", "trusted", "immature", "nonmempool"]:
- assert_equal(balance[balance_type], 0)
-
- assert_equal(self.alice.getrawmempool(), [])
- assert_equal(self.bob.getrawmempool(), [])
+ self.generate(self.nodes[0], 1)
+ func(self, *args)
+
+ self.generate(self.nodes[0], 1)
+ for wallet in [self.alice, self.bob]:
+ txs = set(tx["txid"] for tx in wallet.listtransactions("*", 1000) if tx["confirmations"] == 0 and not tx["abandoned"])
+ for tx in txs:
+ wallet.abandontransaction(tx)
+ try:
+ wallet.sendall([self.charlie.getnewaddress()])
+ except JSONRPCException as e:
+ assert "Total value of UTXO pool too low to pay for transaction" in e.error['message']
+ self.generate(self.nodes[0], 1)
+
+ for wallet in [self.alice, self.bob]:
+ balance = wallet.getbalances()["mine"]
+ for balance_type in ["untrusted_pending", "trusted", "immature", "nonmempool"]:
+ assert_equal(balance[balance_type], 0)
+
+ assert_equal(self.alice.getrawmempool(), [])
+ assert_equal(self.bob.getrawmempool(), [])
return wrapper
Risk score
Our methodology →Why this scored 15/100
Human-validated context
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
No validated notes yet.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.