What is the expected behavior for hash_table_multimap::find(Link link) behaviour with non existing link argument? about libbitcoin-database HOT 7 CLOSED

I hope you don't mind me creating the issue here. I find it is easier to link to bits of code for the context, and this would have been a giant wall of text on #general at slack.

from libbitcoin-database.

Now that I know a bit more about how pop header works, I want to look at this again and see how I could handle this.

Can we leave it open for a few more days. I'll report back here.

from libbitcoin-database.

I believe the presumption is that links always exist and that therefore failure to dereference a link is undefined behavior. This is achieved by never deleting store elements. This is probably why it’s not been clearly dealt with. However given that the performance cost is low we could certainly provide defined behavior here. Returning a not_found element seems appropriate.

Good work.

from libbitcoin-database.

Hi @evoskuil

Thanks for the encouraging words.

I looked at a way to return a not_found element when trying to read past the end of file, but then I came across how slab_manager does it and record_manager used to do it. There's a comment here https://github.com/libbitcoin/libbitcoin-database/blob/master/include/bitcoin/database/impl/record_manager.ipp#L137 which I repeat here:

// If record >= count() then we should still be within the file. The
// condition implies a block has been unconfirmed while reading it.

As I understand, we no longer remove records from record manager. So, will it make sense to bring back this assertion in the form of record_count < link? The change will resolve the situation that started this issue.

However, looking at the change history I can see there was a commit to take out the call to count() from within get to try to reduce the locks taken. Is that still a concern?

I am proposing to change the get method to include an assertion. Otherwise it is a bit twisted for an hash_table using a record_map to know if it is reading past the eof. It is only the record manager that knows the count. Instead we could change record_manager::get to be:

template <typename Link>
memory_ptr record_manager<Link>::get(Link link) const
{
    BITCOIN_ASSERT_MSG(link < count(), "Read past end of file.");

    // The accessor must remain in scope until the end of the block.
    const auto memory = file_.access();
    memory->increment(header_size_ + link_to_position(link));
    return memory;
}

With the change above, all the tests pass on travis, but I wonder if there is a case that is not handled in the tests yet and requires this assertion to not be there.

To be honest, I am not sure how useful it is to catch the "read beyond end of file" case. So I am quite happy to park this whole issue for now and come back to it, if needed, after looking more at data_base class and blockchain repo.

from libbitcoin-database.

I believe it’s possible for the count to shrink during or as the result of a reorg, in the case where the top height is reduced in either the confirmed or candidate chain. This affects only the height arrays. Otherwise count (logical file size) should never be reduced.

from libbitcoin-database.

Status?

from libbitcoin-database.

Status?

from libbitcoin-database.