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

Draft: Store decompressed BlsPublicKeys in IndexedDB #3174

Open
wants to merge 1 commit into
base: albatross
Choose a base branch
from

Conversation

fiaxh
Copy link
Contributor

@fiaxh fiaxh commented Dec 2, 2024

Pull request checklist

  • All tests pass. The project builds and runs.
  • I have resolved any merge conflicts.
  • I have resolved all clippy and rustfmt warnings.

@sisou
Copy link
Member

sisou commented Dec 2, 2024

Without understanding/thoroughly reading the code: is this Indexeddb cache optional? Will it fail without crashing the client?

Indexeddb might not be available in all browser contexts nor in NodeJS, so this needs to be purely optional and not affect the client if not available or unwritable/unreadable for any other reason.

@hrxi
Copy link
Contributor

hrxi commented Dec 11, 2024

Without understanding/thoroughly reading the code: is this Indexeddb cache optional? Will it fail without crashing the client?

Yes, it's in an Option and just does nothing when it's not available.

Copy link
Contributor

@hrxi hrxi left a comment

Choose a reason for hiding this comment

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

There's still an error in the web client compilation:

Generating main WASM for target nodejs...
error: 

it looks like the Rust project used to create this Wasm file was linked against
version of wasm-bindgen that uses a different bindgen format than this binary:

  rust Wasm file schema version: 0.2.95
     this binary schema version: 0.2.97

Currently the bindgen format is unstable enough that these two schema versions
must exactly match. You can accomplish this by either updating this binary or
the wasm-bindgen dependency in the Rust project.

You should be able to update the wasm-bindgen dependency with:

    cargo update -p wasm-bindgen --precise 0.2.97

don't forget to recompile your Wasm file! Alternatively, you can update the
binary with:

    cargo install -f wasm-bindgen-cli --version 0.2.95

if this warning fails to go away though and you're not sure what to do feel free
to open an issue at https://github.com/rustwasm/wasm-bindgen/issues!

futures = { workspace = true }
gloo-timers = { version = "0.3", features = ["futures"] }
hex = "0.4"
js-sys = "0.3"
log = { workspace = true }
rand_core = "0.6.4"
idb = "0.6.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

Alphabetical.

for key in keys {
let mut result = Vec::new();
key.uncompress()
.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it the function's contract that you may only pass valid keys? If so, we should specify that in the documentation. If not, we should not unwrap here, but skip the iteration instead.


for key in keys {
let mut result = Vec::new();
key.uncompress()
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a potentially expensive operation. We should probably tell the caller that the keys should have been uncompressed before already.

.serialize_with_mode(&mut result, Compress::No)
.unwrap();
let public_key = hex::encode(&result);
let compressed_key = hex::encode(key.compressed().serialize_to_vec());
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the data also be stored without the hex encoding, i.e. directly as bytes in the IndexedDB?


spawn_local(async move {
loop {
let (hash, reason, reverted_blocks, adopted_blocks) =
let (hash, reason, reverted_blocks, adopted_blocks_hashs) =
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
let (hash, reason, reverted_blocks, adopted_blocks_hashs) =
let (hash, reason, reverted_blocks, adopted_block_hashes) =

let args = Array::new();
args.push(&hash.to_hex().into());
args.push(&reason.into());
args.push(&reverted_blocks);
args.push(&adopted_blocks);
args.push(&adopted_blocks.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
args.push(&adopted_blocks.clone());
args.push(&adopted_blocks);

#[derive(Serialize, Deserialize, Debug)]
struct BlsKeyEntry {
compressed_key: String,
public_key: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

We talked about adding a "last used" field so we can apply some sort of cache eviction in the future.

@sisou
Copy link
Member

sisou commented Dec 17, 2024

The wasm-bindgen version missmatch will go away when rebasing on top of albatross, as that dependency was updated recently to match.

@hrxi hrxi force-pushed the fiaxh/bls_browser_cache branch from 670dcb5 to ec4de84 Compare December 18, 2024 10:27
@hrxi
Copy link
Contributor

hrxi commented Dec 18, 2024

Rebased on top of albatross.

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