Skip to content

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Nov 20, 2019

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.

@fanquake fanquake added the Tests label Nov 20, 2019
Copy link

@ariard ariard left a comment

Choose a reason for hiding this comment

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

ACK 38e94bc modulo fixing only comment.

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

@jonatack jonatack force-pushed the rpc-wallet-blockheight-followups branch from 38e94bc to da0fd46 Compare November 20, 2019 17:18
Copy link
Member Author

@jonatack jonatack left a comment

Choose a reason for hiding this comment

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

Thanks for the review @ariard. Took your suggestion.

Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Concept ACK

Copy link
Contributor

@practicalswift practicalswift left a comment

Choose a reason for hiding this comment

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

Concept ACK

@jonatack jonatack force-pushed the rpc-wallet-blockheight-followups branch 5 times, most recently from 30eeb56 to d289074 Compare November 21, 2019 10:24
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

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

and correct the header docstring.
@jonatack jonatack force-pushed the rpc-wallet-blockheight-followups branch from d289074 to 5abe488 Compare November 22, 2019 09:10
@jonatack
Copy link
Member Author

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

@jonatack
Copy link
Member Author

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?

@jonatack jonatack closed this Dec 6, 2019
@jonatack jonatack reopened this Dec 6, 2019
@jonatack jonatack force-pushed the rpc-wallet-blockheight-followups branch from 5abe488 to 9501a27 Compare December 6, 2019 16:11
@jonatack
Copy link
Member Author

jonatack commented Dec 6, 2019

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

@jonatack jonatack closed this Jan 30, 2020
maflcko pushed a commit that referenced this pull request Mar 9, 2020
d484279 test: add logging to wallet_listsinceblock.py (Jon Atack)

Pull request description:

  This is the first commit from #17535.

Top commit has no ACKs.

Tree-SHA512: bb4f527a41bca3ffbf69e910311ce7f85dcc7a2be41350b3c653a27f4044f392b7e528f330e9691f497212469f6b16ce263230bb7a919548dd4e3e21cc72142f
maflcko pushed a commit that referenced this pull request Mar 30, 2020
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
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
@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.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants