[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 cross-referenced this on Sep 20, 2017 from issue [rpc] Fix pruneheight help description in getblockchaininfo by esotericnonsense
  9. esotericnonsense force-pushed on Sep 20, 2017
  10. esotericnonsense renamed this:
    RPC: getblockchaininfo: Add disk_size, prune_target_size
    [rpc] getblockchaininfo: add size_on_disk, prune_target_size
    on Sep 20, 2017
  11. esotericnonsense force-pushed on Sep 20, 2017
  12. esotericnonsense force-pushed on Sep 20, 2017
  13. 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.

  14. esotericnonsense force-pushed on Sep 20, 2017
  15. esotericnonsense cross-referenced this on Sep 20, 2017 from issue [test] Add getblockchaininfo functional test by promag
  16. 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

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

    one more documentation nit. Otherwise looks good to me

  18. esotericnonsense force-pushed on Sep 20, 2017
  19. 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...).

  20. 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 = ...;

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

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

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

    Concept ACK

  24. esotericnonsense force-pushed on Sep 22, 2017
  25. 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.

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

    utACK 071879a6263aa286e166f9cf07f409e6df35bc02

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

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

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

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

  31. esotericnonsense force-pushed on Sep 24, 2017
  32. 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,
  33. promag commented at 8:35 PM on September 24, 2017: member

    Please rebase with #11370.

  34. esotericnonsense force-pushed on Sep 25, 2017
  35. 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
  36. 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.

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

  38. 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']
  39. 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.

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

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

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

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

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

    Tested ACK 2b73ece6e34961a6745114b6932945692ed987e7

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

  47. TheBlueMatt commented at 8:15 PM on September 27, 2017: contributor

    utACK 2b73ece6e34961a6745114b6932945692ed987e7

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

    Needs rebaseā„¢

  49. [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
  50. esotericnonsense force-pushed on Sep 29, 2017
  51. esotericnonsense commented at 6:51 PM on September 29, 2017: contributor

    Ā®ebased

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

    utAĀ©K b7dfc6c4b89b62f9bb79ea009ee103a6299ac005

  53. TheBlueMatt commented at 8:35 PM on October 2, 2017: contributor

    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.

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

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

  56. laanwj merged this on Oct 9, 2017
  57. laanwj closed this on Oct 9, 2017
  58. laanwj referenced this in commit 3a93270c55 on Oct 9, 2017
  59. luke-jr referenced this in commit cd46034514 on Nov 11, 2017
  60. luke-jr referenced this in commit fec3a143a9 on Nov 11, 2017
  61. jasonbcox cross-referenced this on Oct 29, 2018 from issue get chain size through rpc by roccomuso
  62. thephez cross-referenced this on Oct 30, 2018 from issue chain size through rpc by roccomuso
  63. Eirik0 cross-referenced this on Oct 31, 2018 from issue Port getblockchaininfo.size_on_disk from BTC master by leto
  64. nmarley referenced this in commit 007c906322 on Nov 1, 2018
  65. nmarley referenced this in commit 5462995194 on Nov 1, 2018
  66. nmarley referenced this in commit 2abb2702b1 on Nov 1, 2018
  67. nmarley referenced this in commit 78654534b9 on Nov 4, 2018
  68. PastaPastaPasta cross-referenced this on Nov 5, 2018 from issue [rpc] getblockchaininfo: add size_on_disk, prune_target_size, automatā€¦ by PastaPastaPasta
  69. bitcartel referenced this in commit 719508e703 on Nov 14, 2018
  70. renuzit referenced this in commit 4232bc4fed on May 16, 2019
  71. milesmanley referenced this in commit 1b83c1dd5c on May 16, 2019
  72. garethtdavies referenced this in commit 1b791d18a4 on May 30, 2019
  73. garethtdavies referenced this in commit c347d88e43 on May 30, 2019
  74. garethtdavies referenced this in commit 69f404ed00 on May 30, 2019
  75. renuzit referenced this in commit e864cba9be on May 31, 2019
  76. PastaPastaPasta referenced this in commit 7190e996cd on Dec 22, 2019
  77. PastaPastaPasta referenced this in commit dd0b490a59 on Jan 2, 2020
  78. PastaPastaPasta referenced this in commit c1772617d4 on Jan 4, 2020
  79. PastaPastaPasta referenced this in commit 910e24125e on Jan 12, 2020
  80. PastaPastaPasta referenced this in commit 83b15d2cd2 on Jan 12, 2020
  81. PastaPastaPasta referenced this in commit 7b9b9dfb64 on Jan 12, 2020
  82. PastaPastaPasta referenced this in commit 459be640db on Jan 12, 2020
  83. PastaPastaPasta referenced this in commit d3ae079c05 on Jan 12, 2020
  84. PastaPastaPasta referenced this in commit ea146df3f0 on Jan 12, 2020
  85. PastaPastaPasta referenced this in commit 23ec9e7cc8 on Jan 12, 2020
  86. sickpig cross-referenced this on May 5, 2020 from issue [rpc] getblockchaininfo: add size_on_disk, prune_target_size by sickpig
  87. ckti referenced this in commit 88bafb3112 on Mar 28, 2021
  88. gades referenced this in commit 990493d2e9 on Jun 26, 2021
  89. bitcoin locked this on Sep 8, 2021
Contributors
esotericnonsensejnewberypromagsipajonasschnellimeshcolliderTheBlueMattMarcoFalke
Labels
RPC/REST/ZMQ
Linked (view graph)
#11359 Add a pruning 'high water mark' to reduce the frequency of pruning events#11366 [rpc] Fix pruneheight help description in getblockchaininfo#11370 [test] Add getblockchaininfo functional test
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-05-19 06:54 UTC