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

Added deployments tests from bitcoin #451

Merged
merged 2 commits into from
Sep 1, 2017
Merged

Conversation

svyatonik
Copy link
Contributor

  • fixed deployments calculation

It is a part of #443 fix - I'll return expect() call after synchronizing testnet

@svyatonik svyatonik added A0-pleasereview Pull request needs code review. M4-core Core client code / Rust. labels Aug 31, 2017
@svyatonik svyatonik requested a review from debris August 31, 2017 06:39
if let Some(ref header) = result {
self.block = BlockRef::Hash(header.previous_header_hash.clone());
}
let result = self.block.take().and_then(|block| self.headers.block_header(block));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise it was infinite iterator - don't know if this was intended. Just noticed.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would finish. For genesis block, line 24 would return Hash(0000) and next iteration would not find header for that block :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but what if in first next() call, result is None ? Then self.block will never change && it would became infinite. I've actually stumbled upon this.

Copy link
Contributor

Choose a reason for hiding this comment

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

first None result should always indicate the end of iteration :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad - you're right :) I'll try to remember original reason && revert if this isn't actually a fix.
I'm pretty sure I've seen infinite iterator in construction this_iterator.take(11) - all 11 elements were the same.
But now I think that it possibly was because of bug in my Headers implementation

// number is number of block which is currently validating
// => it is not in database
// we need to make all checks for previous blocks
let number = number.saturating_sub(1);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was the main reason of panic previously

@svyatonik svyatonik added A3-inprogress Pull request is in progress. No review needed at this stage. and removed A0-pleasereview Pull request needs code review. labels Aug 31, 2017
@svyatonik
Copy link
Contributor Author

also fixed typo in HeaderCheck

@svyatonik svyatonik added A0-pleasereview Pull request needs code review. and removed A3-inprogress Pull request is in progress. No review needed at this stage. labels Aug 31, 2017
if let Some(ref header) = result {
self.block = BlockRef::Hash(header.previous_header_hash.clone());
}
let result = self.block.take().and_then(|block| self.headers.block_header(block));
Copy link
Contributor

Choose a reason for hiding this comment

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

It would finish. For genesis block, line 24 would return Hash(0000) and next iteration would not find header for that block :)

(self.header.raw.version < 3 && self.height >= self.consensus_params.bip65_height) ||
(self.header.raw.version < 4 && self.height >= self.consensus_params.bip66_height) {
(self.header.raw.version < 3 && self.height >= self.consensus_params.bip66_height) ||
(self.header.raw.version < 4 && self.height >= self.consensus_params.bip65_height) {
Copy link
Contributor

Choose a reason for hiding this comment

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

great catch!

@debris debris merged commit 0eefd51 into master Sep 1, 2017
@debris debris deleted the proper_fix_deployments branch September 1, 2017 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview Pull request needs code review. M4-core Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants