Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add new test case not covered by RLP tests #554

Merged
merged 1 commit into from
Nov 29, 2018

Conversation

ayrat555
Copy link
Contributor

We discovered that our implementation of RLP encoding decodes some RLP binaries incorrectly after recent PR (mana-ethereum/ex_rlp#17) but it passes all RLP test cases.

This PR adds a new test case which is not covered by existing RLP tests

We discovered that our implementation of RLP encoding decodes some RLP binaries incorrectly after recent PR (mana-ethereum/ex_rlp#17) but it passes all RLP test cases. 

This PR adds a new test case which is not covered by existing RLP tests
@ayrat555
Copy link
Contributor Author

Also, I suggest adding 0x prefix for all hex encoded values

@holgerd77
Copy link
Contributor

Adding hex prefixes here probably makes very much sense. Nevertheless don't introduce this here on only one new test case, since this makes test format syntax inconsistent. Instead let's address this separately on a new PR.

@holgerd77
Copy link
Contributor

I tested this new case over on our RLP implementation ethereumjs/rlp#47 and it currently fails, so I can't really help in confirming if the test case is valid (this might very well also be a bug in our implementation). 😄

@ayrat555
Copy link
Contributor Author

@holgerd77 Can you please share what is the result of decoding in your implementation?

@holgerd77
Copy link
Contributor

@ayrat555 I had another look and came to the conclusion that we generally have a problem with our RLP library and should this test a lot better, we especially have some super-grave 0-value bug issue open which (probably) also came into effect here.

So I will share our result, you will nevertheless probably not be able to do very much with it, this likely contains other bugs:

f90316b88232393963613661636664333565336437326438626133643165326236306235353631643561663532313865623562633138323034353736396562343232363931306133303161636165336233363966666663346134383939643662303235333165383966643466653336613263663064393336303762613437306235306637383030b8806664613163666636373463393063396131393735333966653364666235333038366163653634663833656437633665616265633734316637663338316363383033653532616232636435356435353639626365343334373130376133313064666435663838613031306364326666643130303563613430366631383432383737b84037653936386262613133623663353065326334636437663234316363306436346431616332356337663539353264663233316163366132626461386565356436823034823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030823030

When having a second and somewhat more distant look on the current RLP tests file I came to the meta-conclusion that there should be actually much much more and also more structured test cases and this very much needs an overhaul and expansion in general.

So much more systematically providing several tests for the different data types on a first round and then combinations of those in a second.

@holgerd77
Copy link
Contributor

@winsvega We were in the middle of a discussion if this is correct or can be confirmed or not. Have you checked in some way that this is the correct encoding result before merging this?

@winsvega
Copy link
Collaborator

winsvega commented Dec 1, 2018

oh. I thought it is finalised. Sorry.
The Cpp has error on this:

/home/wins/Ethereum/cpp-ethereum/test/tools/jsontests/RLPTests.cpp(88): error: in "RlpTests/rlptest": (listofbinaries) Encoding Failed: expected: 0xf8a7b841299ca6acfd35e3d72d8ba3d1e2b60b5561d5af5218eb5bc182045769eb4226910a301acae3b369fffc4a4899d6b02531e89fd4fe36a2cf0d93607ba470b50f7800b840fda1cff674c90c9a197539fe3dfb53086ace64f83ed7c6eabec741f7f381cc803e52ab2cd55d5569bce4347107a310dfd5f88a010cd2ffd1005ca406f1842877a07e968bba13b6c50e2c4cd7f241cc0d64d1ac25c7f5952df231ac6a2bda8ee5d604000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000
 But Computed: 0xf90450b884307832393963613661636664333565336437326438626133643165326236306235353631643561663532313865623562633138323034353736396562343232363931306133303161636165336233363966666663346134383939643662303235333165383966643466653336613263663064393336303762613437306235306637383030b88230786664613163666636373463393063396131393735333966653364666235333038366163653634663833656437633665616265633734316637663338316363383033653532616232636435356435353639626365343334373130376133313064666435663838613031306364326666643130303563613430366631383432383737b8423078376539363862626131336236633530653263346364376632343163633064363464316163323563376635393532646632333161633661326264613865653564368430783034843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030843078303084307830308430783030

@winsvega
Copy link
Collaborator

winsvega commented Dec 1, 2018

could you please make another PR?

@ayrat555
Copy link
Contributor Author

ayrat555 commented Dec 2, 2018

#562

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants