Change Details

PR description: > The CBufferedFile object guarantees its user is able to "rewind" the data stream (that's being read from a file) up to a certain number of bytes, as specified by the user in the constructor. This guarantee is not honored due to a bug in the SetPos method. > > Such rewinding is done in LoadExternalBlockFile() (currently the only user of this object), which deserializes a series of CBlock objects. If that function encounters something unexpected in the data stream, which is coming from a blocks/blk00???.dat file, it "rewinds" to an earlier position in the stream to try to get in sync again. The CBufferedFile object does not actually rewind its file offset; it simply repositions its internal offset, nReadPos, to an earlier position within the object's private buffer; this is why there's a limit to how far the user may rewind. > > If LoadExternalBlockFile() needs to rewind (call blkdat.SetPos()), the stream may not be positioned as it should be, causing errors in deserialization. This need to rewind is probably rare, which is likely why this bug hasn't been noticed already. But if this object is used elsewhere in the future, this could be a serious problem, especially as, due to the nature of the bug, the SetPos() sometimes works. > > This PR adds a unit test for CBufferedFile that fails due to this bug. (Until now it has had no unit tests.) The unit test provides good documentation and examples for developers trying to understand LoadExternalBlockFile() and for future users of this object. > > This PR also adds code to throw an exception from the constructor if the rewind argument is not less than the buffer size (since that doesn't make any sense). > > Finally, I discovered that the object is too restrictive in one respect: When the deserialization methods call this object's read method, a check ensures that the number of bytes being requested is less than the size of the buffer (adjusting for the rewind size), else it throws an exception. This restriction is unnecessary; the object being deserialized can be larger than the buffer because multiple reads from disk can satisfy the request. This is a backport of Core PR16577

PR description: > The CBufferedFile object guarantees its user is able to "rewind" the data stream (that's being read from a file) up to a certain number of bytes, as specified by the user in the constructor. This guarantee is not honored due to a bug in the SetPos method. > > Such rewinding is done in LoadExternalBlockFile() (currently the only user of this object), which deserializes a series of CBlock objects. If that function encounters something unexpected in the data stream, which is coming from a blocks/blk00???.dat file, it "rewinds" to an earlier position in the stream to try to get in sync again. The CBufferedFile object does not actually rewind its file offset; it simply repositions its internal offset, nReadPos, to an earlier position within the object's private buffer; this is why there's a limit to how far the user may rewind. > > If LoadExternalBlockFile() needs to rewind (call blkdat.SetPos()), the stream may not be positioned as it should be, causing errors in deserialization. This need to rewind is probably rare, which is likely why this bug hasn't been noticed already. But if this object is used elsewhere in the future, this could be a serious problem, especially as, due to the nature of the bug, the SetPos() sometimes works. > > This PR adds a unit test for CBufferedFile that fails due to this bug. (Until now it has had no unit tests.) The unit test provides good documentation and examples for developers trying to understand LoadExternalBlockFile() and for future users of this object. > > This PR also adds code to throw an exception from the constructor if the rewind argument is not less than the buffer size (since that doesn't make any sense). > > Finally, I discovered that the object is too restrictive in one respect: When the deserialization methods call this object's read method, a check ensures that the number of bytes being requested is less than the size of the buffer (adjusting for the rewind size), else it throws an exception. This restriction is unnecessary; the object being deserialized can be larger than the buffer because multiple reads from disk can satisfy the request. This is a backport of Core [[https://github.com/bitcoin/bitcoin/pull/16577 | PR16577]]

PR description: > The CBufferedFile object guarantees its user is able to "rewind" the data stream (that's being read from a file) up to a certain number of bytes, as specified by the user in the constructor. This guarantee is not honored due to a bug in the SetPos method. > > Such rewinding is done in LoadExternalBlockFile() (currently the only user of this object), which deserializes a series of CBlock objects. If that function encounters something unexpected in the data stream, which is coming from a blocks/blk00???.dat file, it "rewinds" to an earlier position in the stream to try to get in sync again. The CBufferedFile object does not actually rewind its file offset; it simply repositions its internal offset, nReadPos, to an earlier position within the object's private buffer; this is why there's a limit to how far the user may rewind. > > If LoadExternalBlockFile() needs to rewind (call blkdat.SetPos()), the stream may not be positioned as it should be, causing errors in deserialization. This need to rewind is probably rare, which is likely why this bug hasn't been noticed already. But if this object is used elsewhere in the future, this could be a serious problem, especially as, due to the nature of the bug, the SetPos() sometimes works. > > This PR adds a unit test for CBufferedFile that fails due to this bug. (Until now it has had no unit tests.) The unit test provides good documentation and examples for developers trying to understand LoadExternalBlockFile() and for future users of this object. > > This PR also adds code to throw an exception from the constructor if the rewind argument is not less than the buffer size (since that doesn't make any sense). > > Finally, I discovered that the object is too restrictive in one respect: When the deserialization methods call this object's read method, a check ensures that the number of bytes being requested is less than the size of the buffer (adjusting for the rewind size), else it throws an exception. This restriction is unnecessary; the object being deserialized can be larger than the buffer because multiple reads from disk can satisfy the request. This is a backport of Core [[https://github.com/bitcoin/bitcoin/pull/16577 | PR16577]]