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

Port getblockchaininfo.size_on_disk from BTC master #3639

Merged
merged 2 commits into from Feb 25, 2019

Conversation

@leto
Copy link
Contributor

commented Oct 29, 2018

Closes #3630.

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

For reference, the upstream change is: bitcoin/bitcoin#11367. Do we not want the information to do with pruning? Is that not relevant to us? I was just curious why we are not pulling in the change directly, but it seems like there is enough difference that it might not be worth the hassle.

@@ -141,6 +141,10 @@ def run_test (self):
self.nodes[0].generate(1)
self.sync_all()
bci = self.nodes[0].getblockchaininfo()

# size_on_disk should be > 0
assert_greater_than(bci['size_on_disk'], 0)

This comment has been minimized.

Copy link
@Eirik0

Eirik0 Oct 31, 2018

Contributor

I feel like this test makes more sense in blockchain.py (as in the upstream PR), but I suppose in the grand scheme this doesn't really matter. This would be easier if we had already pulled in bitcoin/bitcoin#11370 which adds the call to getblockchaininfo in that test.

This comment has been minimized.

Copy link
@mdr0id

mdr0id Feb 19, 2019

Contributor

Looks good. I am curious if other people have any thoughts on my questions/comments above.

Doesn't seem we pulled in the pruning code, so not sure it would be useful.

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Oct 31, 2018

Looks good. I am curious if other people have any thoughts on my questions/comments above.

@Eirik0 Eirik0 added this to In Review in Arborist Team Oct 31, 2018

@Eirik0 Eirik0 self-requested a review Oct 31, 2018

@mdr0id mdr0id requested review from mdr0id and daira Nov 8, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

Rebased, fixed test, force-push, updated commit message to reference upstream commit which Leto extracted from.

leto and others added some commits Oct 29, 2018

Add size_on_disk test
Co-authored-by: Simon <simon@bitcartel.com>

@bitcartel bitcartel force-pushed the leto:size_on_disk branch from 6b4e0d6 to 6c1bf4e Nov 14, 2018

@bitcartel bitcartel self-requested a review Nov 14, 2018

@bitcartel
Copy link
Contributor

left a comment

ACK

@mdr0id

mdr0id approved these changes Nov 15, 2018

Copy link
Contributor

left a comment

ACK

@leto

This comment has been minimized.

Copy link
Contributor Author

commented Nov 15, 2018

@bitcartel FYI, I ported this to KMD and the numbers seem to under-report on-disk usage by 30%. There may be some code that assumes the size of BTC xtns and blocks, which is incorrect for ZEC and definitely incorrect for KMD. Just a heads up, to check reported size from RPC against actual filesystem size.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2018

Let's put a hold on merging until we verify how close the numbers are. (Thanks @leto !)

@bitcartel
Copy link
Contributor

left a comment

Computations need to be updated. With testnet, I see "size_on_disk": 803502096
whereas du -sh blocks returns 1.5G blocks.

@bitcartel bitcartel moved this from In Review to In Progress in Arborist Team Nov 16, 2018

@mms710 mms710 moved this from In Progress to Current Sprint in Arborist Team Dec 10, 2018

@mms710 mms710 moved this from Current Sprint to Sprint Backlog in Arborist Team Dec 10, 2018

@mms710

This comment has been minimized.

Copy link

commented Dec 13, 2018

@leto are you able to reproduce the issue bitcartel is seeing where the "size_on_disk" is smaller than the actual size as measured by du -sh?

@mms710 mms710 moved this from Sprint Backlog to Blocked in Arborist Team Dec 13, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

