Skip to content

rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage#24944

Merged
maflcko merged 2 commits intobitcoin:masterfrom
jonatack:getblockfrompeer-param-inputs
Jul 12, 2022
Merged

rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage#24944
maflcko merged 2 commits intobitcoin:masterfrom
jonatack:getblockfrompeer-param-inputs

Conversation

@jonatack
Copy link
Member

@jonatack jonatack commented Apr 22, 2022 •
edited
Loading

The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.

Fix all issues.

@jonatack jonatack mentioned this pull request Apr 22, 2022
Closed
@maflcko
Copy link
Member

maflcko commented Apr 22, 2022

Closes #24943.

Pretty sure the changes here have nothing to do with the issue. The issue report is about bitcoin-cli. Here, you are modifying the server code.

@jonatack
Copy link
Member Author

jonatack commented Apr 22, 2022

Yes. Removed the mention in the description.

maflcko
maflcko reviewed Apr 22, 2022
View reviewed changes
luke-jr
luke-jr approved these changes May 6, 2022
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

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

utACK

w0xlt
w0xlt approved these changes May 9, 2022
View reviewed changes
Copy link
Contributor

@w0xlt w0xlt left a comment

Choose a reason for hiding this comment

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

ACK a926025

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
@jonatack @luke-jr
test: add getblockfrompeer coverage of invalid inputs
93ad98c 
Github-Pull: bitcoin#24944
Rebased-From: 734b966
luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022
@jonatack @luke-jr
rpc: add RPCTypeCheck for getblockfrompeer inputs
4fe12e6 
Github-Pull: bitcoin#24944
Rebased-From: a926025ca822ae1fe480c6b1a2c480d1d4641c06
@jonatack jonatack force-pushed the getblockfrompeer-param-inputs branch from a926025 to 2ef5294 Compare June 27, 2022 11:05
@jonatack
Copy link
Member Author

jonatack commented Jun 27, 2022 •
edited by maflcko
Loading

Thanks @luke-jr and @w0xlt for the reviews, updated per git diff a926025 2ef5294 to take @w0xlt's suggestion.

brunoerg
brunoerg approved these changes Jul 5, 2022
View reviewed changes
Copy link
Contributor

@brunoerg brunoerg left a comment

Choose a reason for hiding this comment

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

ACK 2ef5294

@maflcko
Copy link
Member

maflcko commented Jul 12, 2022 •
edited
Loading

edited comment (#24944 (comment)) due to a bug or feature (?) in github-merge.py:

Merge message contains an @!
---------------------------------------------------------------------------
Merge bitcoin/bitcoin#24944: rpc: add getblockfrompeer RPCTypeCheck and invalid input test coverage

2ef5294a5bb68ceb3797d2638567a172cc21699f rpc: add RPCTypeCheck for getblockfrompeer inputs (Jon Atack)
734b9669ff7b2f5e2820993443a6f868f6b0b20a test: add getblockfrompeer coverage of invalid inputs (Jon Atack)

Pull request description:

  The new getblockfrompeer RPC lacks test coverage for invalid arguments, and its error messages are not harmonized with the existing RPCs.
  
  Fix all issues.


ACKs for top commit:
  jonatack:
    Thanks @luke-jr and @w0xlt for the ACKs, updated per `git diff a926025 2ef5294` to take @w0xlt's suggestion.
  brunoerg:
    ACK 2ef5294a5bb68ceb3797d2638567a172cc21699f

@maflcko
Copy link
Member

maflcko commented Jul 12, 2022

My thinking is that we should make RPCTypeCheck to be run internally and consistently on all methods, not intermittently on only those that it was added to. See also #24944 (comment)

@maflcko
Copy link
Member

maflcko commented Jul 12, 2022

So I am ~0, but happy to merge if reviewers think it is useful.

@maflcko maflcko merged commit 46fcb52 into bitcoin:master Jul 12, 2022
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 12, 2022
Merge bitcoin#24944: rpc: add getblockfrompeer RPCTypeCheck and inval…
acba770 
…id inpu…
@jonatack jonatack deleted the getblockfrompeer-param-inputs branch July 19, 2022 10:29
@jonatack jonatack mentioned this pull request Jul 30, 2022
Merged
@bitcoin bitcoin locked and limited conversation to collaborators Oct 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

4 more reviewers

@maflcko maflcko maflcko left review comments

@luke-jr luke-jr luke-jr approved these changes

@brunoerg brunoerg brunoerg approved these changes

@w0xlt w0xlt w0xlt 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.

6 participants

@jonatack @maflcko @luke-jr @brunoerg @w0xlt @DrahtBot