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

Fix/5249 #5253

Merged
merged 22 commits into from
Oct 3, 2024
Merged

Fix/5249 #5253

merged 22 commits into from
Oct 3, 2024

Conversation

jcnelson
Copy link
Member

Fixes #5249. The bug was that we were not invalidating cached negatives (i.e. marking the absence of a tenure) when the Nakamoto tip advanced. As a result, tenures that did not yet exist as of the Nakamoto tip when the cache was initially populated, but later came into existence, would never be reported in the inventory vector.

Leaving this as a draft until I can add some more test coverage.

…tate between (and including) the new and old tips, since cached data from the old tip will have stored a negative cache result that ought now to be positive in the context of the new tip.
@jcnelson jcnelson marked this pull request as ready for review September 30, 2024 14:59
@jcnelson jcnelson requested a review from a team as a code owner September 30, 2024 14:59
@jcnelson
Copy link
Member Author

Don't let the line count scare you. The fix is very small; over 90% of the new lines are tests.

@jcnelson jcnelson changed the base branch from release/3.0.0.0.0-rc1 to develop September 30, 2024 15:16
stackslib/src/main.rs Outdated Show resolved Hide resolved
stackslib/src/main.rs Outdated Show resolved Hide resolved
stackslib/src/main.rs Outdated Show resolved Hide resolved
stackslib/src/net/inv/nakamoto.rs Show resolved Hide resolved
stackslib/src/net/inv/nakamoto.rs Show resolved Hide resolved
obycode
obycode previously approved these changes Oct 1, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

LGTM

jferrant
jferrant previously approved these changes Oct 1, 2024
@jferrant
Copy link
Collaborator

jferrant commented Oct 1, 2024

I think this is okay, but i have never seen tests::nakamoto_integrations::continue_tenure_extend fail before so wonder if its an issue with this branch...EDIT: I ran it locally in develop and your branch and both pass so I guess this is just a new flakey test. XD

@jcnelson jcnelson dismissed stale reviews from jferrant and obycode via 90b98f0 October 2, 2024 03:42
@hstove
Copy link
Contributor

hstove commented Oct 3, 2024

Confirmed that running this on my Nakamoto Testnet node has fixed the issue of not being able to catch all the way back up to the tip

Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

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

LGTM, just a few comments.

@jcnelson jcnelson requested review from kantai and hstove October 3, 2024 19:00
hstove
hstove previously approved these changes Oct 3, 2024
obycode
obycode previously approved these changes Oct 3, 2024
Copy link
Contributor

@obycode obycode left a comment

Choose a reason for hiding this comment

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

Looks good. Just pointed out a copy pasta in a comment.

@jcnelson jcnelson dismissed stale reviews from obycode and hstove via 638b26a October 3, 2024 19:31
@jcnelson jcnelson added this pull request to the merge queue Oct 3, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Oct 3, 2024
@jcnelson jcnelson added this pull request to the merge queue Oct 3, 2024
Merged via the queue into develop with commit 84a0f9a Oct 3, 2024
1 check passed
@blockstack-devops
Copy link
Contributor

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.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Oct 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants