Ensure we do not decode invalid RLPs

[jochem-brouwer]

There are some cases where we successfully decode invalid RLPs - we should throw in these cases. In this PR, (rather old) tests from ethereum/tests are added, as well some tests from Geth.

This also updates the tests, as they are using deprecated methods like equal which should be strictEqual.

[coveralls]

Coverage increased (+5.2%) to 95.763% when pulling ec6737d on invalid-tests into efe7073 on master.

[jochem-brouwer]

Note: f114898 in this commit I remove one invalid test. If we use Geth's rlpdump -hex 0x then it does not throw. Thus an empty buffer is apparently OK as input..?

The majority of the invalid RLP errors come from rather interesting Buffer.slice(start, end) behavior. If either start or end is larger than the length of the Buffer, then this method should not throw. There are some cases where you can produce an RLP which does not throw in our library but are invalid nevertheless.

We also don't have this specific check from Geth which I have added as well.

I also had to edit some tests which now throw with a different error message. I'm not super happy with the error messages and we should probably make them more explicit. Am also not entirely convinced that we have now covered all cases, especially because comparing the RLP implementation in Go and in JS is prone to off-by-one errors; in JS we use Buffer.slice(start, end) where you thus provide a region where you want to slice, but in Go you have the semantics of Buffer.slice(start, length), i.e. in JS that would be Buffer.slice(start, start+length), and since most times we use the first byte of the RLP to switch on behavior, we thus have to subtract 1 on the reported "length" to actually get the end parameter which we should slice into.

[jochem-brouwer]

Maybe it is an idea to convert this file to JS tests?

[ryanio]

i think part of the issue with the linting was your master branch was pointed to an older commit.

rebased on master and tidied up official.spec.ts

looking good!

[ryanio]

Maybe it is an idea to convert this file to JS tests?

that's a good idea, one step further would be to see if any of the cases there are not covered in ethereum/tests and have them added upstream for the greater benefit

[jochem-brouwer]

Maybe it is an idea to convert this file to JS tests?

that's a good idea, one step further would be to see if any of the cases there are not covered in ethereum/tests and have them added upstream for the greater benefit

I'll check with ethereum/tests team to see if these RLP tests are managed since the last change was 2+ years ago :) But nevertheless it would be great to see if we have now covered all cases, since fuzzing this thing seems to be rather complicated.

[jochem-brouwer]

Thanks for the rebase @ryanio 😄

I added the test cases from Geth (not all, since some were duplicate or some threw in Go because of type errors, which I have converted).

[jochem-brouwer]

I'd like at least 2 approvals before merging here, since this is a rather big change.

[jochem-brouwer]

Two things to do;

• Check if these new error cases are covered by tests, coverage seems to say no to some cases
• Cleanup tests, move all invalid tests to invalid.spec.ts

[jochem-brouwer]

This one can also be removed but is a bit more complex. We only have innerRemainder.length === 0 in case that llength and totalLength are equal (any other combination would throw safeSlice. However, we have totalLength = llength + length and therefore we have that length should be 0, but a few lines above there is a check whether length < 56; if that is the case then it throws. So, this condition can also not be reached.

[jochem-brouwer]

Coverage increased to more than 95% :) Now it is really ready for review, will not push any changes anymore.

[ryanio]

lgtm, nice! great PR. we can wait for one more approval before merging

[jochem-brouwer]

Thanks for the review and changes, let's indeed wait for one more approval 😄

[acolytec3]

Left a couple of comments but looks good otherwise!

[acolytec3]

LGTM!