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

Reduce number of unwraps in chain crate #2679

Merged
merged 3 commits into from
Mar 17, 2019
Merged
Show file tree
Hide file tree
Changes from 2 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
31 changes: 18 additions & 13 deletions chain/src/chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -765,11 +765,15 @@ impl Chain {
}

/// Check chain status whether a txhashset downloading is needed
pub fn check_txhashset_needed(&self, caller: String, hashes: &mut Option<Vec<Hash>>) -> bool {
pub fn check_txhashset_needed(
&self,
caller: String,
hashes: &mut Option<Vec<Hash>>,
) -> Result<bool, Error> {
let horizon = global::cut_through_horizon() as u64;
let body_head = self.head().unwrap();
let header_head = self.header_head().unwrap();
let sync_head = self.get_sync_head().unwrap();
let body_head = self.head()?;
let header_head = self.header_head()?;
let sync_head = self.get_sync_head()?;

debug!(
"{}: body_head - {}, {}, header_head - {}, {}, sync_head - {}, {}",
Expand All @@ -787,7 +791,7 @@ impl Chain {
"{}: no need txhashset. header_head.total_difficulty: {} <= body_head.total_difficulty: {}",
caller, header_head.total_difficulty, body_head.total_difficulty,
);
return false;
return Ok(false);
}

let mut oldest_height = 0;
Expand Down Expand Up @@ -828,13 +832,14 @@ impl Chain {
"{}: need a state sync for txhashset. oldest block which is not on local chain: {} at {}",
caller, oldest_hash, oldest_height,
);
return true;
Ok(true)
} else {
error!("{}: something is wrong! oldest_height is 0", caller);
return false;
};
Ok(false)
}
} else {
Ok(false)
}
return false;
}

/// Writes a reading view on a txhashset state that's been provided to us.
Expand All @@ -851,7 +856,7 @@ impl Chain {

// Initial check whether this txhashset is needed or not
let mut hashes: Option<Vec<Hash>> = None;
if !self.check_txhashset_needed("txhashset_write".to_owned(), &mut hashes) {
if !self.check_txhashset_needed("txhashset_write".to_owned(), &mut hashes)? {
warn!("txhashset_write: txhashset received but it's not needed! ignored.");
return Err(ErrorKind::InvalidTxHashSet("not needed".to_owned()).into());
}
Expand Down Expand Up @@ -1230,10 +1235,10 @@ impl Chain {
/// Builds an iterator on blocks starting from the current chain head and
/// running backward. Specialized to return information pertaining to block
/// difficulty calculation (timestamp and previous difficulties).
pub fn difficulty_iter(&self) -> store::DifficultyIter<'_> {
let head = self.head().unwrap();
pub fn difficulty_iter(&self) -> Result<store::DifficultyIter<'_>, Error> {
let head = self.head()?;
let store = self.store.clone();
store::DifficultyIter::from(head.last_block_h, store)
Ok(store::DifficultyIter::from(head.last_block_h, store))
}

/// Check whether we have a block without reading it
Expand Down
26 changes: 15 additions & 11 deletions chain/src/pipe.rs
Original file line number Diff line number Diff line change
Expand Up @@ -183,16 +183,21 @@ pub fn sync_block_headers(
headers: &[BlockHeader],
ctx: &mut BlockContext<'_>,
) -> Result<Option<Tip>, Error> {
if let Some(header) = headers.first() {
debug!(
"pipe: sync_block_headers: {} headers from {} at {}",
headers.len(),
header.hash(),
header.height,
);
} else {
return Ok(None);
}
let first_header = match headers.first() {
Some(header) => {
debug!(
"pipe: sync_block_headers: {} headers from {} at {}",
headers.len(),
header.hash(),
header.height,
);
header
}
None => {
error!("failed to get the first header");
return Ok(None);
}
};

let all_known = if let Some(last_header) = headers.last() {
ctx.batch.get_block_header(&last_header.hash()).is_ok()
Expand All @@ -201,7 +206,6 @@ pub fn sync_block_headers(
};

if !all_known {
let first_header = headers.first().unwrap();
let prev_header = ctx.batch.get_previous_header(&first_header)?;
txhashset::sync_extending(&mut ctx.txhashset, &mut ctx.batch, |extension| {
extension.rewind(&prev_header)?;
Expand Down
2 changes: 1 addition & 1 deletion chain/src/store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ impl<'a> Batch<'a> {
/// Clear all entries from the output_pos index (must be rebuilt after).
pub fn clear_output_pos(&self) -> Result<(), Error> {
let key = to_key(COMMIT_POS_PREFIX, &mut "".to_string().into_bytes());
for (k, _) in self.db.iter::<u64>(&key).unwrap() {
for (k, _) in self.db.iter::<u64>(&key)? {
self.db.delete(&k)?;
}
Ok(())
Expand Down
7 changes: 5 additions & 2 deletions chain/src/txhashset/txhashset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ impl<T: PMMRable> PMMRHandle<T> {
) -> Result<PMMRHandle<T>, Error> {
let path = Path::new(root_dir).join(sub_dir).join(file_name);
fs::create_dir_all(path.clone())?;
let backend = PMMRBackend::new(path.to_str().unwrap().to_string(), prunable, header)?;
let path_str = path.to_str().ok_or(Error::from(ErrorKind::Other(
"invalid file path".to_owned(),
)))?;
let backend = PMMRBackend::new(path_str.to_string(), prunable, header)?;
let last_pos = backend.unpruned_size();
Ok(PMMRHandle { backend, last_pos })
}
Expand Down Expand Up @@ -1470,7 +1473,7 @@ fn expected_file(path: &Path) -> bool {
)
.as_str()
)
.unwrap();
.expect("invalid txhashset regular expression");
}
RE.is_match(&s_path)
}
Expand Down
2 changes: 1 addition & 1 deletion chain/tests/data_file_integrity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ fn data_files() {

for n in 1..4 {
let prev = chain.head_header().unwrap();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
let pk = ExtKeychainPath::new(1, n as u32, 0, 0, 0).to_identifier();
let reward = libtx::reward::output(&keychain, &pk, 0).unwrap();
let mut b =
Expand Down
6 changes: 3 additions & 3 deletions chain/tests/mine_simple_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ where

for n in 1..4 {
let prev = chain.head_header().unwrap();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
let pk = ExtKeychainPath::new(1, n as u32, 0, 0, 0).to_identifier();
let reward = libtx::reward::output(keychain, &pk, 0).unwrap();
let mut b =
Expand Down Expand Up @@ -409,7 +409,7 @@ fn output_header_mappings() {

for n in 1..15 {
let prev = chain.head_header().unwrap();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
let pk = ExtKeychainPath::new(1, n as u32, 0, 0, 0).to_identifier();
let reward = libtx::reward::output(&keychain, &pk, 0).unwrap();
reward_outputs.push(reward.0.clone());
Expand Down Expand Up @@ -545,7 +545,7 @@ fn actual_diff_iter_output() {
Arc::new(Mutex::new(StopState::new())),
)
.unwrap();
let iter = chain.difficulty_iter();
let iter = chain.difficulty_iter().unwrap();
let mut last_time = 0;
let mut first = true;
for elem in iter.into_iter() {
Expand Down
8 changes: 4 additions & 4 deletions chain/tests/test_coinbase_maturity.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ fn test_coinbase_maturity() {
let key_id3 = ExtKeychainPath::new(1, 3, 0, 0, 0).to_identifier();
let key_id4 = ExtKeychainPath::new(1, 4, 0, 0, 0).to_identifier();

let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
let reward = libtx::reward::output(&keychain, &key_id1, 0).unwrap();
let mut block = core::core::Block::new(&prev, vec![], Difficulty::min(), reward).unwrap();
block.header.timestamp = prev.timestamp + Duration::seconds(60);
Expand Down Expand Up @@ -113,7 +113,7 @@ fn test_coinbase_maturity() {
let fees = txs.iter().map(|tx| tx.fee()).sum();
let reward = libtx::reward::output(&keychain, &key_id3, fees).unwrap();
let mut block = core::core::Block::new(&prev, txs, Difficulty::min(), reward).unwrap();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
block.header.timestamp = prev.timestamp + Duration::seconds(60);
block.header.pow.secondary_scaling = next_header_info.secondary_scaling;

Expand Down Expand Up @@ -147,7 +147,7 @@ fn test_coinbase_maturity() {

let reward = libtx::reward::output(&keychain, &pk, 0).unwrap();
let mut block = core::core::Block::new(&prev, vec![], Difficulty::min(), reward).unwrap();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
block.header.timestamp = prev.timestamp + Duration::seconds(60);
block.header.pow.secondary_scaling = next_header_info.secondary_scaling;

Expand All @@ -172,7 +172,7 @@ fn test_coinbase_maturity() {

let txs = vec![coinbase_txn];
let fees = txs.iter().map(|tx| tx.fee()).sum();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
let reward = libtx::reward::output(&keychain, &key_id4, fees).unwrap();
let mut block = core::core::Block::new(&prev, txs, Difficulty::min(), reward).unwrap();

Expand Down
2 changes: 1 addition & 1 deletion servers/src/grin/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,7 @@ impl Server {
// for release
let diff_stats = {
let last_blocks: Vec<consensus::HeaderInfo> =
global::difficulty_data_to_vector(self.chain.difficulty_iter())
global::difficulty_data_to_vector(self.chain.difficulty_iter()?)
.into_iter()
.collect();

Expand Down
9 changes: 8 additions & 1 deletion servers/src/grin/sync/body_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,10 +69,17 @@ impl BodySync {
/// Return true if txhashset download is needed (when requested block is under the horizon).
fn body_sync(&mut self) -> bool {
let mut hashes: Option<Vec<Hash>> = Some(vec![]);
if self
let txhashset_needed = match self
.chain
.check_txhashset_needed("body_sync".to_owned(), &mut hashes)
{
Ok(v) => v,
Err(e) => {
error!("body_sync: failed to call txhashset_needed: {:?}", e);
return false;
}
};
if txhashset_needed {
debug!(
"body_sync: cannot sync full blocks earlier than horizon. will request txhashset",
);
Expand Down
12 changes: 9 additions & 3 deletions servers/src/grin/sync/syncer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,15 @@ impl SyncRunner {
}
} else {
// sum the last 5 difficulties to give us the threshold
let threshold = self
.chain
.difficulty_iter()
let diff_iter = match self.chain.difficulty_iter() {
Copy link
Member

Choose a reason for hiding this comment

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

Note we instantiated the difficulty_iter inline prior to this change. So it immediately went out of scope.
The difficulty_iter maintains a lock on lmdb db and we need to be very careful with how we use it and in what scope.

Its been the source of a lot of pain and debugging...

That said I'm pretty sure this is safe to do here as we are scoped to the else { ... } block.
But it is something we should confirm.

We may want to rework this slightly to ensure the scope of the difficulty_iter is more explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@antiochp I reduced the scope

Ok(v) => v,
Err(e) => {
error!("failed to get difficulty iterator: {:?}", e);
// we handle 0 height in the caller
return (false, 0);
}
};
let threshold = diff_iter
.map(|x| x.difficulty)
.take(5)
.fold(Difficulty::zero(), |sum, val| sum + val);
Expand Down
2 changes: 1 addition & 1 deletion servers/src/mining/mine_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ fn build_block(

// Determine the difficulty our block should be at.
// Note: do not keep the difficulty_iter in scope (it has an active batch).
let difficulty = consensus::next_difficulty(head.height + 1, chain.difficulty_iter());
let difficulty = consensus::next_difficulty(head.height + 1, chain.difficulty_iter()?);

// Extract current "mineable" transactions from the pool.
// If this fails for *any* reason then fallback to an empty vec of txs.
Expand Down
2 changes: 1 addition & 1 deletion wallet/src/test_framework/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ fn get_outputs_by_pmmr_index_local(
/// Adds a block with a given reward to the chain and mines it
pub fn add_block_with_reward(chain: &Chain, txs: Vec<&Transaction>, reward: CbData) {
let prev = chain.head_header().unwrap();
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter());
let next_header_info = consensus::next_difficulty(1, chain.difficulty_iter().unwrap());
let out_bin = util::from_hex(reward.output).unwrap();
let kern_bin = util::from_hex(reward.kernel).unwrap();
let output = ser::deserialize(&mut &out_bin[..]).unwrap();
Expand Down