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

Balances: Arbitrary number of genesis accounts #6040

Open
ggwpez opened this issue Oct 14, 2024 · 14 comments · May be fixed by #6267
Open

Balances: Arbitrary number of genesis accounts #6040

ggwpez opened this issue Oct 14, 2024 · 14 comments · May be fixed by #6267
Assignees
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T2-pallets This PR/Issue is related to a particular pallet.

Comments

@ggwpez
Copy link
Member

ggwpez commented Oct 14, 2024

We have the issue for benchmarking that we need at least 1M accounts in the genesis state, which does not work well with JSON files and can cause some parts to allocate high memory.

Additionally to having a Vec here, we should add a dev_accounts: (u32, T::Balance) field that makes the runtime generate an arbitrary number of derived dev accounts when building the genesis state:

pub balances: Vec<(T::AccountId, T::Balance)>,

Should probably be feature-flagged for benchmarks.

@ggwpez ggwpez added T2-pallets This PR/Issue is related to a particular pallet. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. labels Oct 14, 2024
@ggwpez ggwpez changed the title Balances: Arbitrary number of funded genesis accounts Balances: Arbitrary number of genesis accounts Oct 14, 2024
@xlc
Copy link
Contributor

xlc commented Oct 14, 2024

why JSON genesis for such use case? wouldn't it be faster to just operate on the raw storage?

@runcomet
Copy link

relevant other links would be helpful to look closely on this :)

@ggwpez
Copy link
Member Author

ggwpez commented Oct 17, 2024

why JSON genesis for such use case? wouldn't it be faster to just operate on the raw storage?

@xlc our omni tooling works with genesis presets. Relying on specific storage addresses would probably work in practice, but it is a bit hacky IMHO. What downside do you see in having this?

relevant other links would be helpful to look closely on this :)

I added the link to the exact line of code in the issue. Not sure what else i can explain about it, but you can check the build function where the genesis state is build:

assert!(T::AccountStore::insert(who, AccountData { free, ..Default::default() })

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Oct 17, 2024

Would be nice if the accounts were compatible with polkadot-stps and ttxt tools.

They use this derivation pattern:
https://github.com/paritytech/polkadot-stps/blob/8de8d4af332b278bda4f99e7ed619caa6b581de0/utils/src/lib.rs#L37
https://github.com/paritytech/polkadot-stps/blob/8de8d4af332b278bda4f99e7ed619caa6b581de0/utils/sender/src/pre/mod.rs#L19

Or even better, we could pass the derivation string (actually address uri string) that has a placeholder for the index: "//Sender/{}"

That would allow some flexibility.

@xlc
Copy link
Contributor

xlc commented Oct 17, 2024

it is just there is big overhead of creating such big json and parse it in the node and convert to raw compare to just supply the raw format. I am worried there is possibility of some steps may not work well with such big file. but maybe it will just works.

@michalkucharczyk
Copy link
Contributor

michalkucharczyk commented Oct 17, 2024

it is just there is big overhead of creating such big json and parse it in the node and convert to raw compare to just supply the raw format. I am worried there is possibility of some steps may not work well with such big file. but maybe it will just works.

With this proposal chain spec JSON file will be simple and small. Here is example (1000000 accounts each with 500 balance):

{
  "name": "Custom",
  "id": "custom",
  "chainType": "Live",
  "bootNodes": [],
  "telemetryEndpoints": null,
  "protocolId": null,
  "properties": null,
  "codeSubstitutes": {},
  "genesis": {
    "runtimeGenesis": {
      "code": "0x..",
      "patch": {
        "balances": {
          "dev_accounts": [1000000, 500]
        },
      }
    }
  }
}

It would be:

...
 "dev_accounts": [1000000, 500, "//Sender/{}"]
...

if my proposal with address uri sounds ok.

@xlc
Copy link
Contributor

xlc commented Oct 17, 2024

oh I see. I misunderstood the issue.

@runcomet
Copy link

would like to work on this

Assign if possible

@kianenigma
Copy link
Contributor

One of the only downsides of adding a new field to RuntimeGenesisConfig would be that it might will a breaking change, at least when compiling a node with hardcoded chainspecs.

@michalkucharczyk
Copy link
Contributor

One of the only downsides of adding a new field to RuntimeGenesisConfig would be that it might will a breaking change, at least when compiling a node with hardcoded chainspecs.

That would be the case only for hardcoded RuntimeGenesisConfig (meaning native runtime) or for the nodes that are using full genesis config (which I don't think is the case as we advocate patches).

@bkchr
Copy link
Member

bkchr commented Oct 18, 2024

That would be the case only for hardcoded RuntimeGenesisConfig (meaning native runtime) or for the nodes that are using full genesis config (which I don't think is the case as we advocate patches).

Also most of the code probably already used ..Default::default() syntax for this since quite some time.

Generally I like the proposed solution here! Also means that not every tooling needs to replicate a way to generate these accounts and can instead just set this field.

@ggwpez
Copy link
Member Author

ggwpez commented Oct 23, 2024

@runcomet I assigned you. We will need this for: #5891
LMK if you can pick it up soon, otherwise i will do so.

@kianenigma
Copy link
Contributor

@gpestana you might want to add a similar mechanism for staking: just receive two numbers, and generate that many stakers.

At least it removes one issue with creating testnets with large number of stakers.

@runcomet
Copy link

@runcomet I assigned you. We will need this for: #5891

LMK if you can pick it up soon, otherwise i will do so.

🙏

Hacking on it

github-merge-queue bot pushed a commit that referenced this issue Oct 30, 2024
# Benchmark Overhead Command for Parachains

This implements the `benchmark overhead` command for parachains. Full
context is available at:
#5303. Previous attempt
was this #5283, but here
we have integration into frame-omni-bencher and improved tooling.

## Changes Overview

Users are now able to use `frame-omni-bencher` to generate
`extrinsic_weight.rs` and `block_weight.rs` files for their runtime. The
core logic for generating these remains untouched; this PR provides
mostly machinery to make it work for parachains at all.

Similar to the pallet benchmarks, we gain the option to benchmark based
on just a runtime:

```
frame-omni-bencher v1 benchmark overhead --runtime {{runtime}}
```

or with a spec:

```
frame-omni-bencher v1 benchmark overhead --chain {{spec}} --genesis-builder spec
```

In this case, the genesis state is generated from the runtime presets.
However, it is also possible to use `--chain` and genesis builder `spec`
to generate the genesis state from the chain spec.

Additionally, we use metadata to perform some checks based on the
pallets the runtime exposes:

- If we see the `ParaInherent` pallet, we assume that we are dealing
with a relay chain. This means that we don't need proof recording during
import (since there is no storage weight).
- If we detect the `ParachainSystem` pallet, we assume that we are
dealing with a parachain and take corresponding actions like patching a
para id into the genesis state.

On the inherent side, I am currently supplying the standard inherents
every parachain needs.

In the current state, `frame-omni-bencher` supports all system chains.
In follow-up PRs, we could add additional inherents to increase
compatibility.

Since we are building a block during the benchmark, we also need to
build an extrinsic. By default, I am leveraging subxt to build the xt
dynamically. If a chain is not compatible with the `SubstrateConfig`
that comes with `subxt`, it can provide a custom extrinsic builder to
benchmarking-cli. This requires either a custom bencher implementation
or an integration into the parachains node.

Also cumulus-test-runtime has been migrated to provide genesis configs.

## Chain Compatibility
The current version here is compatible with the system chains and common
substrate chains. The way to go for others would be to customize the
frame-omni-bencher by providing a custom extrinsicbuilder. I did an
example implementation that works for mythical:
https://github.com/skunert/mythical-bencher

## Follow-Ups
- After #6040 is finished, we should integrate this here to make the
tooling truly useful. In the current form, the state is fairly small and
not representative.

## How to Review
I recommend starting from
[here](https://github.com/paritytech/polkadot-sdk/pull/5891/files#diff-50830ff756b3ac3403b7739d66c9e3a5185dbea550669ca71b28d19c7a2a54ecR264),
this method is the main entry point for omni-bencher and `polkadot`
binary.

TBD:
- [x] PRDoc

---------

Co-authored-by: Michal Kucharczyk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C2-good-first-issue A task for a first time contributor to become familiar with the Polkadot-SDK. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. T2-pallets This PR/Issue is related to a particular pallet.
Projects
Status: No status
Status: Backlog
6 participants