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

eth/channel/test: Replace custom genesis code in SimulatedBackend constructor with core.DeveloperGenesisBlock #15

Open
sebastianst opened this issue Apr 22, 2021 · 5 comments

Comments

@sebastianst
Copy link
Contributor

sebastianst commented Apr 22, 2021

Our constructor still uses some, now outdated, copied code from core.DeveloperGenesisBlock. We should use this function instead (passing it as faucet address the key that we generate at the top), avoiding code duplication and being automatically up to date with the latest pre-compiles.

I don't know the meaning of parameter period, that's up for research. Looks like it is relevant in our sim backend use case.

Note that the current gas limit seems to be 12.5 Mio (with a jump today to 14 Mio?).

@ggwpez
Copy link

ggwpez commented Jun 14, 2021

The period values seems to set the target block time for the consensus.
Our tests work with all values that i tried, probably since we are not using the PoA consensus algorithm 'clique'.

The gas limit for the genesis is fixed block to 11.5M. The exact value for successive blocks can be calculated with core.CalcGasLimit.

@ggwpez
Copy link

ggwpez commented Jun 15, 2021

As it turned out in hyperledger-labs/go-perun#107, the DeveloperGenesisBlock function is not threadsafe.
It reads+writes a package variable in this line (f), which makes it a race condition since we use it in tests with t.Parallel().

Introducing a package-wide mutex in [backend/eth/channel/test] would be a possibility… that is a bit ugly IMHO.
What do we do now, other suggestions than keeping the hardcoded stuff?
@sebastianst

@sebastianst
Copy link
Contributor Author

It doesn't write to the package variable but creates a local copy. I'm surprised this causes a race condition.

I agree that a package-wide mutex would be ugly.

@sebastianst
Copy link
Contributor Author

Ah looks like an unintentional shallow copy 👍🏻

@ggwpez
Copy link

ggwpez commented Jun 21, 2021

Since ethereum/go-ethereum#23068 was merged, this can be properly addressed when the next go-ethereum version is released.
We would still need to wait for miguelmota/go-ethereum-hdwallet#13 and our integration of it, however.

@matthiasgeihs matthiasgeihs transferred this issue from hyperledger-labs/go-perun Jun 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants