Skip to content

Conversation

@chirag-parmar
Copy link
Contributor

No description provided.

@chirag-parmar chirag-parmar marked this pull request as ready for review January 5, 2026 14:34
@chirag-parmar chirag-parmar requested a review from tersec January 5, 2026 14:34
@chirag-parmar chirag-parmar marked this pull request as draft January 12, 2026 10:36
@chirag-parmar chirag-parmar force-pushed the vp-eff-walk branch 2 times, most recently from 605f08c to 0fc9d5b Compare January 21, 2026 04:19
@chirag-parmar chirag-parmar marked this pull request as ready for review January 21, 2026 04:38
of "Auto": StdoutLogKind.Auto
else: StdoutLogKind.None
maxBlockWalk = jsonNode.getOrDefault("maxBlockWalk").getInt(1000)
parallelBlockDownloads = jsonNode.getOrDefault("parallelBlockDownloads").getInt(10)
Copy link
Contributor

Choose a reason for hiding this comment

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

For this (and all the other jsonNode.getOrDefault("foo").getInt(...) lines), because it's tied to an architecture-size specific integer (int, which is 32-bit on 32-bit platforms and 64-bit on 64-bit platforms), it creates a tricky-to-discover/test failure mode. Instead of using getInt, it's more predictable across 32/64-bit platforms to use getBiggestInt, which returns BiggestInt, currently an int64 regardless of whether it's on a 32-bit or 64-bit platform.

It does say that it's

an alias for the biggest signed integer type the Nim compiler supports. Currently this is int64, but it is platform-dependent in general.

But at least it's no more, and currently less, platform-dependent than getInt/int.

Copy link
Contributor Author

@chirag-parmar chirag-parmar Jan 21, 2026

Choose a reason for hiding this comment

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

but the consumer of these integers also uses int. Wouldn't it make more sense to be consistent. What is the tricky-to-discover/test failure mode you are talking about? any examples?

EDIT: also why should the bit width of these configuration values be platform independent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, it's good to be consistent and minimize the integer type conversions required (exception: safe conversions which don't change signedness and don't decrease range, so e.g., uintfoo to uint64 or intfoo to int64). So in general int is a type which might be approached cautiously. Sometimes it's basically required, e.g., anything which ends up as an index into an array or seq directly or indirectly, has to be an int anyway to work. But sometimes there's no strong constraint of that sort and one can get this consistency without using platform-dependent types.

EDIT: also why should the bit width of these configuration values be platform independent?

A couple of main advantages:

  1. it means that the end-user UX and functionality is as much the same as possible regardless of whether it's compiled as 32-bit or 64-bit. For the rest of the nimbus-eth1 repo, practically it will almost always be used having been compiled as 64-bit code, but the NVP has a decent chance of being used in some 32-bit applications due to its target embedded functionality/API; and

  2. arguably at least as importantly, it decreases the scenarios wherein someone tests it on a 32-bit platform and some issue proves difficult to reproduce on the 64-bit platforms which run in CI, on the machines on which we're developing all this software, etc. It's easier for minimal differences to exist which can cause either UX or debugging isues.

@chirag-parmar chirag-parmar requested a review from tersec January 22, 2026 07:37
else:
try:
downloadedHeaders[nextHash]
except KeyError as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

e isn't used here; can just use except KeyError.

futs = @[]
downloadedHeaders.clear()

while nextNum > targetNum and uint64(futs.len) < engine.parallelBlockDownloads:
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like it won't actually download the targetNum block itself? (> vs >=)

hash = nextHash,
number = blk.number,
remaining = distinctBase(blk.number) - targetNum
nextNum -= 1
Copy link
Contributor

Choose a reason for hiding this comment

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

See https://github.com/status-im/nimbus-eth1/pull/3892/changes#diff-223d8876a66a7bfacccc6d0d0c52b706341981ee91c954e03c5c4d64a268701fR119 for context, somewhat: because it won't try for targetNum itself this underflow/wraparound won't hit. But if targetNum == 0, and nextNum manages to hit 0, then nextNum -= 1 wraps to high(uint64), and this loop might not exit, because nextNum > targetNum will be true again.

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.

2 participants