Skip to content

Conversation

@brunoerg
Copy link
Contributor

This PR adds test coverage for invalid requests (Invalid hash and Unknown filtertype) for /blockfilterheaders in REST functional test.

@brunoerg brunoerg changed the title test: add coverage for invalid requests for blockfilterheaders test: add coverage for invalid requests for blockfilterheaders (REST) Apr 30, 2022
@fanquake fanquake added the Tests label Apr 30, 2022
Copy link
Contributor

@jacobpfickes jacobpfickes left a comment

Choose a reason for hiding this comment

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

ACK f11b8216fe6ec79b23c603ac185095222d0e9ff5

Pulled and verified additions. Great work

Copy link
Member

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

Light ACK f11b8216fe6ec79b23c603ac185095222d0e9ff5, this builds on and is similar to predecessor commit bd52684

Should the logging be updated? e.g.

--- a/test/functional/interface_rest.py
+++ b/test/functional/interface_rest.py
@@ -219,7 +219,7 @@ class RESTTest (BitcoinTestFramework):
 
         self.generate(self.nodes[0], 1)  # generate block to not affect upcoming tests
 
-        self.log.info("Test the /block, /blockhashbyheight and /headers URIs")
+        self.log.info("Test the /block, /blockhashbyheight, /headers, and /blockfilterheaders URIs")
         bb_hash = self.nodes[0].getbestblockhash()

@brunoerg
Copy link
Contributor Author

brunoerg commented May 3, 2022

Should the logging be updated?

@jonatack, yes! Not only because of this PR, but it should be updated anyway because in that range of code there are some tests involving /blockfilterheaders. Nice catch!

@brunoerg brunoerg force-pushed the 2022-04-invalid-blockfilterheaders branch from c817d89 to d1bfe5e Compare May 3, 2022 18:05
@jonatack
Copy link
Member

jonatack commented May 3, 2022

ACK d1bfe5e

Copy link

@vincenzopalazzo vincenzopalazzo left a comment

Choose a reason for hiding this comment

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

ACK d1bfe5e

@maflcko maflcko merged commit 9b42d62 into bitcoin:master May 4, 2022
@w0xlt
Copy link
Contributor

w0xlt commented May 4, 2022

Post-merged ACK d1bfe5e

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 4, 2022
…ckfilterheaders` (REST)

d1bfe5e test: add coverage for invalid requests for `blockfilterheaders` (brunoerg)

Pull request description:

  This PR adds test coverage for invalid requests (`Invalid hash` and `Unknown filtertype`) for `/blockfilterheaders` in REST functional test.

ACKs for top commit:
  jonatack:
    ACK d1bfe5e
  vincenzopalazzo:
    ACK bitcoin@d1bfe5e

Tree-SHA512: 9ab7efe7131296577c60642f95921799cf1dbae9c2aaea6752d2ac9f35a1bcc72b9d742a146c314f82fe1848190a80c88836ab78fc28773ed12e97fa327828e7
@bitcoin bitcoin locked and limited conversation to collaborators May 4, 2023
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.

7 participants