Balances: Configurable Number of Genesis Accounts with Specified Balances for Benchmarking #6267

runcomet · 2024-10-28T22:39:05Z

Derived Dev Accounts

Resolves #6040

Description

This update introduces support for creating an arbitrary number of developer accounts at the genesis block based on a specified derivation path. This functionality is gated by the runtime-benchmarks feature, ensuring it is only enabled during benchmarking scenarios.

Key Features

Arbitrary Dev Accounts at Genesis: Developers can now specify any number of accounts to be generated at genesis using a derivation path. This enables more flexibility for benchmarking use cases.
Default Derivation Path: If no derivation path is provided (i.e., when dev_accounts: (..., None) is set at genesis), the system will default to the path //Sender/{}.
Default Single Dev Account: A single developer account is created by default using the path //Sender/{}.
No Impact on Total Token Issuance: Developer accounts are excluded from the total issuance of the token supply at genesis, ensuring they do not affect the overall balance or token distribution.

polkadot address: 14SRqZTC1d8rfxL8W1tBTnfUBPU23ACFVPzp61FyGf4ftUFg

cla-bot-2021 · 2024-10-28T22:39:12Z

User @runcomet, please sign the CLA here.

substrate/frame/balances/Cargo.toml

runcomet · 2024-11-13T11:20:28Z

Balances of dev_accounts should not be included in the total issuance? @ggwpez

substrate/frame/balances/src/lib.rs

bkchr · 2024-12-25T21:49:50Z

substrate/frame/balances/src/lib.rs

+					.expect(&format!("Failed to parse derivation string: {}", derivation_string));
+
+				// Convert the public key to AccountId.
+				let who = T::AccountId::decode(&mut &pair.public().encode()[..])
+					.expect(&format!("Failed to decode public key from pair: {:?}", pair.public()));


Suggested change

.expect(&format!("Failed to parse derivation string: {}", derivation_string));

// Convert the public key to AccountId.

let who = T::AccountId::decode(&mut &pair.public().encode()[..])

.expect(&format!("Failed to decode public key from pair: {:?}", pair.public()));

.unwrap_or_else(|| format!("Failed to parse derivation string: {derivation_string}"));

// Convert the public key to AccountId.

let who = T::AccountId::decode(&mut &pair.public().encode()[..])

.unwrap_or_else(|| format!("Failed to decode public key from pair: {:?}", pair.public()));

This would expect a default value for the respective operations, .expect() can't be used at genesis?

substrate/frame/balances/src/lib.rs

ggwpez · 2025-01-02T15:39:49Z

substrate/frame/balances/src/lib.rs

+				let who = T::AccountId::decode(&mut &pair.public().encode()[..])
+					.expect(&format!("Failed to decode public key from pair: {:?}", pair.public()));
+
+				frame_system::Pallet::<T>::inc_providers(&who);


Why this instead of doing a force_set_balance or something?

The less cases where we manually modify account references, the better.

Getting familiar with the codebase.

I prefer your suggestion.

Thanks for the review!

You can look at the tests of the balances pallet, it has plenty of examples on how to force-set the balance of an account 😄

I used mutate_account_handling_dust

ggwpez

Looks broadly good, thanks 😄

runcomet · 2025-01-03T22:39:03Z

please review @ggwpez @bkchr

substrate/frame/balances/src/lib.rs

runcomet · 2025-01-09T17:07:03Z

please review @michalkucharczyk @bkchr @ggwpez

michalkucharczyk · 2025-01-10T08:58:59Z

substrate/frame/balances/src/tests/mod.rs

