Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Configurable call fee refund for signed submissions #11002

Merged
merged 31 commits into from
Apr 20, 2022
Merged

Configurable call fee refund for signed submissions #11002

merged 31 commits into from
Apr 20, 2022

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Mar 9, 2022 •
edited
Loading

Previously, the winning solution had it deposit unreserved, and was rewarded with the reward base + call fee. All other unchecked solutions had their deposit unreserved, but still lost their call fee. This PR changes things so a runtime can be configured to refund the call fees for some (or all) of the stored, non-winning solutions.

Configuring SignedMaxRefunds:

The first x unchecked solutions can get their call fee refunded by setting SignedMaxRefunds to x. This will lead to no ejected solutions getting a refund.

polkadot companion: paritytech/polkadot#5309

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Mar 9, 2022
@emostov emostov requested a review from kianenigma March 9, 2022 23:57
@emostov emostov added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 9, 2022
@kianenigma
Copy link
Contributor

if not overly complicated, can we make it such that you specify how many of the "non-winnings" get a refund? i.e 'type MaxFullRefunds'. This way we can enable and merge this on Kusama ahead of time as well.

ggwpez
ggwpez reviewed Mar 11, 2022
View reviewed changes
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just some comments 😄

frame/election-provider-multi-phase/src/lib.rs Outdated Show resolved Hide resolved
frame/election-provider-multi-phase/src/signed.rs Outdated Show resolved Hide resolved
kianenigma
kianenigma reviewed Mar 12, 2022
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

As discussed elsewhere, when we push out a solution in the middle of the signed phase we should also refund that potentially.

For these solutions, I presume we always refund their call-fee? hard to decide on it.

@kianenigma
Copy link
Contributor

@niklasad1 @ggwpez can either of you take a second look! much appreciated.

niklasad1
niklasad1 reviewed Apr 16, 2022
View reviewed changes
frame/election-provider-multi-phase/src/signed.rs
.filter(|k| {
if self.deletion_overlay.contains(k) {
// Remove submissions that should be deleted.
SignedSubmissionsMap::<T>::remove(k);
Copy link
Member

Choose a reason for hiding this comment

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

I think SignedSubmissionsMap::<T>::remove(k) (side-effects) in filter is a bit unexpected...

Personally I would prefer filter_map or something even if the actual iterator isn't affected by it.

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5309

@kianenigma
Copy link
Contributor

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5309

@emostov
Copy link
Contributor Author

emostov commented Apr 19, 2022

bot merge

@paritytech-processbot
Copy link

Error: "Check reviews" status is not passing for paritytech/polkadot#5309

@emostov
Copy link
Contributor Author

emostov commented Apr 20, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 7dd9798 into master Apr 20, 2022
@paritytech-processbot paritytech-processbot bot deleted the zeke-refund-miner-fee branch April 20, 2022 20:56
@github-actions github-actions bot mentioned this pull request May 31, 2022
Closed
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
@emostov @kianenigma
@niklasad1 @shawntabrizi @godcodehunter
Configurable call fee refund for signed submissions (paritytech#11002)
0c22cb5 
* Refund call fee for all non-invalid signed submissions

* Clean up

* Fix benchmarks

* Remove reward from struct

* WIP SignedMaxRefunds

* Apply suggestions from code review

* Add test for ejected call_fee refunds

* Add test for number of calls refunded

* Account for read op in mutate

* Apply suggestions from code review

* Add to node runtime

* Don't refund ejected solutions

* Update frame/election-provider-multi-phase/src/lib.rs

Co-authored-by: Kian Paimani <[email protected]>

* Inegrity test SignedMaxRefunds

* Use reward handle to refund call fee

* Fix node runtime build

* Drain in order of submission

* Update frame/election-provider-multi-phase/src/signed.rs

* save

* Update frame/election-provider-multi-phase/src/signed.rs

Co-authored-by: Niklas Adolfsson <[email protected]>

* Update frame/election-provider-multi-phase/src/signed.rs

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Niklas Adolfsson <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
@github-actions github-actions bot mentioned this pull request Jun 22, 2022
Closed
@kianenigma kianenigma mentioned this pull request Jun 30, 2022
Open
@Dengjianping Dengjianping mentioned this pull request Jul 7, 2022
Merged
10 tasks
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@emostov @kianenigma
@niklasad1 @shawntabrizi @DaviRain-Su
Configurable call fee refund for signed submissions (paritytech#11002)
d15379d 
* Refund call fee for all non-invalid signed submissions

* Clean up

* Fix benchmarks

* Remove reward from struct

* WIP SignedMaxRefunds

* Apply suggestions from code review

* Add test for ejected call_fee refunds

* Add test for number of calls refunded

* Account for read op in mutate

* Apply suggestions from code review

* Add to node runtime

* Don't refund ejected solutions

* Update frame/election-provider-multi-phase/src/lib.rs

Co-authored-by: Kian Paimani <[email protected]>

* Inegrity test SignedMaxRefunds

* Use reward handle to refund call fee

* Fix node runtime build

* Drain in order of submission

* Update frame/election-provider-multi-phase/src/signed.rs

* save

* Update frame/election-provider-multi-phase/src/signed.rs

Co-authored-by: Niklas Adolfsson <[email protected]>

* Update frame/election-provider-multi-phase/src/signed.rs

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Niklas Adolfsson <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@emostov @kianenigma
@niklasad1 @shawntabrizi @ark0f
Configurable call fee refund for signed submissions (paritytech#11002)
4b46c83 
* Refund call fee for all non-invalid signed submissions

* Clean up

* Fix benchmarks

* Remove reward from struct

* WIP SignedMaxRefunds

* Apply suggestions from code review

* Add test for ejected call_fee refunds

* Add test for number of calls refunded

* Account for read op in mutate

* Apply suggestions from code review

* Add to node runtime

* Don't refund ejected solutions

* Update frame/election-provider-multi-phase/src/lib.rs

Co-authored-by: Kian Paimani <[email protected]>

* Inegrity test SignedMaxRefunds

* Use reward handle to refund call fee

* Fix node runtime build

* Drain in order of submission

* Update frame/election-provider-multi-phase/src/signed.rs

* save

* Update frame/election-provider-multi-phase/src/signed.rs

Co-authored-by: Niklas Adolfsson <[email protected]>

* Update frame/election-provider-multi-phase/src/signed.rs

Co-authored-by: Kian Paimani <[email protected]>
Co-authored-by: Niklas Adolfsson <[email protected]>
Co-authored-by: Shawn Tabrizi <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@niklasad1 niklasad1 niklasad1 approved these changes

@kianenigma kianenigma kianenigma approved these changes

@ggwpez ggwpez Awaiting requested review from ggwpez

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
(Nominated) Proof of Stake
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@emostov @kianenigma @ggwpez @niklasad1 @jakoblell @shawntabrizi