Skip to content

Conversation

laanwj
Copy link
Member

@laanwj laanwj commented Apr 27, 2016

I made a silly mistake in an experimental database wrapper where keys were sorted by char instead of uint8_t. As x86 char is signed the sorting (and thus seeking) for the block index database was messed up, resulting in a segfault due to missing records. This should be caught by the tests.

Add a test to catch:

  • Wrong sorting
  • Seeking errors
  • Iteration result not complete
  • Wrong keys/values from iterator

Also add an assertion to CBlockIndex::GetAncestor, as this it the place it will segfault when block index records are missing. An assertion error is easier to diagnose than a pointer crash (although what we really need is a start-up check whether the database is complete otherwise suggest a reindex).

laanwj added 2 commits April 27, 2016 11:11
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
@jonasschnelli
Copy link
Contributor

utACK 6030625

@laanwj
Copy link
Member Author

laanwj commented Apr 28, 2016

I think this needs another check: that shorter keys get sorted before longer keys if they're otherwise the same. I don't think that we use variable-length keys anywhere, at least not within the same key prefix, but it's good to have the sorting be well-defined as lexicographic.

@laanwj
Copy link
Member Author

laanwj commented May 3, 2016

Closing in favor of #7992

@laanwj laanwj closed this May 3, 2016
sipa added a commit that referenced this pull request Jun 2, 2016
269a440 Add test for dbwrapper iterators with same-prefix keys. (Matt Corallo)
6030625 test: Add more thorough test for dbwrapper iterators (Wladimir J. van der Laan)
84c13e7 chain: Add assertion in case of missing records in index db (Wladimir J. van der Laan)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants