-
Notifications
You must be signed in to change notification settings - Fork 10
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
fix: size 1 anchor trees #565
Conversation
@@ -75,7 +75,7 @@ impl Store for MockAnchorEventService { | |||
highwater: i64, | |||
limit: i64, | |||
) -> Result<Vec<AnchorRequest>> { | |||
let ints = ((highwater as u64)..self.anchor_req_count).take(limit as usize); | |||
let ints = ((highwater as u64 + 1)..=self.anchor_req_count).take(limit as usize); |
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.
this is why all the test vectors channged
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 add a comment why we are adding 1 to the water mark?
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.
LGTM with a comment explaing the off by one.
@@ -75,7 +75,7 @@ impl Store for MockAnchorEventService { | |||
highwater: i64, | |||
limit: i64, | |||
) -> Result<Vec<AnchorRequest>> { | |||
let ints = ((highwater as u64)..self.anchor_req_count).take(limit as usize); | |||
let ints = ((highwater as u64 + 1)..=self.anchor_req_count).take(limit as usize); |
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 add a comment why we are adding 1 to the water mark?
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.
do we know why we didn't see this issue triggered in our integration tests? Specifically this test of single-event anchor batches? https://github.com/3box/ceramic-tests/blob/main/suite/src/__tests__/correctness/slow/ceramic-cas-integration.test.ts#L30
The integration tests didn't have an event with high water mark |
"0" => merkle_node[0].context("missing left node")?, | ||
"1" => merkle_node[1].context("missing right node")?, | ||
_ => return Err(anyhow!("invalid path part in time event path: {}", part)), | ||
if !path.is_empty() { |
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.
huh, your answer to my question about the integration tests makes me think I don't actually understand what the bug was. Wasn't this change here the actual bug fix? What does the high water mark have to do with this?
No description provided.