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

Chore/cache timestamp calc #5494

Merged
merged 11 commits into from
Nov 27, 2024

Conversation

jferrant
Copy link
Collaborator

@jferrant jferrant commented Nov 21, 2024

I originally tried to create a db that just tracked a cumulative tenure processing time that would reset each time a new tenure change payload arrived, but these had a lot of edge cases due to out of order blocks. Hank suggested keeping a much smaller db copy of blocks to determine the tenure processing time instead which was much cleaner. I also implemented a cleanup for it since technically this is redundant/heuristic info and don't think we should have to keep it around indefinitely like we do other signer data.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
…previous) into tenure_blocks

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested review from hstove and obycode November 21, 2024 23:39
@jferrant jferrant requested a review from a team as a code owner November 21, 2024 23:39
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Overall LGTM! You have some compilation issues in nakamoto_integrations

Base automatically changed from chore/ignore-stx-transfer-blocks to feat/time-based-tenure-extend November 22, 2024 15:42
…ecution cost of block validation

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
stacks-signer/src/signerdb.rs Outdated Show resolved Hide resolved
@jferrant jferrant requested a review from a team as a code owner November 26, 2024 19:49
@jferrant jferrant requested review from hstove and jcnelson November 26, 2024 19:49
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested a review from obycode November 26, 2024 20:50
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

I could be wrong about the comment of "do we need to query by state" - but that's really all I saw. Nice work!

stacks-signer/src/signerdb.rs Show resolved Hide resolved
stacks-signer/src/signerdb.rs Outdated Show resolved Hide resolved
@jferrant
Copy link
Collaborator Author

I could be wrong about the comment of "do we need to query by state" - but that's really all I saw. Nice work!

You are right! I missed it when converting to use the same table!

…et_tenure_times

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant requested a review from hstove November 26, 2024 21:54
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 pending CI! Please also test it out together with #5471 and see if those tests still work.

DROP TABLE blocks;

ALTER TABLE temp_blocks RENAME TO blocks;"#;

Copy link
Member

Choose a reason for hiding this comment

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

👏 👏 👏

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

This looks a lot better! Before approving, I just had one question about the use of user-define sqlite functions.

@jferrant jferrant requested a review from jcnelson November 26, 2024 22:12
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

Approving pending CI

Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

LGTM! Very clever migration scheme - I like it.

Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant merged commit 2920032 into feat/time-based-tenure-extend Nov 27, 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 Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants