Skip to content
This repository has been archived by the owner on Oct 28, 2021. It is now read-only.

Retesteth support #5909

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Retesteth support #5909

wants to merge 7 commits into from

Conversation

winsvega
Copy link
Contributor

No description provided.

@winsvega winsvega requested a review from gumb0 June 21, 2020 13:45
@winsvega winsvega mentioned this pull request Jun 21, 2020
@@ -116,8 +116,10 @@ h256 BlockHeader::hash(IncludeSeal _i) const

void BlockHeader::streamRLPFields(RLPStream& _s) const
{
bigint nnn = m_number; // Fix rlp stream issue with uint65_t
Copy link
Member

@gumb0 gumb0 Jun 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it supposed to fix the case when block number doesn't in 64 bit?
But it doesn't fix it, because m_number is still uint64_t

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some proposals to require the limit for block nuumber and timestamp, so I'm not sure it makes sense to change it now.
https://eips.ethereum.org/EIPS/eip-1985

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It fixes the test. Because RLPStream << was working incorrectly with u64int_t

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean exactly? Maybe we should fix operator<< for uint64_t or something

(Some tests currently fail anyway)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I had blockchain/invalidblocks/bcInvalidHeader/timestamp4 test failing without this change. I assume the issue is in << operation

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't find this test, what are header values there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I was out of laptop.
it is this test:
BlockchainTests/InvalidBlocks/bcUncleHeaderValidity/incorrectUncleTimestamp4

I set timestamp of the uncleheader to 500000000000 + previous block (~1000)
and the value I get rlp::85746a528800. but testeth gives me RLP encode of this argument rlp::846a528800
unless I explicitly say that it is a bigint in that function. the fix is dirty because I just try to make the tests pass

@@ -189,7 +189,6 @@ BOOST_AUTO_TEST_CASE(stBadOpcode){}

//New Tests
BOOST_AUTO_TEST_CASE(stArgsZeroOneBalance){}
BOOST_AUTO_TEST_CASE(stEWASMTests){}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean we can't use testeth anymore with https://github.com/ewasm/hera ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use your fork sure.
The issue is that new forks will not be supported in testeth.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What fork do you mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean if you have stEWASMTests somewhere and use testeth to run it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants