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.

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

Fix reading POW for edge bits 59 and higher #3069

Merged
merged 2 commits into from
Oct 2, 2019
Merged
Changes from all commits
Commits
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
87 changes: 76 additions & 11 deletions core/src/pow/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -391,21 +391,39 @@ impl Proof {
}
}

#[inline(always)]
fn extract_bits(bits: &Vec<u8>, bit_start: usize, bit_count: usize, read_from: usize) -> u64 {
let mut buf: [u8; 8] = [0; 8];
Copy link
Contributor

@tromp tromp Oct 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible in rust to directly assign

let bits64 = u64::from_le_bytes(&bits[read_from..read_from + 8]);

?
if so, that would be much preferable over the extra buffer and copying....

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that, this PR looks fine to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array and slice are different types in Rust, so we can't directly use bits in from_le_bytes

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and there's no function or method we can call on the slice to convert to array?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, remaining comments are just about aesthetics (can always be reconsidered later) and should not hold up this PR...

buf.copy_from_slice(&bits[read_from..read_from + 8]);
if bit_count == 64 {
return u64::from_le_bytes(buf);
}
let skip_bits = bit_start - read_from * 8;
let bit_mask = (1 << bit_count) - 1;
u64::from_le_bytes(buf) >> skip_bits & bit_mask
}

fn read_number(bits: &Vec<u8>, bit_start: usize, bit_count: usize) -> u64 {
if bit_count == 0 {
return 0;
}
let mut buf: [u8; 8] = [0; 8];
let mut byte_start = bit_start / 8;
if byte_start + 8 > bits.len() {
byte_start = bits.len() - 8;
}
buf.copy_from_slice(&bits[byte_start..byte_start + 8]);
buf.reverse();
let mut nonce = u64::from_be_bytes(buf);
nonce = nonce << 64 - (bit_start - byte_start * 8) - bit_count;
nonce >> 64 - bit_count
// find where the first byte to read starts
let mut read_from = bit_start / 8;
// move back if we are too close to the end of bits
if read_from + 8 > bits.len() {
read_from = bits.len() - 8;
}
// calculate max bit we can read up to (+64 bits from the start)
let max_bit_end = (read_from + 8) * 8;
// calculate max bit we want to read
let max_pos = bit_start + bit_count;
// check if we can read it all at once
if max_pos <= max_bit_end {
extract_bits(bits, bit_start, bit_count, read_from)
} else {
let low = extract_bits(bits, bit_start, 8, read_from);
let high = extract_bits(bits, bit_start + 8, bit_count - 8, read_from + 1);
(high << 8) + low
}
}

impl Readable for Proof {
Expand All @@ -420,6 +438,9 @@ impl Readable for Proof {
let nonce_bits = edge_bits as usize;
let bits_len = nonce_bits * global::proofsize();
let bytes_len = BitVec::bytes_len(bits_len);
if bytes_len < 8 {
return Err(ser::Error::CorruptedData);
}
let bits = reader.read_fixed_bytes(bytes_len)?;

for n in 0..global::proofsize() {
Expand Down Expand Up @@ -478,3 +499,47 @@ impl BitVec {
self.bits[pos / 8] |= 1 << (pos % 8) as u8;
}
}

#[cfg(test)]
mod tests {
use super::*;
use crate::ser::{BinReader, BinWriter, ProtocolVersion};
use rand::Rng;
use std::io::Cursor;

#[test]
fn test_proof_rw() {
for edge_bits in 10..64 {
let mut proof = Proof::new(gen_proof(edge_bits as u32));
proof.edge_bits = edge_bits;
let mut buf = Cursor::new(Vec::new());
let mut w = BinWriter::new(&mut buf, ProtocolVersion::local());
if let Err(e) = proof.write(&mut w) {
panic!("failed to write proof {:?}", e);
}
buf.set_position(0);
let mut r = BinReader::new(&mut buf, ProtocolVersion::local());
match Proof::read(&mut r) {
Err(e) => panic!("failed to read proof: {:?}", e),
Ok(p) => assert_eq!(p, proof),
}
}
}

fn gen_proof(bits: u32) -> Vec<u64> {
let mut rng = rand::thread_rng();
let mut v = Vec::with_capacity(42);
for _ in 0..42 {
v.push(rng.gen_range(
u64::pow(2, bits - 1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, isn't it easier to write 1 << (bits - 1) than u64::pow(2, bits - 1) ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's easier to read u64::pow(2, bits - 1), so personally prefer it iff there's no performance hit.

if bits == 64 {
std::u64::MAX
} else {
u64::pow(2, bits)
},
))
}
v
}

}