-
Notifications
You must be signed in to change notification settings - Fork 527
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
common ancestor should return the youngest ancestor instead of the oldest one #2056
Conversation
I think it would be useful to have a unit test of this. Can you write one in |
@@ -770,6 +770,13 @@ struct | |||
let new_root_node = move_root t heir_node in | |||
(* 4.V *) | |||
let garbage = List.bind bad_hashes ~f:(successor_hashes_rec t) in | |||
Logger.info t.logger ~module_:__MODULE__ ~location:__LOC__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also change this to a Logger.trace
?
Transition_frontier.add_breadcrumb_exn frontier ancestor ) | ||
in | ||
let%bind branch1 = | ||
create_breadcrumbs ~logger ~size:(max_length / 2) youngest_ancestor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit, can you randomly generate a size between 1 and max_legnth/2 for both branches? I want to show in the test that we can retrieve the breadcrumbs successfully even with two uneven branches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I would do that.
I don't understand what you're saying here. The oldest common ancestor of two breadcrumbs will always be the root of the transition frontier, which is definitely not what it used to return. What is the change? Given this diagram
What did |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work finding this bug!
in | ||
let%bind branch1 = | ||
create_breadcrumbs ~logger | ||
~size:(1 + Random.int ((max_length / 2) - 1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use QuickCheck to make this try a bunch of possibilities every time you run the test, rather than picking one each time. Also I think the probability of randomly getting two paths of the same length is very low, so add a test that explicitly checks that situation, since that's where the bug was.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite slow to generate breadcrumbs. As a result, we only generate one instance of it. I think it would be a good idea to have a test where the forks have even lengths and where the forks don't have even lengths.
@enolan is right, the bug only happens on branches of the same length. So instead of chosing randomly, I would just make 2 ancestors and 1 child on each path. |
previously it returned the root if the branches were the same length, but you seem to have figured that out.
In this PR, I fixed a bug in
Transition_frontier.get_path_diff
. Namely when we try to find the ancestor for two blocks, we should always try to find the youngest ancestor.