[resharding] Handle resharding of delayed receipts #12513

shreyan-gupta · 2024-11-25T17:19:51Z

This PR is the core of delayed receipt handling for resharding V3.

Writeup from NEP

The delayed receipts queue contains all incoming receipts that could not be executed as part of a block due to resource constraints like compute cost, gas limits etc. The entries in the delayed receipt queue can belong to any of the accounts as part of the shard. During a resharding event, we ideally need to split the delayed receipts across both the child shards according to the associated account_id with the receipt.

The singleton trie key DelayedReceiptIndices holds the start_index and end_index associated with the delayed receipt entries for the shard. The trie key DelayedReceipt { index } contains the actual delayed receipt associated with some account_id. These are processed in a fifo queue order during chunk execution.

Note that the delayed receipt trie keys do not have the account_id prefix. In ReshardingV2, we followed the trivial solution of iterating through all the delayed receipt queue entries and assigning them to the appropriate child shard, however due to constraints on the state witness size limits and instant resharding, this approach is no longer feasible for ReshardingV3.

For ReshardingV3, we decided to handle the resharding by duplicating the entries of the delayed receipt queue across both the child shards. This is great from the perspective of state witness size and instant resharding as we only need to access the delayed receipt queue root entry in the trie, however it breaks the assumption that all delayed receipts in a shard belong to the accounts within that shard.

To resolve this, with the new protocol version, we changed the implementation of runtime to discard executing delayed receipts that don't belong to the account_id on that shard.

Note that no delayed receipts are lost during resharding as all receipts get executed exactly once based on which of the child shards does the associated account_id belong to.

Source NEP receipt handling section.

Changes in this PR

We change the implementation of DelayedReceiptQueueWrapper to add a new filter function that filters out all delayed receipts that do not belong to the current shard.
peek_iter functions is updated appropriately to filter out delayed receipts as well to ensure compatibility with pipelining.
Main loop in process_delayed_receipts as updated from "while processing_state.delayed_receipts.len() > 0" to something like "loop while Some(delayed_receipt) in queue" as we no longer have a precise count of the number of delayed receipts to process.

Next steps

Change memtrie split function to include delayed receipts in child shards
Testloop test.

…_receipts

codecov · 2024-11-25T19:13:18Z

Codecov Report

Attention: Patch coverage is 81.53846% with 12 lines in your changes missing coverage. Please review.

Project coverage is 70.09%. Comparing base (3068464) to head (4806b74).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
runtime/runtime/src/congestion_control.rs	86.04%	1 Missing and 5 partials ⚠️
core/primitives/src/test_utils.rs	44.44%	5 Missing ⚠️
runtime/runtime/src/lib.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #12513      +/-   ##
==========================================
+ Coverage   70.07%   70.09%   +0.01%     
==========================================
  Files         840      840              
  Lines      170128   170159      +31     
  Branches   170128   170159      +31     
==========================================
+ Hits       119225   119265      +40     
+ Misses      45740    45731       -9     
  Partials     5163     5163

