refactor: Remove unused nchaintx from SnapshotMetadata constructor, fix test, add test#28639
Merged
fanquake merged 2 commits intobitcoin:masterfrom Oct 13, 2023
Merged
refactor: Remove unused nchaintx from SnapshotMetadata constructor, fix test, add test#28639fanquake merged 2 commits intobitcoin:masterfrom
fanquake merged 2 commits intobitcoin:masterfrom
Conversation
added 2 commits
October 12, 2023 11:14
Also, remove wrong nChainTx comment and cast.
Also, fix a bug in an assert_debug_log call.
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
DrahtBot
changed the title
Sjors
reviewed
Oct 12, 2023
| // Cast required because univalue doesn't have serialization specified for | ||
| // `unsigned int`, nChainTx's type. | ||
| result.pushKV("nchaintx", uint64_t{tip->nChainTx}); | ||
| result.pushKV("nchaintx", tip->nChainTx); |
Member
There was a problem hiding this comment.
faa90f6: was this always wrong or is there some recent change to UniValue?
Sjors
reviewed
Oct 12, 2023
|
|
||
| expected_log = f"assumeutxo height in snapshot metadata not recognized ({bad_snapshot_height}) - refusing to load snapshot" | ||
| with self.nodes[1].assert_debug_log(expected_log): | ||
| with self.nodes[1].assert_debug_log([expected_log]): |
Member
There was a problem hiding this comment.
fafde92: I verified this test indeed would pass regardless of expected_log. Wen type safety?
Member
|
utACK fafde92 |
theStack
approved these changes
Oct 12, 2023
| assert_raises_rpc_error(-32603, "Unable to load UTXO snapshot", self.nodes[1].loadtxoutset, bad_snapshot_path) | ||
|
|
||
| self.log.info(" - snapshot file with wrong number of coins") | ||
| valid_num_coins = struct.unpack("<I", valid_snapshot_contents[32:32 + 4])[0] |
Contributor
There was a problem hiding this comment.
nit: could do the same without the struct module by using int.from_bytes (and int.to_bytes below):
Suggested change
| valid_num_coins = struct.unpack("<I", valid_snapshot_contents[32:32 + 4])[0] | |
| valid_num_coins = int.from_bytes(valid_snapshot_contents[32:32 + 4], 'little') |
(mostly a matter of taste I guess though)
Member
Author
There was a problem hiding this comment.
Thanks, will do in a follow-up
Member
Author
There was a problem hiding this comment.
Also, 4 bytes is wrong. The count is u64, which is closer to 8 bytes.
achow101
added a commit
to bitcoin-core/gui
that referenced
this pull request
Oct 13, 2023
…ugs, add type checks ac4caf3 test: fix `assert_debug_log` call-site bugs, add type checks (Sebastian Falbesoner) Pull request description: Two recently added tests (PR #28625 / commit 2e31250 and PR #28634 / commit 3bb51c2) introduced bugs by wrongly using the `assert_debug_log` helper: https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/feature_assumeutxo.py#L84-L85 (already fixed in bitcoin/bitcoin#28639) https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L148 https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L159 Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. Thanks to maflcko for discovering: bitcoin/bitcoin#28625 (comment) In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in bitcoin/bitcoin#28625 (comment). Using mypy might be an alternative, but I guess it takes quite a bit of effort to properly integrate this into CI for the whole functional test suite (including taking care of false-positives), so I decided to go with the simpler "manual asserts" hack. Suggestions are very welcome of course. ACKs for top commit: achow101: ACK ac4caf3 maflcko: lgtm ACK ac4caf3 dergoegge: ACK ac4caf3 Tree-SHA512: a9677af76a0c370e71f0411339807b1dc6b2a81763db4ec049cd6d766404b916e2bdd002883db5a79c9c388d7d8ebfcbd5f31d43d50be868eeb928e3c906a746
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 13, 2023
…tadata constructor, fix test, add test fafde92 test: Check snapshot file with wrong number of coins (MarcoFalke) faa90f6 refactor: Remove unused nchaintx from SnapshotMetadata constructor (MarcoFalke) Pull request description: See commit messages ACKs for top commit: Sjors: utACK fafde92 theStack: ACK fafde92 Tree-SHA512: 9ed2720b50d1c0938f30543ba143e1a4c6af3a0ff166f8b3eb452e1d99ddee6e3443a4c99f77efe94b8c3eb2feff984bf5259807ee8085e1e0e1e0d1de98227e
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 21, 2023
… type checks ac4caf3 test: fix `assert_debug_log` call-site bugs, add type checks (Sebastian Falbesoner) Pull request description: Two recently added tests (PR bitcoin#28625 / commit 2e31250 and PR bitcoin#28634 / commit 3bb51c2) introduced bugs by wrongly using the `assert_debug_log` helper: https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/feature_assumeutxo.py#L84-L85 (already fixed in bitcoin#28639) https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L148 https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L159 Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. Thanks to maflcko for discovering: bitcoin#28625 (comment) In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in bitcoin#28625 (comment). Using mypy might be an alternative, but I guess it takes quite a bit of effort to properly integrate this into CI for the whole functional test suite (including taking care of false-positives), so I decided to go with the simpler "manual asserts" hack. Suggestions are very welcome of course. ACKs for top commit: achow101: ACK ac4caf3 maflcko: lgtm ACK ac4caf3 dergoegge: ACK ac4caf3 Tree-SHA512: a9677af76a0c370e71f0411339807b1dc6b2a81763db4ec049cd6d766404b916e2bdd002883db5a79c9c388d7d8ebfcbd5f31d43d50be868eeb928e3c906a746
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See commit messages