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

Add tenure_extend_timestamp to Block Response Accept messages #5466

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

jferrant
Copy link
Collaborator

Needed to complete #5434

@@ -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.

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 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

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
@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
Signed-off-by: Jacinta Ferrant <[email protected]>
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants