-
Notifications
You must be signed in to change notification settings - Fork 75
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
[ETCM-856] make stSLoadTest pass #995
Conversation
There was a problem hiding this 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
.
@@ -20,7 +20,7 @@ import io.iohk.ethereum.consensus.pow.validators.ValidatorsExecutor | |||
class TestmodeConsensus( | |||
override val vm: VMImpl, | |||
blockchain: BlockchainImpl, | |||
blockchainConfig: BlockchainConfig, | |||
var blockchainConfig: BlockchainConfig, |
There was a problem hiding this comment.
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 var
s to the code base.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
f88e4f4
to
93a7084
Compare
87f6355
to
8beb53d
Compare
There was a problem hiding this comment.
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.
src/main/scala/io/iohk/ethereum/testmode/TestModeComponentsProvider.scala
Outdated
Show resolved
Hide resolved
the code is not passing scalastyle check |
93a7084
to
6a24a53
Compare
8beb53d
to
7568191
Compare
7568191
to
75ddb91
Compare
There is still some mutable state, but at least it's not scattered in several classes. |
} | ||
|
||
case class ForkBlockNumbers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Nice reorganization
…or consensus and ledger
4534af6
to
f627900
Compare
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 :
blockPreparator
andblockGenerator
so the new configuration was not applied in these.I solved this by :
As an aside, I also deactivated upnp when running in testmode.