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

Lazy crypto [alt design #1369] #1371

Merged
merged 4 commits into from
Jul 29, 2020
Merged

Lazy crypto [alt design #1369] #1371

merged 4 commits into from
Jul 29, 2020

Conversation

mratsim
Copy link
Contributor

@mratsim mratsim commented Jul 24, 2020

This is a reboot of #457 which was abandoned because of noSideEffect limitations in Nim 0.20 and the old nim-blscurve (status-im/nim-blscurve#27).

This is an alternative to #1369 (comment).

Unfortunately there is no free lunch and this has tradeoffs somewhat similar to copy-on-write schemes:

  • Mutability is hidden and done through an unsafe pointer (though can only happen once)

Also with regards to serialization and compared to #1369 it's stateless. This is the main difference between both PR:

Stateful requires yet another table, increasing memory usage and complicating threading in the future. It also requires more hashing, indirection and copy to manipulate both cached and non-cached BLS objects.

Stateless (this PR) tricks the compiler with pointers but uses significantly less memory and has no indirection.
It will have to deserialize objects that are received multiple times in different contexts (for example a public key from the network which may be received multiple times).

I.e. in terms of perf, the main difference is deserializing a public key vs looking it up in a table.
The full impact depends on how often we receive the same object in an unrelated context

Both should solve the loading latency from BeaconChainDB.

@mratsim mratsim requested a review from tersec July 24, 2020 14:35
@mratsim mratsim changed the base branch from master to devel July 24, 2020 14:35
@mratsim mratsim changed the title Lazy crypto Lazy crypto [alt design #1369] Jul 24, 2020
@tersec
Copy link
Contributor

tersec commented Jul 24, 2020

As a first impression, it's mechanically messier but isolates it better, while being conceptually cleaner. Avoiding a constantly, even if slowly, growing memory usage source and point of indirection does have value.

I'd be concerned somewhat about the extent to which it's dependent on specifics of old case objects, which I'm fine with in general but also upon which it seems worth considering increasing dependence. Not a major risk, as we have an existence proof of at least one other way to achieve similar ends. Furthermore, as I recall, there might be other reasons we already depend on -d:oldCaseObjects, in which case, this would be a marginal difference only.

Also, precisely because it does fool the compiler with unsafeAddr and somewhat (for a good cause) undermine its effects analysis, it creates some ongoing risk wherein if it introduces certain types of issues, they could prove trickier to track down. Still, it's a small kernel of code, so probably fine.

Finally, it's not clear to me how/if it avoids the deposits issue where searching for a pubkey in beaconstate might get confused about which kind of ValidatorPubKey the searched-for key is and which is in BeaconState.validators, or that it's especially careful to avoid those inconsistencies to begin with. I'm probably just not understanding that though, and regardless, it's not a fundamental part of either this or 1369.

That said, I do, overall, prefer this design abstractly, as an interface for non spec/crypto parts of nim-beacon-chain to use.

@tersec tersec merged commit 023f7f4 into devel Jul 29, 2020
@tersec tersec deleted the lazy-crypto branch July 29, 2020 18:13
onqtam added a commit that referenced this pull request Aug 3, 2020
onqtam added a commit that referenced this pull request Aug 4, 2020
arnetheduck added a commit that referenced this pull request Aug 4, 2020
tersec pushed a commit that referenced this pull request Aug 4, 2020
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