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

Move future block check into validateHeader #204

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

n8maninger
Copy link
Member

No description provided.

Copy link
Member

@lukechampine lukechampine left a comment

Choose a reason for hiding this comment

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

I'm still bothered by the fact that this isn't technically a consensus rule. It can't be a consensus rule, because the network does not reach consensus on the current time! The network only reaches consensus on "history" -- the list of parent blocks. (This is also why SufficientlyHeavierThan is not a consensus rule: it depends on information other than the history.)

Splitting the checks out like this does make the consensus package slightly harder to use correctly. But I think I'm okay with that. consensus is a low-level package whose only "real" consumer is coreutils/chain, so it can have some sharp edges. In theory, someone else could write their own implementation of chain.Manager with a different ErrFutureBlock threshold, and it would still be 100% compliant with Sia consensus.

func validateHeader(s State, parentID types.BlockID, timestamp time.Time, nonce uint64, id types.BlockID) error {
if parentID != s.Index.ID {
return errors.New("wrong parent ID")
} else if timestamp.After(s.MaxFutureTimestamp(time.Now())) {
Copy link
Member

Choose a reason for hiding this comment

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

this makes the function impure -- calling it at different times gives you different results. The current time should be passed in as an argument instead.

@n8maninger
Copy link
Member Author

n8maninger commented Sep 17, 2024

implementation of chain.Manager with a different ErrFutureBlock threshold, and it would still be 100% compliant with Sia consensus.

This seems accurate, but with a big caveat. coreutils based nodes would refuse to accept the block and the chains would split. The secondary implementation would likely continue on its own unless the coreutils implementation could out mine it. If the secondary implementation does manage to continue on its own we have an unplanned hardfork. That seems like something we should try our best to prevent, but maybe not important in practice?

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

Successfully merging this pull request may close these issues.

2 participants