make the thresholds between unsorted bags adjustable #9230

coriolinus · 2021-06-29T13:04:47Z

Adjustable thresholds make it possible to increase specificity in the event that a particular bag is overfilled. "Overfull" is a social construct, not a technical one.

Adds to #9081.

frame/staking/src/lib.rs

kianenigma · 2021-06-30T07:27:38Z

On this occasion only, I think it is more clear if you directly add this stuff to the base PR, not a new one.

coriolinus · 2021-06-30T08:00:22Z

On this occasion only, I think it is more clear if you directly add this stuff to the base PR, not a new one.

I separated it because I'm not yet happy with the abstraction I introduced. As mentioned above, right now bag split/merge operations are inefficient, which implies that more design is necessary. Once I have this in a place where I'm happy with the bag fundamentals, I'm going to merge it into the base PR, to be reviewed / merged into master in one go.

…djustable-thresholds

This reorganizes bag storage such that bags are always referred to by their upper threshold. This in turn means that adding and removing bags is cheaper; you only need to migrate certain voters, not all of them.

The macro approach seems to be a non-starter; that only really works if we're throwing around numeric literals everywhere, and that's just not nice in this case. Instead, let's write helper functions and make it really easy to generate the tables in separate, permanent files, which humans can then edit.

This isn't yet done, becuase it seems to take a Very Long Time to run, and it really shouldn't. Need to look into that. Still, it's a lot closer than it was this morning.

Turns out that when you're working in exponential space, you need to divide, not subtract, in order to keep the math working properly. Also neaten up the output a little bit to make it easier to read.

coriolinus · 2021-07-05T09:34:49Z

I'm now happy with the abstractions we have here. I'm going to self-review it one more time, then merge it into the other PR for external review in that context.

frame/staking/src/mock.rs

coriolinus · 2021-07-05T13:08:48Z

Going to ignore the polkadot companion and benches etc here, because this is not going to be merged into master, but into another PR. That's where those issues will be addressed.

kianenigma · 2021-07-05T13:11:40Z

bin/node/runtime/voter-bags/src/main.rs

