Add extrinsic to improve position in a bag of bags-list #9829

emostov · 2021-09-21T03:18:49Z

This PR adds the extrinsic put_in_front_of which allows the user to specify a lighter and heavier account within the same bag and the extrinsic will move heavier directly in front of lighter. The parameter names refer to the fact that lighter must have a lower VoteWeight then heavier.

In the ideal use case, where a user wants to improve the position of their account within a bag, the user would iterate the bag, starting from the head, and find the first node who's VoteWeight is less than theirs. They would then supply the id of the node as the lighter argument and their own id as the heavier argument.

polkadot companion: paritytech/polkadot#3899

Below is a table with some combinations of edge case and the name of the relevant unit test

	heavier head	heavier tail	heavier non-terminal (in front)	heavier non-terminal (behind)
lighter head	NA	put_in_front_of_heavier_is_tail_lighter_is_head	NA	put_in_front_of_lighter_is_head_heavier_is_non_terminal
lighter tail	put_in_front_of_lighter_is_tail_heavier_is_head	NA	NA	put_in_front_of_lighter_is_tail_heavier_is_not_terminal
lighter non-terminal (in front)	NA	put_in_front_of_heavier_is_tail_lighter_is_non_terminal	NA	put_in_front_of_non_terminal_nodes_heavier_behind
lighter non-terminal (behind)	put_in_front_of_heavier_is_head_lighter_is_not_terminal	NA	put_in_front_of_non_terminal_nodes_heavier_in_front	NA

TODO

if this is permissioned, investigate a reasonable way for allowing the stash to call this - perhaps either via proxy or the controller.

skip check-dependent-cumulus

This PR adds the extrinsic `put_in_front_of` which allows the user to specify a `lighter` and `heavier` account within the same bag. The extrinsic will move `heavier` directly in front of `lighter`. The parameter names refer to the fact that `lighter` must have a lower `VoteWeight` then `heavier`. In the ideal use case, where a user wants to improve the position of their account within a bag, the user would iterate the bag, starting from the head, and find the first node who's `VoteWeight` is less than theirs. They would then supply the `id` of the node as the `lighter` argument and their own `id` as the `heavier` argument.

emostov · 2021-09-21T03:27:14Z

/benchmark runtime pallet pallet_bags_list

frame/bags-list/src/list/mod.rs

frame/bags-list/src/lib.rs

frame/bags-list/src/list/mod.rs

frame/bags-list/src/tests.rs

…found

kianenigma · 2021-09-22T15:13:18Z

frame/bags-list/src/list/mod.rs

