feat: add possibility to save trie nodes while re-applying blocks #11811

Trisfald · 2024-07-18T12:52:08Z

Adds a new argument to apply and apply range commands to save computed trie nodes into hot or cold storage. Normally, this operation should be idempotent, but it can also be used to reconstruct missing trie nodes.

Trisfald · 2024-07-18T12:57:05Z

Tested with the following procedure:

Verify that

curl -X POST http://35.204.169.85:3030 \
    -H "Content-Type: application/json" \
    -d '
    { "id": "dontcare", "jsonrpc": "2.0", "method": "query", "params": { "account_id": "b001b461c65aca5968a0afab3302a5387d128178c99ff5b2592796963407560a", "block_id": 109913260, "request_type": "view_account" } }'

returns an error

Stop neard

Verify that

./neard view-state -t cold view-trie --shard-id 2 --shard-version 1 --max-depth 1000 --hash 36SkUU8tgetUtVL2a5JPwKB6F29yKBFjF5PFukZ8HRFH --from b001aea591ef68681e59a4149b1ab8bc56d8f22e34be24 --to b001c0de4c6929c5289b65044249830466ffea27680bc1 --format pretty --record-type account

Gives MissingTrieValue error

Run

./neard view-state -t cold --readwrite apply-range --start-index 109913255 --end-index 109913260 --shard-id 2 --storage trie-free --save-trie cold sequential

Now step 3 doesn't fail anymore
Start neard
Step 1 returns the same result as read RPC

codecov · 2024-07-18T13:18:29Z

Codecov Report

Attention: Patch coverage is 49.62963% with 136 lines in your changes missing coverage. Please review.

Project coverage is 71.68%. Comparing base (55d0df0) to head (4ca19f4).

Files	Patch %	Lines
core/store/src/db/recoverydb.rs	67.36%	47 Missing ⚠️
tools/state-viewer/src/commands.rs	18.86%	42 Missing and 1 partial ⚠️
tools/state-viewer/src/cli.rs	0.00%	30 Missing ⚠️
tools/state-viewer/src/apply_chain_range.rs	72.97%	8 Missing and 2 partials ⚠️
core/store/src/lib.rs	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #11811      +/-   ##
==========================================
- Coverage   71.72%   71.68%   -0.05%     
==========================================
  Files         803      804       +1     
  Lines      163763   163996     +233     
  Branches   163763   163996     +233     
==========================================
+ Hits       117466   117564      +98     
- Misses      41231    41362     +131     
- Partials     5066     5070       +4

Flag	Coverage Δ
backward-compatibility	`0.23% <0.00%> (-0.01%)`	⬇️
db-migration	`0.23% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.34% <0.00%> (-0.01%)`	⬇️
integration-tests	`37.75% <0.00%> (-0.10%)`	⬇️
linux	`71.45% <49.62%> (-0.04%)`	⬇️
linux-nightly	`71.27% <49.62%> (-0.04%)`	⬇️
macos	`54.31% <49.62%> (-0.01%)`	⬇️
pytests	`1.61% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.41% <0.00%> (-0.01%)`	⬇️
unittests	`66.12% <49.62%> (-0.03%)`	⬇️
upgradability	`0.28% <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.

wacban

I'm very confused :)

You're storing the trie changes (DBCol::TrieChanges) but as far as I know this column is only used for garbage collection and cold migration. In particular I don't think it's used for answering rpc or viewing state with neard subcommands. I thought you'll be storing the trie nodes directly into the DBCol::State column which is where I believe the data is missing. That being said it seems like it worked somehow :) Can you have a look into how this works and explain it briefly for me?

tools/state-viewer/src/commands.rs

Trisfald · 2024-07-18T13:52:20Z

I'm very confused :)

You're storing the trie changes (DBCol::TrieChanges) but as far as I know this column is only used for garbage collection and cold migration. In particular I don't think it's used for answering rpc or viewing state with neard subcommands. I thought you'll be storing the trie nodes directly into the DBCol::State column which is where I believe the data is missing. That being said it seems like it worked somehow :) Can you have a look into how this works and explain it briefly for me?

Yes, I'm saving the trie changes explicitly although I believe it isn't strictly required.
From my understanding trie changes isn't the only column touched by this section of code https://github.com/near/nearcore/blob/master/chain/chain/src/store/mod.rs#L2533-L2550, for example insertions into DBCol::State and changes to memtries.

Trisfald · 2024-07-18T16:05:41Z

Updated PR:

disabled save-trie for hot DB: too dangerous
checked that cold DB operations are applied in the correct way
deletions shouldn't happen because their state apply isn't committed
hardcoded save_trie_changes to false, no need to write into this column

TODO:

check node state after GC
test save-trie on different heights / shards combinations

Trisfald · 2024-07-22T10:18:11Z

On the positive side: recovered data is still present after leaving the node running over the weekend. Regenerating trie nodes is idempotent.

Not so positive: apply-range does not work at all for many ranges of blocks after 1st and 2nd resharding. The reason is that "missing trie values" break the apply stage of transactions. We'll need to proceed step by step and redo resharding before replaying blocks.

Overall I haven't found any inherent issue with this PR

Trisfald · 2024-07-24T17:01:52Z

core/store/src/db/recoverydb.rs

+
+    /// Returns whether the operation should be kept or dropped.
+    fn keep_db_op(&self, op: &mut DBOp) -> bool {
+        let overwrites_same_data = |col: &mut DBCol, key: &mut Vec<u8>, value: &mut Vec<u8>| {


I'm not sure about this pattern of read-before-write to avoid re-writing the entire DB. It should help in theory, but I don't see any speed up in practice.

Well, at least it helps preventing the so-common write stalls.

I think it may not help now but it would be useful to have later. For example you can use this to write only the missing data to a separate db that can later be shared with node operators for recovery.

wacban

LGTM, thanks, this is great effort and a big leap!

I left a few nits but otherwise it's ready to go.

core/store/src/db/recoverydb.rs

wacban · 2024-07-29T14:38:43Z

core/store/src/db/recoverydb.rs

+        self.cold.iter_range(col, lower_bound, upper_bound)
+    }
+
+    /// Atomically applies operations in given transaction.


Can you also mention that it filters / adapts operations?

wacban · 2024-07-29T14:40:24Z

core/store/src/db/recoverydb.rs

+
+    /// Returns whether the operation should be kept or dropped.
+    fn keep_db_op(&self, op: &mut DBOp) -> bool {
+        let overwrites_same_data = |col: &mut DBCol, key: &mut Vec<u8>, value: &mut Vec<u8>| {


nit: Can you refactor it to a helper?

wacban · 2024-07-29T14:41:48Z

core/store/src/db/recoverydb.rs

+
+    /// Returns whether the operation should be kept or dropped.
+    fn keep_db_op(&self, op: &mut DBOp) -> bool {
+        let overwrites_same_data = |col: &mut DBCol, key: &mut Vec<u8>, value: &mut Vec<u8>| {


I think it may not help now but it would be useful to have later. For example you can use this to write only the missing data to a separate db that can later be shared with node operators for recovery.

wacban · 2024-07-29T14:48:22Z

core/store/src/db/recoverydb.rs

+            if col.is_rc() {
+                if let Ok(Some(old_value)) = self.get_with_rc_stripped(*col, &key) {
+                    let value = DBSlice::from_vec(value.clone()).strip_refcount();
+                    if let Some(value) = value {
+                        if value == old_value {
+                            return true;
+                        }
+                    }
+                }


Technically, since you are only interested in State, you can

Assert that is_rc() is true and not worry about the other case

Only check that the key exists, without loading the value into memory to speed up the process. Other than the reference count the value at a key cannot be changed because the key is a hash of the value.

Interesting, I've made the method a bit smarter with this new knowledge

wacban · 2024-07-29T14:50:23Z

core/store/src/db/recoverydb.rs

+                if !matches!(col, DBCol::State) {
+                    return false;
+                }


nit: You can move that check to before the match to make it a bit slimmer here. You can use the col method on DBOp to do that.

wacban · 2024-07-29T14:51:33Z

core/store/src/db/recoverydb.rs

+    }
+
+    /// Returns whether the operation should be kept or dropped.
+    fn keep_db_op(&self, op: &mut DBOp) -> bool {


Are you sure you need mutable here? It seems like it shouldn't be necessary.

wacban · 2024-07-29T14:56:40Z

tools/state-viewer/src/cli.rs

@@ -213,17 +223,27 @@ pub struct ApplyCmd {
    shard_id: ShardId,
    #[clap(long, default_value = "trie")]
    storage: StorageSource,
+    /// Save the trie nodes generated by applying the block into the selected store (hot or cold).


Can you also mention that it will only write to the state column?
Also maybe worth renaming to save_state or similar?

tools/state-viewer/src/commands.rs

add command to save trie nodes while re-applying blocks

bf02320

Trisfald marked this pull request as ready for review July 18, 2024 12:57

Trisfald requested a review from a team as a code owner July 18, 2024 12:57

Trisfald requested review from saketh-are and wacban July 18, 2024 12:57

wacban reviewed Jul 18, 2024

View reviewed changes

tools/state-viewer/src/commands.rs Show resolved Hide resolved

disable save-trie for hot storage, do not store trie changes by default

76c3be9

Trisfald requested a review from shreyan-gupta July 24, 2024 10:18

create specific recovery DB to help speed up the process

c870215

Trisfald commented Jul 24, 2024

View reviewed changes

saketh-are removed their request for review July 25, 2024 15:43

wacban approved these changes Jul 29, 2024

View reviewed changes

Trisfald added 3 commits July 30, 2024 11:17

add store metric about number of ops written in recovery db

aed0e40

apply PR suggestions

5568d33

Merge remote-tracking branch 'origin/master' into add-save-trie-replay

4ca19f4

Trisfald added this pull request to the merge queue Jul 30, 2024

Merged via the queue into near:master with commit f3c219f Jul 30, 2024
28 of 30 checks passed

Trisfald deleted the add-save-trie-replay branch July 30, 2024 13:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add possibility to save trie nodes while re-applying blocks #11811

feat: add possibility to save trie nodes while re-applying blocks #11811

Trisfald commented Jul 18, 2024

Trisfald commented Jul 18, 2024

codecov bot commented Jul 18, 2024 •

edited

Loading

wacban left a comment

Trisfald commented Jul 18, 2024

Trisfald commented Jul 18, 2024 •

edited

Loading

Trisfald commented Jul 22, 2024

Trisfald Jul 24, 2024

Trisfald Jul 25, 2024

wacban Jul 29, 2024

wacban left a comment

wacban Jul 29, 2024

wacban Jul 29, 2024

wacban Jul 29, 2024

wacban Jul 29, 2024

Trisfald Jul 30, 2024

wacban Jul 29, 2024

wacban Jul 29, 2024

wacban Jul 29, 2024

feat: add possibility to save trie nodes while re-applying blocks #11811

feat: add possibility to save trie nodes while re-applying blocks #11811

Conversation

Trisfald commented Jul 18, 2024

Trisfald commented Jul 18, 2024

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

wacban left a comment

Choose a reason for hiding this comment

Trisfald commented Jul 18, 2024

Trisfald commented Jul 18, 2024 • edited Loading

Trisfald commented Jul 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wacban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 18, 2024 •

edited

Loading

Trisfald commented Jul 18, 2024 •

edited

Loading