test: various USDT functional test cleanups (27831 follow-ups) #27944

1. 
   stickies-v commented at 3:58 pm on June 23, 2023: contributor

   Various cleanups to the USDT functional tests, largely (but not exclusively) follow-ups to #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.

2. test: refactor: remove unnecessary blocks_checked counter

   Since we already store all the blocks in `events`, keeping an
   additional counter is redundant.

   afc0224cdb
3. test: refactor: rename inbound to is_inbound

   Makes it easier to recognize this variable represents a flag.

   ad90ba36bd
4. test: refactor: deduplicate handle_utxocache_* logic

   Carve out the comparison logic into a helper function to avoid
   code duplication.

   f1b99ac94f
5. 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")

   f5525ad680
6. test: log sanity check assertion failures 326db63a68
7. test: refactor: remove unnecessary nonlocal

   Since we're only mutating, and not reassigning, we don't need to
   declare `events` as `nonlocal`.

   bc43270450
8. 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.

   9f55773a37
9. 
   DrahtBot commented at 3:58 pm on June 23, 2023: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	0xB10C
   Concept ACK	virtu

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

10. DrahtBot added the label Tests on Jun 23, 2023
11. fanquake requested review from 0xB10C on Jun 26, 2023
12. 
    0xB10C commented at 7:24 pm on June 27, 2023: contributor

    Thanks for picking this up! Concept ACK. I’ll have a closer look.
13. 
    fanquake commented at 11:27 am on June 28, 2023: member

    cc @virtu
14. 
    virtu commented at 8:10 am on June 29, 2023: contributor

    Concept ACK. Will take a closer look and test next week.
15. 
    russeree commented at 10:33 am on August 23, 2023: contributor

    Tested, still need to review code further.

    

16. 
    0xB10C commented at 4:47 pm on September 9, 2023: contributor

    ACK 9f55773a370a0d039e727445ccee6b84e05f562a. Reviewed the code and ran the USDT interface tests. I stepped through the commits and think all changes are reasonable.
17. DrahtBot removed review request from 0xB10C on Sep 9, 2023
18. fanquake merged this on Sep 10, 2023
19. fanquake closed this on Sep 10, 2023

    ---

20. stickies-v deleted the branch on Sep 10, 2023
21. Frank-GER referenced this in commit a27a634c3d on Sep 19, 2023
22. bitcoin locked this on Sep 9, 2024

Contributors
stickies-v DrahtBot 0xB10C fanquake virtu russeree

Labels
Tests