allow trusted node sync based on LC trusted block root

[etan-status]

Extends trustedNodeSync with a new --trusted-block-root option that allows initializing a light client. No --state-id must be provided. The beacon node will then use this light client to obtain the latest finalized state from the remote server in a trust-minimized fashion. Note that the provided --trusted-block-root should be somewhat recent, and that security precautions such as comparing the state root against block explorers is still recommended.

[etan-status]

Requires status-im/nim-stew#174

[etan-status]

build/nimbus_beacon_node trustedNodeSync --data-dir=$HOME/nimbus/data/goerli-trusted --network:goerli \
    --trusted-node-url=http://127.0.0.1:5052 --backfill=false \
    --trusted-block-root=0x24ad5b7d941e147b80edb0aa34c1c454e6e467e68e83142e51e9d21b9226eb79

[github-actions]

Unit Test Results

         9 files  ±0    1 074 suites  ±0   37m 43s ⏱️ + 4m 47s
  3 675 tests ±0    3 396 ✔️ ±0  279 💤 ±0  0 ❌ ±0 
15 668 runs  ±0  15 363 ✔️ ±0  305 💤 ±0  0 ❌ ±0 

Results for commit 371dbe0. ± Comparison against base commit 974b165.

♻️ This comment has been updated with latest results.

[tersec]

Maybe worth doing something like type foo = uint64; const bar = sizeof(foo), not sure.

As it is, there's an implicit relationship here between the two which modifying some, but not all, will cause decoding errors for.

[etan-status]

Hmm, yeah, it's a bit tricky. Two tricky portions further down where reviewing becomes more annoying by reducing magic number:

• int.high.foo only guaranteed to work if foo.high >= int.high.
• The ForkDigest conversion explicitly going from 0 .. 3

Have replaced the magic numbers for now (while keeping them as a comment to be able to reason about it)

[tersec]

Minor, but elif here would clarify the mutually exclusive nature of these in a way beyond the return reses at the end of each, and without requiring verifying that there aren't any other early returns within those conditional blocks.

[etan-status]

Moved the return to the top to prevent accidents, I think that's what you meant.

[tersec]

Partly that, but also, it's structured as

  if mutually_exclusive_condition_1:
    a couple dozen lines of code with multiple indentation levels and control flow
    return

  if mutually_exclusive_condition_2:
    some more code
    return

Both if blocks can't possibly run, but to infer that needs more than a cursory glance for no particular reason that I can discern. It's a case of is-this-switch-statement-meant-to-fall-through-or-not.

[etan-status]

you mean, use elif for the second condition in these?

downsides are:

• it's not consistently usable, e.g., when there is a let in-between the two if.
• it suggests some sort of relations between the two conditions, or more generally, an exhaustion of multiple options
• if just doing if / elif / elif ..., analyzers like clang-tidy like to point out that an else is missing to achieve said exhaustion

ultimately, very minor though.

in the context of this PR, there is only a single return. are you referring to the raise?

for example:

      if responseChunkLen > int.high.chunkLenType:
          raiseRestDecodingBytesError("Malformed data: Unsupported length")
        if l - o < responseChunkLen.int:
          raiseRestDecodingBytesError("Malformed data: Incomplete chunk")   

or, the try / except bloks such as this one:

        try:
          withLcDataFork(lcDataForkAtConsensusFork(consensusFork)):
            when lcDataFork > LightClientDataFork.None:
              type T = typeof(res[0])
              var obj = T(kind: lcDataFork)
              obj.forky(lcDataFork) = SSZ.decode(
                data.toOpenArray(begin + contextLen, after - 1),
                T.Forky(lcDataFork))
              obj.checkForkConsistency(cfg, consensusFork)
              res.add obj
            else:
              raiseRestDecodingBytesError(
                cstring("Unsupported fork: " & $consensusFork))
        except SszError as exc:
          raiseRestDecodingBytesError(cstring("Malformed data: " & $exc.msg))

I can see the "multiple indentation levels and control flow" argument, but nim's try / except kinda pushes that.

Maybe I'm misunderstanding the point.