[rpc] getblockchaininfo: add size_on_disk, prune_target_size #11367

pull esotericnonsense wants to merge 1 commits into bitcoin:master from esotericnonsense:2017-09-getblockchaininfo-addsize changing 4 files +58 −13
  1. esotericnonsense commented at 8:38 AM on September 19, 2017: contributor

    No description provided.

  2. fanquake added the label RPC/REST/ZMQ on Sep 19, 2017
  3. esotericnonsense commented at 8:54 AM on September 19, 2017: contributor

    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.

  4. in src/rpc/blockchain.cpp:1140 in a2d851fff5 outdated 
    1136 | @@ -1137,6 +1137,8 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1137 |              "  \"chainwork\": \"xxxx\"     (string) total amount of work in active chain, in hexadecimal\n"
1138 |              "  \"pruned\": xx,             (boolean) if the blocks are subject to pruning\n"
1139 |              "  \"pruneheight\": xxxxxx,    (numeric) lowest-height complete block stored\n"
1140 | +            "  \"disk_size\": xxxxxx,    (numeric) the estimated size of the block and undo files on disk\n"
    jnewbery commented at 6:08 PM on September 19, 2017:

    nit: perhaps disk_usage or size_on_disk?

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

    promag commented at 10:35 PM on September 19, 2017:

    Also align description.

    promag commented at 10:37 PM on September 19, 2017:

    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 commented at 10:44 PM on September 19, 2017:

    Ah, @jnewbery already recommended a test! 👍

  5. jnewbery commented at 6:13 PM on September 19, 2017: member

    concept ACK. One nit inline.

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

  6. in src/rpc/blockchain.cpp:1185 in a2d851fff5 outdated 
    1181 | @@ -1180,6 +1182,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1182 |      obj.push_back(Pair("verificationprogress",  GuessVerificationProgress(Params().TxData(), chainActive.Tip())));
1183 |      obj.push_back(Pair("chainwork",             chainActive.Tip()->nChainWork.GetHex()));
1184 |      obj.push_back(Pair("pruned",                fPruneMode));
1185 | +    obj.push_back(Pair("disk_size",             CalculateCurrentUsage()));
    promag commented at 10:45 PM on September 19, 2017:

    Missing lock of cs_LastBlockFile.

  7. promag commented at 6:48 AM on September 20, 2017: member

    Test for getblockchaininfo added in #11370.

  8. esotericnonsense force-pushed on Sep 20, 2017
  9. esotericnonsense renamed this:
    RPC: getblockchaininfo: Add disk_size, prune_target_size
    [rpc] getblockchaininfo: add size_on_disk, prune_target_size
    on Sep 20, 2017
  10. esotericnonsense force-pushed on Sep 20, 2017
  11. esotericnonsense force-pushed on Sep 20, 2017
  12. esotericnonsense commented at 3:05 PM on September 20, 2017: contributor

    A few mishaps with the commit message there, sorry.

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

  13. esotericnonsense force-pushed on Sep 20, 2017
  14. in src/rpc/blockchain.cpp:1140 in ae3cbb994c outdated 
    1134 | @@ -1135,8 +1135,10 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1135 |              "  \"mediantime\": xxxxxx,     (numeric) median time for the current best block\n"
1136 |              "  \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
1137 |              "  \"chainwork\": \"xxxx\"     (string) total amount of work in active chain, in hexadecimal\n"
1138 | +            "  \"size_on_disk\": xxxxxx,   (numeric) the estimated size of the block and undo files on disk\n"
1139 |              "  \"pruned\": xx,             (boolean) if the blocks are subject to pruning\n"
1140 |              "  \"pruneheight\": xxxxxx,    (numeric) lowest-height complete block stored\n"
    jnewbery commented at 7:48 PM on September 20, 2017:

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

    esotericnonsense commented at 11:53 PM on September 20, 2017:

    4a61af9

  15. jnewbery commented at 7:49 PM on September 20, 2017: member

    one more documentation nit. Otherwise looks good to me

  16. esotericnonsense force-pushed on Sep 20, 2017
  17. in src/validation.h:159 in 4a61af9666 outdated 
     155 | @@ -156,6 +156,7 @@ struct BlockHasher
 156 |  
 157 |  extern CScript COINBASE_FLAGS;
 158 |  extern CCriticalSection cs_main;
 159 | +extern CCriticalSection cs_LastBlockFile;
    sipa commented at 12:05 AM on September 21, 2017:

    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 commented at 9:03 AM on September 21, 2017:

    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...).

  18. in src/rpc/blockchain.cpp:1187 in 4a61af9666 outdated 
    1180 | @@ -1179,7 +1181,18 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1181 |      obj.push_back(Pair("mediantime",            (int64_t)chainActive.Tip()->GetMedianTimePast()));
1182 |      obj.push_back(Pair("verificationprogress",  GuessVerificationProgress(Params().TxData(), chainActive.Tip())));
1183 |      obj.push_back(Pair("chainwork",             chainActive.Tip()->nChainWork.GetHex()));
1184 | +    obj.push_back(Pair("size_on_disk",          CalculateCurrentUsage()));
1185 |      obj.push_back(Pair("pruned",                fPruneMode));
1186 | +    if (fPruneMode) {
1187 | +        CBlockIndex *block = chainActive.Tip();
    promag commented at 9:04 AM on September 21, 2017:

    CBlockIndex* block = ...;

  19. in src/rpc/blockchain.cpp:1173 in 4a61af9666 outdated 
    1169 | @@ -1168,7 +1170,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1170 |              + HelpExampleRpc("getblockchaininfo", "")
1171 |          );
1172 |  
1173 | -    LOCK(cs_main);
1174 | +    LOCK2(cs_main, cs_LastBlockFile); // cs_LastBlockFile for CalculateCurrentUsage()
    jonasschnelli commented at 5:24 AM on September 22, 2017:

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

  20. in src/rpc/blockchain.cpp:1141 in 4a61af9666 outdated 
    1134 | @@ -1135,8 +1135,10 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1135 |              "  \"mediantime\": xxxxxx,     (numeric) median time for the current best block\n"
1136 |              "  \"verificationprogress\": xxxx, (numeric) estimate of verification progress [0..1]\n"
1137 |              "  \"chainwork\": \"xxxx\"     (string) total amount of work in active chain, in hexadecimal\n"
1138 | +            "  \"size_on_disk\": xxxxxx,   (numeric) the estimated size of the block and undo files on disk\n"
1139 |              "  \"pruned\": xx,             (boolean) if the blocks are subject to pruning\n"
1140 | -            "  \"pruneheight\": xxxxxx,    (numeric) lowest-height complete block stored\n"
1141 | +            "  \"pruneheight\": xxxxxx,    (numeric) lowest-height complete block stored (only present if pruning is enabled)\n"
1142 | +            "  \"prune_target_size\": xxxxxx,  (numeric) the target size used by pruning (only present if pruning is enabled)\n"
    jonasschnelli commented at 5:25 AM on September 22, 2017:

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

    esotericnonsense commented at 6:24 PM on September 22, 2017:

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

  21. jonasschnelli commented at 5:26 AM on September 22, 2017: contributor

    Concept ACK

  22. esotericnonsense force-pushed on Sep 22, 2017
  23. esotericnonsense commented at 6:25 PM on September 22, 2017: contributor

    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.

  24. sipa commented at 6:53 PM on September 22, 2017: member

    utACK 071879a6263aa286e166f9cf07f409e6df35bc02

  25. jnewbery commented at 8:27 PM on September 22, 2017: member

    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).

  26. esotericnonsense commented at 10:04 PM on September 22, 2017: contributor

    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.

  27. meshcollider commented at 3:06 AM on September 23, 2017: contributor

    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

  28. jnewbery commented at 11:36 AM on September 23, 2017: member

    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.

  29. esotericnonsense force-pushed on Sep 24, 2017
  30. esotericnonsense commented at 5:07 PM on September 24, 2017: contributor 
    $ 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,
  31. promag commented at 8:35 PM on September 24, 2017: member

    Please rebase with #11370.

  32. esotericnonsense force-pushed on Sep 25, 2017
  33. esotericnonsense commented at 10:08 PM on September 25, 2017: contributor

    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
  34. in src/rpc/blockchain.cpp:1196 in bbca675c7a outdated 
    1191 | +            block = block->pprev;
