From bedc1abec00ef72f57b447c0136937cc39bf5232 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Mon, 3 Dec 2018 17:13:39 +1100 Subject: [PATCH 1/5] Add failing boolean bitfield test --- beacon_chain/utils/boolean-bitfield/src/lib.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/beacon_chain/utils/boolean-bitfield/src/lib.rs b/beacon_chain/utils/boolean-bitfield/src/lib.rs index e0adc64dd..dae3ed32f 100644 --- a/beacon_chain/utils/boolean-bitfield/src/lib.rs +++ b/beacon_chain/utils/boolean-bitfield/src/lib.rs @@ -153,7 +153,7 @@ impl ssz::Decodable for BooleanBitfield { #[cfg(test)] mod tests { use super::*; - use ssz::SszStream; + use ssz::{ssz_encode, Decodable, SszStream}; #[test] fn test_new_bitfield() { @@ -341,4 +341,12 @@ mod tests { let expected = BooleanBitfield::from_elem(18, true); assert_eq!(field, expected); } + + #[test] + fn test_ssz_round_trip() { + let original = BooleanBitfield::from_bytes(&vec![18; 12][..]); + let ssz = ssz_encode(&original); + let (decoded, _) = BooleanBitfield::ssz_decode(&ssz, 0).unwrap(); + assert_eq!(original, decoded); + } } From 564f13be5fba4cf75979c0e7d2d420cf21c7970a Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 3 Dec 2018 20:45:43 -0800 Subject: [PATCH 2/5] fixes bug with serialization logic for boolean bitfield should match the python impl --- .../utils/boolean-bitfield/src/lib.rs | 49 ++++++++++--------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/beacon_chain/utils/boolean-bitfield/src/lib.rs b/beacon_chain/utils/boolean-bitfield/src/lib.rs index dae3ed32f..fadb47083 100644 --- a/beacon_chain/utils/boolean-bitfield/src/lib.rs +++ b/beacon_chain/utils/boolean-bitfield/src/lib.rs @@ -23,6 +23,10 @@ impl BooleanBitfield { Default::default() } + pub fn with_capacity(initial_len: usize) -> Self { + Self::from_elem(initial_len, false) + } + /// Create a new bitfield with the given length `initial_len` and all values set to `bit`. pub fn from_elem(inital_len: usize, bit: bool) -> Self { Self { @@ -100,25 +104,10 @@ impl default::Default for BooleanBitfield { } } -// borrowed from bit_vec crate -fn reverse_bits(byte: u8) -> u8 { - let mut result = 0; - for i in 0..8 { - result = result | ((byte >> i) & 1) << (7 - i); - } - result -} - impl ssz::Encodable for BooleanBitfield { // ssz_append encodes Self according to the `ssz` spec. - // Note that we have to flip the endianness of the encoding with `reverse_bits` to account for an implementation detail of `bit-vec` crate. fn ssz_append(&self, s: &mut ssz::SszStream) { - let bytes: Vec = self - .to_bytes() - .iter() - .map(|&byte| reverse_bits(byte)) - .collect(); - s.append_vec(&bytes); + s.append_vec(&self.to_bytes()) } } @@ -134,10 +123,11 @@ impl ssz::Decodable for BooleanBitfield { } else { let bytes = &bytes[(index + 4)..(index + len + 4)]; - let mut field = BooleanBitfield::from_elem(0, false); + let count = len * 8; + let mut field = BooleanBitfield::with_capacity(count); for (byte_index, byte) in bytes.iter().enumerate() { for i in 0..8 { - let bit = byte & (1 << i); + let bit = byte & (128 >> i); if bit != 0 { field.set(8 * byte_index + i, true); } @@ -317,28 +307,39 @@ mod tests { #[test] fn test_ssz_encode() { - let field = BooleanBitfield::from_elem(5, true); + let field = create_test_bitfield(); let mut stream = SszStream::new(); stream.append(&field); - assert_eq!(stream.drain(), vec![0, 0, 0, 1, 31]); + assert_eq!(stream.drain(), vec![0, 0, 0, 2, 225, 192]); let field = BooleanBitfield::from_elem(18, true); let mut stream = SszStream::new(); stream.append(&field); - assert_eq!(stream.drain(), vec![0, 0, 0, 3, 255, 255, 3]); + assert_eq!(stream.drain(), vec![0, 0, 0, 3, 255, 255, 192]); + } + + fn create_test_bitfield() -> BooleanBitfield { + let count = 2 * 8; + let mut field = BooleanBitfield::with_capacity(count); + + let indices = &[0, 1, 2, 7, 8, 9]; + for &i in indices { + field.set(i, true); + } + field } #[test] fn test_ssz_decode() { - let encoded = vec![0, 0, 0, 1, 31]; + let encoded = vec![0, 0, 0, 2, 225, 192]; let (field, _): (BooleanBitfield, usize) = ssz::decode_ssz(&encoded, 0).unwrap(); - let expected = BooleanBitfield::from_elem(5, true); + let expected = create_test_bitfield(); assert_eq!(field, expected); let encoded = vec![0, 0, 0, 3, 255, 255, 3]; let (field, _): (BooleanBitfield, usize) = ssz::decode_ssz(&encoded, 0).unwrap(); - let expected = BooleanBitfield::from_elem(18, true); + let expected = BooleanBitfield::from_bytes(&[255, 255, 3]); assert_eq!(field, expected); } From 4d43de1ceae1feb8d0b644bce525864498f7879b Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 10 Dec 2018 20:32:44 -0800 Subject: [PATCH 3/5] rustfmt edits --- beacon_chain/validation/src/attestation_validation.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/beacon_chain/validation/src/attestation_validation.rs b/beacon_chain/validation/src/attestation_validation.rs index e31f3ae52..4953a92c3 100644 --- a/beacon_chain/validation/src/attestation_validation.rs +++ b/beacon_chain/validation/src/attestation_validation.rs @@ -85,9 +85,10 @@ where * The slot of this attestation must not be more than cycle_length + 1 distance * from the parent_slot of block that contained it. */ - if a.slot < self - .parent_block_slot - .saturating_sub(u64::from(self.cycle_length).saturating_add(1)) + if a.slot + < self + .parent_block_slot + .saturating_sub(u64::from(self.cycle_length).saturating_add(1)) { return Err(AttestationValidationError::ParentSlotTooLow); } From 1ffd9e10b3215f897eec3bb9a17bdf45070f6ae7 Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 10 Dec 2018 20:33:43 -0800 Subject: [PATCH 4/5] Fixes bug with attestation validation that arose from change to API --- beacon_chain/validation/src/attestation_validation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/beacon_chain/validation/src/attestation_validation.rs b/beacon_chain/validation/src/attestation_validation.rs index 4953a92c3..5d45bfcd2 100644 --- a/beacon_chain/validation/src/attestation_validation.rs +++ b/beacon_chain/validation/src/attestation_validation.rs @@ -135,7 +135,7 @@ where * Allow extra set bits would permit mutliple different byte layouts (and therefore hashes) to * refer to the same AttesationRecord. */ - if a.attester_bitfield.len() > attestation_indices.len() { + if a.attester_bitfield.num_set_bits() > attestation_indices.len() { return Err(AttestationValidationError::InvalidBitfieldEndBits); } From 6c2c42e6b779eccd55159ed76fad5a0e0d8af6ed Mon Sep 17 00:00:00 2001 From: Alex Stokes Date: Mon, 10 Dec 2018 20:34:35 -0800 Subject: [PATCH 5/5] Adds custom `std::cmp::PartialEq` impl Two bitfields now match if they contain the same information. There were some discrepancies before when comparing fields with the same bits set but came from different sources, e.g. off the wire vs created in memory, due to the existence of unset bits in the high byte. --- beacon_chain/utils/boolean-bitfield/src/lib.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/beacon_chain/utils/boolean-bitfield/src/lib.rs b/beacon_chain/utils/boolean-bitfield/src/lib.rs index fadb47083..98518d70c 100644 --- a/beacon_chain/utils/boolean-bitfield/src/lib.rs +++ b/beacon_chain/utils/boolean-bitfield/src/lib.rs @@ -3,11 +3,12 @@ extern crate ssz; use bit_vec::BitVec; +use std::cmp; use std::default; /// A BooleanBitfield represents a set of booleans compactly stored as a vector of bits. /// The BooleanBitfield is given a fixed size during construction. Reads outside of the current size return an out-of-bounds error. Writes outside of the current size expand the size of the set. -#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone)] pub struct BooleanBitfield(BitVec); /// Error represents some reason a request against a bitfield was not satisfied @@ -104,6 +105,14 @@ impl default::Default for BooleanBitfield { } } +impl cmp::PartialEq for BooleanBitfield { + /// Determines equality by comparing the `ssz` encoding of the two candidates. + /// This method ensures that the presence of high-order (empty) bits in the highest byte do not exclude equality when they are in fact representing the same information. + fn eq(&self, other: &Self) -> bool { + ssz::ssz_encode(self) == ssz::ssz_encode(other) + } +} + impl ssz::Encodable for BooleanBitfield { // ssz_append encodes Self according to the `ssz` spec. fn ssz_append(&self, s: &mut ssz::SszStream) {