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

update defaultRuntimeConfig for v1.1.1 #2958

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

etan-status
Copy link
Contributor

@etan-status etan-status commented Oct 5, 2021

This synchronizes defaultRuntimeConfig with v1.1.1 of the spec.

  • Introduces TERMINAL_TOTAL_DIFFICULTY and TERMINAL_BLOCK_HASH.
  • Sets ALTAIR_FORK_EPOCH in mainnet preset.

@etan-status etan-status marked this pull request as ready for review October 5, 2021 12:18
@etan-status etan-status requested a review from zah October 5, 2021 12:19
@etan-status etan-status marked this pull request as draft October 5, 2021 12:36
@etan-status etan-status force-pushed the v1-1-1-presets branch 6 times, most recently from eaf7221 to 860727f Compare October 5, 2021 14:46
@arnetheduck
Copy link
Member

There's a TODO here to not hardcode the defaultRuntimePreset constants but rather load them from the yaml file - should be fairly simple to fix this properly, can you take a look?

@etan-status
Copy link
Contributor Author

Sure, I'm currently facing weird compile errors, only on i386, here, and meanwhile the CI somehow abandoned this PR completely. Will take a look at the YAML once that is resolved / as a followup.

/home/runner/work/nimbus-eth2/nimbus-eth2/nimbus-eth2/beacon_chain/nimbus_beacon_node.nim(1265, 5) template/generic instantiation of `closureScope` from here
23
/home/runner/work/nimbus-eth2/nimbus-eth2/nimbus-eth2/beacon_chain/nimbus_beacon_node.nim(1267, 19) template/generic instantiation of `addAsyncValidator` from here
24
/home/runner/work/nimbus-eth2/nimbus-eth2/nimbus-eth2/beacon_chain/networking/eth2_network.nim(2011, 26) template/generic instantiation of `decode` from here
25
/home/runner/work/nimbus-eth2/nimbus-eth2/nimbus-eth2/vendor/nim-serialization/serialization.nim(67, 31) Error: type mismatch: got <type SszReader, InputStreamHandle>
26
Makefile:290: recipe for target 'nimbus_beacon_node' failed
27
but expected one of: 
28
proc init[A](s: var HashSet[A]; initialSize = defaultInitialSize)
29
  first type mismatch at position: 1
30
  required type for s: var HashSet[init.A]
31
  but expression 'ReaderType`gensym478035009' is of type: type SszReader
32

33
expression: init(ReaderType`gensym478035009, stream`gensym478035008)
34

@etan-status etan-status closed this Oct 5, 2021
@etan-status etan-status reopened this Oct 5, 2021
@arnetheduck
Copy link
Member

any imports you changed, just try moving them around, for example after json_serialization

@etan-status
Copy link
Contributor Author

CI issues were just intermittent. It resumed to run :-)

@etan-status etan-status force-pushed the v1-1-1-presets branch 2 times, most recently from 4bae21c to c4f8e0f Compare October 5, 2021 21:18
@etan-status etan-status changed the title update presets for v1.1.1 update defaultRuntimeConfig for v1.1.1 Oct 5, 2021
@etan-status
Copy link
Contributor Author

The compilation problems were indeed due to some import / export shenanigans. It's weird that they only affect i386, but whatever :-)

Bundled the fix with #2960, once that one is merged, this here can be rebased and should build successfully.

@etan-status etan-status marked this pull request as ready for review October 5, 2021 21:33
@etan-status
Copy link
Contributor Author

The YAML import I'd like to look into separately, having the defaultRuntimeConfig updated and helpers for the new types active is already good to have, especially with the general flakiness that this area seems to have.

@etan-status etan-status marked this pull request as draft October 5, 2021 22:42
@etan-status etan-status marked this pull request as ready for review October 6, 2021 07:51
@etan-status etan-status requested a review from tersec October 6, 2021 09:09
TERMINAL_TOTAL_DIFFICULTY:
u256"115792089237316195423570985008687907853269984665640564039457584007913129638912",
# By default, don't use this param
TERMINAL_BLOCK_HASH: BlockHash.fromHex(
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep the usage of the BlockHash type to the absolute minimum. The type exists only because of an inappropriate design of our package dependencies resulting in multiple competing definitions for the Ethereum types. We would like to migrate to a world where every type is defined once and used consistently everywhere. BlockHash won't exist in that world.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What would be the appropriate type here? Eth2Digest?

The inspiration for this was your commit 81dc6c8

Copy link
Member

Choose a reason for hiding this comment

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

where every type is defined once and used consistently everywhere

hm, there are some problems with that approach - mainly that you end up with massive depedency trees but also that when you change a type slightly because one consumer of the type needs something (for example a formatting of json or whatever), all dependent code has to change as well - putting up some barriers between types isn't a bad idea, often

Copy link
Contributor

Choose a reason for hiding this comment

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

I would respectufully disagree that putting the said barrier is a good idea in our particular situation.

Copy link
Contributor

@zah zah Oct 7, 2021

Choose a reason for hiding this comment

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

The inspiration for this was your commit 81dc6c8

My changes are in the eth1_monitor module that needs to talk to an execution engine over JSON-RPC. Right now, types like BlockHash are the native types of nim-web3 (our json-rpc based library for consuming the Eth1 APIs). There are no good reasons for code outside the eth1_monitor module to work with these types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

well, the dep tree is a practical problem - right now, to compile some simple spec tests, we also pull in libp2p which pulls in chronos and a bunch of crypto libraries that have nothing to do with the spec, merely because somewhere there's some silly type reused.

the reverse dep problem is actually more of a concern - central points of types also become central points of failure meaning a small seemingly benign change chan break many downstreams.

Anyway, this wasn't about this change in particular, rather a problem that we've been running into more frequently as the dep trees get more complex because not all projects move at the same upgrade pace.

@etan-status etan-status marked this pull request as draft October 7, 2021 16:41
@etan-status
Copy link
Contributor Author

(trying whether this compiles fine without ssz change. SSZ should not be required because this field is not exchanged via RPC only via REST).

This synchronizes defaultRuntimeConfig with v1.1.1 of the spec.
- Introduces `TERMINAL_TOTAL_DIFFICULTY` and `TERMINAL_BLOCK_HASH`.
- Sets `ALTAIR_FORK_EPOCH` in `mainnet` preset.
@etan-status etan-status marked this pull request as ready for review October 7, 2021 20:01
@etan-status
Copy link
Contributor Author

Compiles fine. The CI issues were just intermittent connection issues.

@zah zah merged commit ff5d9bf into status-im:unstable Oct 7, 2021
@etan-status etan-status deleted the v1-1-1-presets branch October 8, 2021 10:30
zah pushed a commit that referenced this pull request Oct 10, 2021
This synchronizes defaultRuntimeConfig with v1.1.1 of the spec.
- Introduces `TERMINAL_TOTAL_DIFFICULTY` and `TERMINAL_BLOCK_HASH`.
- Sets `ALTAIR_FORK_EPOCH` in `mainnet` preset.
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.

3 participants