1192 | +        }
1193 | +
1194 | +        obj.push_back(Pair("pruneheight",        block->nHeight));
1195 | +
1196 | +        bool fPruneAuto = (gArgs.GetArg("-prune", 0) > 1);
    jnewbery commented at 1:48 PM on September 26, 2017:

    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 commented at 2:03 PM on September 26, 2017:

    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 commented at 2:36 PM on September 26, 2017:

    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.

  35. in src/rpc/blockchain.cpp:1197 in bbca675c7a outdated 
    1192 | +        }
1193 | +
1194 | +        obj.push_back(Pair("pruneheight",        block->nHeight));
1195 | +
1196 | +        bool fPruneAuto = (gArgs.GetArg("-prune", 0) > 1);
1197 | +        obj.push_back(Pair("pruneauto",          fPruneAuto));
    jnewbery commented at 1:48 PM on September 26, 2017:

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

  36. in test/functional/blockchain.py:79 in bbca675c7a outdated 
      74 | +
  75 |          # pruneheight should be greater or equal to 0
  76 |          assert res['pruneheight'] >= 0
  77 |  
  78 | +        # check other pruning fields given that prune=1
  79 | +        assert_equal(res['pruned'], True)
    jnewbery commented at 1:50 PM on September 26, 2017:

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

    assert res['pruned']
assert not res['pruneauto']
  37. in test/functional/blockchain.py:95 in bbca675c7a outdated 
      92 | +        # check related fields
  93 | +        assert_equal(res['pruned'], True)
  94 | +        assert_equal(res['pruneheight'], 0)
  95 | +        assert_equal(res['pruneauto'], True)
  96 | +        assert_equal(res['prune_target_size'], 576716800)
  97 | +        assert_equal(res['size_on_disk'], 55550)
    jnewbery commented at 1:51 PM on September 26, 2017:

    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.

  38. in test/functional/blockchain.py:71 in bbca675c7a outdated 
      68 | +
  69 | +        # result should have these additional pruning keys if manual pruning is enabled
  70 | +        assert_equal(sorted(res.keys()), sorted(['pruneheight', 'pruneauto'] + keys))
  71 | +
  72 | +        # size_on_disk should be >= 0
  73 | +        assert res['size_on_disk'] >= 0
    jnewbery commented at 1:52 PM on September 26, 2017:

    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 commented at 2:46 PM on September 26, 2017:

    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.

  39. jnewbery commented at 2:09 PM on September 26, 2017: member

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

    A few style nits inline.

  40. esotericnonsense force-pushed on Sep 26, 2017
  41. esotericnonsense force-pushed on Sep 26, 2017
  42. esotericnonsense commented at 2:49 PM on September 26, 2017: contributor

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

  43. jnewbery commented at 3:35 PM on September 26, 2017: member

    Tested ACK 2b73ece6e34961a6745114b6932945692ed987e7

  44. in src/validation.cpp:3238 in 2b73ece6e3 outdated 
    3229 | @@ -3230,8 +3230,10 @@ bool TestBlockValidity(CValidationState& state, const CChainParams& chainparams,
3230 |   */
3231 |  
3232 |  /* Calculate the amount of disk space the block & undo files currently use */
3233 | -static uint64_t CalculateCurrentUsage()
3234 | +uint64_t CalculateCurrentUsage()
3235 |  {
3236 | +    LOCK(cs_LastBlockFile);
    TheBlueMatt commented at 7:35 PM on September 27, 2017:

    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 commented at 6:51 PM on September 29, 2017:

    Done in b7dfc6c4b89b62f9bb79ea009ee103a6299ac005.

  45. TheBlueMatt commented at 8:15 PM on September 27, 2017: member

    utACK 2b73ece6e34961a6745114b6932945692ed987e7

  46. MarcoFalke commented at 1:19 PM on September 29, 2017: member

    Needs rebase™

  47. [rpc] getblockchaininfo: add size_on_disk, prune_target_size, automatic_pruning
    Fix pruneheight help text.
Move fPruneMode block to match output ordering with help text.
Add functional tests for new fields in getblockchaininfo.
    b7dfc6c4b8
  48. esotericnonsense force-pushed on Sep 29, 2017
  49. esotericnonsense commented at 6:51 PM on September 29, 2017: contributor

    ®ebased

  50. MarcoFalke commented at 9:08 AM on September 30, 2017: member

    utA©K b7dfc6c4b89b62f9bb79ea009ee103a6299ac005

  51. TheBlueMatt commented at 8:35 PM on October 2, 2017: member

    re-utACK b7dfc6c4b89b62f9bb79ea009ee103a6299ac005. 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.

  52. esotericnonsense commented at 10:13 PM on October 2, 2017: contributor

    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)?

  53. MarcoFalke commented at 5:58 AM on October 3, 2017: member

    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.

  54. laanwj merged this on Oct 9, 2017
  55. laanwj closed this on Oct 9, 2017
  56. laanwj referenced this in commit 3a93270c55 on Oct 9, 2017
  57. luke-jr referenced this in commit cd46034514 on Nov 11, 2017
  58. luke-jr referenced this in commit fec3a143a9 on Nov 11, 2017
  59. nmarley referenced this in commit 007c906322 on Nov 1, 2018
  60. nmarley referenced this in commit 5462995194 on Nov 1, 2018
  61. nmarley referenced this in commit 2abb2702b1 on Nov 1, 2018
  62. nmarley referenced this in commit 78654534b9 on Nov 4, 2018
  63. bitcartel referenced this in commit 719508e703 on Nov 14, 2018
  64. renuzit referenced this in commit 4232bc4fed on May 16, 2019
  65. milesmanley referenced this in commit 1b83c1dd5c on May 16, 2019
  66. garethtdavies referenced this in commit 1b791d18a4 on May 30, 2019
  67. garethtdavies referenced this in commit c347d88e43 on May 30, 2019
  68. garethtdavies referenced this in commit 69f404ed00 on May 30, 2019
  69. renuzit referenced this in commit e864cba9be on May 31, 2019
  70. PastaPastaPasta referenced this in commit 7190e996cd on Dec 22, 2019
  71. PastaPastaPasta referenced this in commit dd0b490a59 on Jan 2, 2020
  72. PastaPastaPasta referenced this in commit c1772617d4 on Jan 4, 2020
  73. PastaPastaPasta referenced this in commit 910e24125e on Jan 12, 2020
  74. PastaPastaPasta referenced this in commit 83b15d2cd2 on Jan 12, 2020
  75. PastaPastaPasta referenced this in commit 7b9b9dfb64 on Jan 12, 2020
  76. PastaPastaPasta referenced this in commit 459be640db on Jan 12, 2020
  77. PastaPastaPasta referenced this in commit d3ae079c05 on Jan 12, 2020
  78. PastaPastaPasta referenced this in commit ea146df3f0 on Jan 12, 2020
  79. PastaPastaPasta referenced this in commit 23ec9e7cc8 on Jan 12, 2020
  80. ckti referenced this in commit 88bafb3112 on Mar 28, 2021
  81. gades referenced this in commit 990493d2e9 on Jun 26, 2021
  82. DrahtBot locked this on Sep 8, 2021
Contributors
esotericnonsensejnewberypromagsipajonasschnellimeshcolliderTheBlueMattMarcoFalke
Labels
RPC/REST/ZMQ
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me