rpc: Expose block height of wallet transactions #17437

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-11-rpc-blockheight changing 4 files +15 −4
  1. promag commented at 12:43 AM on November 11, 2019: member

    Closes #17296.

  2. fanquake added the label RPC/REST/ZMQ on Nov 11, 2019
  3. promag commented at 12:46 AM on November 11, 2019: member

    I can add a release note to list affected RPC.

    I think I'm also going to add tests for each affected RPC.

  4. MarcoFalke commented at 2:09 AM on November 11, 2019: member

    Misssing update of the help doc

  5. fanquake added the label Waiting for author on Nov 11, 2019
  6. practicalswift commented at 12:42 PM on November 11, 2019: contributor

    Concept ACK

  7. promag force-pushed on Nov 11, 2019
  8. promag commented at 6:01 PM on November 11, 2019: member

    Added release note, tests and updated help doc.

  9. practicalswift commented at 6:08 PM on November 11, 2019: contributor

    ACK 36d904246aa574f65ecea52b546402c2712a1fc3 -- diff looks correct

  10. in test/functional/wallet_listtransactions.py:47 in 36d904246a outdated 
      44 | +        blockhash = self.nodes[0].generate(1)[0]
  45 |          self.sync_all()
  46 |          assert_array_result(self.nodes[0].listtransactions(),
  47 |                              {"txid": txid},
  48 | -                            {"category": "send", "amount": Decimal("-0.1"), "confirmations": 1})
  49 | +                            {"category": "send", "amount": Decimal("-0.1"), "confirmations": 1, "blockhash": blockhash, "blockheight": 202})
    ryanofsky commented at 6:13 PM on November 11, 2019:

    In commit "rpc: Expose block height of wallet transactions" (36d904246aa574f65ecea52b546402c2712a1fc3)

    In these python tests, I think it would be better to get heights programmatically calling node.getblockheader than to hardcode numeric heights. Would make this PR easier to review because it'd be easier to confirm expected height values are correct.Would also make the tests easier to maintain in the future if more code is added to generate blocks.

    promag commented at 6:29 PM on November 11, 2019:

    Done, also replaced the other getblock with getblockheader.

  11. ryanofsky approved
  12. ryanofsky commented at 6:16 PM on November 11, 2019: member

    Code review ACK 36d904246aa574f65ecea52b546402c2712a1fc3. Nice one line code change

  13. promag force-pushed on Nov 11, 2019
  14. promag force-pushed on Nov 11, 2019
  15. in test/functional/wallet_listsinceblock.py:50 in 19d2f1ae10 outdated 
      46 | @@ -47,6 +47,7 @@ def test_no_blockhash(self):
  47 |              "category": "receive",
  48 |              "amount": 1,
  49 |              "blockhash": blockhash,
  50 | +            "blockheight": 102,
    ryanofsky commented at 7:24 PM on November 11, 2019:

    In commit "rpc: Expose block height of wallet transactions" (19d2f1ae1091c94cc45bac3b258221785a454f90)

    Would be better to also get this height from blockhash above instead of hardcoding it, if possible

    promag commented at 9:38 PM on November 11, 2019:

    Ops, sorry for missing this one, fixed.

  16. ryanofsky approved
  17. ryanofsky commented at 7:24 PM on November 11, 2019: member

    Code review ACK 19d2f1ae1091c94cc45bac3b258221785a454f90. Just suggested test changes since last review

  18. promag force-pushed on Nov 11, 2019
  19. in test/functional/wallet_listsinceblock.py:43 in 865051cb1e outdated 
      39 | @@ -40,13 +40,15 @@ def run_test(self):
  40 |      def test_no_blockhash(self):
  41 |          txid = self.nodes[2].sendtoaddress(self.nodes[0].getnewaddress(), 1)
  42 |          blockhash, = self.nodes[2].generate(1)
  43 | +        blockheight = self.nodes[2].getblockheader(blockhash)
    theStack commented at 10:20 PM on November 11, 2019:

    To get the height, the field "height" from the RPC getblockheaders response has to be accessed, which is missing here.

    promag commented at 10:36 PM on November 11, 2019:

    Fixed.

  20. in test/functional/wallet_listsinceblock.py:282 in 865051cb1e outdated 
     277 | @@ -276,7 +278,8 @@ def test_double_send(self):
 278 |          self.sync_all()
 279 |  
 280 |          # gettransaction should work for txid1
 281 | -        self.nodes[0].gettransaction(txid1)
 282 | +        tx1 = self.nodes[0].gettransaction(txid1)
 283 | +        tx1['blockheight'] = self.nodes[0].getblockheader(tx1['blockhash'])
    theStack commented at 10:22 PM on November 11, 2019:

    As above, the access to the field "height" of the RPCs result is missing here.

    promag commented at 10:35 PM on November 11, 2019:

    @ryanofsky we should pack our bags 🍻 fixed.

  21. theStack changes_requested
  22. rpc: Expose block height of wallet transactions a5e77959c8
  23. promag force-pushed on Nov 11, 2019
  24. theStack approved
  25. theStack commented at 12:11 PM on November 12, 2019: member

    ACK a5e77959c8ff6a8bffa1621d7ea29ee8603c5a14

  26. practicalswift commented at 12:39 PM on November 12, 2019: contributor

    ACK a5e77959c8ff6a8bffa1621d7ea29ee8603c5a14 -- diff looks correct now (good catch @theStack!)

  27. laanwj commented at 12:42 PM on November 12, 2019: member

    I expected this to be more expensive, but as it exposes data that is already in the internal data structures anyway, good idea!

  28. promag commented at 12:44 PM on November 12, 2019: member

    Kudos to @ariard and @ryanofsky!

  29. MarcoFalke removed the label Waiting for author on Nov 12, 2019
  30. ryanofsky approved
  31. ryanofsky commented at 6:00 PM on November 12, 2019: member

    Code review ACK a5e77959c8ff6a8bffa1621d7ea29ee8603c5a14. Changes since last review getblockhash python test fixes, and removing the last hardcoded height

  32. MarcoFalke referenced this in commit 1028882eea on Nov 12, 2019
  33. MarcoFalke merged this on Nov 12, 2019
  34. MarcoFalke closed this on Nov 12, 2019
  35. promag deleted the branch on Nov 12, 2019
  36. luke-jr referenced this in commit 80d348a7ff on Nov 15, 2019
  37. luke-jr commented at 8:05 PM on November 17, 2019: member

    Isn't this semi-redundant with "confirmations"?

  38. laanwj commented at 4:07 PM on November 18, 2019: member

    Isn't this semi-redundant with "confirmations"?

    Lacking an atomic way to query the current height at the same time, this is slightly less error prone.

  39. luke-jr referenced this in commit 3a4b664d76 on Jan 5, 2020
  40. MarcoFalke referenced this in commit 965c0c37d5 on Mar 30, 2020
  41. sidhujag referenced this in commit 6ac6d817e7 on Mar 31, 2020
  42. shesek commented at 10:48 AM on June 22, 2020: contributor

    It would be useful if block heights were also included in listunspent (which currently doesn't have the block hash either and includes the number of confirmations only).

    For anyone stumbling here looking for a workaround, I ended up achieving atomicity for the number of confirmations + current block height by querying for the tip first, then issuing listunspent, then verifying the tip stayed foot and retrying it it didn't. With this, you can derive the block height as tip_height-confirmations+1. An implementation of this in Rust can be seen here.

    A similar approach could also be used for listtransactions with versions prior to 0.20. listsinceblock should've been better suited for this, but its not atomic and therefore requires a similar workaround.

  43. jasonbcox referenced this in commit 6098a1cb2b on Oct 1, 2020
  44. Fabcien referenced this in commit 78de14facc on Jan 13, 2021
  45. DrahtBot locked this on Feb 15, 2022
Contributors
promagMarcoFalkepracticalswiftryanofskytheStacklaanwjluke-jrshesek
Labels
RPC/REST/ZMQ
Linked (view graph)
#17296 Missing block height in listsinceblock and gettransaction#17535 test: add block height test to 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