[NC-1561] Remove RLPUtils from RawBlockIterator #143
Conversation
Create BytesValue backed by ByteBuffer, extract some functionality from RLPInput into static helper methods.
Standardize behavior across implementations. Fix bug in ByteBuffer wrapping BytesValue (byte access should include offset).
|
this looks good to me. kind of a tangent but it reminded me of the |
| @@ -32,7 +32,7 @@ public BytesValueRLPInput(final BytesValue value, final boolean lenient) { | |||
|
|
|||
| @Override | |||
| protected byte inputByte(final long offset) { | |||
| return value.get(Math.toIntExact(offset)); | |||
| return value.get(offset); | |||
There was a problem hiding this comment.
This is required in the case of an input larger than Integer.MAX_VALUE. For example, Importing the blockchain with a 160GB file. The Interface needs to support the long offset. However, the bytes lib doesn't support long offsets.
Without this check, the long will be silently cast to an int, causing corrupted data in this implementation.
There was a problem hiding this comment.
I added a get() method to the BytesValue interface that accepts a long: https://github.com/PegaSysEng/pantheon/pull/143/files#diff-07c80cee998cbce23f1072fbe5c729f1R334
| import java.util.Iterator; | ||
| import java.util.NoSuchElementException; | ||
| import java.util.function.Function; | ||
|
|
||
| public final class RawBlockIterator implements Iterator<Block>, Closeable { | ||
| private static final int DEFAULT_INIT_BUFFER_CAPACITY = 2 << 15; |
There was a problem hiding this comment.
optional style: would 1 << 16 or 0x100 be more clear?
There was a problem hiding this comment.
Yeah, I thought this format was pretty weird, but just moved the value around. Will update.
| @@ -31,7 +32,7 @@ | |||
|
|
|||
| private final boolean lenient; | |||
|
|
|||
| protected long size; | |||
| protected long size; // The number of bytes in this rlp-encoded byte string | |||
There was a problem hiding this comment.
optional style: a javadoc comment would turn up un API docs.
| assertLongScalar(-1L, h("0xFFFFFFFFFFFFFFFF")); | ||
| BytesValue bytes = h("0x88FFFFFFFFFFFFFFFF"); | ||
| final RLPInput in = RLP.input(bytes); | ||
| assertThatThrownBy(in::readLongScalar) |
There was a problem hiding this comment.
The new formulation would read:
assertThatExceptionOfType(RLPException.class)
.isThrownBy(in::readLongScalar)
.withMessageStartingWith("long scalar -1 is not non-negative");
| BytesValue bytes = h("0xB901"); | ||
| assertThatThrownBy(() -> RLP.input(bytes)) | ||
| .isInstanceOf(RLPException.class) | ||
| .hasRootCauseInstanceOf(CorruptedRLPInputException.class) |
There was a problem hiding this comment.
withRootCauseInstanceOf in the new API.
PR description
This PR removes
RLPUtilsfromRawBlockIterator, as part of the work to removingRLPUtilsaltogether.In order to remove this utility, I added a
BytesValueclass that wraps aroundjava.nio.ByteBuffer. The purpose of theBytesValueabstraction is to allow our apis to work generically across various types that represent byte strings, so I felt that it was worth expanding the library to support this common type. Adding anotherBytesValuewrapper is pretty straightforward. However, the tests were not set up to run across the variousBytesValueimplementations. So, I spent some time refactoring the tests to get better coverage across the different implementations as well as standardizing some behavior across implementations.In order to support the functionality that was previously provided by
RLPUtils, I also had to refactor the rlp library to extract some functionality fromAbstractRLPInputso that it could be used in a static helper method inRLP.I added tests for
RawBlockIteratorand additional test coverage for the areas of the rlp functionality that were refactored.