+		ensure!(weight_of(&heavier_id) > weight_of(&lighter_id), pallet::Error::NotHeavier);
+
+		// we need to write the bag to storage if its head and/or tail is updated.
+		if lighter_node.is_terminal() || heavier_node.is_terminal() {


Here you are assuming that the lighter node could only ever be a head, and the heavier node could only ever be a tail, while I don't see that being guaranteed. Can you elaborate all the edge cases that you considered, and why you think this is enough?

here's an idea to break this down into smaller bite-size pieces:

For removing, we simply use the current fn remove(). After all the checks, we immediately remove heavier. The logic for this function should already update the Bag head/tail if heavier.is_terminal().

For inserting, we write a private helper function as such:

fn insert_at(after: Option<Node>) { if let Some(x) = after { // new logic, insert a new node on the prev of `after` } else { // insert at tail: delegate to whatever we do now } }

This will require the least amount of added logic, which helps with verifying the correctness of the PR (assuming that none of the old code was buggy).

If we keep the current code, here's a mental model to check it

none are terminal

lighter is head, heavier is not terminal (common case)

lighter is tail, heavier is not terminal (mistake case)

heavier is tail, lighter is not terminal (common case)

heavier is head, lighter is not terminal (mistake case)

lighter is head, heavier is tail (common case)

lighter is tail, heavier is head (mistake case)

and for any case that contains a non-terminal, we have to consider if they are adjacent or not (in case 5 it totally makes a different).

I'm going to now start thinking about each case and how it is handled.

My analysis shows case 5 is not handled correctly, when the non-terminal lighter is not adjacent to the heavier one.

#[test] fn heavier_is_head_lighter_is_not_terminal() { ExtBuilder::default().add_ids(vec![(5, 1000)]).build_and_execute(|| { // given assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![2, 3, 4, 5])]); // when BagsList::put_in_front_of(Origin::signed(0), 2, 4).unwrap(); // then.. :( assert_eq!(List::<Runtime>::get_bags(), vec![(10, vec![1]), (1_000, vec![3, 2, 4, 5])]); }); }

I'd like to see the fix, but my gut feeling is that rewriting it using smaller functions is easier. This if conditions_in_which_we_might_update_bags {} is a bit hard to verify. If we first remove, then update everything, and then insert, and update everything again, there would probably be a few extra (IMO negligible) lookups to the overlay, but the (non-negligible) db access would be the same.

frame/bags-list/src/list/mod.rs

frame/bags-list/src/mock.rs

kianenigma

Might have some unconsidered edge cases here. https://github.com/paritytech/substrate/pull/9829/files#r714053661

emostov · 2021-09-23T04:04:25Z

Might have some unconsidered edge cases here. https://github.com/paritytech/substrate/pull/9829/files#r714053661

yup, definitely missed some. To try and be thorough I created this table for edge cases and filled it with the names of relevant unit tests

	heavier head	heavier tail	heavier non-terminal (in front)	heavier non-terminal (behind)
lighter head	NA	put_in_front_of_heavier_is_tail_lighter_is_head	NA	put_in_front_of_lighter_is_head_heavier_is_non_terminal
lighter tail	put_in_front_of_lighter_is_tail_heavier_is_head	NA	NA	put_in_front_of_lighter_is_tail_heavier_is_not_terminal
lighter non-terminal (in front)	NA	put_in_front_of_heavier_is_tail_lighter_is_non_terminal	NA	put_in_front_of_non_terminal_nodes_heavier_behind
lighter non-terminal (behind)	put_in_front_of_heavier_is_head_lighter_is_not_terminal	NA	put_in_front_of_non_terminal_nodes_heavier_in_front	NA

frame/bags-list/src/list/mod.rs

frame/bags-list/src/benchmarks.rs

gui1117

looks good to me, minor grumble above, but they should better be addressed IMO.
Also #9829 (comment)

Co-authored-by: Guillaume Thiolliere <[email protected]>

gui1117

all good to me, we can merge

emostov · 2021-12-07T18:16:56Z

bot merge

paritytech-processbot · 2021-12-07T18:17:05Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#3899

emostov · 2021-12-08T04:14:31Z

bot merge

paritytech-processbot · 2021-12-08T04:14:40Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#3899

emostov · 2021-12-08T04:20:33Z

Error: It's not possible to meet the minimal approval count of 1 in paritytech/polkadot#3899

Not sure whats up here, @thiolliere already approved the companion

shawntabrizi · 2021-12-08T04:35:32Z

bot merge

) * pallet-bags-list: Add `put_in_front_of` extrinsic This PR adds the extrinsic `put_in_front_of` which allows the user to specify a `lighter` and `heavier` account within the same bag. The extrinsic will move `heavier` directly in front of `lighter`. The parameter names refer to the fact that `lighter` must have a lower `VoteWeight` then `heavier`. In the ideal use case, where a user wants to improve the position of their account within a bag, the user would iterate the bag, starting from the head, and find the first node who's `VoteWeight` is less than theirs. They would then supply the `id` of the node as the `lighter` argument and their own `id` as the `heavier` argument. * Test & Benchmarks * Respect line width * Remove List::put_in_fron_of tests; Remove AlreadyHigher error * The dispatch origin for this call must be ... * Add some periods * Add back test to list module: put_in_front_of_exits_early_if_bag_not_found * add test tests::pallet::heavier_is_head_lighter_is_not_terminal * Cater for edge case of heavier being head * Add ExtBuilder::add_aux_data; try to make some tests use simpler data * Update frame/bags-list/src/list/tests.rs * make insert_at_unchecked infallible * Make it permissioned - only callable by heavier * Add test cases for insert_at_unchecked * Move counter update to insert_at; fix comments * Address some feedback * Make voteweight constructed with parameter_types * Always set vote weight for Ids in build * Add skip_genesis_ids * Do not pass weight fn to List put_in_front_of * Remove remants of CounterForListNodes * fmt * cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_bags_list --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/bags-list/src/weights.rs --template=./.maintain/frame-weight-template.hbs * Delete messed up weights file * Some place holder stuff so we can run bench in CI * Fix * cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_bags_list --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/bags-list/src/weights.rs --template=./.maintain/frame-weight-template.hbs * Update frame/bags-list/src/list/mod.rs Co-authored-by: Guillaume Thiolliere <[email protected]> * fmt * Log + debug assert when refetching an Id Co-authored-by: Parity Bot <[email protected]> Co-authored-by: Guillaume Thiolliere <[email protected]>

emostov added 2 commits September 20, 2021 20:17

Test & Benchmarks

331df1b

emostov added A0-please_review Pull request needs code review. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. labels Sep 21, 2021

Merge remote-tracking branch 'origin' into zeke-change-bag-position

e7c016c

emostov mentioned this pull request Sep 21, 2021

Companion to Substrate #9829 (Add put_in_front_of extrinsic to bags-list) paritytech/polkadot#3899

Merged

emostov commented Sep 21, 2021

View reviewed changes

frame/bags-list/src/list/mod.rs Outdated Show resolved Hide resolved

Respect line width

2401b2b

emostov requested a review from kianenigma September 21, 2021 05:00

kianenigma reviewed Sep 21, 2021

View reviewed changes

frame/bags-list/src/lib.rs Show resolved Hide resolved

kianenigma reviewed Sep 21, 2021

View reviewed changes

frame/bags-list/src/list/mod.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 21, 2021

View reviewed changes

frame/bags-list/src/list/mod.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 21, 2021

View reviewed changes

frame/bags-list/src/tests.rs Outdated Show resolved Hide resolved

emostov added 5 commits September 21, 2021 10:42

Remove List::put_in_fron_of tests; Remove AlreadyHigher error

60743cc

The dispatch origin for this call must be ...

6bb232f

Add some periods

45f4a73

Add back test to list module: put_in_front_of_exits_early_if_bag_not_…

f04a64c

…found

Merge remote-tracking branch 'origin' into zeke-change-bag-position

ce0fcfb

kianenigma reviewed Sep 22, 2021

View reviewed changes

frame/bags-list/src/list/mod.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 22, 2021

View reviewed changes

frame/bags-list/src/list/mod.rs Outdated Show resolved Hide resolved

kianenigma reviewed Sep 22, 2021

View reviewed changes

frame/bags-list/src/mock.rs Outdated Show resolved Hide resolved

kianenigma suggested changes Sep 22, 2021

View reviewed changes

emostov added 2 commits September 22, 2021 12:37

add test tests::pallet::heavier_is_head_lighter_is_not_terminal

aa0aaeb

Cater for edge case of heavier being head

fa1a33d

Add ExtBuilder::add_aux_data; try to make some tests use simpler data

21aef98

gui1117 reviewed Dec 3, 2021

View reviewed changes

frame/bags-list/src/list/mod.rs Outdated Show resolved Hide resolved

gui1117 reviewed Dec 3, 2021

View reviewed changes

frame/bags-list/src/benchmarks.rs Show resolved Hide resolved

gui1117 approved these changes Dec 3, 2021

View reviewed changes

kianenigma added E6-transactionversion and removed A0-please_review Pull request needs code review. labels Dec 3, 2021

emostov and others added 4 commits December 6, 2021 14:42

Update frame/bags-list/src/list/mod.rs

77fd777

Co-authored-by: Guillaume Thiolliere <[email protected]>

fmt

aa4d896

Log + debug assert when refetching an Id

944dc85

Merge remote-tracking branch 'origin' into zeke-change-bag-position

ae188ae

github-actions bot added the A7-needspolkadotpr label Dec 7, 2021

gui1117 approved these changes Dec 7, 2021

View reviewed changes

Merge remote-tracking branch 'origin' into zeke-change-bag-position

2eb86e5

emostov removed the A7-needspolkadotpr label Dec 8, 2021

paritytech-processbot bot merged commit c9cad66 into master Dec 8, 2021

paritytech-processbot bot deleted the zeke-change-bag-position branch December 8, 2021 04:35

librelois mentioned this pull request Feb 7, 2022

Update substrate/polkadot/cumulus from v0.9.15 to v0.9.16 moonbeam-foundation/moonbeam#1259

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add extrinsic to improve position in a bag of bags-list #9829

Add extrinsic to improve position in a bag of bags-list #9829

emostov commented Sep 21, 2021 •

edited by shawntabrizi

Loading

emostov commented Sep 21, 2021

kianenigma Sep 22, 2021

kianenigma Sep 22, 2021 •

edited

Loading

kianenigma Sep 22, 2021 •

edited

Loading

emostov Sep 23, 2021

kianenigma left a comment

emostov commented Sep 23, 2021 •

edited

Loading

gui1117 left a comment

gui1117 left a comment •

edited

Loading

emostov commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

emostov commented Dec 8, 2021

paritytech-processbot bot commented Dec 8, 2021

emostov commented Dec 8, 2021

shawntabrizi commented Dec 8, 2021

Add extrinsic to improve position in a bag of bags-list #9829

Add extrinsic to improve position in a bag of bags-list #9829

Conversation

emostov commented Sep 21, 2021 • edited by shawntabrizi Loading

emostov commented Sep 21, 2021

kianenigma Sep 22, 2021

Choose a reason for hiding this comment

kianenigma Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

kianenigma Sep 22, 2021 • edited Loading

Choose a reason for hiding this comment

emostov Sep 23, 2021

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

emostov commented Sep 23, 2021 • edited Loading

gui1117 left a comment

Choose a reason for hiding this comment

gui1117 left a comment • edited Loading

Choose a reason for hiding this comment

emostov commented Dec 7, 2021

paritytech-processbot bot commented Dec 7, 2021

emostov commented Dec 8, 2021

paritytech-processbot bot commented Dec 8, 2021

emostov commented Dec 8, 2021

shawntabrizi commented Dec 8, 2021

emostov commented Sep 21, 2021 •

edited by shawntabrizi

Loading

kianenigma Sep 22, 2021 •

edited

Loading

kianenigma Sep 22, 2021 •

edited

Loading

emostov commented Sep 23, 2021 •

edited

Loading

gui1117 left a comment •

edited

Loading