+	}
+}
+
+impl Runtime {


placing this here would inevitable mean that you cannot re-use it in polkadot, since we don't have this crate in polkadot. I recommend a simple version that just lives in pallet-staking or sth.

You can address it later.

Yeah, probably the ideal solution is to move this little helper into its own repo so it can depend on (and produce bag threshold lists for) polkadot, substrate node, etc.

The other option I was considering was to just copy the crate wholesale into polkadot/runtime/common and make appropriate modifications there. It would mean we'd have separate binaries, but it would also mean that each one could be simpler.

kianenigma · 2021-07-05T13:18:08Z

frame/staking/src/lib.rs

+		///
+		/// # Expressing the constant
+		///
+		/// This constant must be sorted in strictly increasing order. Duplicate items are not


both of these can and should be checked in an integrity_test

https://crates.parity.io/frame_support/traits/trait.Hooks.html#method.integrity_test

Yes, and in fact they are, here.

kianenigma · 2021-07-05T13:19:57Z

frame/staking/src/lib.rs

+		/// - If `VoterBagThresholds::get().is_empty()`, then all voters are put into the same bag,
+		///   and iteration is strictly in insertion order.
+		/// - If `VoterBagThresholds::get().len() == 64`, and the thresholds are determined
+		///   according to the procedure given above, then the constant ratio is equal to 2.


why is this the case? I think there is a missing maximum size of the thresholds that we're making this deduction based of?

So, that comment is very slightly inaccurate: it's actually the case that the constant ratio is 2 when there are 65 thresholds.

If you think of it from the other way around, if you wanted to split the u64 space into geometric thresholds as simply as possible, you'd make the thresholds list be (0..64).map(|n| 1 << n).chain(std::iter::once(u64::MAX)). Of course, that ends up being 65 items long. That also assumes that the existential deposit is 1.

We demo that in the list of test thresholds.

kianenigma · 2021-07-05T13:20:43Z

frame/staking/src/lib.rs

-				);
+					);
+
+					assert!(


ahh, it already is :D

kianenigma · 2021-07-05T13:21:49Z

frame/staking/src/lib.rs

@@ -3141,7 +3202,7 @@ impl<T: Config> Pallet<T> {
 	/// Move a stash account from one bag to another, depositing an event on success.
 	///
 	/// If the stash changed bags, returns `Some((from, to))`.
-	pub fn do_rebag(stash: &T::AccountId) -> Option<(BagIdx, BagIdx)> {
+	pub fn do_rebag(stash: &T::AccountId) -> Option<(VoteWeight, VoteWeight)> {


I can guess that there's no more BagIdx around, and each bag is identified by its lower weight now?

Not a big deal, but FYI this is slightly inefficient for the rebag tx. You would have an easier time submitting two indices than two VoteWeights, but anyways.

Upper weight, but yes, that's fundamentally it. Agree that it's slightly less efficient, but we had to do it like that to make it efficient to change the bags with a change of runtime.

yeah that's well worth it IMO. re-binary-searching the bags list should not be super time consuming.

kianenigma

Seems like a big change, I prefer reviewing it in the base PR then. Looks in the correct direction!

kianenigma

Seems like a big change, I prefer reviewing it in the base PR then. Looks in the correct direction!

start sketching out adjustable thresholds

44bb252

github-actions bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Jun 29, 2021

add integrity test for voter bag threshold requirements

7c19f9f

coriolinus mentioned this pull request Jun 29, 2021

Store voters in unsorted bags to get good stake iteration properties #9081

Closed

5 tasks

kianenigma reviewed Jun 30, 2021

View reviewed changes

frame/staking/src/lib.rs Outdated Show resolved Hide resolved

coriolinus added 5 commits June 30, 2021 15:29

Merge branch 'prgn-nominator-unsorted-bags' into prgn-unsorted-bags-a…

b0726c4

…djustable-thresholds

get rid of BagIdx

863519f

This reorganizes bag storage such that bags are always referred to by their upper threshold. This in turn means that adding and removing bags is cheaper; you only need to migrate certain voters, not all of them.

implement migration logic for when the threshold list changes

dfb393b

start sketching out threshold proc macros

0a894ed

further refine macro signatures

4c1f915

coriolinus added 7 commits July 2, 2021 11:36

WIP: implement make_ratio macro

cb59867

write helper functions to emit voter bags module

0d213b8

WIP: demo generating voter bags for a realistic runtime

425ce7b

This isn't yet done, becuase it seems to take a Very Long Time to run, and it really shouldn't. Need to look into that. Still, it's a lot closer than it was this morning.

rm unnecessary arg_enum

e8f4514

fix voter bags math

f034ed6

Turns out that when you're working in exponential space, you need to divide, not subtract, in order to keep the math working properly. Also neaten up the output a little bit to make it easier to read.

add computed voter bags thresholds to node

663442a

coriolinus commented Jul 5, 2021

View reviewed changes

frame/staking/src/mock.rs Outdated Show resolved Hide resolved

coriolinus added 3 commits July 5, 2021 11:47

fixup some docs

9e635a7

iter from large bags to small, fulfuilling the contract

791d9e3

make tests compile

6a8fbeb

coriolinus marked this pull request as ready for review July 5, 2021 12:30

github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Jul 5, 2021

coriolinus added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Jul 5, 2021

add VoterBagThresholds to some configs

89db32e

coriolinus requested a review from andresilva as a code owner July 5, 2021 12:56

github-actions bot added the A7-needspolkadotpr label Jul 5, 2021

kianenigma reviewed Jul 5, 2021

View reviewed changes

frame/staking/src/lib.rs

);

);

assert!(

Copy link

Contributor

kianenigma Jul 5, 2021

Choose a reason for hiding this comment

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

ahh, it already is :D

kianenigma reviewed Jul 5, 2021

View reviewed changes

kianenigma approved these changes Jul 5, 2021

View reviewed changes

coriolinus merged commit b73fd7f into prgn-nominator-unsorted-bags Jul 5, 2021

coriolinus deleted the prgn-unsorted-bags-adjustable-thresholds branch July 5, 2021 13:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

make the thresholds between unsorted bags adjustable #9230

make the thresholds between unsorted bags adjustable #9230

coriolinus commented Jun 29, 2021 •

edited

Loading

kianenigma commented Jun 30, 2021

coriolinus commented Jun 30, 2021

coriolinus commented Jul 5, 2021

coriolinus commented Jul 5, 2021

kianenigma Jul 5, 2021

coriolinus Jul 5, 2021

kianenigma Jul 5, 2021

kianenigma Jul 5, 2021

coriolinus Jul 5, 2021

kianenigma Jul 5, 2021

coriolinus Jul 5, 2021

kianenigma Jul 5, 2021

kianenigma Jul 5, 2021

coriolinus Jul 5, 2021

kianenigma Jul 5, 2021

kianenigma left a comment

kianenigma left a comment

make the thresholds between unsorted bags adjustable #9230

make the thresholds between unsorted bags adjustable #9230

Conversation

coriolinus commented Jun 29, 2021 • edited Loading

kianenigma commented Jun 30, 2021

coriolinus commented Jun 30, 2021

coriolinus commented Jul 5, 2021

coriolinus commented Jul 5, 2021

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

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

kianenigma left a comment

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

coriolinus commented Jun 29, 2021 •

edited

Loading