Flag	Coverage Δ
backward-compatibility	`0.16% <0.00%> (-0.01%)`	⬇️
db-migration	`0.16% <0.00%> (-0.01%)`	⬇️
genesis-check	`1.27% <0.00%> (-0.01%)`	⬇️
linux	`69.30% <70.76%> (-0.01%)`	⬇️
linux-nightly	`69.65% <81.53%> (+0.02%)`	⬆️
macos	`51.28% <49.23%> (-0.01%)`	⬇️
pytests	`1.58% <0.00%> (-0.01%)`	⬇️
sanity-checks	`1.39% <0.00%> (-0.01%)`	⬇️
unittests	`69.91% <81.53%> (+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

LGTM

staffik · 2024-11-26T13:26:40Z

runtime/runtime/src/congestion_control.rs

@@ -656,29 +676,61 @@ impl DelayedReceiptQueueWrapper {
        Ok(())
    }

+    // With ReshardingV3, it's possible for a chunk to have delayed receipts that technically belong
+    // the sibling shard before a resharding event.


"to the sibling shard after a resharding event." ?

staffik · 2024-11-26T14:36:50Z

runtime/runtime/src/congestion_control.rs

+        // While processing receipts, we need to keep track of the gas and bytes
+        // even for receipts that may be filtered out due to a resharding event


So congestion control for both children would initially behave as if all parent shard's delayed receipts belonged to each of children?

This should be the expected behavior but it is not implemented as of this PR. Currently child congestion_info is not populated and therefore the limits aren't populated either.

staffik · 2024-11-26T15:51:24Z

runtime/runtime/src/tests/apply.rs

-    // shard 0. Hence all receipts will be forwarded to shard 0. We don't
-    // want local forwarding in the test, hence we need to use a different
-    // shard id.
+    // In the test setup with he MockEpochInfoProvider, bob_account is on shard 0 while alice_account


wacban

LGTM

Thanks for doing the runtime part in separate PR, it makes it much easier to review.

wacban · 2024-11-27T12:08:20Z

core/store/src/trie/receipts_column_helper.rs

+struct TrieQueueIterator<'a, Q>
+where
+    Q: TrieQueue,
+{


For my education, what's the difference? This one is a bit more verbose so I like the old one more but I guess it's needed?

nit: Q -> Queue

Ugh, this is another byproduct of me trying to do something but then reverting back... Sorry for the change, this probably wasn't required as part of this PR.

That said, there isn't too much of a functional difference between the verbose and consolidated way of writing. I personally prefer the more verbose way as it's clearer to read the constraints on the template types. Anyway, just personal preferences. If you'd like, I can revert back to original, but for now I hope it's fine if I merge in this PR?

wacban · 2024-11-27T12:10:25Z

runtime/runtime/src/congestion_control.rs

@@ -466,7 +447,7 @@ impl ReceiptSinkV2 {
        // version where metadata was not enabled. Metadata doesn't contain information about them.
        // We can't make a proper request in this case, so we make a basic request while we wait for
        // metadata to become fully initialized. The basic request requests just `max_receipt_size`. This is enough to
-        // ensure liveness, as all receipts are smaller than `max_receipt_size`. The resulting behaviour is similar
+        // ensure liveness, as all receipts are smaller than `max_receipt_size`. The resulting behavior is similar


wacban · 2024-11-27T12:11:51Z

runtime/runtime/src/congestion_control.rs

+    // Protocol version, epoch_info_provider, shard_id, and epoch_id are used to
+    // determine if a receipt belongs to the current shard or not after a resharding event.
+    protocol_version: ProtocolVersion,
+    epoch_info_provider: &'a dyn EpochInfoProvider,
+    shard_id: ShardId,
+    epoch_id: EpochId,


Could this be just shard_layout and shard_id?

I guess the epoch info provider that is available in runtime doesn't expose the shard layout directly but instead just provides the account_id_to_shard_id method. I think it would be nice to refactor it to return the shard layout instead but that's perhaps out of scope of this PR. Additionally the provide could be strictly pegged to the current epoch and block from the chunk apply context instead of needing to provide the epoch id or prev_block_hash to every method. Let me know if this makes sense and I'll create an issue for this.

Yes, that makes sense, I was just working with the epoch_info_provider that we currently had in runtime and didn't think about refactoring it.

wacban · 2024-11-27T12:13:01Z

runtime/runtime/src/congestion_control.rs

+    // The function follows the guidelines of standard iterator filter function
+    // We return true if we should retain the receipt and false if we should filter it.
+    fn receipt_filter_fn(&self, receipt: &ReceiptOrStateStoredReceipt) -> bool {


Nice, thanks for the comment.

wacban · 2024-11-27T12:13:40Z

runtime/runtime/src/congestion_control.rs

+        if !ProtocolFeature::SimpleNightshadeV4.enabled(self.protocol_version) {
+            return true;
+        }


Strictly speaking not required - I would remove it if it allows us to get rid of the protocol version from self.

It may be interesting to assert that shard_id == self.shard_id before resharding V3.

Yeah, we can get rid of the protocol_version check. I just thought it made sense to keep as much of the original behavior as possible, but will remove!

wacban · 2024-11-27T12:23:37Z

runtime/runtime/src/congestion_control.rs

+        while let Some(receipt) = self.queue.pop_front(trie_update)? {
+            let delayed_gas = receipt_congestion_gas(&receipt, &config)?;
+            let delayed_bytes = receipt_size(&receipt)? as u64;
            self.removed_delayed_gas = safe_add_gas(self.removed_delayed_gas, delayed_gas)?;
            self.removed_delayed_bytes = safe_add_gas(self.removed_delayed_bytes, delayed_bytes)?;
+
+            // Track gas and bytes for receipt above and return only receipt that belong to the shard.
+            if self.receipt_filter_fn(&receipt) {
+                return Ok(Some(receipt));
+            }


This loop here may be bypassing some of the limits imposed on receipt processing. The proper solution may need to be done on a higher level but that seems a bit nasty. I'd be willing to risk it but I'm curious about your and @jancionear opinions on that.

That's a valid issue. I completely missed it. Let me also think about it a bit

btw, may be worth merging as is and just adding a todo to unblock others

Added TODO comment

wacban · 2024-11-27T12:24:10Z

runtime/runtime/src/congestion_control.rs

+    /// This function returns the maximum length of the delayed receipt queue.
+    /// The only time the real number of delayed receipts differ from the returned value is right
+    /// after a resharding event. During resharding, we duplicate the delayed receipt queue across
+    /// both child shards, which means it's possible that the child shards contain delayed receipts
+    /// that don't belong to them.
+    pub(crate) fn upper_bound_len(&self) -> u64 {


nice comment and nice name!

wacban · 2024-11-27T12:24:38Z

runtime/runtime/src/ext.rs

@@ -70,7 +70,7 @@ impl<'a> RuntimeExt<'a> {
        epoch_id: EpochId,
        prev_block_hash: CryptoHash,
        last_block_hash: CryptoHash,
-        epoch_info_provider: &'a (dyn EpochInfoProvider),
+        epoch_info_provider: &'a dyn EpochInfoProvider,


just out of curiosity, why this? Is it a real change or just formatting?

Just formatting, the brackets looked a weird 😅

wacban · 2024-11-27T12:26:29Z

runtime/runtime/src/lib.rs

+            } else {
+                // Break loop if there are no more receipts to be processed.
+                break;
+            };


I actually like it more than the expect. nice.

wacban · 2024-11-27T12:28:33Z

runtime/runtime/src/lib.rs


+            delayed_receipt_count += 1;


I know it's the wrong place, but can you also keep count of the filtered receipts and log it or put it in metrics? Maybe you can do it with some simple math on the upper bound len?

Added this to the TODOs as well. But I think yes, it should be possible to get the number of filtered receipts
initial_upper_bound_len - delayed_receipt_count - final_upper_bound_len

shreyan-gupta added 3 commits November 25, 2024 16:48

nit: brackets

cfb682f

Merge remote-tracking branch 'origin' into shreyan/resharding/delayed…

5e1bfc3

…_receipts

[resharding] Handle resharding of delayed receipts

6def3d3

shreyan-gupta requested review from nagisa, wacban and Longarithm November 25, 2024 17:19

shreyan-gupta requested a review from a team as a code owner November 25, 2024 17:19

fix tests

4806b74

staffik self-requested a review November 26, 2024 11:42

staffik approved these changes Nov 26, 2024

View reviewed changes

wacban approved these changes Nov 27, 2024

View reviewed changes

address comments

33354f2

shreyan-gupta enabled auto-merge November 28, 2024 21:14

shreyan-gupta added this pull request to the merge queue Nov 28, 2024

Merged via the queue into master with commit 6eeabc3 Nov 28, 2024
22 checks passed

shreyan-gupta deleted the shreyan/resharding/delayed_receipts branch November 28, 2024 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[resharding] Handle resharding of delayed receipts #12513

[resharding] Handle resharding of delayed receipts #12513

shreyan-gupta commented Nov 25, 2024

codecov bot commented Nov 25, 2024

staffik left a comment

staffik Nov 26, 2024

staffik Nov 26, 2024

shreyan-gupta Nov 28, 2024

staffik Nov 26, 2024

wacban left a comment

wacban Nov 27, 2024

shreyan-gupta Nov 28, 2024

wacban Nov 27, 2024

wacban Nov 27, 2024

wacban Nov 27, 2024

shreyan-gupta Nov 28, 2024

wacban Nov 27, 2024

wacban Nov 27, 2024

shreyan-gupta Nov 28, 2024

wacban Nov 27, 2024

shreyan-gupta Nov 27, 2024

wacban Nov 27, 2024

shreyan-gupta Nov 28, 2024

wacban Nov 27, 2024

wacban Nov 27, 2024

shreyan-gupta Nov 28, 2024

wacban Nov 27, 2024

wacban Nov 27, 2024

shreyan-gupta Nov 28, 2024

		// While processing receipts, we need to keep track of the gas and bytes
		// even for receipts that may be filtered out due to a resharding event

[resharding] Handle resharding of delayed receipts #12513

[resharding] Handle resharding of delayed receipts #12513

Conversation

shreyan-gupta commented Nov 25, 2024

Writeup from NEP

Changes in this PR

Next steps

codecov bot commented Nov 25, 2024

Codecov Report

staffik 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

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

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