test: Added coverage to Block not found error using gettxoutsetinfo #30340

kevkevinpal · 2024-06-26T01:02:23Z

Description

There were no tests that checked for the Block not found error called in ParseHashOrHeight when using gettxoutsetinfo, this change adds coverage to it.

You can see there are no tests that do the following by doing the below
grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/

which leads to no results

DrahtBot · 2024-06-26T01:02:26Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, kristapsk, brunoerg, alfonsoromanz, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

1alexbc1

Yea

test/functional/rpc_blockchain.py

brunoerg · 2024-06-26T12:03:29Z

Since calling gettxoutsetinfo with block hash depends on coinstatsindex. It would be simpler to address it on feature_coinstatsindex. It would be just one line, and would avoid the node restart and the function reordering.

tdb3

Concept ACK
Left one minor comment.

test/functional/rpc_blockchain.py

kevkevinpal · 2024-06-26T14:13:48Z

Since calling gettxoutsetinfo with block hash depends on coinstatsindex. It would be simpler to address it on feature_coinstatsindex. It would be just one line, and would avoid the node restart and the function reordering.

That makes more sense I went ahead and did so in f169562

test/functional/feature_coinstatsindex.py

alfonsoromanz

tACK 6809ffd. I ran test/functional/feature_coinstatsindex.py, and the test passes.

+1 to informing the log as @tdb3 suggests.

You can now update your PR description to remove the note about the function reordering and the node restart.

kevkevinpal · 2024-06-27T15:31:50Z

thanks @tdb3 and @alfonsoromanz I added a log in 82c454c

Also @tdb3 I tried doing

b1hash = index_node.getblockhash(1)
index_node.invalidateblock(b1hash)

self.log.info("Test obtaining info for a non-existent block hash")
assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)

index_node.reconsiderblock(b1hash)

But it failed, I think its because even though we invalidate the block the node is still able to find it. Where as a random hash will lead to no outcomes

alfonsoromanz

Re ACK 82c454c

tdb3 · 2024-06-28T17:09:19Z

thanks @tdb3 and @alfonsoromanz I added a log in 82c454c

Also @tdb3 I tried doing
b1hash = index_node.getblockhash(1)
index_node.invalidateblock(b1hash)

self.log.info("Test obtaining info for a non-existent block hash")
assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)

index_node.reconsiderblock(b1hash)
But it failed, I think its because even though we invalidate the block the node is still able to find it. Where as a random hash will lead to no outcomes

Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.

Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn't it be better to use something guaranteed not to collide? Something above the max target for regtest?

kevkevinpal · 2024-06-29T22:02:36Z

thanks @tdb3 and @alfonsoromanz I added a log in 82c454c
Also @tdb3 I tried doing
b1hash = index_node.getblockhash(1)
index_node.invalidateblock(b1hash)

self.log.info("Test obtaining info for a non-existent block hash")
assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=b1hash, use_index=True)

index_node.reconsiderblock(b1hash)
But it failed, I think its because even though we invalidate the block the node is still able to find it. Where as a random hash will lead to no outcomes
Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.

Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn't it be better to use something guaranteed not to collide? Something above the max target for regtest?

sounds good that makes sense I just pushed 837af6e and am now using 0000000000000000000195940324e8fbd1f1d9f872cd16980c826930453bf65e from block 850020 on mainnet

tdb3 · 2024-06-30T00:37:28Z

Regarding the hash used for hash_or_height, from what I can tell regtest blocks are allowed a higher target. While using 00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09 (block hash of mainnet block 1000) would be extremely improbably to collide, wouldn't it be better to use something guaranteed not to collide? Something above the max target for regtest?

sounds good that makes sense I just pushed 837af6e and am now using 0000000000000000000195940324e8fbd1f1d9f872cd16980c826930453bf65e from block 850020 on mainnet

I could be off, but the regtest powlimit may be:

bitcoin/src/kernel/chainparams.cpp

Line 423 in 2f6dca4

    
           consensus.powLimit = uint256S("7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");

which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I'm mistaken.

As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff...

kevkevinpal · 2024-06-30T14:48:40Z

I could be off, but the regtest powlimit may be:

bitcoin/src/kernel/chainparams.cpp

Line 423 in 2f6dca4

consensus.powLimit = uint256S("7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff");

which means we would want to use a value higher than that for the test (i.e. a hash that would have no chance of colliding with a generated block). Someone should correct if I'm mistaken.

As a sanity check, I generated 10,000 blocks on regtest and none had a block hash greater than 0x7ff...

Thats a good idea! I set it to ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff since that is larger than 7fffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff and then pushed to 8ec24bd

tdb3

ACK 8ec24bd

kristapsk

ACK 8ec24bd

brunoerg

crACK 8ec24bd

tdb3 · 2024-07-01T23:35:42Z

Hmm, not sure if that is expected behavior or not for gettxoutsetinfo. If the block is invalidated, seems counterintuitive to consider it as a hash_or_height for calculating txout set info. Not sure what piece is missing here, but will dig more.

Following up:

