Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpc: Expose block height of wallet transactions #17437

Merged
merged 1 commit into from Nov 12, 2019

Conversation

@promag
Copy link
Member

promag commented Nov 11, 2019

Closes #17296.

@promag

This comment has been minimized.

Copy link
Member 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.

@MarcoFalke

This comment has been minimized.

Copy link
Member

MarcoFalke commented Nov 11, 2019

Misssing update of the help doc

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 11, 2019

Concept ACK

@promag promag force-pushed the promag:2019-11-rpc-blockheight branch from 3018a7f to 36d9042 Nov 11, 2019
@promag

This comment has been minimized.

Copy link
Member Author

promag commented Nov 11, 2019

Added release note, tests and updated help doc.

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 11, 2019

ACK 36d9042 -- diff looks correct

Copy link
Contributor

ryanofsky left a comment

Code review ACK 36d9042. Nice one line code change

test/functional/wallet_listtransactions.py Outdated Show resolved Hide resolved
@promag promag force-pushed the promag:2019-11-rpc-blockheight branch 2 times, most recently from d5cfd7a to 19d2f1a Nov 11, 2019
Copy link
Contributor

ryanofsky left a comment

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

test/functional/wallet_listsinceblock.py Outdated Show resolved Hide resolved
@promag promag force-pushed the promag:2019-11-rpc-blockheight branch from 19d2f1a to 865051c Nov 11, 2019
test/functional/wallet_listsinceblock.py Outdated Show resolved Hide resolved
test/functional/wallet_listsinceblock.py Outdated Show resolved Hide resolved
@promag promag force-pushed the promag:2019-11-rpc-blockheight branch from 865051c to a5e7795 Nov 11, 2019
Copy link
Contributor

theStack left a comment

ACK a5e7795

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Nov 12, 2019

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

@laanwj

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

promag commented Nov 12, 2019

Kudos to @ariard and @ryanofsky!

Copy link
Contributor

ryanofsky left a comment

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

MarcoFalke added a commit that referenced this pull request Nov 12, 2019
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
@MarcoFalke MarcoFalke merged commit a5e7795 into bitcoin:master Nov 12, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@promag promag deleted the promag:2019-11-rpc-blockheight branch Nov 12, 2019
luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Nov 15, 2019
@luke-jr

This comment has been minimized.

Copy link
Member

luke-jr commented Nov 17, 2019

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

@laanwj

This comment has been minimized.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.