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

Only consider target_block_height when relevant #2780

Merged
merged 8 commits into from
Jul 29, 2021
Merged

Only consider target_block_height when relevant #2780

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7d7479b
fix: only consider target block if relevant
lgalabru Jul 26, 2021
e9f01fd
chore: cargo fmt
lgalabru Jul 26, 2021
b510e02
chore: add log when ignoring target
lgalabru Jul 26, 2021
f67eaeb
Update src/burnchains/burnchain.rs
lgalabru Jul 27, 2021
7ceec25
Update src/burnchains/burnchain.rs
lgalabru Jul 27, 2021
56c776d
fix: log
lgalabru Jul 27, 2021
d520e83
Merge branch 'fix/graceful-restart' of github.com:blockstack/stacks-b…
lgalabru Jul 27, 2021
5a4a392
Merge branch 'develop' into fix/graceful-restart
lgalabru Jul 28, 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
13 changes: 11 additions & 2 deletions src/burnchains/burnchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1209,16 +1209,25 @@ impl Burnchain {

debug!(
"Sync'ed headers from {} to {}. DB at {}",
start_block, end_block, db_height
sync_height, end_block, db_height
);
Copy link
Contributor

@pavitthrap pavitthrap Jul 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think the debug statement above should be

 debug!(
            "Sync'ed headers from {} to {}. DB at {}",
            sync_height, end_block, db_height
         );

Could you fix this as part of this pr?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed with latest


if let Some(target_block_height) = target_block_height_opt {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, there's no test? How was this change tested and why do we know it is an improvement?

Copy link
Contributor

Choose a reason for hiding this comment

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

+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.

So, no, unfortunately there's no test.
This change was tested locally with a copy the chainstate of a miner that was stuck because of this bug, and this branch was then directly deployed by @CharlieC3 to the miner on the same chainstate, and that helped us unblocking the miner - looks like an improvement.

The code coverage for this routine is pretty bad because this function is doing way too many things.
I think we could rewrite this routine, extract the logic dealing with the boundaries and target block (there are a lot of if in there that would deserve some testing), and unit test that logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK. Can you just summarize this in the PR description?

if target_block_height < end_block {
// `target_block_height` is used as a hint, but could also be completely off
// in certain situations. This function is directly reading the
// headers and syncing with the bitcoin-node, and the interval of blocks
// to download computed here should be considered as our source of truth.
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the comment on line 1144 need to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so. This function is still returning a burnchain tip, at least as high as target_block_height_opt if given.

if target_block_height > start_block && target_block_height < end_block {
debug!(
"Will download up to max burn block height {}",
target_block_height
);
end_block = target_block_height;
} else {
debug!(
"Ignoring target block height {} considered as irrelevant",
target_block_height
);
}
}

Expand Down