The size_on_disk parameter is documented in the RPC help text as returning the sum of the block and undo files. du -ch ~/.zcash/testnet3/blocks/*.dat should return a value that matches the RPC output; du -ch ~/.zcash/testnet3/blocks would include the block index as well.

@mdr0id

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

Minor nit:

du -cb ~/.zcash/testnet3/blocks/*.dat should return a value that matches the RPC output.

When using this approach to check items mentioned above, I was only able to see small variance which is more or less expected.

➜  src git:(size_on_disk) ✗ ./zcash-cli getblockchaininfo | jq '.size_on_disk' ; du -cb --time ~/.zcash/testnet3/blocks/*.dat
939658206
134217297	2018-11-25 19:14	/home/umdev/.zcash/testnet3/blocks/blk00000.dat
134216641	2018-11-25 19:28	/home/umdev/.zcash/testnet3/blocks/blk00001.dat
134217416	2018-11-25 19:37	/home/umdev/.zcash/testnet3/blocks/blk00002.dat
134216487	2018-11-25 19:45	/home/umdev/.zcash/testnet3/blocks/blk00003.dat
134217295	2018-11-25 19:56	/home/umdev/.zcash/testnet3/blocks/blk00004.dat
134216591	2019-02-18 11:00	/home/umdev/.zcash/testnet3/blocks/blk00005.dat
100663296	2019-02-18 14:29	/home/umdev/.zcash/testnet3/blocks/blk00006.dat
5788864	2018-11-25 19:14	/home/umdev/.zcash/testnet3/blocks/rev00000.dat
5370881	2018-11-25 19:28	/home/umdev/.zcash/testnet3/blocks/rev00001.dat
6411849	2018-11-25 19:37	/home/umdev/.zcash/testnet3/blocks/rev00002.dat
6911941	2018-11-25 19:45	/home/umdev/.zcash/testnet3/blocks/rev00003.dat
4819414	2018-11-25 19:56	/home/umdev/.zcash/testnet3/blocks/rev00004.dat
6024734	2019-02-18 11:00	/home/umdev/.zcash/testnet3/blocks/rev00005.dat
5242880	2019-02-18 14:29	/home/umdev/.zcash/testnet3/blocks/rev00006.dat
946535586	2019-02-18 14:29	total

To sanity check, I verified values/timestamps in source:

/* Calculate the amount of disk space the block & undo files currently use */
uint64_t CalculateCurrentUsage()
{
    uint64_t retval = 0;
    BOOST_FOREACH(const CBlockFileInfo &file, vinfoBlockFile) {
        cout <<"\nFILE: " << file.ToString();
        cout <<"\nSIZE: " << file.nSize << " undoSize: " << file.nUndoSize; 
        retval += file.nSize + file.nUndoSize;
    }
    return retval;
}
FILE: CBlockFileInfo(blocks=63778, size=134217297, heights=0...63779, time=2016-10-28...2017-04-11)
SIZE: 134217297 undoSize: 5788864
FILE: CBlockFileInfo(blocks=63447, size=134216641, heights=63775...127271, time=2017-04-11...2017-08-23)
SIZE: 134216641 undoSize: 5370881
FILE: CBlockFileInfo(blocks=62412, size=134217416, heights=127093...189638, time=2017-08-23...2018-02-04)
SIZE: 134217416 undoSize: 6411849
FILE: CBlockFileInfo(blocks=70428, size=134216487, heights=189631...260065, time=2018-02-04...2018-07-04)
SIZE: 134216487 undoSize: 6911941
FILE: CBlockFileInfo(blocks=33431, size=134217295, heights=260062...293495, time=2018-07-04...2018-09-18)
SIZE: 134217295 undoSize: 4819414
FILE: CBlockFileInfo(blocks=71968, size=134216591, heights=293496...365458, time=2018-09-18...2018-12-11)
SIZE: 134216591 undoSize: 6024734
FILE: CBlockFileInfo(blocks=54426, size=94596675, heights=365459...419883, time=2018-12-11...2019-02-18)

Each undoSize corresponds to rev#####.dat and each SIZE corresponds to blk######.dat

@mms710 mms710 moved this from Blocked to Sprint Backlog in Arborist Team Feb 19, 2019

@mms710 mms710 moved this from Sprint Backlog to PRs That Need Review + Their Associated Issues in Arborist Team Feb 19, 2019

@mms710 mms710 moved this from PRs That Need Review + Their Associated Issues to In Progress in Arborist Team Feb 21, 2019

@mms710 mms710 moved this from In Progress to Current Sprint in Arborist Team Feb 21, 2019

@mms710 mms710 moved this from Current Sprint to In Progress in Arborist Team Feb 21, 2019

@mms710 mms710 moved this from In Progress to In Review in Arborist Team Feb 21, 2019

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

I've verified that the size reported is close to the size of blk*.dat and rev*.dat. The issue raised earlier was because the du command included the size of the leveldb index whereas the RPC output does not count this.

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

📌 Commit 6c1bf4e has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

💡 This pull request was already approved, no need to approve it again.

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

📌 Commit 6c1bf4e has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

⌛️ Testing commit 6c1bf4e with merge d81b5f1...

zkbot added a commit that referenced this pull request Feb 25, 2019

Auto merge of #3639 - leto:size_on_disk, r=bitcartel
Port getblockchaininfo.size_on_disk from BTC master

Closes #3630.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

⌛️ Testing commit 6c1bf4e with merge 48b7dea...

zkbot added a commit that referenced this pull request Feb 25, 2019

Auto merge of #3639 - leto:size_on_disk, r=bitcartel
Port getblockchaininfo.size_on_disk from BTC master

Closes #3630.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Feb 25, 2019

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 48b7dea to master...

@zkbot zkbot merged commit 6c1bf4e into zcash:master Feb 25, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from In Review to Released (Merged in Master) Feb 25, 2019

@str4d str4d added this to the v2.0.4 milestone Mar 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.