Generated a chain of length 101.
Invalidated block 2.
gettxoutsetinfo with no arguments returns info for the chain (of height 1) as expected.
gettxoutsetinfo with a block height >= 2 returns the expected error message (Target block height X after current typ 1).
gettxoutsetinfo with an explicitly specified hash of a block 2+ returns the info for the chain up to that hash (ignoring invalidation). At first glance this seemed to be a bug, but it seems like there could be an argument to be made that if the user is providing an explicit block hash then they want info including it (even if it is part of a chain that was invalidated). Conversely, if info is not provided because the hash is part of a chain that was invalidated but the node is aware of it, then this also seems a bit off.

alfonsoromanz

Re ACK 8ec24bd

achow101 · 2024-07-02T20:18:22Z

ACK 8ec24bd

…ing gettxoutsetinfo 8ec24bd test: Added coverage to Block not found error using gettxoutsetinfo (kevkevinpal) Pull request description: #### Description There were no tests that checked for the `Block not found` error called in `ParseHashOrHeight` when using `gettxoutsetinfo`, this change adds coverage to it. You can see there are no tests that do the following by doing the below `grep -nri "Block not found.*gettxoutsetinfo" ./test/functional/` which leads to no results ACKs for top commit: achow101: ACK 8ec24bd tdb3: ACK 8ec24bd kristapsk: ACK 8ec24bd brunoerg: crACK 8ec24bd alfonsoromanz: Re ACK 8ec24bd Tree-SHA512: 2c61c681e7304c679cc3d7dd13af1b795780e85716c25c7423d68104e253d01271e048e21bc21be35dbc7ec1a4fde94e439542f3cfd669fe5a16478c5fa982ab

b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script) 745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow) 01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow) 432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script) 1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script) 8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script) f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script) ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script) e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script) df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script) 57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script) e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script) 62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script) 745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow) 4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov) 69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script) ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script) 9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow) 479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script) ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script) 63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script) 3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script) 3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b654479 kwvg: utACK b654479 Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043

DrahtBot added the Tests label Jun 26, 2024

1alexbc1 approved these changes Jun 26, 2024

View reviewed changes

1alexbc1 reviewed Jun 26, 2024

View reviewed changes

EarlTZ approved these changes Jun 26, 2024

View reviewed changes

brunoerg reviewed Jun 26, 2024

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

brunoerg reviewed Jun 26, 2024

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

tdb3 reviewed Jun 26, 2024

View reviewed changes

test/functional/rpc_blockchain.py Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jun 26, 2024

test: add validation for gettxout RPC response #30226

Merged

kevkevinpal force-pushed the testBlockNotFoundUsingGetTxOutSetInfo branch 2 times, most recently from 315f732 to f169562 Compare June 26, 2024 14:12

brunoerg reviewed Jun 26, 2024

View reviewed changes

test/functional/feature_coinstatsindex.py Outdated Show resolved Hide resolved

kevkevinpal force-pushed the testBlockNotFoundUsingGetTxOutSetInfo branch from f169562 to 6809ffd Compare June 26, 2024 15:01

alfonsoromanz reviewed Jun 27, 2024

View reviewed changes

DrahtBot requested a review from tdb3 June 27, 2024 09:57

kevkevinpal force-pushed the testBlockNotFoundUsingGetTxOutSetInfo branch from 6809ffd to 82c454c Compare June 27, 2024 15:28

This comment was marked as spam.

Sign in to view

alfonsoromanz reviewed Jun 28, 2024

View reviewed changes

kevkevinpal force-pushed the testBlockNotFoundUsingGetTxOutSetInfo branch from 82c454c to 837af6e Compare June 29, 2024 22:01

test: Added coverage to Block not found error using gettxoutsetinfo

8ec24bd

kevkevinpal force-pushed the testBlockNotFoundUsingGetTxOutSetInfo branch from 837af6e to 8ec24bd Compare June 30, 2024 14:47

tdb3 approved these changes Jun 30, 2024

View reviewed changes

DrahtBot requested a review from alfonsoromanz June 30, 2024 15:34

kevkevinpal requested a review from brunoerg June 30, 2024 16:03

kristapsk approved these changes Jun 30, 2024

View reviewed changes

brunoerg approved these changes Jul 1, 2024

View reviewed changes

alfonsoromanz approved these changes Jul 2, 2024

View reviewed changes

achow101 merged commit 1e16b10 into bitcoin:master Jul 2, 2024

bitcoin locked and limited conversation to collaborators Jul 2, 2025

test: Added coverage to Block not found error using gettxoutsetinfo #30340

test: Added coverage to Block not found error using gettxoutsetinfo #30340

Uh oh!

Conversation

kevkevinpal commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

DrahtBot commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

1alexbc1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Jun 26, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevkevinpal commented Jun 26, 2024

Uh oh!

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

kevkevinpal commented Jun 27, 2024

Uh oh!

This comment was marked as spam.

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Jun 28, 2024

Uh oh!

kevkevinpal commented Jun 29, 2024

Uh oh!

tdb3 commented Jun 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevkevinpal commented Jun 30, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

kristapsk left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 commented Jul 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alfonsoromanz left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jul 2, 2024

Uh oh!

Uh oh!

kevkevinpal commented Jun 26, 2024 •

edited

Loading

DrahtBot commented Jun 26, 2024 •

edited

Loading

tdb3 commented Jun 30, 2024 •

edited

Loading

tdb3 commented Jul 1, 2024 •

edited

Loading