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

fix: during resharding, reassign outgoing receipts to lowest index child #9362

Merged
merged 8 commits into from
Aug 1, 2023

Conversation

wacban
Copy link
Contributor

@wacban wacban commented Jul 27, 2023

As described on zulip the current way of reassigning outgoing receipts from parent shard to children shards won't work for any future reshardings. It only worked for 1->4 because there all children share the same parent.

This PR implements a new approach where all outgoing receipts are going to be assigned to the child with the lowest shard id.

chain/chain/src/chain.rs Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Show resolved Hide resolved
@wacban wacban force-pushed the waclaw-resharding-outgoing-receipts branch from bf79a62 to 3b7ff0b Compare July 27, 2023 10:07
@wacban wacban marked this pull request as ready for review July 27, 2023 15:58
@wacban wacban requested a review from a team as a code owner July 27, 2023 15:58
chain/chain/src/chain.rs Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/chain.rs Show resolved Hide resolved
chain/chain/src/store.rs Show resolved Hide resolved
});
}
if block_header.height() != last_included_height {
receipts_block_hash = *block_header.prev_hash();
Copy link
Contributor

Choose a reason for hiding this comment

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

@robin-near, just for my understanding, when is it possible for the prev_block_height to not be last_included_height? Is it when we have an orphan block chain?

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 believe it's when a block is missing a chunk. Then the last_included_height will be the latest block height where a chunk for the shard is present.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, what @wacban said. It's one of the very cryptic parts of the codebase - chunks can be missing from blocks, and the logic to handle that is very complex.

chain/chain/src/store.rs Outdated Show resolved Hide resolved
chain/chain/src/store.rs Outdated Show resolved Hide resolved
@wacban wacban requested a review from robin-near July 28, 2023 09:35
});
}
if block_header.height() != last_included_height {
receipts_block_hash = *block_header.prev_hash();
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, what @wacban said. It's one of the very cryptic parts of the codebase - chunks can be missing from blocks, and the logic to handle that is very complex.

@near-bulldozer near-bulldozer bot merged commit 83d6d08 into master Aug 1, 2023
@near-bulldozer near-bulldozer bot deleted the waclaw-resharding-outgoing-receipts branch August 1, 2023 06:57
/// 4' will get no outgoing receipts from its parent 3
/// All receipts are distributed to children, each exactly once.
#[cfg(feature = "protocol_feature_simple_nightshade_v2")]
fn reassign_outgoing_receipts_for_resharding_v2(
Copy link
Contributor

Choose a reason for hiding this comment

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

How does this handle when split shards are in the middle of shard IDs, e.g. 1 -> 1,2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In resharding as you described we would have:
0->0' ; 1->1', 2' ; 2->3' ; 3->4'
shard 0', 1', 3', 4' each will get all of the ougoing receipts from their respective parent shard
shard 2' will get no outgoing receipts

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I was wondering how this remapping happen for non split shards (e.g. shard 2 and 3 in the example above) since the function seem to care only about split shard ids.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For non-split shards we treat it as splitting a parent shard into a single child shard. So it is covered here. In the example above shard 3' will get all of the outgoing receipts from its parent shard 2 and shard 4' will get all of the outgoing receipts from its parent 3.

Currently because of how storage keys work we need to copy all of the data from shards that are not split anyway. In the future we'll likely optimize that. Assuming we decide to have unique, incremental shard ids (e.g. 0, 1, 2, 3 -> (splitting 1) -> 0, 2, 3, 4, 5) then the outgoing receipts reassignment will be to keep the receipts where they are for non-split shards and reassign to lowest index child for split shards.

nikurt pushed a commit that referenced this pull request Aug 24, 2023
…ild (#9362)

As described on [zulip](https://near.zulipchat.com/#narrow/stream/295558-pagoda.2Fcore/topic/outgoing.20receipts.20and.20resharding) the current way of reassigning outgoing receipts from parent shard to children shards won't work for any future reshardings. It only worked for 1->4 because there all children share the same parent. 

This PR implements a new approach where all outgoing receipts are going to be assigned to the child with the lowest shard id.
nikurt pushed a commit to nikurt/nearcore that referenced this pull request Aug 24, 2023
…ild (near#9362)

As described on [zulip](https://near.zulipchat.com/#narrow/stream/295558-pagoda.2Fcore/topic/outgoing.20receipts.20and.20resharding) the current way of reassigning outgoing receipts from parent shard to children shards won't work for any future reshardings. It only worked for 1->4 because there all children share the same parent. 

This PR implements a new approach where all outgoing receipts are going to be assigned to the child with the lowest shard id.
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.

4 participants