Only consider target_block_height when relevant #2780
Conversation
lgalabru
changed the title
| @@ -1213,12 +1213,21 @@ impl Burnchain { | |||
| ); | |||
|
|
|||
| if let Some(target_block_height) = target_block_height_opt { | |||
There was a problem hiding this comment.
So, there's no test? How was this change tested and why do we know it is an improvement?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
OK. Can you just summarize this in the PR description?
| // target_block_height is used as a hint, but could also be completely off | ||
| // in certain situations. The current 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. |
There was a problem hiding this comment.
Does the comment on line 1144 need to change?
There was a problem hiding this comment.
I don't think so. This function is still returning a burnchain tip, at least as high as target_block_height_opt if given.
|
Also.. is there an Issue for this? |
| @@ -1213,12 +1213,21 @@ impl Burnchain { | |||
| ); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Addressed with latest
…lockchain into fix/graceful-restart
|
@gregorycoppola yes there's an issue - #2783 |
| @@ -1213,12 +1213,21 @@ impl Burnchain { | |||
| ); | |||
|
|
|||
| if let Some(target_block_height) = target_block_height_opt { | |||
There was a problem hiding this comment.
OK. Can you just summarize this in the PR description?
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
With this fix, we're only considering the
target_block_heightif this value is relevant.The
sync_with_indexerfunction is reading the burnchain state from the bitcoin-node and should be considered as the source of truth, and should ignore / correct the hint when it's determined as off.Address #2783.