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

Override childkey take to zero if sharing coldkey with parent #1103

Closed
wants to merge 2 commits into from
Closed

Override childkey take to zero if sharing coldkey with parent #1103

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9533a6c
Override childkey take to zero if sharing coldkey with parent
gztensor Dec 18, 2024
0a456ed
Update pallets/subtensor/src/tests/children.rs
gztensor Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions pallets/subtensor/src/coinbase/run_coinbase.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,7 @@ impl<T: Config> Pallet<T> {
mining_emission: u64,
) {
// --- 1. First, calculate the hotkey's share of the emission.
let childkey_owner = Owner::<T>::get(hotkey);
let childkey_take_proportion: I96F32 =
I96F32::from_num(Self::get_childkey_take(hotkey, netuid))
.saturating_div(I96F32::from_num(u16::MAX));
Expand Down Expand Up @@ -387,9 +388,15 @@ impl<T: Config> Pallet<T> {
proportion_from_parent.saturating_mul(I96F32::from_num(validating_emission));

// --- 4.3 Childkey take as part of parent emission
let child_emission_take: u64 = childkey_take_proportion
.saturating_mul(parent_emission)
.to_num::<u64>();
// If parent and child coldkey is the same, do not apply childkey take
let parent_owner = Owner::<T>::get(&parent);
let child_emission_take: u64 = if parent_owner == childkey_owner {
0
} else {
childkey_take_proportion
.saturating_mul(parent_emission)
.to_num::<u64>()
};
total_childkey_take = total_childkey_take.saturating_add(child_emission_take);
// NOTE: Only the validation emission should be split amongst parents.

Expand Down
191 changes: 163 additions & 28 deletions pallets/subtensor/src/tests/children.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3300,7 +3300,8 @@ fn test_set_weights_no_parent() {
#[test]
fn test_childkey_take_drain() {
new_test_ext(1).execute_with(|| {
let coldkey = U256::from(1);
let coldkey_parent = U256::from(1);
let coldkey_child = U256::from(5);
let parent = U256::from(2);
let child = U256::from(3);
let nominator = U256::from(4);
Expand All @@ -3314,14 +3315,14 @@ fn test_childkey_take_drain() {
// Add network, register hotkeys, and setup network parameters
add_network(root_id, subnet_tempo, 0);
add_network(netuid, subnet_tempo, 0);
register_ok_neuron(netuid, child, coldkey, 0);
register_ok_neuron(netuid, parent, coldkey, 1);
register_ok_neuron(netuid, child, coldkey_child, 0);
register_ok_neuron(netuid, parent, coldkey_parent, 1);

// Set children
mock_set_children(&coldkey, &parent, netuid, &[(proportion, child)]);
mock_set_children(&coldkey_parent, &parent, netuid, &[(proportion, child)]);

SubtensorModule::add_balance_to_coldkey_account(
&coldkey,
&coldkey_parent,
stake + ExistentialDeposit::get(),
);
SubtensorModule::add_balance_to_coldkey_account(
Expand All @@ -3338,7 +3339,7 @@ fn test_childkey_take_drain() {
let max_take: u16 = 0xFFFF / 5;
SubtensorModule::set_max_childkey_take(max_take);
assert_ok!(SubtensorModule::set_childkey_take(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_child),
child,
netuid,
max_take
Expand All @@ -3347,14 +3348,14 @@ fn test_childkey_take_drain() {
// Set zero hotkey take for childkey
SubtensorModule::set_min_delegate_take(0);
assert_ok!(SubtensorModule::do_become_delegate(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_child),
child,
0
));

// Set zero hotkey take for parent
assert_ok!(SubtensorModule::do_become_delegate(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_parent),
parent,
0
));
Expand All @@ -3364,7 +3365,7 @@ fn test_childkey_take_drain() {
// Stake from nominator to childkey
// Give 100% of parent stake to childkey
assert_ok!(SubtensorModule::add_stake(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_parent),
parent,
stake
));
Expand All @@ -3374,7 +3375,7 @@ fn test_childkey_take_drain() {
stake
));
// Make all stakes viable
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(parent, coldkey, -1);
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(parent, coldkey_parent, -1);
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(child, nominator, -1);

// Setup YUMA so that it creates emissions:
Expand All @@ -3384,11 +3385,11 @@ fn test_childkey_take_drain() {
crate::Weights::<Test>::insert(netuid, 0, vec![(0, 0xFFFF), (1, 0xFFFF)]);
crate::Weights::<Test>::insert(netuid, 1, vec![(0, 0xFFFF), (1, 0xFFFF)]);
assert_ok!(SubtensorModule::do_root_register(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_parent),
parent,
));
assert_ok!(SubtensorModule::do_root_register(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_child),
child,
));
crate::Weights::<Test>::insert(root_id, 0, vec![(0, 0xFFFF), (1, 0xFFFF)]);
Expand All @@ -3410,8 +3411,8 @@ fn test_childkey_take_drain() {
// - Child stake increased by its child key take only (20% * 50% = 10% of total emission)
// - Parent stake increased by 40% of total emission
// - Nominator stake increased by 50% of total emission
let child_emission = crate::Stake::<Test>::get(child, coldkey);
let parent_emission = crate::Stake::<Test>::get(parent, coldkey) - stake;
let child_emission = crate::Stake::<Test>::get(child, coldkey_child);
let parent_emission = crate::Stake::<Test>::get(parent, coldkey_parent) - stake;
let nominator_emission = crate::Stake::<Test>::get(child, nominator) - stake;
let total_emission = child_emission + parent_emission + nominator_emission;

Expand All @@ -3437,7 +3438,8 @@ fn test_childkey_take_drain() {
#[test]
fn test_childkey_take_drain_validator_take() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be refactored into a helper function that would then be used in two tests:

  • test_childkey_take_same_coldkey
  • test_childkey_take_drain_validator_take

The helper function in question wouild accept parameters for coldkey_parent and coldkey_child, which would be the same in the first test and different in the other test. The other parameters of the helper would be:

  • expected_child_emission
  • expected_parent_emission
  • expected_nominator_emission

This should save ~120 LOC and it will make maintenance of the test suite less costly as adjustments to the internal subtensor interface woudln't have to be replicated in two places, but only in one.

new_test_ext(1).execute_with(|| {
let coldkey = U256::from(1);
let coldkey_parent = U256::from(1);
let coldkey_child = U256::from(5);
let parent = U256::from(2);
let child = U256::from(3);
let nominator = U256::from(4);
Expand All @@ -3451,10 +3453,10 @@ fn test_childkey_take_drain_validator_take() {
// Add network, register hotkeys, and setup network parameters
add_network(root_id, subnet_tempo, 0);
add_network(netuid, subnet_tempo, 0);
register_ok_neuron(netuid, child, coldkey, 0);
register_ok_neuron(netuid, parent, coldkey, 1);
register_ok_neuron(netuid, child, coldkey_child, 0);
register_ok_neuron(netuid, parent, coldkey_parent, 1);
SubtensorModule::add_balance_to_coldkey_account(
&coldkey,
&coldkey_parent,
stake + ExistentialDeposit::get(),
);
SubtensorModule::add_balance_to_coldkey_account(
Expand All @@ -3468,13 +3470,13 @@ fn test_childkey_take_drain_validator_take() {
crate::SubnetOwnerCut::<Test>::set(0);

// Set children
mock_set_children(&coldkey, &parent, netuid, &[(proportion, child)]);
mock_set_children(&coldkey_parent, &parent, netuid, &[(proportion, child)]);

// Set 20% childkey take
let max_take: u16 = 0xFFFF / 5;
SubtensorModule::set_max_childkey_take(max_take);
assert_ok!(SubtensorModule::set_childkey_take(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_child),
child,
netuid,
max_take
Expand All @@ -3483,14 +3485,14 @@ fn test_childkey_take_drain_validator_take() {
// Set 20% hotkey take for childkey
SubtensorModule::set_max_delegate_take(max_take);
assert_ok!(SubtensorModule::do_become_delegate(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_child),
child,
max_take
));

// Set 20% hotkey take for parent
assert_ok!(SubtensorModule::do_become_delegate(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_parent),
parent,
max_take
));
Expand All @@ -3500,7 +3502,7 @@ fn test_childkey_take_drain_validator_take() {
// Stake from nominator to childkey
// Give 100% of parent stake to childkey
assert_ok!(SubtensorModule::add_stake(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_parent),
parent,
stake
));
Expand All @@ -3510,7 +3512,7 @@ fn test_childkey_take_drain_validator_take() {
stake
));
// Make all stakes viable
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(parent, coldkey, -1);
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(parent, coldkey_parent, -1);
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(child, nominator, -1);

// Setup YUMA so that it creates emissions:
Expand All @@ -3520,11 +3522,11 @@ fn test_childkey_take_drain_validator_take() {
crate::Weights::<Test>::insert(netuid, 0, vec![(0, 0xFFFF), (1, 0xFFFF)]);
crate::Weights::<Test>::insert(netuid, 1, vec![(0, 0xFFFF), (1, 0xFFFF)]);
assert_ok!(SubtensorModule::do_root_register(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_parent),
parent,
));
assert_ok!(SubtensorModule::do_root_register(
RuntimeOrigin::signed(coldkey),
RuntimeOrigin::signed(coldkey_child),
child,
));
crate::Weights::<Test>::insert(root_id, 0, vec![(0, 0xFFFF), (1, 0xFFFF)]);
Expand All @@ -3546,8 +3548,8 @@ fn test_childkey_take_drain_validator_take() {
// - Child stake increased by its child key take (20% * 50% = 10% of total emission) plus childkey's delegate take (10%)
// - Parent stake increased by 40% of total emission
// - Nominator stake increased by 40% of total emission
let child_emission = crate::Stake::<Test>::get(child, coldkey);
let parent_emission = crate::Stake::<Test>::get(parent, coldkey) - stake;
let child_emission = crate::Stake::<Test>::get(child, coldkey_child);
let parent_emission = crate::Stake::<Test>::get(parent, coldkey_parent) - stake;
let nominator_emission = crate::Stake::<Test>::get(child, nominator) - stake;
let total_emission = child_emission + parent_emission + nominator_emission;

Expand Down Expand Up @@ -3747,3 +3749,136 @@ fn test_do_set_child_cooldown_period() {
assert_eq!(children_after, vec![(proportion, child)]);
});
}

/// Test that childkey take is zero if the parent and child share the same coldkey
#[test]
fn test_childkey_take_same_coldkey() {
new_test_ext(1).execute_with(|| {
let coldkey = U256::from(1);
let parent = U256::from(2);
let child = U256::from(3);
let nominator = U256::from(4);
let netuid: u16 = 1;
let root_id: u16 = 0;
let subnet_tempo = 10;
let hotkey_tempo = 20;
let stake = 100_000_000_000;
let proportion: u64 = u64::MAX;

// Add network, register hotkeys, and setup network parameters
add_network(root_id, subnet_tempo, 0);
add_network(netuid, subnet_tempo, 0);
register_ok_neuron(netuid, child, coldkey, 0);
register_ok_neuron(netuid, parent, coldkey, 1);

// Set children
mock_set_children(&coldkey, &parent, netuid, &[(proportion, child)]);

SubtensorModule::add_balance_to_coldkey_account(
&coldkey,
stake + ExistentialDeposit::get(),
);
SubtensorModule::add_balance_to_coldkey_account(
&nominator,
stake + ExistentialDeposit::get(),
);
SubtensorModule::set_hotkey_emission_tempo(hotkey_tempo);
SubtensorModule::set_weights_set_rate_limit(netuid, 0);
SubtensorModule::set_max_allowed_validators(netuid, 2);
step_block(subnet_tempo);
crate::SubnetOwnerCut::<Test>::set(0);

// Set 20% childkey take
let max_take: u16 = 0xFFFF / 5;
SubtensorModule::set_max_childkey_take(max_take);
assert_ok!(SubtensorModule::set_childkey_take(
RuntimeOrigin::signed(coldkey),
child,
netuid,
max_take
));

// Set zero hotkey take for childkey
SubtensorModule::set_min_delegate_take(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

In this test child take and childkey take should be non-zero and it should be observed that the parent didn't get a deduction from either and has extraced full dividend that was due. The current implementation doesn't have this bug, but the test should not assume that - one day someone might be refactoring the code and might make a mistake that this test would then not detect. Maybe just use 0.01, 0.02, 0.05 so that we are sure they have never been confused with each other.

assert_ok!(SubtensorModule::do_become_delegate(
RuntimeOrigin::signed(coldkey),
child,
0
));

// Set zero hotkey take for parent
assert_ok!(SubtensorModule::do_become_delegate(
RuntimeOrigin::signed(coldkey),
parent,
0
));

// Setup stakes:
// Stake from parent
// Stake from nominator to childkey
// Give 100% of parent stake to childkey
assert_ok!(SubtensorModule::add_stake(
RuntimeOrigin::signed(coldkey),
parent,
stake
));
assert_ok!(SubtensorModule::add_stake(
RuntimeOrigin::signed(nominator),
child,
stake
));
// Make all stakes viable
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(parent, coldkey, -1);
crate::StakeDeltaSinceLastEmissionDrain::<Test>::set(child, nominator, -1);

// Setup YUMA so that it creates emissions:
// Parent and child both set weights
// Parent and child register on root and
// Set root weights
crate::Weights::<Test>::insert(netuid, 0, vec![(0, 0xFFFF), (1, 0xFFFF)]);
crate::Weights::<Test>::insert(netuid, 1, vec![(0, 0xFFFF), (1, 0xFFFF)]);
assert_ok!(SubtensorModule::do_root_register(
RuntimeOrigin::signed(coldkey),
parent,
));
assert_ok!(SubtensorModule::do_root_register(
RuntimeOrigin::signed(coldkey),
child,
));
crate::Weights::<Test>::insert(root_id, 0, vec![(0, 0xFFFF), (1, 0xFFFF)]);
crate::Weights::<Test>::insert(root_id, 1, vec![(0, 0xFFFF), (1, 0xFFFF)]);

// Run run_coinbase until PendingHotkeyEmission are populated
while crate::PendingdHotkeyEmission::<Test>::get(child) == 0 {
step_block(1);
}

// Prevent further subnet epochs
crate::Tempo::<Test>::set(netuid, u16::MAX);
crate::Tempo::<Test>::set(root_id, u16::MAX);

// Run run_coinbase until PendingHotkeyEmission is drained for both child and parent
step_block((hotkey_tempo * 2) as u16);

// Verify how emission is split between keys
// - Child stake is not increased (child & parent have same owner => no child-take)
// - Parent stake increased by 50% of total emission
// - Nominator stake increased by 50% of total emission
let child_emission = crate::Stake::<Test>::get(child, coldkey);
let parent_emission = crate::Stake::<Test>::get(parent, coldkey) - stake;
let nominator_emission = crate::Stake::<Test>::get(child, nominator) - stake;
let total_emission = child_emission + parent_emission + nominator_emission;

assert!(is_within_tolerance(child_emission, 0, 500));
assert!(is_within_tolerance(
parent_emission,
total_emission / 2,
500
));
assert!(is_within_tolerance(
nominator_emission,
total_emission / 2,
500
));
});
}
Loading