Revert "stop testing with broken upstream/version-2-0"

[tersec]

Reverts status-im/nimbus-eth2#6554

If upstream Nim has been fixed, can test version-2-0 again

[tersec]

still nope:

2024-09-21T03:25:59.6500018Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/ncli/ncli_db.nim(781, 19) template/generic instantiation of `info` from here
2024-09-21T03:25:59.6501775Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/ncli/ncli_db.nim(780, 32) Error: template instantiation too nested
2024-09-21T03:25:59.8078153Z make: *** [Makefile:447: ncli_db] Error 1

at least it's different than the first two version-2-0/nimbus-eth2 regressions

[github-actions[bot]]

Unit Test Results

         9 files  ±0    1 355 suites  ±0   42m 23s :stopwatch: + 3m 30s   5 152 tests ±0    4 804 :heavy_check_mark: ±0  348 :zzz: ±0  0 :x: ±0  21 531 runs  ±0  21 127 :heavy_check_mark: ±0  404 :zzz: ±0  0 :x: ±0 

Results for commit ecd1e0b4. ± Comparison against base commit faca46ba.

:recycle: This comment has been updated with latest results.

[tersec]

That one is arguably a not-great template in Nimbus, fixed by https://github.com/status-im/nimbus-eth2/pull/6573/commits/b7f3f0c2e8f58686393f15d063f17dcf4a19d4f2 but that allowed finding https://github.com/nim-lang/Nim/issues/24150 in its place, which shows up as e.g., https://github.com/status-im/nimbus-eth2/actions/runs/10972715518/job/30469294056?pr=6573

2024-09-21T13:08:42.2537044Z /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_operations.nim(97, 43) Error: request to generate code for .compileTime proc: preset

[tersec]

job-logs.txt

