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

[RPC] Reworked getnetworkhashps with tweaks to -mine and getmininginfo #876

Merged
merged 2 commits into from
Dec 10, 2020

Conversation

CaveSpectre11
Copy link
Collaborator

[Problem]
#340 - getnetworkhashps seems to be unrelated to actual network hash
#32 - getmininginfo has vastly changing difficulties

[Root Cause]
As greatly discussed in #340, the getnetworkhashps never accurately reflected PoW vs. PoS; as if one of the blocks was a PoS block, and one was a PoW block, the average calculated would vary wildly. Likewise, getmininginfo would only report on the last block, which gets even crazier with 4 different block types.

[Solution]
First was a need to get your current mining algorithm; this is a separate commit. The original code was getting the mining algorithm only when generation was started; so the startup flag sat unused until that time. This also had the side effect of not realizing you would be mining randomX, until you started mining, due to a typo in the -mine flag [e.g. sha256 instead of sha256d. By changing the code to read the flag on startup, it opened the ability to sanity check it on startup; so users using an incorrect flag will now be told so. This also opens the ability to switch mining algorithms without stopping the daemon [although that's for a future PR, as care is needed to make sure you're told to stop mining before switching algorithms].

% veild -mine=sha256
Error: Invalid mining algorithm: sha256

With that out of the way, getnetworkhashps was expanded to still function as would be expected, but instead of just randomly using whatever blocks it found; it now focuses on only the blocks related to the currently set algorithm. e.g. if you want the average network hashes per second over the last 120 blocks [the default] it will be between the last 120 blocks of the specific algorithm you're configured for. An algorithm parameter was added so that you can specify a different algorithm if you chose, including the historical xr16t, or if you care about PoS. Note that this is actually calculating the change in difficulty over time, and not hashes. That research was also out of scope for this PR, and if the logic needs to change it will be a separate PR.

The output to getnetworkhashps is also included in getmininginfo, which in testing revealed that getmininginfo displays the difficulty for the last block; and not the difficulty for the algorithm that you're actually mining. So that logic was changed to report the difficulty for whatever algorithm you're set to mine. An 'algorithm' field was also added to the output so it can be clear what algorithm the difficulty relates to:

$ veil-cli getmininginfo
{
  "blocks": 988984,
  "currentblockweight": 0,
  "currentblocktx": 0,
  "algorithm": "progpow",
  "difficulty": 174.107098381071,
  "networkhashps": 22.83160812508121,
  "pooledtx": 2,
  "chain": "main"
}
$ veil-cli getmininginfo
{
  "blocks": 988986,
  "currentblockweight": 0,
  "currentblocktx": 0,
  "algorithm": "sha256d",
  "difficulty": 19.31658925581963,
  "networkhashps": 23.3181708549265,
  "pooledtx": 1,
  "chain": "main"
}

Given the availability of all the difficulties with the getblockchaininfo command, a parameter to get mining info for a different algorithm was not added.

@CaveSpectre11 CaveSpectre11 added Tag: PoW Related to Proof of Work consensus Component: RPC Related to the console commands themselves. Tag: Needs Release Notes Any PR or Issue that needs notable mention in release notes Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 5, 2020
@CaveSpectre11 CaveSpectre11 self-assigned this Dec 5, 2020
Copy link
Collaborator

@codeofalltrades codeofalltrades left a comment

Choose a reason for hiding this comment

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

0807f52
One question about logic.

@codeofalltrades
Copy link
Collaborator

ACK 0807f52

@codeofalltrades codeofalltrades added Code Review: Passed Tag: Waiting For QA A pull review is waiting for QA to test the pull request and removed Tag: Waiting For Code Review Waiting for code review from a core developer labels Dec 6, 2020
@codeofalltrades codeofalltrades merged commit fb2a216 into Veil-Project:master Dec 10, 2020
codeofalltrades added a commit that referenced this pull request Dec 10, 2020
…thms without restarting wallet

6ed4626 [RPC][Mining] Provide ability to change mining algorithm (Cave Spectre)

Pull request description:

  ### Note
  This is built on top of #876 and also contains a couple corrections to get the `getnetworkhashps` help.

  ### Problem
  In order to switch mining algorithms, you must shut down the wallet and restart with a new `-mine=` flag.

  ### Root Cause
  Never implemented

  ### Solution
  Add `setminingalgo` cli command to allow you to switch between different algorithms.  This RPC command requires you to turn off mining before issuing the command and will error if it does not work.

  ### Testing
  ```
  $ veil-cli help setminingalgo
  setminingalgo algorithm

  Changes your mining algorithm to [algorithm].  Note that mining must be turned off when command is used.

  Arguments:
  1. algorithm   (string, required) Algorithm to mine [progpow, randomx, sha256d].

  Result:
  {
    "success": true|false, (boolean) Status of the switch
    "message": "text",     (text) Informational message about the switch
  }

  Examples:
  > veil-cli setminingalgo sha256d
  > curl --user myusername --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "setminingalgo", "params": [sha256d] }' -H 'content-type: text/plain;' http://127.0.0.1:58812/
  ```
  ```
  $ veil-cli setminingalgo randomx
  {
    "success": true,
    "message": "Mining algorithm changed from sha256d to randomx"
  }
   veil-cli getmininginfo
  {
  [...]
    "algorithm": "randomx",
    "difficulty": 0.003707938785486674,
    "networkhashps": 22.73835278085342,
  [...]
  }
  ```
  ```
  $ veil-cli setminingalgo sha257d
  error code: -8
  error message:
  sha257d is not a supported mining type
  $ veil-cli setminingalgo sha256d
  {
    "success": true,
    "message": "Mining algorithm changed from randomx to sha256d"
  }
  $ veil-cli getmininginfo
  {
  [...]
    "algorithm": "sha256d",
    "difficulty": 18.1774015474177,
    "networkhashps": 23.05669884097939,
  [...]
  }
  ```
  ```
  $ veil-cli generatecontinuous true 1
  $ veil-cli setminingalgo progpow
  error code: -8
  error message:
  mining must be stopped to change algorithm
  $ veil-cli generatecontinuous false
  $ veil-cli setminingalgo progpow
  {
    "success": true,
    "message": "Mining algorithm changed from sha256d to progpow"
  }
  $ veil-cli getmininginfo
  {
  [...]
    "algorithm": "progpow",
    "difficulty": 164.7481194885794,
    "networkhashps": 23.16038554216868,
  [...]
  }
  ```

Tree-SHA512: 89c8a5032856afcf48ba0ec6c0c9636d9f451e705efe1b9545c3b1719e7db7897d6721a5bf2663f4e3b0d05600c46acff5b6e9c4dd7d3ec3ab2fc47311a328d4
@CaveSpectre11 CaveSpectre11 added Dev Status: Merged Issue is completely finished. QA: Passed This has passed QA testing and can be merged to master and removed Tag: Waiting For QA A pull review is waiting for QA to test the pull request labels Dec 11, 2020
@CaveSpectre11 CaveSpectre11 deleted the getnetworkhashps branch December 11, 2020 14:21
@CaveSpectre11 CaveSpectre11 added this to the v1.1.1 milestone Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Review: Passed Component: RPC Related to the console commands themselves. Dev Status: Merged Issue is completely finished. QA: Passed This has passed QA testing and can be merged to master Tag: Needs Release Notes Any PR or Issue that needs notable mention in release notes Tag: PoW Related to Proof of Work consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants