test: Flatten miniwallet array and remove random fee in longpoll #26996

1. 
   maflcko commented at 3:46 PM on January 30, 2023: member

   • Using a single MiniWallet is enough.
   • A random fee isn't needed either.
2. 
   DrahtBot commented at 3:46 PM on January 30, 2023: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theStack

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

3. DrahtBot renamed this:
   test: Log when LongpollThread is ending
   test: Log when LongpollThread is ending
   on Jan 30, 2023
4. DrahtBot added the label Tests on Jan 30, 2023
5. maflcko force-pushed on Jan 30, 2023
6. in test/functional/mining_getblocktemplate_longpoll.py:18 in fa6a2be8ef outdated

     12 | @@ -14,8 +13,9 @@
     13 |  
     14 |  
     15 |  class LongpollThread(threading.Thread):
     16 | -    def __init__(self, node):
     17 | +    def __init__(self, node, log):
     18 |          threading.Thread.__init__(self)
     19 | +        self.log = log
   

   ---

   ---

   

   kouloumos commented at 4:54 PM on January 30, 2023:

   Is there an advantage of passing log instead of

           self.log = logging.getLogger('TestFramework')
   

   ---

   

   maflcko commented at 4:48 PM on March 7, 2023:

   Thanks, dropped the commit

7. in test/functional/mining_getblocktemplate_longpoll.py:28 in fa6a2be8ef outdated

     24 | @@ -25,6 +25,7 @@ def __init__(self, node):
     25 |  
     26 |      def run(self):
     27 |          self.node.getblocktemplate({'longpollid': self.longpollid, 'rules': ['segwit']})
     28 | +        self.log.info("LongpollThread ending")
   

   ---

   ---

   

   kouloumos commented at 5:42 PM on January 30, 2023:

   Maybe logging the thread id could add even more value? Something like:

   ---

   

   maflcko commented at 4:49 PM on March 7, 2023:

   Thanks, dropped the commit

8. 
   kouloumos commented at 6:10 PM on January 30, 2023: contributor

   Two questions:

   • Is there an example where logging the end of a thread in such a way could offer an advantage during debugging?
   • Is this a good opportunity to also replace bare asserts with assertion helpers based on #23119? The PRs that are doing that in batches seems to be not moving forward, so I was wondering if opportunistically changing them could be another way.

   Based on my questions above and some comments that I left inline, I did some changes to see in practice what I was thinking. You can find them here (diff).

9. fanquake requested review from theStack on Mar 7, 2023
10. theStack approved
11. 
    theStack commented at 4:20 PM on March 7, 2023: contributor

    ACK fa6a2be8eff9b67e5f6e378c2d5999981804c692

12. 
    maflcko commented at 4:29 PM on March 7, 2023: member

    Sorry, this slipped the inbox. I'll reply to the feedback

13. 
    maflcko commented at 4:31 PM on March 7, 2023: member

    Is there an example where logging the end of a thread in such a way could offer an advantage during debugging?

    Good point. It probably doesn't. It should already be clear from the assertion being hit (or not being hit) whether the thread exited or not.

14. test: Flatten miniwallet array and remove random fee in longpoll fa0abcdafe
15. maflcko force-pushed on Mar 7, 2023
16. maflcko renamed this:
    test: Log when LongpollThread is ending
    test: Flatten miniwallet array and remove random fee in longpoll
    on Mar 7, 2023
17. DrahtBot renamed this:
    test: Flatten miniwallet array and remove random fee in longpoll
    test: Flatten miniwallet array and remove random fee in longpoll
    on Mar 7, 2023
18. fanquake requested review from theStack on Mar 7, 2023
19. fanquake requested review from kouloumos on Mar 7, 2023
20. theStack approved
21. 
    theStack commented at 5:21 PM on March 8, 2023: contributor

    re-ACK fa0abcdafeb74aa7a312095becd55b1cee48cd99

22. fanquake merged this on Mar 8, 2023
23. fanquake closed this on Mar 8, 2023

    ---

24. sidhujag referenced this in commit e9a97c1802 on Mar 8, 2023
25. maflcko deleted the branch on Mar 10, 2023
26. bitcoin locked this on Mar 9, 2024

Contributors

maflckoDrahtBotkouloumostheStack

Review Requested
kouloumos