Skip to content

[ETCM-856] make stSLoadTest pass#995

Merged
AurelienRichez merged 8 commits intodevelopfrom
fix/ETCM-856-make-stSLoadTest-pass
May 28, 2021
Merged

[ETCM-856] make stSLoadTest pass#995
AurelienRichez merged 8 commits intodevelopfrom
fix/ETCM-856-make-stSLoadTest-pass

Conversation

@AurelienRichez
Copy link
Copy Markdown
Contributor

@AurelienRichez AurelienRichez commented May 25, 2021

Note : this PR is currently using fix/ETCM-846-make-stBadOpcode-pass as base (#992) because it depends on it. I will change the target to develop when #992 is merged.

Description

This PR makes stSLoadTest pass. The was with the way we condifured hard fork transition in the tests :

  • We set the default block number of a given fork to 0 when not specified, so a hard fork was active by default
  • We did not recreate a new blockPreparator and blockGenerator so the new configuration was not applied in these.

I solved this by :

  • By default, set the hard fork block to a high number if not specified (I chose Int.MaxNumber, but it is arbitrary)
  • For hard forks which are not specified by ETS (basically ETC hard fork), I aligned their block number to an ETH one : We might want to check that these are adequate (but at least the test is passing).
  • I changed some val to def to be sure the config

As an aside, I also deactivated upnp when running in testmode.

jvdp
jvdp suggested changes May 26, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jvdp jvdp left a comment

Choose a reason for hiding this comment

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

Requesting changes because of the var.

Comment thread src/main/resources/conf/testmode.conf Outdated
Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala
Comment thread src/main/scala/io/iohk/ethereum/testmode/TestmodeConsensus.scala Outdated
override val vm: VMImpl,
blockchain: BlockchainImpl,
blockchainConfig: BlockchainConfig,
var blockchainConfig: BlockchainConfig,
Copy link
Copy Markdown
Contributor

@jvdp jvdp May 26, 2021

Choose a reason for hiding this comment

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

Is there any other way? I really feel we should avoid adding any more vars to the code base.

Copy link
Copy Markdown
Contributor Author

@AurelienRichez AurelienRichez May 26, 2021

Choose a reason for hiding this comment

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

Well, I think we could replace TestLedgerWrapper with some utility class that provide a ledger and also a consensus implementation. We would still have a var somewhere but at least it would be more centralized. I will try something along those lines.

Copy link
Copy Markdown
Contributor

@jvdp jvdp May 26, 2021

Choose a reason for hiding this comment

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

Is thread safety a concern btw? This is essentially a shared resource right?

Copy link
Copy Markdown
Contributor Author

@AurelienRichez AurelienRichez May 26, 2021

Choose a reason for hiding this comment

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

Not really. Clearly thread safety is not taken into account in the test service, but retesteth will run the test sequentially anway. There is a way to run tests in parallel but only with several instance of the client. But it wouldn't hurt to put the state in one place and put it behind an AtomicReference.

Comment thread src/main/scala/io/iohk/ethereum/jsonrpc/TestService.scala Outdated
@AurelienRichez AurelienRichez force-pushed the fix/ETCM-846-make-stBadOpcode-pass branch 4 times, most recently from f88e4f4 to 93a7084 Compare May 26, 2021 13:36
@AurelienRichez AurelienRichez force-pushed the fix/ETCM-856-make-stSLoadTest-pass branch 4 times, most recently from 87f6355 to 8beb53d Compare May 27, 2021 13:14
jvdp
jvdp approved these changes May 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@jvdp jvdp left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Nice 👍 so the only mutable thing is now the timestamp? nvm, there's still the blockchain config var of course.

Comment thread src/main/scala/io/iohk/ethereum/testmode/TestModeComponentsProvider.scala Outdated
@dzajkowski
Copy link
Copy Markdown
Contributor

dzajkowski commented May 27, 2021

the code is not passing scalastyle check

@AurelienRichez AurelienRichez force-pushed the fix/ETCM-846-make-stBadOpcode-pass branch from 93a7084 to 6a24a53 Compare May 27, 2021 14:58
@AurelienRichez AurelienRichez force-pushed the fix/ETCM-856-make-stSLoadTest-pass branch from 8beb53d to 7568191 Compare May 27, 2021 15:42
Base automatically changed from fix/ETCM-846-make-stBadOpcode-pass to develop May 27, 2021 15:58
@AurelienRichez AurelienRichez force-pushed the fix/ETCM-856-make-stSLoadTest-pass branch from 7568191 to 75ddb91 Compare May 27, 2021 16:04
@AurelienRichez
Copy link
Copy Markdown
Contributor Author

AurelienRichez commented May 27, 2021

There is still some mutable state, but at least it's not scattered in several classes. TestService holds all the state and TestModeComponentsProvider actually take the "mutable" parts as arguments.

leo-bogastry
leo-bogastry reviewed May 28, 2021
View reviewed changes
Comment thread src/main/scala/io/iohk/ethereum/utils/BlockchainConfig.scala
copy(forkBlockNumbers = update(forkBlockNumbers))
}

case class ForkBlockNumbers(
Copy link
Copy Markdown
Contributor

@leo-bogastry leo-bogastry May 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

👍 Nice reorganization

@AurelienRichez AurelienRichez force-pushed the fix/ETCM-856-make-stSLoadTest-pass branch from 4534af6 to f627900 Compare May 28, 2021 12:28
@AurelienRichez AurelienRichez merged commit 0cc1c27 into develop May 28, 2021
@AurelienRichez AurelienRichez deleted the fix/ETCM-856-make-stSLoadTest-pass branch May 28, 2021 13:02
@AurelienRichez AurelienRichez mentioned this pull request Jun 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jvdp jvdp jvdp approved these changes

@leo-bogastry leo-bogastry leo-bogastry approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@AurelienRichez @dzajkowski @jvdp @leo-bogastry