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] Cache money supply on memory. Introduce getsupplyinfo #1906

Merged
merged 7 commits into from
Oct 13, 2020

Conversation

random-zebra
Copy link

@random-zebra random-zebra commented Oct 9, 2020

With #1878 the RPC call getinfo becomes awfully slow (it triggers a flush of the chainstate to disk and recalculates the amount).

In this PR we introduce a in-memory global object (instance of a dumb storage class) to cache the money supply, as well as the height of the chain when it was last updated.
The cache is updated in 3 cases:

  • at startup
  • after periodic full flushes to disk (blocks and chainstate), except during shutdown or IBD
  • on-demand via RPC (see below)

A new RPC getsupplyinfo is introduced. It returns the contents of the cached object, after an optional flush/recalculation (if the forceupdate argument is set to true).

The money supply is still part of getinfo output, for backward compatibility, but the fetching is redirected to the new getsupplyinfo (with default value, false, for the forceupdate argument, thus without flush/recalculation).

Examples:

> getblockcount
2542258

> getsupplyinfo
{
  "updateheight": 2542240,
  "supply": 64090963.91257160
}

> getinfo
{
  ...
  "moneysupply": 64090963.91257160
  ...
}

> getmoneysupplyinfo true
{
  "updateheight": 2542258,
  "supply": 64091053.91230406
}

> getinfo
{
  ...
  "moneysupply": 64091053.91230406
  ...
}

Based on top of

Starts with [Core] Introduce CMoneySupply class (6554757)

@random-zebra random-zebra added this to the 5.0.0 milestone Oct 9, 2020
@random-zebra random-zebra self-assigned this Oct 9, 2020
@random-zebra random-zebra force-pushed the 202010_moneysupply-cache branch 2 times, most recently from fbbda68 to b91eef0 Compare October 10, 2020 06:57
@random-zebra
Copy link
Author

Rebased on master. Ready for review.

@Fuzzbawls
Copy link
Collaborator

Concept ACK.

However, something that isn't quite sitting right with me (and is strictly a convention/semantic issue) is the name of the new RPC command: getmoneysupplyinfo.

In our other get***info commands, we typically use a three-word approach to naming: get + section + info, examples:
getNETWORKinfo
getWALLETinfo
getPEERinfo
getADDRESSinfo
getMININGinfo
getMEMPOOLinfo
getFEEinfo
getBUDGETinfo
getBLOCKCHAINinfo ("blockchain" is typically used as a single word throughout)

We do have a few existing outliers though:
getADDEDNODEinfo
getTXOUTSETinfo

Proposing to use getSUPPLYinfo to fit in with the three-word majority convention. It, to me at least, rolls off the tongue a bit more naturally, and I don't think there will be any confusion about the purpose of the command by omitting the "money" part.

Additionally, most 3rd party services simply use "Supply" and not "Money Supply" when referencing the amount of available and/or eventual number of coins.


In hindsight, probably should have brought this up way back when the moneysupply return field was first introduced to the getinfo command, but never thought of a future where we would have a dedicated RPC command for this.

So, now, do we continue the use of a somewhat unique term ("moneysupply"), or do we move forward with a more universal term in the new command ("supply")?

@random-zebra
Copy link
Author

I've no particular preference, as long as it's clear that we are talking about the "spendable" supply (sum of utxo values).
Wouldn't change the output of getinfo (still for backward compatibility), but can definitely rename getmoneysupplyinfo and its output if preferred.

@Fuzzbawls
Copy link
Collaborator

yeah certainly wouldn't change the output field in getinfo as that is needed for backwards compatibility as you said. my entire concern/point was about the NEW introductions going forward.

As for ensuring that the return value is known to be the "spendable" supply, you've already stated as much in the new command's description, which I think is sufficient enough.

@furszy
Copy link

furszy commented Oct 11, 2020

Needs a rebase after #1909 merge.

Dumb storage class. This will be used to cache the sum of spendable
transaction outputs (and the height of the chain when this sum was last
updated).
which returns the current cached MoneySupply contents (total amount and
height) after optional flush/recalculation.
@random-zebra random-zebra changed the title [RPC] Cache money supply on memory. Introduce getmoneysupplyinfo [RPC] Cache money supply on memory. Introduce getsupplyinfo Oct 11, 2020
@random-zebra
Copy link
Author

random-zebra commented Oct 11, 2020

Rebased on master and renamed new rpc getmoneysupplyinfo to getsupplyinfo, as well as its output field "moneysupply" to just "supply", as per @Fuzzbawls suggestion (updated PR title/description too).

Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

The new RPC command will need to be added to the release notes, other than that ACK ae4069e. Nice improvement

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK ae4069e

@furszy furszy merged commit 051719e into PIVX-Project:master Oct 13, 2020
@Fuzzbawls Fuzzbawls added the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Dec 14, 2020
@random-zebra random-zebra removed the Needs Release Notes Placeholder tag for anything needing mention in the "Notable Changes" section of release notes label Jan 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants