approval-voting: Make importing of duplicate assignment idempotent #6971
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Alexandru Gheorghe <[email protected]>
Signed-off-by: Alexandru Gheorghe <[email protected]>
alexggh
requested review from
sandreim,
ordian and
AndreiEres
and removed request for
sandreim
eskimor
reviewed
Dec 21, 2024
| @@ -214,7 +215,14 @@ impl ApprovalEntry { | |||
| }, | |||
| }; | |||
|
|
|||
| self.tranches[idx].assignments.push((validator_index, tick_now)); | |||
| if !is_duplicate || | |||
There was a problem hiding this comment.
I am not getting the !is_duplicate part. Can you give me your line of reasoning here, that would help I think. Thanks!
There was a problem hiding this comment.
We keep the assignments in two places in assigned_validators: Bitfield, and tranches: Vec<TrancheEntry>, so when we call this function we already know if we saw an assignment for this validator which is really rare case. Given that this is on the hot-path I want to prevent running this iteration if it is not truly needed which is where !is_duplicate helps.
There was a problem hiding this comment.
I think this deserves a comment in the code as it's not obvious how is_duplicate is different from the iter check
There was a problem hiding this comment.
Discussed with @ordian this and we convinced ourselves that iterating on duplicates is actually superfluous, so I removed that part.
AndreiEres
reviewed
Jan 3, 2025
Signed-off-by: Alexandru Gheorghe <[email protected]>
ordian
reviewed
Jan 7, 2025
eskimor
approved these changes
Jan 13, 2025
Signed-off-by: Alexandru Gheorghe <[email protected]>
sandreim
approved these changes
Jan 13, 2025
Signed-off-by: Alexandru Gheorghe <[email protected]>
alexggh
added
the
A4-needs-backport
ordian
approved these changes
Jan 14, 2025
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-6971-to-stable2407
git worktree add --checkout .worktree/backport-6971-to-stable2407 backport-6971-to-stable2407
cd .worktree/backport-6971-to-stable2407
git reset --hard HEAD^
git cherry-pick -x 0d660a420fbc11a90cde5aa4e43ce2027b502162
git push --force-with-lease |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-6971-to-stable2409
git worktree add --checkout .worktree/backport-6971-to-stable2409 backport-6971-to-stable2409
cd .worktree/backport-6971-to-stable2409
git reset --hard HEAD^
git cherry-pick -x 0d660a420fbc11a90cde5aa4e43ce2027b502162
git push --force-with-lease |
github-actions bot
pushed a commit
that referenced
this pull request
Jan 15, 2025
…6971) Normally, approval-voting wouldn't receive duplicate assignments because approval-distribution makes sure of it, however in the situation where we restart we might receive the same assignment again and since approval-voting already persisted it we will end up inserting it twice in `ApprovalEntry.tranches.assignments` because that's an array. Fix this by making sure duplicate assignments are a noop if the validator already had an assignment imported at the same tranche. --------- Signed-off-by: Alexandru Gheorghe <[email protected]> Co-authored-by: ordian <[email protected]> (cherry picked from commit 0d660a4)
|
Successfully created backport PR for |
EgorPopelyaev
pushed a commit
that referenced
this pull request
Jan 15, 2025
Backport #6971 into `stable2409` from alexggh. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Signed-off-by: Alexandru Gheorghe <[email protected]> Co-authored-by: Alexandru Gheorghe <[email protected]> Co-authored-by: Alexandru Gheorghe <[email protected]>
EgorPopelyaev
pushed a commit
that referenced
this pull request
Jan 15, 2025
Backport #6971 into `stable2412` from alexggh. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> Co-authored-by: Alexandru Gheorghe <[email protected]>
EgorPopelyaev
pushed a commit
that referenced
this pull request
Jan 15, 2025
Backport #6971 into `stable2407` from alexggh. See the [documentation](https://github.com/paritytech/polkadot-sdk/blob/master/docs/BACKPORT.md) on how to use this bot. <!-- # To be used by other automation, do not modify: original-pr-number: #${pull_number} --> --------- Signed-off-by: Alexandru Gheorghe <[email protected]> Co-authored-by: Alexandru Gheorghe <[email protected]> Co-authored-by: Alexandru Gheorghe <[email protected]>
ordian
added a commit
that referenced
this pull request
Jan 16, 2025
* master: (33 commits) Implement `pallet-asset-rewards` (#3926) [pallet-revive] Add host function `to_account_id` (#7091) [pallet-revive] Remove revive events (#7164) [pallet-revive] Remove debug buffer (#7163) litep2p: Provide partial results to speedup GetRecord queries (#7099) [pallet-revive] Bump asset-hub westend spec version (#7176) Remove 0 as a special case in gas/storage meters (#6890) [pallet-revive] Fix `caller_is_root` return value (#7086) req-resp/litep2p: Reject inbound requests from banned peers (#7158) Add "run to block" tools (#7109) Fix reversed error message in DispatchInfo (#7170) approval-voting: Make importing of duplicate assignment idempotent (#6971) Parachains: Use relay chain slot for velocity measurement (#6825) PRDOC: Document `validate: false` (#7117) xcm: convert properly assets in xcmpayment apis (#7134) CI: Only format umbrella crate during umbrella check (#7139) approval-voting: Fix sending of assignments after restart (#6973) Retry approval on availability failure if the check is still needed (#6807) [pallet-revive-eth-rpc] persist eth transaction hash (#6836) litep2p: Sufix litep2p to the identify agent version for visibility (#7133) ...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Normally, approval-voting wouldn't receive duplicate assignments because approval-distribution makes sure of it, however in the situation where we restart we might receive the same assignment again and since approval-voting already persisted it we will end up inserting it twice in
ApprovalEntry.tranches.assignmentsbecause that's an array.Fix this by making sure duplicate assignments are a noop if the validator already had an assignment imported at the same tranche.