From 37050f2315638b068527ebbbf34dd44895dfc2f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 4 Oct 2021 10:55:04 +0200 Subject: [PATCH 1/3] Fix unoccupied bitfields If there is an unoccupied bitfield set, we should just ignore it and not keep it for the rest of the logic in `process_bitfields`. --- runtime/parachains/src/inclusion.rs | 37 +++++++++++++---------------- 1 file changed, 16 insertions(+), 21 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 1c37bce3bd27..0c323210b999 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -263,6 +263,14 @@ impl Pallet { // 3. each bitfield has exactly `expected_bits` // 4. signature is valid. let signed_bitfields = { + let occupied_bitmask: BitVec = assigned_paras_record + .iter() + .map(|p| { + p.as_ref() + .map_or(false, |(_id, pending_availability)| pending_availability.is_some()) + }) + .collect(); + let mut last_index = None; let signing_context = SigningContext { @@ -289,6 +297,13 @@ impl Pallet { Error::::ValidatorIndexOutOfBounds, ); + // If there is a bit set that shouldn't bet set, we ignore it. + if occupied_bitmask.clone() & unchecked_bitfield.unchecked_payload().0.clone() != + unchecked_bitfield.unchecked_payload().0 + { + continue + } + let validator_public = &validators[unchecked_bitfield.unchecked_validator_index().0 as usize]; @@ -329,7 +344,7 @@ impl Pallet { }) { *bit = true; } else if cfg!(debug_assertions) { - ensure!(false, Error::::InternalError); + return Err(Error::::InternalError.into()) } } @@ -1401,26 +1416,6 @@ mod tests { .is_err()); } - // non-pending bit set. - { - let mut bare_bitfield = default_bitfield(); - *bare_bitfield.0.get_mut(0).unwrap() = true; - let signed = block_on(sign_bitfield( - &keystore, - &validators[0], - ValidatorIndex(0), - bare_bitfield, - &signing_context, - )); - - assert!(ParaInclusion::process_bitfields( - expected_bits(), - vec![signed.into()], - &core_lookup, - ) - .is_err()); - } - // empty bitfield signed: always OK, but kind of useless. { let bare_bitfield = default_bitfield(); From 02f67e5b5af106b6e7669dd8339c28218f66f65d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 4 Oct 2021 11:05:59 +0200 Subject: [PATCH 2/3] Bring back test, but this time corrected --- runtime/parachains/src/inclusion.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index 0c323210b999..cf98b34ec2e9 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -1416,6 +1416,27 @@ mod tests { .is_err()); } + // non-pending bit set. + { + let mut bare_bitfield = default_bitfield(); + *bare_bitfield.0.get_mut(0).unwrap() = true; + let signed = block_on(sign_bitfield( + &keystore, + &validators[0], + ValidatorIndex(0), + bare_bitfield, + &signing_context, + )); + assert_eq!( + ParaInclusion::process_bitfields( + expected_bits(), + vec![signed.into()], + &core_lookup, + ), + Ok(vec![]) + ); + } + // empty bitfield signed: always OK, but kind of useless. { let bare_bitfield = default_bitfield(); From ed33128cf0d841a25b0d88b90756583799c49d32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastian=20K=C3=B6cher?= Date: Mon, 4 Oct 2021 12:41:45 +0200 Subject: [PATCH 3/3] Remove incorrect code --- runtime/parachains/src/inclusion.rs | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/runtime/parachains/src/inclusion.rs b/runtime/parachains/src/inclusion.rs index cf98b34ec2e9..7150dbbfb798 100644 --- a/runtime/parachains/src/inclusion.rs +++ b/runtime/parachains/src/inclusion.rs @@ -180,8 +180,6 @@ pub mod pallet { NotCollatorSigned, /// The validation data hash does not match expected. ValidationDataHashMismatch, - /// Internal error only returned when compiled with debug assertions. - InternalError, /// The downward message queue is not processed correctly. IncorrectDownwardMessageHandling, /// At least one upward message sent does not pass the acceptance criteria. @@ -263,14 +261,6 @@ impl Pallet { // 3. each bitfield has exactly `expected_bits` // 4. signature is valid. let signed_bitfields = { - let occupied_bitmask: BitVec = assigned_paras_record - .iter() - .map(|p| { - p.as_ref() - .map_or(false, |(_id, pending_availability)| pending_availability.is_some()) - }) - .collect(); - let mut last_index = None; let signing_context = SigningContext { @@ -297,13 +287,6 @@ impl Pallet { Error::::ValidatorIndexOutOfBounds, ); - // If there is a bit set that shouldn't bet set, we ignore it. - if occupied_bitmask.clone() & unchecked_bitfield.unchecked_payload().0.clone() != - unchecked_bitfield.unchecked_payload().0 - { - continue - } - let validator_public = &validators[unchecked_bitfield.unchecked_validator_index().0 as usize]; @@ -343,8 +326,6 @@ impl Pallet { candidate_pending_availability.availability_votes.get_mut(val_idx) }) { *bit = true; - } else if cfg!(debug_assertions) { - return Err(Error::::InternalError.into()) } }