2024-09-28T07:57:10.0430336Z Running consensus_spec_tests_minimal --xml:build/consensus_spec_tests_minimal.xml --console
2024-09-28T07:57:10.0431245Z 
2024-09-28T07:57:10.0750325Z EF - Phase 0 - Operations - Attestation  [Preset: minimal] ......................................... (0.03s)
2024-09-28T07:57:10.1163210Z EF - Phase 0 - Operations - Attester Slashing  [Preset: minimal] .............................. (0.04s)
2024-09-28T07:57:10.1169134Z EF - Phase 0 - Operations - Block Header  [Preset: minimal] ...... (0.00s)
2024-09-28T07:57:10.1291446Z EF - Phase 0 - Operations - Deposit  [Preset: minimal] ................. (0.01s)
2024-09-28T07:57:10.1396738Z EF - Phase 0 - Operations - Proposer Slashing  [Preset: minimal] ............... (0.01s)
2024-09-28T07:57:10.1467567Z EF - Phase 0 - Operations - Voluntary Exit  [Preset: minimal] .......... (0.01s)
2024-09-28T07:57:10.2874909Z EF - Phase 0 - SSZ consensus objects  [Preset: minimal] ..................F........ (0.14s)
2024-09-28T07:57:10.2876131Z ========================
2024-09-28T07:57:10.2898990Z   /github-runner/workspace/nimbus-eth2/nimbus-eth2/build/consensus_spec_tests_minimal 'EF - Phase 0 - SSZ consensus objects  [Preset: minimal]::  Testing    BeaconState'
2024-09-28T07:57:10.2900699Z ------------------------
2024-09-28T07:57:10.2902795Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2905184Z     expectedHash.root was 0xff424eeb68789f8123627d7abfce0050ed707c1752a4c80f777addb51de5ac01
2024-09-28T07:57:10.2906842Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x78f70f66e84748f1f1a2495bf2a8e7363412ebce0f8692dac1fff1e0505cac98
2024-09-28T07:57:10.2909643Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2912301Z     expectedHash.root was 0x023b00ad252467d9dffebfcd016fc94516ba71d8de61bc8af860f68257ee06b7
2024-09-28T07:57:10.2913966Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x1a5c025934bc7f18b489e50076c5c756fac324c6baf8ba2ded5f0beda358e9d3
2024-09-28T07:57:10.2916770Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2919251Z     expectedHash.root was 0x43c7e039fec598ec54e1b0aa823e079029c1436b3b5d928262bf81b3886566bd
2024-09-28T07:57:10.2920905Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x1707cdac6a5c5162a6ad29f4c2f93b61c8e8c637128e66283b89291816b7fb2b
2024-09-28T07:57:10.2923699Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2926060Z     expectedHash.root was 0x98c80c340b8fb281253edb34ffc9597d4fead1583320fad45ca0d171b4f138a0
2024-09-28T07:57:10.2927708Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0xe973712b5b84f850ea4131ee4da7e7f3a0c8c2ff547d342062be96ca90e8e262
2024-09-28T07:57:10.2930613Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2932982Z     expectedHash.root was 0x6068a239b12715918930a3c5c77506f41c9eb740fbf57e9b2dadc2821d73e3f7
2024-09-28T07:57:10.2934597Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0xa1140fda281242226934647e7287151c750e9b557bf86ed04cdb2cdf046b468b
2024-09-28T07:57:10.2937367Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2939727Z     expectedHash.root was 0x44397798b575a1ba6ff3bb0a94fb58c17a4bfd095f216b1e67a481387476be13
2024-09-28T07:57:10.2941354Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x031909bcf183a779f85e057437b276062f45949a01f14f16fde777f1970b3a03
2024-09-28T07:57:10.2944133Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2946477Z     expectedHash.root was 0x979ca13e9eb1b756e768a56d640b736d41bce7874f9203f558d8c118487c9806
2024-09-28T07:57:10.2948120Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x2f574bc0a5b3f5e48be9880ed1618e013a8bc0a6f6d1812a32921a61dff7cd7e
2024-09-28T07:57:10.2950901Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2953495Z     expectedHash.root was 0x94433016b32b2523c4e5cde51ae0bfd3407adc965e64ebdef19debfcd1f75bbb
2024-09-28T07:57:10.2955172Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x678da218d75fb08cfc670a0b7003ebc006692767dfa753df34e2e1fffe5e83d7
2024-09-28T07:57:10.2957975Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2960321Z     expectedHash.root was 0x6f3d693bb9c40823b7281fa8c92098e7bc1a418cf1bb6f6429f9f7ad976a0efe
2024-09-28T07:57:10.2961945Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0xa57022a986d9f4c95ee287fa8060229c0387722145f972af7d3f5770335f3f1f
2024-09-28T07:57:10.2964711Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2967073Z     expectedHash.root was 0x835cfadbe0b49978ae346ec753a4e757cc9bed7405d3f2e401638c9f4c4b7909
2024-09-28T07:57:10.2968718Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0xd15f625aad1196a709ba52cc13237280e0ed614259ded7c8c9f9f45885b8929f
2024-09-28T07:57:10.2972046Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2974418Z     expectedHash.root was 0x9a3b4ebe3eaabe4099bf420e6daa3255542fc81bff8aa631dfd31ff6e5b13c93
2024-09-28T07:57:10.2976101Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x12b4442cf5b661fe929cb7b5f02f90d3d8bdaab4f86390c8640bd0c9ca9e67c4
2024-09-28T07:57:10.2978474Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2979738Z     expectedHash.root was 0x5fec86e2bb827f802e13b795998d481065c6a641d419673c06495c554be6221b
2024-09-28T07:57:10.2980615Z     "0x" & toLowerAscii($hash_tree_root(deserialized[])) was 0x0016139de5b23886eff23da40d5e5eea9fef87873b2604fb99e61c39a891321a
2024-09-28T07:57:10.2982112Z     /github-runner/workspace/nimbus-eth2/nimbus-eth2/tests/consensus_spec/phase0/test_fixture_ssz_consensus_objects.nim(76, 27): Check failed: expectedHash.root == "0x" & toLowerAscii($hash_tree_root(deserialized[]))
2024-09-28T07:57:10.2983364Z     expectedHash.root was 0x97da29b0838177197755bdd02c6cb83ded7885764b5334cca74edb304dfcf1b3

[tersec]

It's related to

type U = distinct uint64
template toSszType(v: U): auto = uint64(v)
echo $hash_tree_root(default(List[U, 2]))

which on respectively 2.0.8, version-2-0, version-2-2, and devel:

F5A5FD42D16A20302798EF6ED309979B43003D2320D9F0E8EA9831A92759FB4B
7A0501F5957BDF9CB3A8FF4966F02265F968658B7A9C62642CBA1165E86642F5
7A0501F5957BDF9CB3A8FF4966F02265F968658B7A9C62642CBA1165E86642F5
7A0501F5957BDF9CB3A8FF4966F02265F968658B7A9C62642CBA1165E86642F5

[tersec]

This is causing https://github.com/status-im/nim-ssz-serialization/blob/cc09635ff06a337087ffeb83f51b8ee1e70a105c/ssz_serialization/types.nim#L67-L78 to return 1, since compiles(...) is false and T is BasicType is also false. This confuses the rest of the merkleization.

[tersec]

Requires https://github.com/status-im/nim-ssz-serialization/pull/95

[tersec]

I'm not sure if this should be necessary though. https://github.com/nim-lang/Nim/pull/22022 is the Nim commit/PR which triggers this particular CI failure.

A simplified form is that:

proc k(_: int | int): int {.compileTime.} =
  doAssert false
  0
doAssert compiles(k 0)

compiles before that PR and does not compile after it.

proc k(_: int | int): int {.compileTime.} =
  doAssert false
  0
doAssert compiles(typeof k(0))

compiles both before and after it.

For non-generic compileTime functions:

proc k(_: int): int {.compileTime.} =
  doAssert false
  0
doAssert compiles(typeof k(0))

does not compile before, but it does compile after, even with the typeof. Presumably here the k(0) gets resolved eagerly, before it sees the typeof, in earlier versions.

proc k(_: int): int {.compileTime.} =
  doAssert false
  0
doAssert compiles(k(0))

With neither typeof nor generics doesn't compile anywhere.

So while https://github.com/status-im/nim-ssz-serialization/pull/95 does work around this in general, I'm not sure what the design intention is, or if its documented what should happen.

ssz-serialization `supports

template supports*(_: type SSZ, T: type): bool =
  mixin toSszType
  anonConst compiles(fixedPortionSize toSszType(declval T))

offers another example, which it's also not clear to me even whether it should work. fixedPortionSize is typeof``/sizeof-like but it's not quite exactly that.

[metagn]

It's related to

type U = distinct uint64
template toSszType(v: U): auto = uint64(v)
echo $hash_tree_root(default(List[U, 2]))

Is this caused by the compiles change mentioned below it? We can bring back the old behavior if we disable compileTime proc folding in compiles, but which one do you think is correct?

fixedPortionSize seems to take a type argument, interesting that not using typeof even works at all.

[arnetheduck]

We can bring back the old behavior if we disable compileTime proc folding in compiles, but which one do you think is correct?

To begin with, there should be no difference between the generic and non-generic cases - in both compiles questions, we've already selected a generic instantiation, so the behavior should not be the same. This is yet another case of generic-or-not that makes the language inconsistent.

Now, as to which behavior to pick .. this is harder - if {.compileTime.} means "force compile-time evaluation even in runtime contexts", then compiles(...) should probably return the eager ("folded") result regardless if it's generic or not.

However, this is also very hard to teach - when reading code, {.compileTime.} doesn't "look like" something that forces eager evaluation - it looks like something that prevents the function from being used at runtime, which also means that it sees a fair bit of misuse. My preference would actually be to remove compileTime entirely and either replace it with something better named or not at all ("forcing" compile-time evaluation can already be done at the call site via const / static and it doesn't feel like compileTime adds significant benefits on top)

[tersec]

fixedPortionSize seems to take a type argument, interesting that not using typeof even works at all.

Well, it doesn't

[tersec]

Now, as to which behavior to pick .. this is harder - if {.compileTime.} means "force compile-time evaluation even in runtime contexts", then compiles(...) should probably return the eager ("folded") result regardless if it's generic or not.

https://nim-lang.org/docs/manual.html#pragmas-compiletime-pragma claims:

The compileTime pragma is used to mark a proc or variable to be used only during compile-time execution. No code will be generated for it. Compile-time procs are useful as helpers for macros.

It seems inconsistent even on this point though. Just checking devel,

proc r(): int {.compileTime.} = discard
discard r()

compiles, but

proc r() {.compileTime.} = discard
r()

does not. It seems like either they should both compile or neither compile.

[metagn]

It seems like either they should both compile or neither compile.

The void version was apparently specifically disabled, probably because it would silently run at compile time without people expecting it to (imagine it calls randomize for example). Then again noSideEffect is enforced for this constant folding, which itself is another "inconsistency". This means marking as sideEffect stops the folding but it's a bit awkward, the effect tracking might not be smart enough to ignore it. It could also disable the folding for raises effects (if we don't use AssertionDefect) but the same problem exists.

I think compileTime being just an effect that prevents runtime execution is fine (although effect tracking should error for this and not codegen). I think the implicit constant folding behavior is still worth being in the language though but it needs a proper design, maybe via something like static return types which are completely unused in the current language. Anything that doesn't make the situation worse than before.