Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Test] Expand dbwrapper unit test coverage #2313

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

laanwj and others added 6 commits April 13, 2021 15:32
I made a silly mistake in a database wrapper where keys
were sorted by char instead of uint8_t. As x86 char is signed
the sorting for the block index database was messed up, resulting
in a segfault due to missing records.

Add a test to catch:
- Wrong sorting
- Seeking errors
- Iteration result not complete
Use of `sprintf` is seen as a red flag as many of its uses are insecure.
OpenBSD warns about it while compiling, and some modern platforms, e.g.
[cloudlibc from cloudabi](https://github.com/NuxiNL/cloudlibc) don't
even provide it anymore.

Although our uses of these functions are secure, it can't hurt to
replace them anyway. There are only 3 occurences left, all in the
tests.
The LevelDB docs seem to indicate that an iterator will not take
snapshots (even providing instructions on how to do so yourself).
In several of the places we use them, we assume snapshots to have
been taken.

In order to make sure LevelDB doesn't change out from under us
(and to prevent the next person who reads the docs from having the
same fright I did), verify that snapshots are taken in our tests.
@furszy furszy self-assigned this Apr 13, 2021
@furszy furszy added this to the 6.0.0 milestone Apr 13, 2021
@furszy furszy added the Tests label Apr 13, 2021
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Nice! ACK bc3d3fa

@random-zebra
Copy link

Going to merge this one, as it is only adding new test cases.

@random-zebra random-zebra merged commit d1210fa into PIVX-Project:master Apr 23, 2021
@furszy furszy deleted the 2021_dbwrapper_test_update branch November 29, 2022 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants