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.

Sign up for GitHub

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

Jump to bottom

chore: remove now useless bls init #6513

Merged
merged 2 commits into from
Mar 13, 2024
Merged

chore: remove now useless bls init #6513

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4582956
feat: allow to select bls implementation
jeluard Mar 4, 2024
bd3e29e
chore: remove unneeded init
jeluard Mar 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 5 additions & 15 deletions packages/light-client/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import mitt from "mitt";
import {init as initBls} from "@chainsafe/bls/switchable";
import {Implementation} from "@chainsafe/bls/types";
import {fromHexString, toHexString} from "@chainsafe/ssz";
import {EPOCHS_PER_SYNC_COMMITTEE_PERIOD} from "@lodestar/params";
import {phase0, RootHex, Slot, SyncPeriod, allForks} from "@lodestar/types";
Expand Down Expand Up @@ -108,7 +109,7 @@ export class Lightclient {

private runStatus: RunStatus = {code: RunStatusCode.stopped};

constructor({config, logger, genesisData, bootstrap, transport}: LightclientInitArgs) {
private constructor({config, logger, genesisData, bootstrap, transport}: LightclientInitArgs) {
this.genesisTime = genesisData.genesisTime;
this.genesisValidatorsRoot =
typeof genesisData.genesisValidatorsRoot === "string"
Expand Down Expand Up @@ -149,15 +150,16 @@ export class Lightclient {
static async initializeFromCheckpointRoot(
args: Omit<LightclientInitArgs, "bootstrap"> & {
checkpointRoot: phase0.Checkpoint["root"];
blsImplementation?: Implementation;
Copy link
Member

@nflaig nflaig Mar 6, 2024 •
edited

Choose a reason for hiding this comment

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

Having an internal implementation as part of arguments seems bit strange to me, if this is really required can't we follow same approach used in ssz and discv5, i.e. using a setter function

another example

lodestar/packages/cli/src/applyPreset.ts

Line 9 in 918924e

setHasher(hasher);

Copy link
Member Author

Choose a reason for hiding this comment

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

setHasher is setting some global then? I think we should move away from those, they introduce fragility and break expectations.

e.g. setImplementation could be called after the init is done, making it void.

There is a similar issue with the active preset, where you have to do things in the right order or it breaks. It also introduces coupling (or forces lack of coupling) between dependencies.

In general I fell like explicit is better over implicit (env variable, global variable), and letting user decide of implementation sounds like it.

Copy link
Member

@nflaig nflaig Mar 6, 2024 •
edited

Choose a reason for hiding this comment

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

they introduce fragility and break expectations.

How does this change expectations though? From the code I would expect now I can do this

lc1.initializeFromCheckpointRoot({blsImplementation: "herumi"})

// this light client instance will use different bls implementation
lc2.initializeFromCheckpointRoot({blsImplementation: "blst-native"})

but does this actually work and it that the intended use case?

There is a similar issue with the active preset, where you have to do things in the right order or it breaks. It also introduces coupling (or forces lack of coupling) between dependencies.

This is a protection that ensures preset values can't be modified after those are loaded, much better to throw an error explicitly than to risk inconsistencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

How does this change expectations though? From the code I would expect now I can do this

It looks marginally better to me but indeed there is still room to shoot yourself in the foot.

Your comment made me realize that having explicit init specifically here is probably wrong (and hopefully not required anymore as related to karma). Indeed, some code changes might force init to be already done before this code is reached.

A better solution might be:

  • to removeinit code from light-client
  • to address configurability directly in bls library

Copy link
Member

Choose a reason for hiding this comment

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

I see two patterns we can follow here

  • either use dependency injection
  • or load and set internally

If we wanna use dependency injection, the whole implementation should be passed imo and not just a name which then causing the library to be loaded internally.

I feel like the change in this PR was mixing both concepts.

they introduce fragility and break expectations.

I think the safest approach is to allow setter to be called exactly once to ensure same implementation is used globally. Dependency injection gives more flexibility but also has the risk that you have to pass the implementation in possibly multiple places and you might accidentally miss one and it implicitly uses a default / fallback implementation.

That's how setHasher works and I think it's good for this use case as you would usually wanna use same hashing implementation across the whole project.

maybe @wemeetagain can give some more insights on this

Copy link
Member Author

Choose a reason for hiding this comment

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

I shared some ideas here: ChainSafe/bls#157

Copy link
Member Author

Choose a reason for hiding this comment

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

With setHasher, user must ensure that the call is made at the right moment or there is a risk of having some default implementation being used (the one that kicks in when setHasher is not called).
Combined with other libs being intertwined, this looks a bit fragile to me (and hard to know for sure the call has been properly taken into consideration).

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the implementation selection, this code was specific to karma anyway.

}
): Promise<Lightclient> {
const {transport, checkpointRoot} = args;
const {transport, checkpointRoot, blsImplementation} = args;

// Initialize the BLS implementation. This may requires initializing the WebAssembly instance
// so why it's an async process. This should be initialized once before any bls operations.
// This process has to be done manually because of an issue in Karma runner
// https://github.com/karma-runner/karma/issues/3804
await initBls(isNode ? "blst-native" : "herumi");
await initBls(blsImplementation ?? (isNode ? "blst-native" : "herumi"));

// Fetch bootstrap state with proof at the trusted block root
const {data: bootstrap} = await transport.getBootstrap(toHexString(checkpointRoot));
Expand Down Expand Up @@ -189,12 +191,6 @@ export class Lightclient {
}

async sync(fromPeriod: SyncPeriod, toPeriod: SyncPeriod): Promise<void> {
// Initialize the BLS implementation. This may requires initializing the WebAssembly instance
// so why it's a an async process. This should be initialized once before any bls operations.
// This process has to be done manually because of an issue in Karma runner
// https://github.com/karma-runner/karma/issues/3804
await initBls(isNode ? "blst-native" : "herumi");

const periodRanges = chunkifyInclusiveRange(fromPeriod, toPeriod, MAX_PERIODS_PER_REQUEST);

for (const [fromPeriodRng, toPeriodRng] of periodRanges) {
Expand All @@ -211,12 +207,6 @@ export class Lightclient {
}

private async runLoop(): Promise<void> {
// Initialize the BLS implementation. This may requires initializing the WebAssembly instance
// so why it's a an async process. This should be initialized once before any bls operations.
// This process has to be done manually because of an issue in Karma runner
// https://github.com/karma-runner/karma/issues/3804
await initBls(isNode ? "blst-native" : "herumi");

// eslint-disable-next-line no-constant-condition
while (true) {
const currentPeriod = computeSyncPeriodAtSlot(this.currentSlot);
Expand Down