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.

Sign up for GitHub

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

Jump to bottom

Extract persistence of BSQ blocks out of DaoStateStore [5] #5790

Merged
merged 13 commits into from
Nov 9, 2021
Merged

Extract persistence of BSQ blocks out of DaoStateStore [5] #5790

merged 13 commits into from
Nov 9, 2021

Conversation

chimp1984
Copy link
Contributor

@chimp1984 chimp1984 commented Oct 31, 2021 •
edited
Loading

Based on #5782
(will merge/rebase once other PRs are in master)
Starts at ff702ae

Fixes #5783

EDIT: Merged and rebased with #5782

@chimp1984 chimp1984 changed the title Extract persistence of blocks [5] Extract persistence of BSQ blocks out of DaoStateStore [5] Oct 31, 2021
@chimp1984 chimp1984 force-pushed the extract-persistence-of-blocks branch from 70cbf55 to 401054f Compare October 31, 2021 23:14
@chimp1984 chimp1984 mentioned this pull request Nov 2, 2021
Merged
chimp1984
chimp1984 commented Nov 2, 2021
View reviewed changes
Copy link
Contributor Author

@chimp1984 chimp1984 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

utACK Upps was on the wrong tab...

@chimp1984 chimp1984 requested a review from sqrrm November 2, 2021 00:22
@chimp1984 chimp1984 force-pushed the extract-persistence-of-blocks branch from 401054f to 2b4309c Compare November 2, 2021 15:57
@ripcurlx ripcurlx added this to the v1.8.0 milestone Nov 4, 2021
@chimp1984 chimp1984 force-pushed the extract-persistence-of-blocks branch 3 times, most recently from 792730c to e941c28 Compare November 4, 2021 12:13
ripcurlx
ripcurlx reviewed Nov 8, 2021
View reviewed changes
common/src/main/java/bisq/common/util/GcUtil.java
Comment on lines +74 to +81
/* if (DevEnv.isDevMode()) {
try {
// To see from where we got called
throw new RuntimeException("Dummy Exception for print stacktrace at maybeReleaseMemory");
} catch (Throwable t) {
t.printStackTrace();
}
}
}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

If commented out, better to remove it completely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I left it as its still helpful for memory/performance profiling to see from where we call it too often...
But Ok to me to remove it as well...

ripcurlx
ripcurlx reviewed Nov 8, 2021
View reviewed changes
core/src/main/java/bisq/core/dao/state/storage/BlocksPersistence.java Outdated
Comment on lines 55 to 57
/* if (!storageDir.exists()) {
storageDir.mkdir();
}*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this commented out?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess it can be safely removed as it is checked during the write procedure anyways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got enabeld in a later commit again as it was required at some cases (resync from resources i guess, or regtest fresh start)

@chimp1984
Copy link
Contributor Author

I will wait until #5782 before merging/rebasing it...

@ripcurlx
Copy link
Contributor

ripcurlx commented Nov 9, 2021 •
edited
Loading

I think we should move the block snapshots to Git LFS. If I'm not mistaken adding p2p/src/main/resources/BsqBlocks_BTC_MAINNET/* filter=lfs diff=lfs merge=lfs -text to the .gitattributes file should do the trick.

ripcurlx
ripcurlx previously approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK - besides the Git LFS config changes. As soon as it is in and rebased to master after merging #5782 I'll run it again against Mainnet before merging.

@ripcurlx
Copy link
Contributor

ripcurlx commented Nov 9, 2021

There are also two minor Codacy complaints regarding an import statement: https://github.com/bisq-network/bisq/pull/5790/checks?check_run_id=4111754368

@chimp1984
Remove blockHashes set from daoState
779bd0c 
Was only used from one use case which can be done differently.
@chimp1984
Add getBlockTimeAtBlockHeight method
4d2d071 
Improve getBlockStartDateByCycleIndex method
@chimp1984
Remove unused field
b619afa
@chimp1984
Avoid unnecessary serialisation/deserialisation cycle
6990486 
For the snapshot we create a deep clone by protobuf serialisation.
We do not need the deserialisation back to the java object as it is
only kept in memory for later persistence where we need to do protobuf
serialisation again. So we can skip that cycle and safe a bit of
time at creating and persisting snapshots.
@chimp1984
Persist bsq blocks as buckets of 1000 blocks
7c32587 
Improve logging

Add BsqBlockStore to protobuf

Remove DaoStateMonitoringService field

Do not persist the blocks in daoState anymore.

This improves persistence performance and reduces memory
requirements for snapshots.
@chimp1984
Improve log
a3fbdf4
@chimp1984
Remove unnecessary sorting
3487932 
Blocks are always sorted by block height
@chimp1984
Cleanups, change log levels
62b3e51
@chimp1984
Add BsqBlocks resources and update DaoState
80e7c45 
Add BsqBlocks resources
@chimp1984
Add check that height of last block matches chainHeight from daoState
97e356c
@chimp1984 chimp1984 force-pushed the extract-persistence-of-blocks branch from 198e16e to 1108e98 Compare November 9, 2021 12:03
@chimp1984
Copy link
Contributor Author

Rebases on maser.

@chimp1984
Copy link
Contributor Author

@ripcurlx
Re Codacy complaints: Unnecessary use of fully qualified name 'protobuf.BaseBlock' due to existing import 'protobuf.BaseBlock'
I intentionally kept the protobuf package to make it clear that we dont use the java model here but the proto data.
So I prefer to keep it. I find those compaints anyway questionable. Leaving package qualifiers can help in certain context to improve readibility IMO.

ripcurlx
ripcurlx previously approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

@chimp1984 chimp1984 force-pushed the extract-persistence-of-blocks branch from b817c4f to 87f4444 Compare November 9, 2021 16:23
ripcurlx
ripcurlx approved these changes Nov 9, 2021
View reviewed changes
Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

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

utACK

@ripcurlx ripcurlx merged commit 7b6f971 into bisq-network:master Nov 9, 2021
@ripcurlx ripcurlx mentioned this pull request Nov 16, 2021
Closed
@chimp1984 chimp1984 mentioned this pull request Nov 17, 2021
Closed
@chimp1984 chimp1984 deleted the extract-persistence-of-blocks branch November 18, 2021 00:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ripcurlx ripcurlx ripcurlx approved these changes

@sqrrm sqrrm Awaiting requested review from sqrrm

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v1.8.0
Development

Successfully merging this pull request may close these issues.

Improve DAO state peristence
2 participants
@chimp1984 @ripcurlx