Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Elmattic/actors review F12 #1290

Merged
merged 16 commits into from
Dec 7, 2021
Merged

Elmattic/actors review F12 #1290

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
6146cf1
Add test for incorrect version
elmattic Nov 22, 2021
a3fd90d
Fix when last block is a varint that is not minimally encoded
elmattic Nov 22, 2021
d07a80e
Fix when a varint is not minimally encoded
elmattic Nov 22, 2021
8254d23
Add test that checks if a varint takes more than 9 bytes
elmattic Nov 22, 2021
e7d1527
Fix total running length that could overflow
elmattic Nov 22, 2021
5ba2317
Add test of a valid varint taking one byte
elmattic Nov 22, 2021
f853420
Add test when a varint has a length of 0
elmattic Nov 22, 2021
24a0d73
Add tests related to uniqueness of RLE+ encoding
elmattic Nov 22, 2021
d628ba7
Fix invalid error code and use the same message as in specification
elmattic Nov 22, 2021
48ecdbe
Refactor test to use more idiomatic code
elmattic Nov 24, 2021
b626395
Remove unnecessary assert
elmattic Nov 24, 2021
9fc5615
Remove unnecessary temporary binding
elmattic Nov 24, 2021
2b67f0c
Merge branch 'main' of https://github.com/chainsafe/forest into elmat…
elmattic Dec 1, 2021
0320ab2
Merge branch 'main' of github.com:ChainSafe/forest into elmattic/acto…
noot Dec 7, 2021
1a4daf0
Merge branch 'main' of github.com:ChainSafe/forest into elmattic/acto…
noot Dec 7, 2021
42c7d8d
fmt
noot Dec 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 139 additions & 8 deletions utils/bitfield/src/rleplus/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,12 @@ impl<'de> Deserialize<'de> for BitField {
impl BitField {
/// Decodes RLE+ encoded bytes into a bit field.
pub fn from_bytes(bytes: &[u8]) -> Result<Self> {
if let Some(value) = bytes.last() {
if *value == 0 {
return Err("not minimally encoded");
}
}

let mut reader = BitReader::new(bytes);

let version = reader.read(2);
Expand All @@ -116,8 +122,15 @@ impl BitField {
let mut next_value = reader.read(1) == 1;
let mut ranges = Vec::new();
let mut index = 0;
let mut total_len: u64 = 0;

while let Some(len) = reader.read_len()? {
let (new_total_len, ovf) = total_len.overflowing_add(len as u64);
if ovf {
return Err("RLE+ overflow");
}
total_len = new_total_len;

let start = index;
index += len;
let end = index;
Expand Down Expand Up @@ -182,15 +195,24 @@ mod tests {
#[test]
fn test() {
for (bits, expected) in vec![
(vec![], bitfield![]),
(vec![], Ok(bitfield![])),
(
vec![
1, 0, // incorrect version
1, // starts with 1
0, 1, // fits into 4 bits
0, 0, 0, 1, // 8 - 1
],
Err("incorrect version"),
),
(
vec![
0, 0, // version
1, // starts with 1
0, 1, // fits into 4 bits
0, 0, 0, 1, // 8 - 1
],
bitfield![1, 1, 1, 1, 1, 1, 1, 1],
Ok(bitfield![1, 1, 1, 1, 1, 1, 1, 1]),
),
(
vec![
Expand All @@ -202,7 +224,7 @@ mod tests {
0, 1, // fits into 4 bits
1, 1, 0, 0, // 3 - 1
],
bitfield![1, 1, 1, 1, 0, 1, 1, 1],
Ok(bitfield![1, 1, 1, 1, 0, 1, 1, 1]),
),
(
vec![
Expand All @@ -211,9 +233,9 @@ mod tests {
0, 0, // does not fit into 4 bits
1, 0, 0, 1, 1, 0, 0, 0, // 25 - 1
],
bitfield![
Ok(bitfield![
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1
],
]),
),
// when a length of 0 is encountered, the rest of the encoded bits should be ignored
(
Expand All @@ -225,15 +247,124 @@ mod tests {
0, 0, 0, 0, // 0 - 0
1, // 1 - 1
],
bitfield![1],
Ok(bitfield![1]),
),
// when a length of 0 is encountered, the rest of the encoded bits should be ignored
(
vec![
0, 0, // version
1, // starts with 1
1, // 1 - 1
0, 0, // fits into a varint
0, 0, 0, 0, 0, 0, 0, 0, // 0 - 0
1, // 1 - 1
],
Ok(bitfield![1]),
),
// when the last byte is zero, this should fail
(
vec![
0, 0, // version
1, // starts with 1
0, 1, // fits into 4 bits
1, 0, 1, // 5 - 1
0, 0, 0, 0, 0, 0, 0, 0,
],
Err("not minimally encoded"),
),
// a valid varint
(
vec![
0, 0, // version
1, // starts with 1
0, 0, // fits into a varint
1, 0, 0, 0, 1, 0, 0, 0, // 17 - 1
0, 0, 0,
],
Ok(bitfield![1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1]),
),
// a varint that is not minimally encoded
(
vec![
0, 0, // version
1, // starts with 1
0, 0, // fits into a varint
1, 1, 0, 0, 0, 0, 0, 1, // 3 - 1
0, 0, 0, 0, 0, 0, 0, 0, 1, 1, 1,
],
Err("Invalid varint"),
),
// a varint must not take more than 9 bytes
(
vec![
0, 0, // version
1, // starts with 1
0, 0, // fits into a varint
1, 0, 0, 0, 0, 0, 0, 1, // 1 - 1
0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0,
0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0,
0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0,
],
Err("Invalid varint"),
),
// total running length should not overflow
(
vec![
0, 0, // version
1, // starts with 1
0, 0, // fits into a varint
1, 1, 1, 1, 1, 1, 1, 1, // 9223372036854775807 - 1
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 0, // fits into a varint
1, 1, 1, 1, 1, 1, 1, 1, // 9223372036854775807 - 0
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,
1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 0, 0, 1, // fits into 4 bits
0, 1, 0, 0, // 2 - 1
],
Err("RLE+ overflow"),
),
// block_long that could have fit on block_short. TODO: is this legit?
(
vec![
0, 0, // version
1, // starts with 1
0, 0, // fits into a varint
1, 1, 0, 0, 0, 0, 0, 0, // 3 - 1
1, 1, 1,
],
Ok(bitfield![1, 1, 1, 0, 1, 0]),
),
// block_long that could have fit on block_single. TODO: is this legit?
(
vec![
0, 0, // version
1, // starts with 1
0, 0, // fits into a varint
1, 0, 0, 0, 0, 0, 0, 0, // 1 - 1
1, 1, 1,
],
Ok(bitfield![1, 0, 1, 0]),
),
// block_short that could have fit on block_single. TODO: is this legit?
(
vec![
0, 0, // version
1, // starts with 1
0, 1, // fits into 4 bits
1, 0, 0, 0, // 1 - 1
1, 1, 1, 1, 1, 1, 1,
],
Ok(bitfield![1, 0, 1, 0, 1, 0, 1, 0]),
),
] {
let mut writer = BitWriter::new();
for bit in bits {
writer.write(bit, 1);
}
let bf = BitField::from_bytes(&writer.finish()).unwrap();
assert_eq!(bf, expected);
let res = BitField::from_bytes(&writer.finish_test());
assert_eq!(res, expected);
}
}

Expand Down
7 changes: 6 additions & 1 deletion utils/bitfield/src/rleplus/reader.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,12 @@ impl<'a> BitReader<'a> {
// if the most significant bit is a 0, we've
// reached the end of the varint
if byte & 0x80 == 0 {
return Ok(len);
if i == 0 || byte != 0 {
// only the first byte can be zero in a varint
return Ok(len);
}
// not minimally encoded
break;
}
}

Expand Down
12 changes: 12 additions & 0 deletions utils/bitfield/src/rleplus/writer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,18 @@ impl BitWriter {
}
self.bytes
}

/// Writes any remaining bits to the buffer and returns it.
/// We write remaining bits even if they are are 0s.
/// This method is for testing purpose only.
#[cfg(test)]
pub fn finish_test(mut self) -> Vec<u8> {
if self.num_bits > 0 {
self.bytes.push(self.bits as u8);
}

self.bytes
}
}

#[cfg(test)]
Expand Down
7 changes: 3 additions & 4 deletions vm/actor/src/builtin/miner/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -679,10 +679,9 @@ impl Actor {
BS: BlockStore,
RT: Runtime<BS>,
{
let sector_numbers = params
.sector_numbers
.validate()
.map_err(|e| actor_error!(ErrIllegalArgument, "Invalid Bitfield argument:{}", e))?;
let sector_numbers = params.sector_numbers.validate().map_err(|e| {
actor_error!(ErrIllegalState, "fail to count aggregated sectors: {}", e)
})?;
let agg_sectors_count = sector_numbers.len();

if agg_sectors_count > MAX_AGGREGATED_SECTORS {
Expand Down