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

feat: Opt-in index reduction for Validator nodes #10748

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

valar999
Copy link
Contributor

In order to reduce the number of indexes stored in the database (Issue #10357), we are adding the "--validator-minimal-store" flag, which tells the node to store only the data necessary for the operation of a validator node.

Currently, three indexes listed in the issue are not being written: DBCol::Transactions, DBCol::Receipts, DBCol::TransactionResultForBlock.

@valar999
Copy link
Contributor Author

I've verified that these indexes are not used in any validator processes. Additionally, the validator has been running successfully on the testnet for several days.

Copy link

codecov bot commented Mar 11, 2024

Codecov Report

Attention: Patch coverage is 64.15094% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 71.62%. Comparing base (8caa46b) to head (01081a2).
Report is 1 commits behind head on master.

Files Patch % Lines
tools/state-viewer/src/commands.rs 4.54% 21 Missing ⚠️
neard/src/cli.rs 0.00% 4 Missing ⚠️
tools/state-viewer/src/state_changes.rs 0.00% 2 Missing ⚠️
chain/chain/src/store.rs 96.87% 0 Missing and 1 partial ⚠️
nearcore/src/migrations.rs 0.00% 1 Missing ⚠️
tools/database/src/analyse_gas_usage.rs 0.00% 1 Missing ⚠️
tools/epoch-sync/src/cli.rs 0.00% 1 Missing ⚠️
tools/flat-storage/src/commands.rs 0.00% 1 Missing ⚠️
tools/fork-network/src/cli.rs 0.00% 1 Missing ⚠️
tools/mirror/src/offline.rs 0.00% 1 Missing ⚠️
... and 4 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10748      +/-   ##
==========================================
- Coverage   71.65%   71.62%   -0.04%     
==========================================
  Files         758      758              
  Lines      152705   152771      +66     
  Branches   152705   152771      +66     
==========================================
- Hits       109425   109421       -4     
- Misses      38308    38380      +72     
+ Partials     4972     4970       -2     
Flag Coverage Δ
backward-compatibility 0.24% <0.00%> (-0.01%) ⬇️
db-migration 0.24% <0.00%> (-0.01%) ⬇️
genesis-check 1.42% <1.06%> (+<0.01%) ⬆️
integration-tests 36.85% <46.22%> (-0.06%) ⬇️
linux 70.39% <64.15%> (-0.02%) ⬇️
linux-nightly 71.11% <64.15%> (-0.04%) ⬇️
macos 53.05% <61.32%> (-1.63%) ⬇️
pytests 1.64% <1.06%> (+<0.01%) ⬆️
sanity-checks 1.43% <1.06%> (+<0.01%) ⬆️
unittests 67.43% <62.26%> (-0.03%) ⬇️
upgradability 0.29% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@valar999 valar999 marked this pull request as ready for review March 11, 2024 21:15
@valar999 valar999 requested a review from a team as a code owner March 11, 2024 21:15
@valar999 valar999 requested a review from wacban March 11, 2024 21:15
@wacban
Copy link
Contributor

wacban commented Mar 13, 2024

Hey @valar999, thanks for this submission! It's a cool idea indeed. I'm a bit busy right now but I'll try to find a good reviewer for this PR.
My high level comments:

  • Do you have any stats on how much storage would that save on a mainnet node compared to total storage? While the idea is good in principle if it only saves a tiny amount of storage it wouldn't be worth the added complexity.
  • If set to true will the node no longer be able to answer RPC queries? If so we should also make sure that the RPC is disabled. Perhaps some sort of sanity check on configs at neard startup.
  • What happens if a node operator sets it to true and then sets it to false after a while? Would the node regain it's RPC abilities? Should that scenario be forbidden?
  • This option probably should be disabled for archival nodes. Again some sort of sanity check on config at neard startup should do the trick.
  • Are those columns garbage collected? Either way would it make sense to cleanup the columns on startup if the config is set to true?

@posvyatokum posvyatokum self-requested a review March 13, 2024 14:24
@posvyatokum
Copy link
Member

@wacban I will review this PR also, and ask for more support if needed

@valar999
Copy link
Contributor Author

valar999 commented Mar 13, 2024

Do you have any stats on how much storage would that save on a mainnet node compared to total storage? While the idea is good in principle if it only saves a tiny amount of storage it wouldn't be worth the added complexity.

I have only testnet data:

# sorted by size in bytes
Column family FlatState has 425173195 number of pairs and 31343173362 bytes size
Column family State has 724504501 number of pairs and 28980180040 bytes size
[x] Column family TransactionResultForBlock has 57593315 number of pairs and 3685972160 bytes size
Column family BlockMerkleTree has 111586952 number of pairs and 3570782464 bytes size
Column family BlockHeader has 111586952 number of pairs and 3570782464 bytes size
Column family StateChanges has 19943432 number of pairs and 1402978214 bytes size
[x] Column family Transactions has 37649773 number of pairs and 1204792736 bytes size
Column family BlockOrdinal has 111575507 number of pairs and 892604056 bytes size
Column family BlockHeight has 111575507 number of pairs and 892604056 bytes size
Column family TrieChanges has 6813400 number of pairs and 272536000 bytes size
Column family ChunkExtra has 6810385 number of pairs and 272415400 bytes size
Column family ReceiptIdToShardId has 6976153 number of pairs and 223236896 bytes size
[x] Column family Receipts has 5390081 number of pairs and 172482592 bytes size
Column family IncomingReceipts has 982230 number of pairs and 39289200 bytes size
Column family OutgoingReceipts has 972947 number of pairs and 38917880 bytes size
Column family OutcomeIds has 972947 number of pairs and 38917880 bytes size

If set to true will the node no longer be able to answer RPC queries? If so we should also make sure that the RPC is disabled. Perhaps some sort of sanity check on configs at neard startup.

At least DBCol::Transactions should be garbage collected. The RPC node will reply with a "not found" error anyway.

What happens if a node operator sets it to true and then sets it to false after a while? Would the node regain it's RPC abilities? Should that scenario be forbidden?

I think there's also no problem here. The node simply won't write data for some time, which isn't permanent anyway.

This option probably should be disabled for archival nodes. Again some sort of sanity check on config at neard startup should do the trick.

Agree, I'll try to commit it tomorrow.

Are those columns garbage collected? Either way would it make sense to cleanup the columns on startup if the config is set to true?

I believe it is. But I'm not sure exactly when and how. I haven't delved deeply into this.

@akhi3030
Copy link
Collaborator

This option probably should be disabled for archival nodes. Again some sort of sanity check on config at neard startup should do the trick.

Agree, I'll try to commit it tomorrow.

Hey @valar999. Just a ping to see if you managed to get to this work yet.

@posvyatokum
Copy link
Member

Sorry for the huge delay in review.
I like the idea, but I want to ensure maximum security of this feature.
My main concern is archival nodes. Not only we shouldn't do this for archival nodes, but we should know that any rpc db that does not have full history for all columns from tail to head cannot be used to "heal" any archival node. That us something we do in this situation for example.

So, additionally to this, I would like to:

  1. preserve some information in DB about not saving some columns from height A to height B. This can be done by writing new keys into BlockMisc column (like, LAST_FULL_HISTORY_HEIGHT, or FIRST_PARTIAL_HISTORY_HEIGHT)
  2. when opening split storage archival node, check that hot storage has the full history of all columns (or at least that all blocks after cold head have full history)
  3. have extra override of this config option somewhere in Chain initialisation, in case config wasn't validated, and we ended up with archival && validator_minimal_store

I don't know how this change will affect network communication, and sync of other nodes. I want to be sure that if all peers of some node enable this option, the node can still sync if it fell behind. I will look deeper into that.

Finally, not a concern, but some stats for these columns. In our fleet the average size of
TransactionResultForBlock -- 37GB
Receipts -- 12GB
Transactions -- 7GB.

@marcelo-gonzalez
Copy link
Contributor

One thing that's hard to have noticed is that this actually won't play well with the current implementation of the "tx" RPC method. You would expect that if you're running a validator with this option, you'd get some sort of unknown or unimplemented error when asking for transaction status via JSON RPC. But with this PR, if you try to send a "tx" RPC request to a validator running with this change, it will actually just hang forever because of the logic in this loop:

maybe @telezhnaya might have an idea of how to fix that in this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants