feat(resharding): resharding archival node #12578

staffik · 2024-12-09T11:35:12Z

Mapping from child to parent shard is stored in DBCol::StateShardUIdMapping, which is marked as a cold column.
By default, it would be copied from hot to cold storage at each block.
In this PR, we do add is_last_block_in_epoch parameter to update_cold_db() so that it is only copied once per epoch.

For that, a slight refactor of cold_store_copy() was needed.
If it is possible to have skips at epoch boundary, then cold_store_copy() had a bug, where previous epoch shard layout could be used to copy data for a block from the new shard layout.

Added archival node and rpc node to reshardingv3 testloop.
In testloop, create temporary account before resharding and remove it after resharding. After gc period, the account is available at the archival node and it is not available at rpc node.
However, it works without actually testing the cold_store_copy path, because neither cold_store_copy nor garbage collection is called for archival node in testloop. So it needs fixing testloop and a follow-up testing.

Trisfald

Looks good to me, but I'd like someone else to take a look too

Also, maybe we should trigger GC in the tests before merging? Not a strong opinion.

integration-tests/src/tests/client/cold_storage.rs

Trisfald · 2024-12-11T10:00:43Z

integration-tests/src/test_loop/tests/resharding_v3.rs

    }

-    fn with_clients(num_clients: u64) -> Self {
+    fn with_validators(num_validators: u64) -> Self {


It would be nice to configure num of archivals, rpc, etc. Not in this PR though

Yeah, just wanted to have at least one archival and one rpc so that they are tested. Not sure what bigger number of these would give us though.

Trisfald · 2024-12-11T10:09:42Z

integration-tests/src/test_loop/tests/resharding_v3.rs

@@ -1036,7 +1080,7 @@ fn test_resharding_v3_base(params: TestReshardingParameters) {
        params
            .loop_actions
            .iter()
-            .for_each(|action| action(&node_datas, test_loop_data, client_handles[0].clone()));
+            .for_each(|action| action(&env.datas, test_loop_data, client_handles[0].clone()));


I think if you get an RPC client here instead of client at index 0, the rpc_id param should work

Added TODO, won't do it in this PR.

Trisfald · 2024-12-11T10:11:14Z

integration-tests/src/test_loop/tests/resharding_v3.rs

+    // and it is not accessible through a regular, RPC node.
+    assert!(account_exist_in_view_client(
+        &mut env,
+        num_clients - 1,


num_client - 1 is archival and num_client - 2 is RPC?

For clarity can you assign them to variables for clarity and assert they are truly archival and RPCs (if there's a way)?

wacban

LGTM, nice!

wacban · 2024-12-10T13:45:31Z

core/store/src/adapter/trie_store.rs

+pub fn get_shard_uid_mapping(store: &Store, child_shard_uid: ShardUId) -> ShardUId {
+    store
+        .get_ser::<ShardUId>(DBCol::StateShardUIdMapping, &child_shard_uid.to_bytes())
+        .expect("get_shard_uid_mapping() failed")


please add the child shard uid to the error message

wacban · 2024-12-10T13:45:42Z

core/store/src/adapter/trie_store.rs

-        .get_ser::<ShardUId>(DBCol::StateShardUIdMapping, &shard_uid.to_bytes())
-        .expect("get_key_from_shard_uid_and_hash() failed")
-        .unwrap_or(shard_uid);
+    let mapped_shard_uid = get_shard_uid_mapping(store, shard_uid);


wacban · 2024-12-11T12:35:30Z

core/store/src/archive/cold_storage.rs

@@ -57,8 +58,7 @@ struct BatchTransaction {
 }

 /// Updates provided cold database from provided hot store with information about block at `height`.
-/// Returns if the block was copied (false only if height is not present in `hot_store`).
-/// Block as `height` has to be final.
+/// Block at `height` has to be final and present in `hot_store`.


If it is not too hard can you assert that this is the case?

If it is not present, the function will return error.
Regarding finality, it was asserted in cold_store_copy():

if next_height > hot_final_head_height { return Err(ColdStoreError::SkippedBlocksBetweenColdHeadAndNextHeightError { cold_head_height, next_height, hot_final_head_height, }); }

wacban · 2024-12-11T12:39:02Z

core/store/src/archive/cold_storage.rs

    num_threads: usize,
-) -> io::Result<bool> {
+) -> io::Result<()> {


What happened to the return value? Was is redundant?

It is redundant after refactor. Before, it was used to return whether the height exist in hot store. Now we would return an error if it does not exist.

wacban · 2024-12-11T12:39:36Z

core/store/src/archive/cold_storage.rs

        for op in trie_changes.insertions() {
-            // TODO(reshardingV3) Handle shard_uid not mapped there
-            let key = join_two_keys(&shard_uid_key, op.hash().as_bytes());
+            // TODO(reshardingV3) Test it properly. Currently this path is not triggered in testloop.


nit: TODO(resharding) is the convention

I was using TODO(reshardingV3) for a few months to differentiate it from old todos for previous reshardingV2. Will replace all of them with TODO(resharding) now.

wacban · 2024-12-11T12:43:06Z

integration-tests/src/test_loop/tests/resharding_v3.rs

+        QueryRequest::ViewAccount { account_id },
+    );
+    let result = near_async::messaging::Handler::handle(view_client, msg);
+    result.is_ok()


Is this sufficient to check that it returned ok?

Yes, the result is Result<QueryResponse, QueryError>
and QueryResponse contains a data of a succesful response.

wacban · 2024-12-11T12:44:56Z

integration-tests/src/test_loop/tests/resharding_v3.rs

    }

+    let rpc_id = params.rpc_clients[0].clone();
+    let temporary_account =


please add a comment what's this for

Do you want this to happen in all tests or should it be part of the TestParams?

Added a comment. I would prefer it to happen in all tests, because it currently fails in a test that I did not expect to, so I think it can catch nontrivial bugs.

wacban · 2024-12-11T12:45:36Z

integration-tests/src/test_loop/tests/resharding_v3.rs


-    let TestLoopEnv { mut test_loop, datas: node_datas, tempdir } = builder
+    let mut env = builder


Why not unpack? It was a bit nicer when unpacked.

I also do not like it, but it is required by create_account / delete_account

wacban · 2024-12-11T12:46:39Z

integration-tests/src/test_loop/tests/resharding_v3.rs

    let receiver_account: AccountId = "account4".parse().unwrap();
    let account_1_in_stable_shard: AccountId = "account1".parse().unwrap();
    let account_2_in_stable_shard: AccountId = "account2".parse().unwrap();
+    let rpc_id = params.rpc_clients[0].clone();
    let params = TestReshardingParameters::new()


Please avoid creating params twice, instead reuse the one that you have above.

Some in other tests.

nit: The following pattern may be a bit nicer:

let rpc_id = ...; let params = TestReshardingParameters::new() .rpc_id(rpc_id) ...

Mistakenly called new() twice in two tests, fixed it now.
But in general, I need:

let params = TestReshardingParameters::new(); let rpc_id = params.rpc_clients[0].clone(); let params = params .add_loop_action(call_burn_gas_contract( ... rpc_id, ))

because rpc_id is calculated in TestReshardingParameters::new().
Are you ok with that?

@staffik I proposed to make TestReshardingParameters use a builder pattern to delay construction at the end instead of initializing everything in new (#12610). Maybe it can make applying your changes easier.

wacban · 2024-12-11T12:51:04Z

nearcore/src/cold_storage.rs

+        // Here it should be sufficient to just read from hot storage.
+        // Because BlockHeight is never garbage collectable and is not even copied to cold.
+        match hot_store.get_ser::<CryptoHash>(DBCol::BlockHeight, &next_height.to_le_bytes())? {
+            Some(next_height_block_hash) => break next_height_block_hash,


That's fancy, this pattern is not commonly used. Would it be any more readable in a dedicated function?

I do not see how to make it more readable with a dedicated function.

Would this be good enough?

let next_height_block_hash = hot_store.get_ser::<CryptoHash>(DBCol::BlockHeight, &next_height.to_le_bytes())?; if let Some(next_height_block_hash) = next_height_block_hash { break next_height_block_hash; } next_height = next_height + 1;

Don't worry about it then, it's fine.

…rchival

staffik · 2024-12-16T11:46:34Z

integration-tests/src/test_loop/tests/resharding_v3.rs

+    // TODO(resharding) Test chunk validators too (would need to change the lines below).
+    base_epoch_config.num_block_producer_seats = params.num_validators;
+    base_epoch_config.num_chunk_producer_seats = params.num_validators;
+    base_epoch_config.num_chunk_validator_seats = params.num_validators;


Without this shard shuffling test failed because default number of seats was 100, and it treated rpc and archival nodes as validators.

indeed.. I've run into the same issue while adding other node types.

shreyan-gupta

Could you please check my comment in nearcore/src/cold_storage.rs before merging?

shreyan-gupta · 2024-12-16T12:08:22Z

core/store/src/archive/cold_storage.rs

+            if !col.is_cold() {
+                return false;
+            }
+            if col == &DBCol::StateShardUIdMapping && !is_last_block_in_epoch {


Not a big fan of this is_last_block_in_epoch check here. Is it possible to just remove this and copy DBCol::StateShardUIdMapping at every height? I don't suppose it would add too much inefficiency, would it?

Yeah, my preference is to copy it at each height too, for simplicity.
Just during team discussion we decided to copy it in last block of epoch.
@wacban @Trisfald @Longarithm wdyt?

Yeah, personally since you make the effort to implement the logic to make the copy only at the last block, I'm ok with keeping it as is. Not a strong preference.

Oh sorry, didn't know it was already discussed. We can keep the majority preference

shreyan-gupta · 2024-12-16T12:11:40Z

core/store/src/adapter/trie_store.rs

@@ -173,6 +173,17 @@ impl<'a> TrieStoreUpdateAdapter<'a> {
    }
 }

+/// Get the `ShardUId` mapping for child_shard_uid. If the mapping does not exist, map the shard to itself.
+/// Used by Resharding V3 for State mapping.
+pub fn get_shard_uid_mapping(store: &Store, child_shard_uid: ShardUId) -> ShardUId {


Could we push this pub function to TrieStoreAdapter? In cold_storage.rs we can call hot_store.trie_store().get_shard_uid_mapping(...)

I would like to but it is problematic, please see #12232 (comment).

Aah okay! Yes, we don't want to be cloning store I guess. Could you add a quick comment just so that next time we have context for why it's where it is?

Added here: f46714f

shreyan-gupta · 2024-12-16T12:22:29Z

nearcore/src/cold_storage.rs

+    let shard_layout = epoch_manager.get_shard_layout(&epoch_id)?;
+    let is_last_block_in_epoch =
+        epoch_manager.is_next_block_epoch_start(&next_height_block_hash)?;
+    update_cold_db(


Wait, this isn't correct right? Shouldn't we be calling update_cold_db function for all heights between cold_head_height + 1 and hot_final_head_height? It should be within the loop right?

copies data for the next available produced block after current cold store head

The loop exist in order to find the first available block in hot store

No but in the original code, update_cold_db is being called in the loop for each height right?? That has side effects??

The function first checked if the block is available and returned false if it wasn't:
https://github.com/near/nearcore/pull/12578/files#diff-33e110068d4d20b9833e59ebb06676f0fcbdc3e79b11241c50a969727d5642deL88-L91.

Instead, we now find the correct height first, then call update_cold_db() with the correct height and is_last_block_in_epoch info.

Ohhh..... Originally poorly written 😢
Apologies for the panic!()

codecov · 2024-12-16T14:57:10Z

Codecov Report

Attention: Patch coverage is 70.58824% with 20 lines in your changes missing coverage. Please review.

Project coverage is 70.45%. Comparing base (4fff0bb) to head (f46714f).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
tools/cold-store/src/cli.rs	0.00%	10 Missing ⚠️
nearcore/src/cold_storage.rs	63.15%	2 Missing and 5 partials ⚠️
core/store/src/adapter/trie_store.rs	77.77%	1 Missing and 1 partial ⚠️
...egration-tests/src/test_loop/utils/transactions.rs	90.90%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12578      +/-   ##
==========================================
- Coverage   70.46%   70.45%   -0.01%     
==========================================
  Files         845      845              
  Lines      172261   172287      +26     
  Branches   172261   172287      +26     
==========================================
+ Hits       121377   121387      +10     
- Misses      45759    45789      +30     
+ Partials     5125     5111      -14

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (-0.01%)`	⬇️
db-migration	`0.16% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.36% <0.00%> (-0.01%)`	⬇️
linux	`69.34% <70.58%> (-0.02%)`	⬇️
linux-nightly	`70.05% <70.58%> (+0.01%)`	⬆️
pytests	`1.67% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.48% <0.00%> (-0.01%)`	⬇️
unittests	`70.28% <70.58%> (-0.01%)`	⬇️
upgradability	`0.20% <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.

staffik force-pushed the stafik/resharding/archival branch 2 times, most recently from 8a9f670 to 5444b0c Compare December 9, 2024 11:44

staffik added 2 commits December 9, 2024 13:21

Resharding archival node

f3f4c7e

update tests

fd1b7c1

staffik force-pushed the stafik/resharding/archival branch from 0f9eb90 to fd1b7c1 Compare December 9, 2024 12:21

staffik and others added 2 commits December 10, 2024 13:05

fix

4265ee7

Merge branch 'master' into stafik/resharding/archival

d5da412

staffik marked this pull request as ready for review December 10, 2024 12:09

staffik requested a review from a team as a code owner December 10, 2024 12:09

staffik requested review from Longarithm and removed request for Longarithm December 10, 2024 12:09

staffik changed the title ~~Resharding archival node~~ feat(resharding): resharding archival node Dec 10, 2024

staffik marked this pull request as draft December 10, 2024 12:40

improve

a728ca8

staffik marked this pull request as ready for review December 10, 2024 13:21

staffik requested review from wacban, Longarithm, shreyan-gupta and Trisfald December 10, 2024 13:21

Trisfald reviewed Dec 11, 2024

View reviewed changes

wacban approved these changes Dec 11, 2024

View reviewed changes

address pr comments

0394da3

staffik marked this pull request as draft December 12, 2024 12:11

staffik added 3 commits December 12, 2024 13:13

Merge remote-tracking branch 'origin/master' into stafik/resharding/a…

6dea4a0

…rchival

merge fixes

893ea1a

fmt

7e9e716

staffik force-pushed the stafik/resharding/archival branch from 2ad01ed to 7e9e716 Compare December 12, 2024 13:24

staffik added 2 commits December 13, 2024 09:50

Merge remote-tracking branch 'origin/master' into stafik/resharding/a…

146b2aa

…rchival

Merge remote-tracking branch 'origin/master' into stafik/resharding/a…

ed8f37d

…rchival

staffik force-pushed the stafik/resharding/archival branch from 041bb7a to ae2e7c2 Compare December 15, 2024 23:59

staffik force-pushed the stafik/resharding/archival branch from ae2e7c2 to 61482cd Compare December 16, 2024 00:00

fix test

029cd6c

staffik force-pushed the stafik/resharding/archival branch from 61482cd to 029cd6c Compare December 16, 2024 00:08

set seats number

fc62e55

staffik requested review from Trisfald and wacban December 16, 2024 11:44

staffik marked this pull request as ready for review December 16, 2024 11:45

staffik commented Dec 16, 2024

View reviewed changes

shreyan-gupta reviewed Dec 16, 2024

View reviewed changes

comment

f46714f

staffik added this pull request to the merge queue Dec 16, 2024

Merged via the queue into master with commit d3b8e0b Dec 16, 2024
27 checks passed

staffik deleted the stafik/resharding/archival branch December 16, 2024 15:42


		let TestLoopEnv { mut test_loop, datas: node_datas, tempdir } = builder
		let mut env = builder

feat(resharding): resharding archival node #12578

feat(resharding): resharding archival node #12578

Conversation

staffik commented Dec 9, 2024 • edited Loading

Trisfald 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

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

Choose a reason for hiding this comment

staffik Dec 12, 2024 • edited Loading

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

staffik Dec 12, 2024 • edited Loading

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

shreyan-gupta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staffik Dec 16, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 16, 2024 • edited Loading

Codecov Report

staffik commented Dec 9, 2024 •

edited

Loading

staffik Dec 12, 2024 •

edited

Loading

staffik Dec 12, 2024 •

edited

Loading

staffik Dec 16, 2024 •

edited

Loading

codecov bot commented Dec 16, 2024 •

edited

Loading