[rpc] getblockchaininfo: add size_on_disk, prune_target_size

[esotericnonsense]

No description provided.

[esotericnonsense]

If #11359 gets in it might be nice to have the prune hwm in there too, but I didn't want to add the dependency.

[jnewbery]

concept ACK. One nit inline.

Consider adding an explicit test for the getblockchaininfo RPC method to test/functional/blockchain.py.

[jnewbery]

nit: perhaps disk_usage or size_on_disk?

Also consider placing this above pruned so all of the pruning-related fields are grouped.

[promag]

Also align description.

[promag]

IMO we should test the response result. In this case, at least, check that the key exists and value is a number. cc @jnewbery

[promag]

Ah, @jnewbery already recommended a test! 👍

[promag]

Missing lock of cs_LastBlockFile.

[promag]

Test for getblockchaininfo added in #11370.

[esotericnonsense]

A few mishaps with the commit message there, sorry.

I have merged this with #11366 and closed #11366.

[jnewbery]

one more documentation nit. Otherwise looks good to me

[jnewbery]

The help text for pruneheight should also include "(only present if pruning is enabled)"

[esotericnonsense]

4a61af9

[sipa]

I'd rather not expose more variables that are private to validation to the outside world.

Perhaps you can instead create a wrapper around CalculateCurrentUsage, which just grabs cs_LastBlockFile and calls the internal one, and expose that? That way the RPC code wouldn't need to know about the existence of that lock eve.

[promag]

Why not just lock cs_LastBlockFile in CalculateCurrentUsage? It's a recursive mutex and IMO should be locked where it's needed (BTW the same for remaining locks...).

[promag]

CBlockIndex* block = ...;

[promag]

Why not just lock cs_LastBlockFile in CalculateCurrentUsage? It's a recursive mutex and IMO should be locked where it's needed (BTW the same for remaining locks...).

[jonasschnelli]

As @sipa already commented, CalculateCurrentUsage() should be responsible for concurrency locking (not the calling code part).

[jonasschnelli]

Indentation seems wrong (others need to space-up).

[esotericnonsense]

I followed the example of 'verificationprogress'; can indent the others further if you think that makes sense.

[jonasschnelli]

Concept ACK

[esotericnonsense]

Rebased on master, moved the lock into CalculateCurrentUsage, CBlockIndex *block => CBlockIndex* block.

Travis failed on one platform (iirc linux64) on a timeout in p2p-segwit last time. Not sure why. We'll see how it goes. edit: looks good.

[sipa]

utACK 071879a

[jnewbery]

I've just tested this with -prune=1 and I get the bizarre result:

→ bitcoin-cli getblockchaininfo
{
...
  "prune_target_size": 18446744073709551615,

That's because of this section in init.cpp:

    nPruneTarget = (uint64_t) nPruneArg * 1024 * 1024;
    if (nPruneArg == 1) {  // manual pruning: -prune=1
        LogPrintf("Block pruning enabled.  Use RPC call pruneblockchain(height) to manually prune block and undo files.\n");
        nPruneTarget = std::numeric_limits<uint64_t>::max();
        fPruneMode = true;
...

Returning the max int for prune_target_size is meaningless and confusing. I recommend you don't return prune_target_size if bitcoind is configured for manual pruning (ie if -prune=1) (and perhaps also add another field pruning_mode that returns whether pruning is automatic or manual).

[esotericnonsense]

Hmmm. Yes, that isn't desirable behaviour.

Options I can see:
a) nPruneArg is exposed from init.cpp, or an fPruneManualMode (in which case it could be explicitly checked for in FindFilesToPrune)
or
b) the RPC checks for int_max to determine whether automatic pruning is disabled

b) fixes the RPC without having to touch pruning code but otherwise seems a bit messy to me.

[MeshCollider]

Or instead of b) just see if gArgs.GetArg("-prune", 0) is 1?
But yeah maybe adding a manual prune bool to validation.cpp would be more sensible

[jnewbery]

I think just testing the value of gArgs.GetArg("-prune", 0) is appropriate for this pr. No need to touch the init or validation code.

[esotericnonsense]

$ grep "prune" ~/.bitcoin/bitcoin.conf
prune=550
$ ./bitcoin-cli getblockchaininfo | grep "prune"
  "pruned": true,
  "pruneheight": 0,
  "pruneauto": true,
  "prune_target_size": 576716800,

$ grep "prune" ~/.bitcoin/bitcoin.conf
prune=1
$ ./bitcoin-cli getblockchaininfo | grep "prune"
  "pruned": true,
  "pruneheight": 0,
  "pruneauto": false,

$ grep "prune" ~/.bitcoin/bitcoin.conf
prune=0
$ ./bitcoin-cli getblockchaininfo | grep "prune"
  "pruned": false,

[promag]

Please rebase with #11370.

[esotericnonsense]

Rebased on master and added tests for the relevant fields in test/functional/blockchain.py.

My box:

...
blockchain.py                  | ✓ Passed  | 13 s
...
ALL                            | ✓ Passed  | 695 s (accumulated) 
Runtime: 180 s

[jnewbery]

current code style is snake_case for variables (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#developer-notes). prune_auto or auto_prune should be fine.

[jnewbery]

nit: Since 1 is a magic number meaning manual pruning, I think it's clearer to use equality like in init.cpp, rather than greater-than:

        bool fPruneAuto = (gArgs.GetArg("-prune", 0) != 1);

[esotericnonsense]

I was torn there, because that actually gives True in the case prune=0, but that case can never happen because execution would skip over the if (fPruneMode) block. I'll change it.

[jnewbery]

Please also use snake_case for RPC fields. I think automatic_pruning is appropriate here.

[jnewbery]

Implementation looks good. I've slightly tested and it works well.

A few style nits inline.

[jnewbery]

nit: Since 1 is a magic number meaning manual pruning, I think it's clearer to use equality like in init.cpp, rather than greater-than:

        bool fPruneAuto = (gArgs.GetArg("-prune", 0) != 1);

[jnewbery]

assert_greater_than_or_equal() is better here (since it will print out the value of res['size_on_disk'] if the assert fails)

[esotericnonsense]

As below I've dropped the 'equal to' because I don't see why it should ever be 0 unless there's some caching going on.

[jnewbery]

No need for assert_equal when comparing a value to True or False. The following is fine:

assert res['pruned']
assert not res['pruneauto']

[jnewbery]

suggest that you change this to assert that size_on_disk is greater than zero. This test would fail unnecessarily if a change in the implementation changed the size on disk.

[esotericnonsense]

Changes made and rebased on master (second push, first was a mistake).

[jnewbery]

Tested ACK 2b73ece

[TheBlueMatt]

utACK 2b73ece

[TheBlueMatt]

While you're at it can you add this lock to PruneOneBlockFile and GetBlockFileInfo (both of which are only called without lock during testing, so its not an actual issue, just annoying).

[esotericnonsense]

Done in b7dfc6c.

[MarcoFalke]

Needs rebase™

[esotericnonsense]

®ebased

[MarcoFalke]

utA©K b7dfc6c

[TheBlueMatt]

re-utACK b7dfc6c. In the future, can you avoid rebasing when squashing unless you need to? Avoiding rebasing onto latest master makes it easier for reviewers to identify what changed between reviews.

[esotericnonsense]

Yes, I can do that in the future. I hadn't considered that.
Do you prefer for each change to be made in an individual commit (with a squash before the PR is implemented)?

[MarcoFalke]

It is fine to just amend the existing commits with fix-ups, as reviewers have the original commits fetched locally. So it is easy to compare the two versions. However, with a rebase on master in between, the reviewer needs to painfully repeat the same rebase in some way.