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

rpcserver: Modify getnetworkhashps -1 blocks logic. #3181

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Sep 2, 2023

Now that the target difficulty is calculated every block as opposed to on an interval, the logic in the getnetworkhashps RPC that treated a negative number of blocks as a signal to calculate back to the most recent difficulty change no longer makes sense.

Thus, this updates the getnetworkhashps RPC to ignore negative values so that the default number of blocks is used instead.

It also updates the RPC server help description accordingly to remove mention of the previous behavior.

@davecgh davecgh added the rpc server api change Issues and/or pull requests that involve a new RPC server version or breaking to change to the API. label Sep 2, 2023
@davecgh davecgh added this to the 1.9.0 milestone Sep 2, 2023
@davecgh davecgh force-pushed the multi_networkhashps_retgt_interval branch from 5a4daf5 to 5b55ff2 Compare September 2, 2023 01:59
Copy link
Member

@jholdstock jholdstock left a comment

Choose a reason for hiding this comment

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

Silently transforming a negative input to the default value is non-intuitive to the caller. IMO that behaviour should either be clearly documented, or negative input should be treated as invalid and an error should be returned.

Now that the target difficulty is calculated every block as opposed to
on an interval, the logic in the getnetworkhashps RPC that treated a
negative number of blocks as a signal to calculate back to the most
recent difficulty change no longer makes sense.

Thus, this updates the getnetworkhashps RPC to ignore negative values
so that the default number of blocks is used instead.

It also updates the RPC server help description accordingly to remove
mention of the previous behavior.
@davecgh davecgh force-pushed the multi_networkhashps_retgt_interval branch from 5b55ff2 to ad96f2c Compare September 3, 2023 03:16
@davecgh
Copy link
Member Author

davecgh commented Sep 3, 2023

Silently transforming a negative input to the default value is non-intuitive to the caller. IMO that behaviour should either be clearly documented, or negative input should be treated as invalid and an error should be returned.

I updated the RPC help to call out the behavior. While I agree that theoretically negative values would be better as invalid and rejected, as I've never really liked that particular inherited feature, changing it now would be a breaking change the API. You're right to call out that it should be documented (and now it is).

Also, as it stands, "-1" (or really negative values in general) have always had a dynamic and ambiguous nature to them since the number of blocks it represented changed with every block. As a result, it has always been best interpreted as "give me a useful value" since values across retarget intervals when those intervals are large are otherwise significantly skewed and not particularly accurate.

However, now that the difficulty changes with every block, that longer applies and therefore treating "-1" as the default number of blocks can still be interpreted as "give me a useful value".

Given that, I decided to avoid breaking the API and keep the spirit of it.

In some unspecified future if we ever create a per-version RPC where the legacy versions are still available but callers can opt into newer versions, I would be in favor of disallowing negative values.

@davecgh davecgh merged commit ad96f2c into decred:master Sep 9, 2023
2 checks passed
@davecgh davecgh deleted the multi_networkhashps_retgt_interval branch September 9, 2023 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rpc server api change Issues and/or pull requests that involve a new RPC server version or breaking to change to the API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants