test: add block height test to listsinceblock.py #17535

pull jonatack wants to merge 4 commits into bitcoin:master from jonatack:rpc-wallet-blockheight-followups changing 3 files +37 −26
  1. jonatack commented at 4:32 PM on November 20, 2019: member

    Follow-up to PR #17437.

    Add block height test, expected fields tests, logging, and correct header docstring in listsinceblock.py, and improve the release notes.

  2. test: add logging to listsinceblock.py 48d0561980
  3. fanquake added the label Tests on Nov 20, 2019
  4. in test/functional/wallet_listsinceblock.py:115 in 771f41dc67 outdated 
     110 | @@ -111,23 +111,25 @@ def test_reorg(self):
 111 |          senttx = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
 112 |  
 113 |          # generate on both sides
 114 | -        lastblockhash = self.nodes[1].generate(6)[5]
 115 | -        self.nodes[2].generate(7)
 116 | -        self.log.info('lastblockhash=%s' % (lastblockhash))
 117 | +        lastblockhash_nodes_1 = self.nodes[1].generate(6)[5]
 118 | +        lastblockhash_nodes_2 = self.nodes[2].generate(7)[0]
    ariard commented at 4:57 PM on November 20, 2019:

    Isn't firstblockhash more accurate here ?

    jonatack commented at 5:20 PM on November 20, 2019:

    Good point! Done.

  5. ariard commented at 5:01 PM on November 20, 2019: member

    ACK 38e94bc modulo fixing only comment.

    I think you can also harden wallet_reorgsrestore, wallet_listtransactions with block height assertions.

  6. in doc/release-notes-17437.md:6 in 38e94bcb33 outdated 
       0 | @@ -1,5 +1,6 @@
   1 |  Low-level RPC Changes
   2 |  ===
   3 |  
   4 | -- The RPC gettransaction, listtransactions and listsinceblock responses now also
   5 | -includes the height of the block that contains the wallet transaction, if any.
   6 | +- The RPC gettransaction, listtransactions, and listsinceblock responses now
   7 | +include the height of the block that contains the wallet transaction, if any.
   8 | +Please refer to the RPC help of these calls for details.
    promag commented at 5:08 PM on November 20, 2019:

    As usual? :smile:

    jonatack commented at 5:29 PM on November 20, 2019:

    As per @laanwj suggestion in IRC a few months ago: Release notes should refer to the RPC help for details instead of substituting for full documentation. Example: "Please refer to the RPC help of getbalances for details."

    The commit also fixes a grammar error: s/includes/include/

    jonatack commented at 5:30 PM on November 20, 2019:

    (Thanks for reviewing :)

    MarcoFalke commented at 5:45 PM on November 20, 2019:

    Maybe instead of mentioning it in every bullet point, once per heading is enough?

    jonatack commented at 5:59 PM on November 20, 2019:

    Done

  7. jonatack force-pushed on Nov 20, 2019
  8. jonatack commented at 5:21 PM on November 20, 2019: member

    Thanks for the review @ariard. Took your suggestion.

  9. promag commented at 5:22 PM on November 20, 2019: member

    Concept ACK

  10. in test/functional/wallet_listsinceblock.py:132 in da0fd462f6 outdated 
     135 |              if tx['txid'] == senttx:
 136 | -                found = True
 137 | +                found = tx
 138 |                  break
 139 | -        assert found
 140 | +        assert_equal(found['blockheight'],
    practicalswift commented at 5:37 PM on November 20, 2019:

    This will throw NameError if no match is found above? :)

    jonatack commented at 5:45 PM on November 20, 2019:

    Could add found = {'blockheight': 0} if you would prefer it to fail with AssertionError: not(0 == 103) instead of NameError: name 'found' is not defined

    practicalswift commented at 5:49 PM on November 20, 2019:

    Perhaps found = None and then assert found is not None on the line before the assert_equal? :)

    jonatack commented at 5:56 PM on November 20, 2019:

    You've added 2 lines! :smile: ... SGTM, done

    MarcoFalke commented at 5:57 PM on November 20, 2019:

    nit: You can also use next(), as we do in other tests.

    jonatack commented at 10:03 AM on November 21, 2019:

    @MarcoFalke something like this?

    -        found = None
-        for tx in transactions:
-            if tx['txid'] == senttx:
-                found = tx
-                break
-        assert found is not None
+        found = next(tx for tx in transactions if tx['txid'] == senttx)
  11. practicalswift commented at 5:37 PM on November 20, 2019: contributor

    Concept ACK

  12. jonatack force-pushed on Nov 20, 2019
  13. jonatack force-pushed on Nov 20, 2019
  14. jonatack force-pushed on Nov 20, 2019
  15. jonatack force-pushed on Nov 21, 2019
  16. jonatack force-pushed on Nov 21, 2019
  17. in test/functional/wallet_listsinceblock.py:114 in ecbd8a2526 outdated 
     110 | @@ -111,23 +111,22 @@ def test_reorg(self):
 111 |          senttx = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
 112 |  
 113 |          # generate on both sides
 114 | -        lastblockhash = self.nodes[1].generate(6)[5]
 115 | -        self.nodes[2].generate(7)
 116 | -        self.log.info('lastblockhash=%s' % (lastblockhash))
 117 | +        nodes1_last_blockhash = self.nodes[1].generate(6)[5]
    promag commented at 12:39 AM on November 22, 2019:

    ecbd8a2526122fc6bd472b702580ebb221df00d5

    nit, pythonic last [-1]?

    jonatack commented at 9:10 AM on November 22, 2019:

    done

  18. in doc/release-notes-17437.md:4 in 4e206a8938 outdated 
       0 | @@ -1,5 +1,5 @@
   1 |  Low-level RPC Changes
   2 |  ===
   3 |  
   4 | -- The RPC gettransaction, listtransactions and listsinceblock responses now also
   5 | -includes the height of the block that contains the wallet transaction, if any.
   6 | +- The RPC gettransaction, listtransactions, and listsinceblock responses now
    promag commented at 12:42 AM on November 22, 2019:

    4e206a89383eaca83acbbf9533e4aec62fe9fee7

    Comma before "and" is not needed here? TIL both ways are correct but do you think this is more correct?

    jonatack commented at 8:48 AM on November 22, 2019:

    I grew up being taught to not use the Oxford comma, and it could be considered less aesthetic, but it seems to have made a resurgence. It has the benefit of reducing potential confusion in writing that needs to be clear. Without it, meanings of sentences can change. So it can be a good idea to use it for documentation writing. That said, I don't feel strongly about it here :)

    https://www.rd.com/culture/oxford-comma-proper-use/ https://thewritelife.com/is-the-oxford-comma-necessary/ https://thewritepractice.com/why-you-need-to-be-using-oxford-commas/ https://en.wikipedia.org/wiki/Serial_comma

  19. promag commented at 12:47 AM on November 22, 2019: member

    ACK d28907423344a6bafb09ebd422a20cd11ff0cf41, I think 27450e0 could be squashed in ecbd8a2 or d289074.

  20. test: add block height test to listsinceblock.py
    and correct the header docstring.
    fc502da89d
  21. jonatack force-pushed on Nov 22, 2019
  22. jonatack commented at 9:12 AM on November 22, 2019: member

    Thanks for reviewing, @promag! I took your suggestions and squashed the commit.

  23. jonatack commented at 8:47 AM on November 26, 2019: member

    ACK 38e94bc modulo fixing only comment.

    I think you can also harden wallet_reorgsrestore, wallet_listtransactions with block height assertions. @ariard thank you, I will look at those in a follow-up. @practicalswift, @MarcoFalke, any further suggestions here?

  24. jonatack closed this on Dec 6, 2019
  25. jonatack reopened this on Dec 6, 2019
  26. in doc/release-notes.md:85 in 5abe488c61 outdated 
      80 | @@ -81,8 +81,9 @@ Updated settings
  81 |  Updated RPCs
  82 |  ------------
  83 |  
  84 | -Note: some low-level RPC changes mainly useful for testing are described in the
  85 | -Low-level Changes section below.
  86 | +Please refer to the help of the relevant RPC for more details on an update
  87 | +below. Note that some low-level RPC changes mainly useful for testing are
    MarcoFalke commented at 1:39 PM on December 6, 2019:

    nit:

    below. Some low-level RPC changes mainly useful for testing are
    jonatack commented at 4:12 PM on December 6, 2019:

    done

  27. test: add expected fields tests to listsinceblock.py ab71f64c7e
  28. doc: release note improvements 9501a27c99
  29. jonatack force-pushed on Dec 6, 2019
  30. jonatack commented at 4:17 PM on December 6, 2019: member

    Updated the release notes commit with @MarcoFalke's suggestion and rebased; no other changes.

  31. jonatack closed this on Jan 30, 2020
  32. MarcoFalke referenced this in commit ccb2c9e789 on Mar 9, 2020
  33. MarcoFalke referenced this in commit 965c0c37d5 on Mar 30, 2020
  34. sidhujag referenced this in commit 6ac6d817e7 on Mar 31, 2020
  35. DrahtBot locked this on Feb 15, 2022
Contributors
jonatackariardpromagMarcoFalkepracticalswift
Labels
Tests
Linked (view graph)
#17437 rpc: Expose block height of wallet transactions#18306 test: add logging to wallet_listsinceblock.py#18420 test: listsinceblock block height checks
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me