Enable parallel key scraping

[eagr]

closes #174

[liamaharon]

bot fmt

[command-bot]

@liamaharon https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4070204 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 5-9db85ab5-7d64-4187-a9d0-d607a2a2fb49 to cancel this command or bot cancel to cancel all commands in this pull request.

[command-bot]

@liamaharon Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4070204 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4070204/artifacts/download.

[liamaharon]

Thanks for this!

Some comments:

1. Could you please split up the logic of generating batches of key ranges to scrape and fetching them into seperate methods? This way, we can write a test to ensure generating the batches to fetch works as expected.

2. Please write some comments explaining what's happening to explain code that otherwise may not be immediately obvious.

3. Please replace existing serial scraping logic with your new method, so we can run the tests in this file with your new logic and also I'll do some manual testing.

[eagr]

Sure, gladly!

[eagr]

Got many 503s running the remote tests and some tests failed. I guess this may need more work.

failures:
    remote_tests::can_create_child_snapshot
    remote_tests::child_keys_are_loaded
    remote_tests::offline_else_online_works

[eagr]

Good news is the tests run a lot faster with the changes.

---

Just realized that without the changes, there were more failing tests on my machine and they took 12x longer to run.

failures:
    remote_tests::can_build_few_pallet
    remote_tests::can_build_one_small_pallet
    remote_tests::can_create_child_snapshot
    remote_tests::can_create_snapshot
    remote_tests::child_keys_are_loaded
    remote_tests::offline_else_online_works
    remote_tests::snapshot_block_hash_works

[liamaharon]

I had a quick go creating a snapshot using try-runtime-cli pactched with this branch and using it, but got a bunch of errors indicating state was not scraped correctly.

Perhaps an easy way to debug could be to create a remote-test which scrapes the keys from a chain without much state like wss://kusama-bridge-hub-rpc.polkadot.io:443 (so it's fast) using both your parallel method and the existing method, and compares the keys scraped using easy method. The keys scraped should be equivalent.

[liamaharon]

Suggested change

	// Create a batch of storage key prefixes each starting with `prefix`, meant to be used for key
	// scraping. Given the prefix 00, the return can be 000-00F or 0000-00FF, depending on `size`.
	// `size` will be rounded to power of 16 if not already, so is the returned batch size.
	/// Create a batch of storage key prefixes each starting with `prefix`, meant to be used for key
	/// scraping. Given the prefix 00, the return can be 000-00F or 0000-00FF, depending on `size`.
	/// `size` will be rounded to power of 16 if not already, so is the returned batch size.

[liamaharon]

What stops pow becoming > MAX_BATCH_SIZE here?

[eagr]

I got the totally wrong idea of how a storage key is represented with StorageKey. Should've known the problem better before hacking it 🤣

[eagr]

I haven't benched this. But this approach seems a lot slower than rpc_get_keys_paged() if the block is small (~4000).

[liamaharon]

I haven't benched this. But this approach seems a lot slower than rpc_get_keys_paged() if the block is small (~4000).

Yeah, seems something is wrong with it when I choose a parallelisation value >1.

4 is much slower than 1, and 16 (like in the test currently) causes the server to 503 for some reason (if we figure out which requests are causing that, we should open a new issue for the node/infra team)

[eagr]

I'm not surpised at the result though, after seeing how many storage keys are there. The workload is always divided into 256 calls to rpc_get_keys_paged() when parallel>1. It may be slow because this is a dumb idea 🤣. But I couldn't think of a better way to do this, at least not without changing the underlying client methods. Any suggestions?

Out of curiosity, what is the average block size in terms of keys in reality?

[eagr]

Is it possible to get the block size (or how many keys it contains) with the block hash before scraping? If so, maybe we could adjust the param dynamically based on the size?

[liamaharon]

I'm not surpised at the result though, after seeing how many storage keys are there. The workload is always divided into 256 calls to rpc_get_keys_paged() when parallel>1. It may be slow because this is a dumb idea 🤣. But I couldn't think of a better way to do this, at least not without changing the underlying client methods. Any suggestions?

Why is it always divided into 256 calls when parallel > 1?

I don't really understand why your implementation must be so complex, I wonder if we can solve this problem with a simpler approach of creating a function which breaks up the keyspace in to N segments:

use num_bigint::{BigUint, ToBigUint};

/// Given the index of a segment and the total number of segments, compute an approximate
/// starting key for the segment. This assumes a 256-bit hash space.
fn get_starting_key_for_segment(segment_index: usize, total_segments: usize) -> StorageKey {
    // Define the total hash space for a 256-bit number.
    let total_hash_space: BigUint = (1.to_biguint().unwrap() << 256) - 1u32.to_biguint().unwrap();
    
    // Determine the size of each segment.
    let segment_size = total_hash_space / total_segments.to_biguint().unwrap();
    
    // Calculate the starting point of the given segment.
    let segment_start = segment_size * segment_index.to_biguint().unwrap();
    
    // Convert the starting point to a byte vector.
    let segment_start_bytes = segment_start.to_bytes_be();

    // Create a StorageKey with the byte vector.
    StorageKey(segment_start_bytes)
}

Then we can use this function to get the start and end of each segment we want to scrape?

WDYT?

[eagr]

I don't really understand why your implementation must be so complex

Coz I had the wrong assumption about how the scraping works, maybe also how data is organized in a block. Thanks for sticking with me here!

Quick question. Given a prefix [42], could there be any matches if the start key is set to [43, 0, ...]? If not, maybe we could narrow down the hash space.

[liamaharon]

Coz I had the wrong assumption about how the scraping works, maybe also how data is organized in a block. Thanks for sticking with me here!

Sorry I worded that poorly, didn't mean to sound harsh. really do appreciate your work here :)

Quick question. Given a prefix [42], could there be any matches if the start key is set to [43, 0, ...]?

No I don't think so, but probably worth checking to make sure.

[liamaharon]

Then let's wrap this up for now, alright?

Sounds good. Tomorrow I'll re-benchmark your parallel implementation at 05ebb51 and if it's similar performance to HEAD shall we revert to 05ebb51 given the additional complexity in HEAD doesn't seem to provide tangible gains?

[eagr]

I'd suggest that we do things the current approach, unless it performs worse.

I suppose how to divide the workload shouldn't be tied to parallel in the first place, not a perfect design. Not to mention if the hashes could be distributed unevenly. That could potentially be a bottleneck depending how uneven the distribution becomes.

If you agree, I'll clean up the code and remove the complicated parts.

On second thought, do you we should keep the old paged function for testing purpose? Never mind, I forgot we have a range version now.

[liamaharon]

Suggested change

	// Divide the workload and return the start key of each chunks. Guarantee to return a
	// non-empty list.
	/// Divide the workload and return the start key of each chunks. Guaranteed to return a
	/// non-empty list.

[liamaharon]

Nice to also have some documentation explaining how the algorithm works in some detail.

[liamaharon]

Let's use the PARALLEL_REQUESTS const

[liamaharon]

Looking good. Excited to get this merged and into try-runtime-cli :)

[ggwpez]

/tip medium

[substrate-tip-bot]

@ggwpez A medium (16 KSM) tip was successfully submitted for @eagr (FvpsvV1GQAAbwqX6oyRjemgdKV11QU5bXsMg9xsonD1FLGK on kusama).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Fkusama-rpc.polkadot.io#/referenda