Skip to content

Commit

Permalink
approval-voting: Make importing of duplicate assignment idempotent (#…
Browse files Browse the repository at this point in the history 
…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)
  • Loading branch information
@alexggh @github-actions
alexggh authored and github-actions[bot] committed Jan 15, 2025
1 parent 2b34f2d commit f92e73a
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 33 deletions.
78 changes: 50 additions & 28 deletions polkadot/node/core/approval-voting/src/approval_checking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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];

Expand Down Expand Up @@ -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];

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Expand Down
11 changes: 9 additions & 2 deletions polkadot/node/core/approval-voting/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2680,8 +2680,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.
Expand Down
14 changes: 11 additions & 3 deletions polkadot/node/core/approval-voting/src/persisted_entries.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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())
})
Expand All @@ -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) {
Expand All @@ -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);
}

Expand Down
16 changes: 16 additions & 0 deletions prdoc/pr_6971.prdoc
View file
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit f92e73a

Please sign in to comment.