From afc0224cdbe73e326addf5fb98a3e95d941f2104 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 11:02:59 +0100 Subject: [PATCH 1/7] test: refactor: remove unnecessary blocks_checked counter Since we already store all the blocks in `events`, keeping an additional counter is redundant. --- test/functional/interface_usdt_validation.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/functional/interface_usdt_validation.py b/test/functional/interface_usdt_validation.py index f9d9b525cd3..f32d83a50ca 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,11 +97,10 @@ def __repr__(self): usdt_contexts=[ctx], debug=0) def handle_blockconnected(_, data, __): - nonlocal events, blocks_checked + nonlocal events event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") events.append(event) - blocks_checked += 1 bpf["block_connected"].open_perf_buffer( handle_blockconnected) @@ -127,7 +125,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() From ad90ba36bd930f00753643cd1fe0af72d1c828c2 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 11:05:18 +0100 Subject: [PATCH 2/7] test: refactor: rename inbound to is_inbound Makes it easier to recognize this variable represents a flag. --- test/functional/interface_usdt_net.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/functional/interface_usdt_net.py b/test/functional/interface_usdt_net.py index d1f94637c9d..921af71aa9d 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,7 +133,7 @@ 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 @@ -157,8 +157,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, From f1b99ac94fb77340c4d3a5b4bbc3df28009bc773 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 11:32:10 +0100 Subject: [PATCH 3/7] test: refactor: deduplicate handle_utxocache_* logic Carve out the comparison logic into a helper function to avoid code duplication. --- test/functional/interface_usdt_utxocache.py | 36 +++++++++------------ 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index 5f2ba49026b..7988ff23645 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -258,37 +258,33 @@ def test_add_spent(self): expected_utxocache_spents = [] expected_utxocache_adds = [] + def compare_utxo_with_event(utxo, event): + """Returns 1 if a utxo is identical to the event produced by BPF, otherwise""" + try: + 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) + except AssertionError: + self.log.exception("Assertion failed") + return 0 + else: + return 1 + 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 + handle_add_succeeds += compare_utxo_with_event(add, 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 + handle_spent_succeeds += compare_utxo_with_event(spent, event) bpf["utxocache_add"].open_perf_buffer(handle_utxocache_add) bpf["utxocache_spent"].open_perf_buffer(handle_utxocache_spent) From f5525ad6808df6afc38e5c6e4767ab577e30629c Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 11:57:18 +0100 Subject: [PATCH 4/7] test: store utxocache events By storing the events instead of doing the comparison inside the handle_utxocache_* functions, we simplify the overall logic and potentially making debugging easier, by allowing pdb to access the events. Mostly a refactor, but changes logging behaviour slightly by not raising and not calling self.log.exception("Assertion failed") --- test/functional/interface_usdt_utxocache.py | 50 +++++++++------------ 1 file changed, 20 insertions(+), 30 deletions(-) diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index 7988ff23645..aa4216942d0 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -252,39 +252,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): - """Returns 1 if a utxo is identical to the event produced by BPF, otherwise""" - try: - 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) - except AssertionError: - self.log.exception("Assertion failed") - return 0 - else: - return 1 + """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) - handle_add_succeeds += compare_utxo_with_event(add, event) + 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) - handle_spent_succeeds += compare_utxo_with_event(spent, event) + actual_utxocache_spents.append(event) bpf["utxocache_add"].open_perf_buffer(handle_utxocache_add) bpf["utxocache_spent"].open_perf_buffer(handle_utxocache_spent) @@ -320,19 +311,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. From 326db63a6819813db55ba0d01ab4fe80f7a0d818 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 12:00:39 +0100 Subject: [PATCH 5/7] test: log sanity check assertion failures --- test/functional/interface_usdt_utxocache.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index aa4216942d0..2fc5981451c 100755 --- a/test/functional/interface_usdt_utxocache.py +++ b/test/functional/interface_usdt_utxocache.py @@ -353,9 +353,13 @@ def handle_utxocache_flush(_, data, __): "size": event.size }) # 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) From bc432704505eb165dd86de39ea3434c6fb7a2514 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 12:01:12 +0100 Subject: [PATCH 6/7] test: refactor: remove unnecessary nonlocal Since we're only mutating, and not reassigning, we don't need to declare `events` as `nonlocal`. --- test/functional/interface_usdt_net.py | 1 - test/functional/interface_usdt_validation.py | 1 - 2 files changed, 2 deletions(-) diff --git a/test/functional/interface_usdt_net.py b/test/functional/interface_usdt_net.py index 921af71aa9d..e15ac3c1f2a 100755 --- a/test/functional/interface_usdt_net.py +++ b/test/functional/interface_usdt_net.py @@ -139,7 +139,6 @@ def check_p2p_message(event, is_inbound): checked_outbound_version_msg += 1 def handle_inbound(_, data, __): - nonlocal events event = ctypes.cast(data, ctypes.POINTER(P2PMessage)).contents events.append((event, True)) diff --git a/test/functional/interface_usdt_validation.py b/test/functional/interface_usdt_validation.py index f32d83a50ca..e29b2c46ebb 100755 --- a/test/functional/interface_usdt_validation.py +++ b/test/functional/interface_usdt_validation.py @@ -97,7 +97,6 @@ def __repr__(self): usdt_contexts=[ctx], debug=0) def handle_blockconnected(_, data, __): - nonlocal events event = ctypes.cast(data, ctypes.POINTER(Block)).contents self.log.info(f"handle_blockconnected(): {event}") events.append(event) From 9f55773a370a0d039e727445ccee6b84e05f562a Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 23 Jun 2023 12:05:34 +0100 Subject: [PATCH 7/7] test: refactor: usdt_mempool: store all events Even though we expect these functions to only produce one event, we still keep a counter to check if that's true. By simply storing all the events, we can remove the counters and make debugging easier, by allowing pdb to access the events. --- test/functional/interface_usdt_mempool.py | 44 +++++++++-------------- 1 file changed, 16 insertions(+), 28 deletions(-) diff --git a/test/functional/interface_usdt_mempool.py b/test/functional/interface_usdt_mempool.py index f138fa44cc5..208b065c34e 100755 --- a/test/functional/interface_usdt_mempool.py +++ b/test/functional/interface_usdt_mempool.py @@ -137,9 +137,7 @@ def added_test(self): """Add a transaction to the mempool and make sure the tracepoint returns the expected txid, vsize, and fee.""" - EXPECTED_ADDED_EVENTS = 1 - handled_added_events = 0 - event = None + events = [] self.log.info("Hooking into mempool:added tracepoint...") node = self.nodes[0] @@ -148,9 +146,7 @@ def added_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_added_event(_, data, __): - nonlocal event, handled_added_events - event = bpf["added_events"].event(data) - handled_added_events += 1 + events.append(bpf["added_events"].event(data)) bpf["added_events"].open_perf_buffer(handle_added_event) @@ -165,7 +161,8 @@ def handle_added_event(_, data, __): self.generate(node, 1) self.log.info("Ensuring mempool:added event was handled successfully...") - assert_equal(EXPECTED_ADDED_EVENTS, handled_added_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), tx["txid"]) assert_equal(event.vsize, tx["tx"].get_vsize()) assert_equal(event.fee, fee) @@ -177,9 +174,7 @@ def removed_test(self): """Expire a transaction from the mempool and make sure the tracepoint returns the expected txid, expiry reason, vsize, and fee.""" - EXPECTED_REMOVED_EVENTS = 1 - handled_removed_events = 0 - event = None + events = [] self.log.info("Hooking into mempool:removed tracepoint...") node = self.nodes[0] @@ -188,9 +183,7 @@ def removed_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_removed_event(_, data, __): - nonlocal event, handled_removed_events - event = bpf["removed_events"].event(data) - handled_removed_events += 1 + events.append(bpf["removed_events"].event(data)) bpf["removed_events"].open_perf_buffer(handle_removed_event) @@ -212,7 +205,8 @@ def handle_removed_event(_, data, __): bpf.perf_buffer_poll(timeout=200) self.log.info("Ensuring mempool:removed event was handled successfully...") - assert_equal(EXPECTED_REMOVED_EVENTS, handled_removed_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), txid) assert_equal(event.reason.decode("UTF-8"), "expiry") assert_equal(event.vsize, tx["tx"].get_vsize()) @@ -226,9 +220,7 @@ def replaced_test(self): """Replace one and two transactions in the mempool and make sure the tracepoint returns the expected txids, vsizes, and fees.""" - EXPECTED_REPLACED_EVENTS = 1 - handled_replaced_events = 0 - event = None + events = [] self.log.info("Hooking into mempool:replaced tracepoint...") node = self.nodes[0] @@ -237,9 +229,7 @@ def replaced_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_replaced_event(_, data, __): - nonlocal event, handled_replaced_events - event = bpf["replaced_events"].event(data) - handled_replaced_events += 1 + events.append(bpf["replaced_events"].event(data)) bpf["replaced_events"].open_perf_buffer(handle_replaced_event) @@ -261,7 +251,8 @@ def handle_replaced_event(_, data, __): bpf.perf_buffer_poll(timeout=200) self.log.info("Ensuring mempool:replaced event was handled successfully...") - assert_equal(EXPECTED_REPLACED_EVENTS, handled_replaced_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.replaced_hash)[::-1].hex(), original_tx["txid"]) assert_equal(event.replaced_vsize, original_tx["tx"].get_vsize()) assert_equal(event.replaced_fee, original_fee) @@ -277,9 +268,7 @@ def rejected_test(self): """Create an invalid transaction and make sure the tracepoint returns the expected txid, rejection reason, peer id, and peer address.""" - EXPECTED_REJECTED_EVENTS = 1 - handled_rejected_events = 0 - event = None + events = [] self.log.info("Adding P2P connection...") node = self.nodes[0] @@ -291,9 +280,7 @@ def rejected_test(self): bpf = BPF(text=MEMPOOL_TRACEPOINTS_PROGRAM, usdt_contexts=[ctx], debug=0) def handle_rejected_event(_, data, __): - nonlocal event, handled_rejected_events - event = bpf["rejected_events"].event(data) - handled_rejected_events += 1 + events.append(bpf["rejected_events"].event(data)) bpf["rejected_events"].open_perf_buffer(handle_rejected_event) @@ -305,7 +292,8 @@ def handle_rejected_event(_, data, __): bpf.perf_buffer_poll(timeout=200) self.log.info("Ensuring mempool:rejected event was handled successfully...") - assert_equal(EXPECTED_REJECTED_EVENTS, handled_rejected_events) + assert_equal(1, len(events)) + event = events[0] assert_equal(bytes(event.hash)[::-1].hex(), tx["tx"].hash) assert_equal(event.reason.decode("UTF-8"), "min relay fee not met")