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.

Sign up for GitHub

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

Jump to bottom

Add tenure_extend_timestamp to Block Response Accept messages #5466

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Add tenure_extend_timestamp to Block Response Accept messages #5466

merged 3 commits into from
Nov 18, 2024
+76 −10

Conversation

jferrant
Copy link
Collaborator

Needed to complete #5434

@jferrant jferrant requested review from a team as code owners November 15, 2024 21:19
jferrant
jferrant commented Nov 15, 2024
View reviewed changes
stacks-signer/src/tests/conf/signer-0.toml
@@ -1,6 +1,6 @@
stacks_private_key = "6a1fc1a3183018c6d79a4e11e154d2bdad2d89ac8bc1b0a021de8b4d28774fbb01"
node_host = "127.0.0.1:20443"
endpoint = "localhost:30000"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed this to make config to str test pass.

hstove
hstove requested changes Nov 15, 2024
View reviewed changes
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.

This needs to be backwards compatible - right now it'll fail if the payload doesn't have the timestamp. I think there's already a unit test with fixtures for backwards compatibility

@jferrant
Add tenure_extend_timestamp to Block Response Reject and Accept messages
52e0d1f 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@jferrant jferrant force-pushed the chore/add-timestamp-to-block-response branch from db013bc to 52e0d1f Compare November 15, 2024 22:03
@jferrant
Copy link
Collaborator Author

jferrant commented Nov 15, 2024 •
edited
Loading

This needs to be backwards compatible - right now it'll fail if the payload doesn't have the timestamp. I think there's already a unit test with fixtures for backwards compatibility

I mean I can easily make this default fill the timestamp with u64::MAX if not found. Not sure that is considered backwards compatible enough. I don't actually see why this has to be backwards compatible though. It would of course require signers to upgrade, by why does this need to be backwards compatible? (I just assumed not required as we do not back process Block Responses nor do we store them anywhere internally in the signers DB....EDIT: I think I just answered my question though. miners would have issues.)

@jferrant jferrant requested a review from hstove November 15, 2024 22:04
@jferrant
Change to use u64::MAX in default value of tenure_extend_timestamp
9a327c4 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
@obycode
Copy link
Contributor

obycode commented Nov 15, 2024

I don't actually see why this has to be backwards compatible though.

Since we are doing this without a hard fork, miners that upgrade should be able to talk to signers that have not upgraded, and vice versa.

@jferrant
Copy link
Collaborator Author

I don't actually see why this has to be backwards compatible though.

Since we are doing this without a hard fork, miners that upgrade should be able to talk to signers that have not upgraded, and vice versa.

haha I just answered this in my own response to Hank right before I saw this XD

@jferrant jferrant changed the title Add tenure_extend_timestamp to Block Response Reject and Accept messages Add tenure_extend_timestamp to Block Response Accept messages Nov 15, 2024
@jferrant
Fix failing test
9b4b89c 
Signed-off-by: Jacinta Ferrant <jacinta@trustmachines.co>
hstove
hstove approved these changes Nov 16, 2024
View reviewed changes
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! Should we merge this into a broader feature branch? This PR itself is probably fine to merge into develop, but with other PRs we'll probably want some other base branch.

@obycode
Copy link
Contributor

obycode commented Nov 17, 2024 •
edited
Loading

LGTM! Should we merge this into a broader feature branch? This PR itself is probably fine to merge into develop, but with other PRs we'll probably want some other base branch.

Yeah, that's a good point. Let's merge everything into feat/time-based-tenure-extend until we're ready.

I updated the target branch for this PR.

@obycode obycode changed the base branch from develop to feat/time-based-tenure-extend November 17, 2024 14:23
obycode
obycode approved these changes Nov 17, 2024
View reviewed changes
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

@aldur aldur merged commit a8c09a1 into feat/time-based-tenure-extend Nov 18, 2024
143 of 147 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@obycode obycode obycode approved these changes

@hstove hstove hstove approved these changes

@jcnelson jcnelson Awaiting requested review from jcnelson

@kantai kantai Awaiting requested review from kantai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jferrant @obycode @hstove @aldur