From e8c3710b434215fc788800e19f9dcfb087b02104 Mon Sep 17 00:00:00 2001
From: "paritytech-cmd-bot-polkadot-sdk[bot]"
<179002856+paritytech-cmd-bot-polkadot-sdk[bot]@users.noreply.github.com>
Date: Wed, 15 Jan 2025 17:30:47 +0100
Subject: [PATCH] [stable2412] Backport #6971 (#7174)
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 <49718502+alexggh@users.noreply.github.com>
---
.../approval-voting/src/approval_checking.rs | 78 ++++++++++++-------
polkadot/node/core/approval-voting/src/lib.rs | 11 ++-
.../approval-voting/src/persisted_entries.rs | 14 +++-
prdoc/pr_6971.prdoc | 16 ++++
4 files changed, 86 insertions(+), 33 deletions(-)
create mode 100644 prdoc/pr_6971.prdoc
diff --git a/polkadot/node/core/approval-voting/src/approval_checking.rs b/polkadot/node/core/approval-voting/src/approval_checking.rs
index 3b7262a4682..c7f38619ea1 100644
--- a/polkadot/node/core/approval-voting/src/approval_checking.rs
+++ b/polkadot/node/core/approval-voting/src/approval_checking.rs
@@ -712,13 +712,13 @@ mod tests {
}
.into();
- approval_entry.import_assignment(0, ValidatorIndex(0), block_tick);
- approval_entry.import_assignment(0, ValidatorIndex(1), block_tick);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false);
+ approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false);
- approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1);
- approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1, false);
+ approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1, false);
- approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + 2);
+ approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + 2, false);
let approvals = bitvec![u8, BitOrderLsb0; 1; 5];
@@ -757,8 +757,10 @@ mod tests {
}
.into();
- approval_entry.import_assignment(0, ValidatorIndex(0), block_tick);
- approval_entry.import_assignment(1, ValidatorIndex(2), block_tick);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, true);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, false);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, true);
let approvals = bitvec![u8, BitOrderLsb0; 0; 10];
@@ -798,10 +800,10 @@ mod tests {
}
.into();
- approval_entry.import_assignment(0, ValidatorIndex(0), block_tick);
- approval_entry.import_assignment(0, ValidatorIndex(1), block_tick);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false);
+ approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false);
- approval_entry.import_assignment(1, ValidatorIndex(2), block_tick);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, false);
let mut approvals = bitvec![u8, BitOrderLsb0; 0; 10];
approvals.set(0, true);
@@ -844,11 +846,11 @@ mod tests {
}
.into();
- approval_entry.import_assignment(0, ValidatorIndex(0), block_tick);
- approval_entry.import_assignment(0, ValidatorIndex(1), block_tick);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false);
+ approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false);
- approval_entry.import_assignment(1, ValidatorIndex(2), block_tick);
- approval_entry.import_assignment(1, ValidatorIndex(3), block_tick);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick, false);
+ approval_entry.import_assignment(1, ValidatorIndex(3), block_tick, false);
let mut approvals = bitvec![u8, BitOrderLsb0; 0; n_validators];
approvals.set(0, true);
@@ -913,14 +915,24 @@ mod tests {
}
.into();
- approval_entry.import_assignment(0, ValidatorIndex(0), block_tick);
- approval_entry.import_assignment(0, ValidatorIndex(1), block_tick);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false);
+ approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false);
- approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1);
- approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1, false);
+ approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1, false);
- approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + no_show_duration + 2);
- approval_entry.import_assignment(2, ValidatorIndex(5), block_tick + no_show_duration + 2);
+ approval_entry.import_assignment(
+ 2,
+ ValidatorIndex(4),
+ block_tick + no_show_duration + 2,
+ false,
+ );
+ approval_entry.import_assignment(
+ 2,
+ ValidatorIndex(5),
+ block_tick + no_show_duration + 2,
+ false,
+ );
let mut approvals = bitvec![u8, BitOrderLsb0; 0; n_validators];
approvals.set(0, true);
@@ -1007,14 +1019,24 @@ mod tests {
}
.into();
- approval_entry.import_assignment(0, ValidatorIndex(0), block_tick);
- approval_entry.import_assignment(0, ValidatorIndex(1), block_tick);
+ approval_entry.import_assignment(0, ValidatorIndex(0), block_tick, false);
+ approval_entry.import_assignment(0, ValidatorIndex(1), block_tick, false);
- approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1);
- approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1);
+ approval_entry.import_assignment(1, ValidatorIndex(2), block_tick + 1, false);
+ approval_entry.import_assignment(1, ValidatorIndex(3), block_tick + 1, false);
- approval_entry.import_assignment(2, ValidatorIndex(4), block_tick + no_show_duration + 2);
- approval_entry.import_assignment(2, ValidatorIndex(5), block_tick + no_show_duration + 2);
+ approval_entry.import_assignment(
+ 2,
+ ValidatorIndex(4),
+ block_tick + no_show_duration + 2,
+ false,
+ );
+ approval_entry.import_assignment(
+ 2,
+ ValidatorIndex(5),
+ block_tick + no_show_duration + 2,
+ false,
+ );
let mut approvals = bitvec![u8, BitOrderLsb0; 0; n_validators];
approvals.set(0, true);
@@ -1066,7 +1088,7 @@ mod tests {
},
);
- approval_entry.import_assignment(3, ValidatorIndex(6), block_tick);
+ approval_entry.import_assignment(3, ValidatorIndex(6), block_tick, false);
approvals.set(6, true);
let tranche_now = no_show_duration as DelayTranche + 3;
@@ -1176,7 +1198,7 @@ mod tests {
// Populate the requested tranches. The assignments aren't inspected in
// this test.
for &t in &test_tranche {
- approval_entry.import_assignment(t, ValidatorIndex(0), 0)
+ approval_entry.import_assignment(t, ValidatorIndex(0), 0, false);
}
let filled_tranches = filled_tranche_iterator(approval_entry.tranches());
diff --git a/polkadot/node/core/approval-voting/src/lib.rs b/polkadot/node/core/approval-voting/src/lib.rs
index b4c2a6afee0..2deca5a1aba 100644
--- a/polkadot/node/core/approval-voting/src/lib.rs
+++ b/polkadot/node/core/approval-voting/src/lib.rs
@@ -2808,8 +2808,15 @@ where
Vec::new(),
)),
};
- is_duplicate &= approval_entry.is_assigned(assignment.validator);
- approval_entry.import_assignment(tranche, assignment.validator, tick_now);
+
+ let is_duplicate_for_candidate = approval_entry.is_assigned(assignment.validator);
+ is_duplicate &= is_duplicate_for_candidate;
+ approval_entry.import_assignment(
+ tranche,
+ assignment.validator,
+ tick_now,
+ is_duplicate_for_candidate,
+ );
// We've imported a new assignment, so we need to schedule a wake-up for when that might
// no-show.
diff --git a/polkadot/node/core/approval-voting/src/persisted_entries.rs b/polkadot/node/core/approval-voting/src/persisted_entries.rs
index d891af01c3a..238e2c3706e 100644
--- a/polkadot/node/core/approval-voting/src/persisted_entries.rs
+++ b/polkadot/node/core/approval-voting/src/persisted_entries.rs
@@ -172,7 +172,7 @@ impl ApprovalEntry {
});
our.map(|a| {
- self.import_assignment(a.tranche(), a.validator_index(), tick_now);
+ self.import_assignment(a.tranche(), a.validator_index(), tick_now, false);
(a.cert().clone(), a.validator_index(), a.tranche())
})
@@ -197,6 +197,7 @@ impl ApprovalEntry {
tranche: DelayTranche,
validator_index: ValidatorIndex,
tick_now: Tick,
+ is_duplicate: bool,
) {
// linear search probably faster than binary. not many tranches typically.
let idx = match self.tranches.iter().position(|t| t.tranche >= tranche) {
@@ -213,8 +214,15 @@ impl ApprovalEntry {
self.tranches.len() - 1
},
};
-
- self.tranches[idx].assignments.push((validator_index, tick_now));
+ // At restart we might have duplicate assignments because approval-distribution is not
+ // persistent across restarts, so avoid adding duplicates.
+ // We already know if we have seen an assignment from this validator and since this
+ // function is on the hot path we can avoid iterating through tranches by using
+ // !is_duplicate to determine if it is already present in the vector and does not need
+ // adding.
+ if !is_duplicate {
+ self.tranches[idx].assignments.push((validator_index, tick_now));
+ }
self.assigned_validators.set(validator_index.0 as _, true);
}
diff --git a/prdoc/pr_6971.prdoc b/prdoc/pr_6971.prdoc
new file mode 100644
index 00000000000..4790d773fee
--- /dev/null
+++ b/prdoc/pr_6971.prdoc
@@ -0,0 +1,16 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Make importing of duplicate assignment idempotent
+
+doc:
+ - audience: Node Dev
+ description: |
+ 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 inserting only assignments that are not duplicate.
+
+crates:
+ - name: polkadot-node-core-approval-voting
+ bump: minor
--
GitLab