Skip to content

rpc: Expose block height of wallet transactions#17437

Merged
maflcko merged 1 commit intobitcoin:masterfrom
promag:2019-11-rpc-blockheight
Nov 12, 2019
Merged

rpc: Expose block height of wallet transactions#17437
maflcko merged 1 commit intobitcoin:masterfrom
promag:2019-11-rpc-blockheight

Conversation

@promag
Copy link
Contributor

@promag promag commented Nov 11, 2019

Closes #17296.

@promag
Copy link
Contributor Author

promag commented Nov 11, 2019

I can add a release note to list affected RPC.

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

@maflcko
Copy link
Member

maflcko commented Nov 11, 2019

Misssing update of the help doc

@practicalswift
Copy link
Contributor

practicalswift commented Nov 11, 2019

Concept ACK

@promag promag force-pushed the 2019-11-rpc-blockheight branch from 3018a7f to 36d9042 Compare November 11, 2019 17:58
@promag
Copy link
Contributor Author

promag commented Nov 11, 2019

Added release note, tests and updated help doc.

@practicalswift
Copy link
Contributor

practicalswift commented Nov 11, 2019

ACK 36d904246aa574f65ecea52b546402c2712a1fc3 -- diff looks correct

ryanofsky
ryanofsky approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review ACK 36d904246aa574f65ecea52b546402c2712a1fc3. Nice one line code change

@promag promag force-pushed the 2019-11-rpc-blockheight branch 2 times, most recently from d5cfd7a to 19d2f1a Compare November 11, 2019 18:29
ryanofsky
ryanofsky approved these changes Nov 11, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@promag promag force-pushed the 2019-11-rpc-blockheight branch from 19d2f1a to 865051c Compare November 11, 2019 21:37
theStack
theStack suggested changes Nov 11, 2019
View reviewed changes
@promag promag force-pushed the 2019-11-rpc-blockheight branch from 865051c to a5e7795 Compare November 11, 2019 22:32
theStack
theStack approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK a5e7795

@practicalswift
Copy link
Contributor

practicalswift commented Nov 12, 2019

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

@laanwj
Copy link
Member

laanwj commented Nov 12, 2019

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

@promag
Copy link
Contributor Author

promag commented Nov 12, 2019

Kudos to @ariard and @ryanofsky!

ryanofsky
ryanofsky approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

maflcko pushed a commit that referenced this pull request Nov 12, 2019
Merge #17437: rpc: Expose block height of wallet transactions
1028882 
a5e7795 rpc: Expose block height of wallet transactions (João Barbosa)

Pull request description:

  Closes #17296.

ACKs for top commit:
  practicalswift:
    ACK a5e7795 -- diff looks correct now (good catch @theStack!)
  theStack:
    ACK a5e7795
  ryanofsky:
    Code review ACK a5e7795. Changes since last review getblockhash python test fixes, and removing the last hardcoded height

Tree-SHA512: 57dcd0e4e7083f34016bf9cf8ef578fbfde49e882b6cd8623dd1c64716e096e62f6177a4c2ed94f5de304e751fe23fb9d11cf107a86fbf0a3c5f539cd2844916
@maflcko maflcko merged commit a5e7795 into bitcoin:master Nov 12, 2019
@promag promag deleted the 2019-11-rpc-blockheight branch November 12, 2019 20:26
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 2019
@promag @luke-jr
rpc: Expose block height of wallet transactions
80d348a 
Github-Pull: bitcoin#17437
Rebased-From: a5e7795
@luke-jr
Copy link
Member

luke-jr commented Nov 17, 2019

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

@laanwj
Copy link
Member

laanwj commented Nov 18, 2019

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.

@jonatack jonatack mentioned this pull request Nov 20, 2019
Closed
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jan 5, 2020
@promag @luke-jr
rpc: Expose block height of wallet transactions
3a4b664 
Github-Pull: bitcoin#17437
Rebased-From: a5e7795
@jonatack jonatack mentioned this pull request Mar 24, 2020
Merged
maflcko pushed a commit that referenced this pull request Mar 30, 2020
Merge #18420: test: listsinceblock block height checks
965c0c3 
83e1d92 test: listsinceblock block height checks (Jon Atack)

Pull request description:

  This is the second commit of #17535.

  This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in #17437. It also cleans up code in the test function without changing or removing existing checks.

ACKs for top commit:
  fjahr:
    tested ACK 83e1d92
  ryanofsky:
    Code review ACK 83e1d92. Nice test improvements!

Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 31, 2020
Merge bitcoin#18420: test: listsinceblock block height checks
6ac6d81 
83e1d92 test: listsinceblock block height checks (Jon Atack)

Pull request description:

  This is the second commit of bitcoin#17535.

  This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in bitcoin#17437. It also cleans up code in the test function without changing or removing existing checks.

ACKs for top commit:
  fjahr:
    tested ACK 83e1d92
  ryanofsky:
    Code review ACK 83e1d92. Nice test improvements!

Tree-SHA512: 92874b49a3bc0236500495f32dfcf683e1971ca3d4c51702c69ed4ce7dfce21273754f02f93d1243d73793701d9fdf49e14b149477cd249cbbd9e4e8d5bd49f8
@MarkLTZ MarkLTZ mentioned this pull request Apr 4, 2020
Closed
@shesek
Copy link
Contributor

shesek commented Jun 22, 2020

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.

jasonbcox pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 1, 2020
@majcosta
[backport#17437] rpc: Expose block height of wallet transactions
6098a1c 
Summary:
a5e77959c8ff6a8bffa1621d7ea29ee8603c5a14 rpc: Expose block height of wallet transactions (João Barbosa)

Pull request description:

  Closes #17296.

---

Backport of Core [[bitcoin/bitcoin#17437 | PR17437]]

Test Plan:
  ninja all check check-functional

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D7712
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 13, 2021
@jonatack @PiRK
test: listsinceblock block height checks
78de14f 
Summary:
> This PR extends a listsinceblock test to check the new transaction 'blockheight' field recently added in [[bitcoin/bitcoin#17437 | PR17437]]. It also cleans up code in the test function without changing or removing existing checks.

This is a backport of Core [[bitcoin/bitcoin#18420 | PR18420]]

Test Plan: `ninja && test/functional/test_runner.py wallet_listsinceblock`

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Subscribers: Fabien

Differential Revision: https://reviews.bitcoinabc.org/D8895
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Missing block height in listsinceblock and gettransaction

9 participants

@promag @maflcko @practicalswift @laanwj @luke-jr @shesek @theStack @ryanofsky @fanquake