@@ -281,7 +282,32 @@ pub fn info_from_weight(w: Weight) -> DispatchInfo {
 pub fn ensure_ti_valid() {
 	let mut sum = 0;

+	// Fetch the dev accounts from Account Storage.
+	let dev_accounts = (1000, EXISTENTIAL_DEPOSIT, "//Sender/{}".to_string()); // You can customize this as needed


Should we use default const here for derivation string?

for uniformity, probably

michalkucharczyk

Frame is not my main area, but from what I can say it looks good.

skunert · 2025-01-10T14:12:10Z

substrate/frame/balances/src/lib.rs

+			Self {
+				balances: Default::default(),
+				dev_accounts: (
+					One::one(),


Why is this one by default? Since this is now enabled without any feature flag, we should not have a dev account on all chains by default.

Following up on this one - random thought: shouldn't we make entire tuple optional?
e.g:

`dev_accounts: Option<(...)>`

That would allow us to easily cut dev-accounts related logic?

Makes for shorter code.

Having the whole field be optional is great, default in the instances will be None.

Testing the logic through ensure_ti_valid(i.e. omitting the dev accounts from total_issuance) would still valid.

skunert · 2025-01-10T14:13:42Z

substrate/frame/balances/src/lib.rs

@@ -189,6 +195,8 @@ pub use pallet::*;

 const LOG_TARGET: &str = "runtime::balances";

+const DEFAULT_ADDRESS_URI: &str = "//Sender/{}";


When trying to play with this feature in a local zombienet, I encountered:

2025-01-10 14:25:26 panicked at /home/sebastian/.cargo/registry/src/index.crates.io-6f17d22bba15001f/getrandom_or_panic-0.0.3/src/lib.rs:29:55: Attempted to use functionality that requires system randomness!! Error: Service(Other("wasm call error Execution aborted due to trap: wasm trap: wasm `unreachable` instruction executed\nWASM backtrace:\nerror while executing at wasm backtrace:\n 0: 0x19cec2 - cumulus_test_runtime.wasm!rust_begin_unwind\n 1: 0xddd8 - cumulus_test_runtime.wasm!core::panicking::panic_fmt::hb2d62c8e031896d7\n 2: 0x17b0a9 - cumulus_test_runtime.wasm!<getrandom_or_panic::getrandom_or_panic::PanicRng as rand_core::RngCore>::fill_bytes::panic_cold_display::he88660074c870e5f\n 3: 0xa81e1 - cumulus_test_runtime.wasm!schnorrkel::derive::Derivation::derived_key_simple::h3bfc095ab0e4fa27\n 4: 0x8990c - cumulus_test_runtime.wasm!<alloc::vec::into_iter::IntoIter<T,A> as core::iter::traits::iterator::Iterator>::fold::he6b04a97ec042f96\n 5: 0x5d589 - cumulus_test_runtime.wasm!pallet_balances::pallet::Pallet<T,I>::derive_dev_account::h2e451dd4081837dd\n 6: 0x5a537 - cumulus_test_runtime.wasm!<pallet_balances::pallet::GenesisConfig<T,I> as frame_support::traits::hooks::BuildGenesisConfig>::build::h0f54c9a4dd70ffa7\n 7: 0x13ac9a - cumulus_test_runtime.wasm!<cumulus_test_runtime::RuntimeGenesisConfig as frame_support::traits::hooks::BuildGenesisConfig>::build::h2b96dd85273b8fbf\n 8: 0x1232f1 - cumulus_test_runtime.wasm!frame_support::genesis_builder_helper::build_state::h5de7317ba07c9598\n 9: 0x89143 - cumulus_test_runtime.wasm!GenesisBuilder_build_state"))

I think the problem is that we are using soft derivation here which afaik requires randomness, which we don't have in the runtime. We could use //Sender//{} instead. This should work.

Failed to run this test in all honesty.

Thanks for the review @skunert

runcomet · 2025-01-10T21:13:50Z

Could I possibly see the CI run @ggwpez @skunert @michalkucharczyk, so I know what to test for?

add dev_accounts field

f3bc304

ggwpez reviewed Oct 30, 2024

View reviewed changes

substrate/frame/balances/Cargo.toml Outdated Show resolved Hide resolved

runcomet added 4 commits November 4, 2024 06:46

populate dev_accounts field

cb0d3ba

use default dev_accounts

78fa5fc

BalancesConfig

a4a1eff

Merge branch 'master' into dev-accounts

3c968f4

runcomet marked this pull request as ready for review November 5, 2024 20:30

runcomet requested review from cheme and a team as code owners November 5, 2024 20:30

paritytech-review-bot bot requested a review from a team November 5, 2024 20:31

configure ensure_ti_valid awareness of dev_accounts

3163d5f

runcomet added 2 commits November 13, 2024 14:59

Merge branch 'master' into dev-accounts

d8701a8

nit

25ca49c

michalkucharczyk reviewed Nov 19, 2024

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

michalkucharczyk reviewed Nov 19, 2024

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

runcomet added 3 commits November 21, 2024 03:32

Option String

4cac3a8

Merge branch 'master' into dev-accounts

b3e4daf

doc fix

beb8056

michalkucharczyk reviewed Nov 22, 2024

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

derive_dev_account helper function

e647bca

runcomet requested review from ggwpez and michalkucharczyk November 26, 2024 00:41

runcomet changed the title ~~Balances: Allow Configurable Number of Genesis Accounts with Specified Balances for Benchmarking~~ Balances: Configurable Number of Genesis Accounts with Specified Balances for Benchmarking Nov 30, 2024

runcomet added 4 commits November 30, 2024 10:40

add prdoc

e8efd68

Merge branch 'master' into dev-accounts

dddac55

adjust genesisconfig addition

d53d795

nit

9128ab9

Merge branch 'master' into dev-accounts

4e8ae42

bkchr reviewed Dec 25, 2024

View reviewed changes

remove feature flag

be5acd6

runcomet requested a review from bkchr December 28, 2024 13:15

ggwpez reviewed Jan 2, 2025

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jan 2, 2025

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

ggwpez reviewed Jan 2, 2025

View reviewed changes

substrate/frame/balances/src/lib.rs Show resolved Hide resolved

ggwpez reviewed Jan 2, 2025

View reviewed changes

ggwpez approved these changes Jan 2, 2025

View reviewed changes

runcomet added 2 commits January 3, 2025 22:37

mutate_account

fed102f

update PR doc

5e8f9e7

michalkucharczyk reviewed Jan 6, 2025

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

michalkucharczyk reviewed Jan 6, 2025

View reviewed changes

substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved

runcomet added 2 commits January 9, 2025 14:36

as_deref, dev_accounts

0474e2c

DEFAULT_ADDRESS_URI

00a7a0f

michalkucharczyk reviewed Jan 10, 2025

View reviewed changes

michalkucharczyk approved these changes Jan 10, 2025

View reviewed changes

skunert reviewed Jan 10, 2025

View reviewed changes

runcomet added 3 commits January 11, 2025 00:57

fix conficts + Optional dev_accounts + hard derivation

88fc87a

Merge branch 'master' into dev-accounts

c987262

cargo +nightly fmt

589af1f

runcomet requested a review from acatangiu as a code owner January 11, 2025 01:04

runcomet added 2 commits January 11, 2025 02:52

add dev_accounts field in missing pallet tests

d13f0d6

CI check to confirm chain-spec issue

dea6676

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Balances: Configurable Number of Genesis Accounts with Specified Balances for Benchmarking #6267

Balances: Configurable Number of Genesis Accounts with Specified Balances for Benchmarking #6267

runcomet commented Oct 28, 2024 •

edited

Loading

cla-bot-2021 bot commented Oct 28, 2024

runcomet commented Nov 13, 2024

bkchr Dec 25, 2024

runcomet Dec 27, 2024

ggwpez Jan 2, 2025

runcomet Jan 2, 2025

ggwpez Jan 3, 2025

runcomet Jan 3, 2025

ggwpez left a comment

runcomet commented Jan 3, 2025

runcomet commented Jan 9, 2025

michalkucharczyk Jan 10, 2025 •

edited

Loading

runcomet Jan 10, 2025 •

edited

Loading

michalkucharczyk left a comment

skunert Jan 10, 2025

michalkucharczyk Jan 10, 2025 •

edited

Loading

runcomet Jan 10, 2025

skunert Jan 10, 2025

runcomet Jan 10, 2025 •

edited

Loading

runcomet commented Jan 10, 2025 •

edited

Loading

		@@ -189,6 +195,8 @@ pub use pallet::*;

		const LOG_TARGET: &str = "runtime::balances";

		const DEFAULT_ADDRESS_URI: &str = "//Sender/{}";

Balances: Configurable Number of Genesis Accounts with Specified Balances for Benchmarking #6267

Are you sure you want to change the base?

Balances: Configurable Number of Genesis Accounts with Specified Balances for Benchmarking #6267

Conversation

runcomet commented Oct 28, 2024 • edited Loading

Derived Dev Accounts

Description

Key Features

cla-bot-2021 bot commented Oct 28, 2024

runcomet commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ggwpez left a comment

Choose a reason for hiding this comment

runcomet commented Jan 3, 2025

runcomet commented Jan 9, 2025

michalkucharczyk Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

runcomet Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

michalkucharczyk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalkucharczyk Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

runcomet Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

runcomet commented Jan 10, 2025 • edited Loading

runcomet commented Oct 28, 2024 •

edited

Loading

michalkucharczyk Jan 10, 2025 •

edited

Loading

runcomet Jan 10, 2025 •

edited

Loading

michalkucharczyk Jan 10, 2025 •

edited

Loading

runcomet Jan 10, 2025 •

edited

Loading

runcomet commented Jan 10, 2025 •

edited

Loading