Block relay improvements #1883

rahulksnv · 2023-08-25T18:08:42Z

Add support for returning multiple complete blocks, without going through the protocol. This is meant for scenarios where a node is syncing upon start
Enable block relay by default.
Clean ups

Fixes #1901

Code contributor checklist:

I have read, understood and followed contributing guide

nazar-pc · 2023-08-25T18:58:49Z

Why draft again, should this be reviewed already?

rahulksnv · 2023-08-25T19:42:17Z

Why draft again, should this be reviewed already?

Local testing looks good, made this to get a run on dev net. Pls ignore till then

Enable block relay by default, with override to disable.

nazar-pc

One concern about size estimation and some questions

crates/sc-subspace-block-relay/src/consensus.rs

nazar-pc · 2023-08-29T00:21:14Z

crates/sc-subspace-block-relay/src/consensus.rs


    /// Message to be handled by the protocol
-    ProtocolRequest(ProtocolRequest),
+    Protocol(ProtocolRequest),


Everything here seems to be handled by the protocol 🤔 Is there a better name for it?

This looks short/self explanatory, but open to suggestions

Well, that is my point. It is not realy explanatory, those requests are protocol requests just like other requests which are not called protocol requests, but clearly still are.

nazar-pc · 2023-08-29T00:31:37Z

crates/sc-subspace-block-relay/src/consensus.rs

+            // Enforce the max limit on response size.
+            let mut bytes = body.as_ref().map_or(0, |extrinsics| {
+                extrinsics.iter().map(|ext| ext.encoded_size()).sum()
+            });
+            bytes += partial_block
+                .indexed_body
+                .as_ref()
+                .map_or(0, |entries| entries.iter().map(|entry| entry.len()).sum());
+            if !blocks.is_empty() && (total_size + bytes) > MAX_RESPONSE_SIZE.into() {
+                break;
+            }
+            total_size += bytes;
+
+            block_id = match block_request.direction {
+                Direction::Ascending => BlockId::Number(partial_block.block_number + One::one()),
+                Direction::Descending => {
+                    if partial_block.block_number.is_zero() {
+                        break;
+                    }
+                    BlockId::Hash(partial_block.parent_hash)
+                }
+            };
+            blocks.push(partial_block.block_data(body));


Looks like you're pushing partial blocks into response, but measure something different when checking bytes size. I see Substrate is probably buggy the same way (will submit PR there too). I think what you should do instead is to push new entry into blocks, check blocks.encoded_size() and if it exceeds the limit just pop last entry before breaking out of this loop.

This tries to mirror substrate as much as possible. They omit headers, justifications in the size (possible to pack more blocks in response). Unless we have big hdrs (related to segment index, PoT), this should not make much difference IMO

Do you know why would they omit them though? Because to me having a size limit and then consciously exceeding that such that response doesn't fit into protocol message size limit makes no sense whatsoever and most likely is a bug, not a feature. Hence I reported it as such in Substrate: paritytech/polkadot-sdk#1232

So rather than mirroring bugs (that will be hard to debug BTW) we should fix them here and report upstream as well.

The extrinsics is the bulk of the transfer I really care about, I am fine either ways with the other fields. Do you want to include whole encoded size?

I certainly want to include the whole encoded size. Otherwise we're waiting for issue to happen and it certainly will and there will be no recovery out of it. I don't see why we want to intentionally bake such bugs into the protocol.

In fact, I would have liked to just call the default block handler in cases like these, instead of duplicating the logic.. but that would need more upstream changes

I think calling .encoded_size() is not difficult and upstream does have a bug, so it is actually nice that we handle it ourselves and can bypass it.

will make the change, I am not so much worried about the size of the change itself, but deviating from upstream behavior during sync and hitting some other issue (this upstream path has proved to be fragile in the past)

Changed, retested

crates/sc-subspace-block-relay/src/consensus.rs

nazar-pc · 2023-08-29T17:20:57Z

crates/sc-subspace-block-relay/src/consensus.rs

+            let bytes = block_data.encoded_size();
+            if !blocks.is_empty() && (total_size + bytes) > MAX_RESPONSE_SIZE.into() {


This isn't 100% correct. Encoding depends on number of elements, total_size + bytes will not account for that. This is why I was recommending to push a new element, use blocks.encoded_size() from the beginning to check the size and popping last element if after insertion the limit was exceeded.

See https://docs.substrate.io/reference/scale-codec/#fn-1 for details of how compact numbers are encoded in SCALE codec.

I understand, but that is bit ugly. This may be off by a few bytes over, but would be negligible compared to the 8MB size limit (which itself is arbitrary)

It is annoying because we're doing more calculation. But those few bytes might be enough for node to get stuck and being unable to continue sync because it is one or two bytes above the limit.

If you want to optimize this (not checking encoded_size on the same blocks over and over again), you can initialize total_size with max size of compact length encoding (so compact length encoding of 128 in our case, can be done with https://docs.rs/parity-scale-codec/latest/parity_scale_codec/trait.CompactLen.html#tymethod.compact_len), then you can just use addition after that the same way you do now.

This is blockchain, why be not precise when you can be exact?

But those few bytes might be enough for node to get stuck and being unable to continue sync because it is one or two bytes above the limit.

That is not how it works (e.g) say the block has one extrinsic which is > the limit, it will get sent (see similar check in upstream). The right solution for this would be chunking at the source.

I don't mind making the change, but if there is a strong reason.

That is not how it works (e.g) say the block has one extrinsic which is > the limit, it will get sent (see similar check in upstream). The right solution for this would be chunking at the source.

I do not understand what you mean here. I think we have already established that upstream is buggy and shouldn't be followed. We also know that at least a single block will fit into the response, so we don't need to worry about that. Thus we don't need any chunking, we just send as many block as fit into the limit and client will reach out again if they need more blocks later.

yeah I was concerned about the repeated encoded_size() calls(+ duplicate encode() in the end). Other option would be to keep the encoded Vec<> as we add entries, and use something like EncodeAppend to append. But the rollback of the last entry becomes expensive/messy.

I could not find anything satisfactory, so ended up implementing a simpler scheme. This allows appending encoded blocks, while able to check for overflow before append (no rollback needed)

nazar-pc · 2023-08-30T21:59:04Z

Was merged as part of #1911

rahulksnv added 4 commits August 28, 2023 15:25

Refactor, add types

cfd2a57

Client changes

a32be33

Server changes

ab23f54

Cmd line changes.

72cf71d

Enable block relay by default, with override to disable.

rahulksnv force-pushed the rsub/br-changes branch from 5bbf59a to 72cf71d Compare August 28, 2023 22:39

rahulksnv marked this pull request as ready for review August 28, 2023 22:42

rahulksnv requested review from NingLin-P, nazar-pc and rg3l3dr as code owners August 28, 2023 22:42

nazar-pc reviewed Aug 29, 2023

View reviewed changes

rahulksnv force-pushed the rsub/br-changes branch from f367c03 to 602170f Compare August 29, 2023 16:26

Address comments

06146e1

rahulksnv force-pushed the rsub/br-changes branch from 602170f to 06146e1 Compare August 29, 2023 17:17

nazar-pc reviewed Aug 29, 2023

View reviewed changes

Implement incremental encoder

5e82721

nazar-pc mentioned this pull request Aug 30, 2023

Block relay improvements (take 2) #1911

Merged

1 task

nazar-pc closed this Aug 30, 2023

nazar-pc deleted the rsub/br-changes branch August 30, 2023 21:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block relay improvements #1883

Block relay improvements #1883

rahulksnv commented Aug 25, 2023 •

edited

Loading

nazar-pc commented Aug 25, 2023

rahulksnv commented Aug 25, 2023

nazar-pc left a comment

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 29, 2023

nazar-pc Aug 29, 2023

rahulksnv Aug 30, 2023

nazar-pc commented Aug 30, 2023

		let bytes = block_data.encoded_size();
		if !blocks.is_empty() && (total_size + bytes) > MAX_RESPONSE_SIZE.into() {

Block relay improvements #1883

Block relay improvements #1883

Conversation

rahulksnv commented Aug 25, 2023 • edited Loading

Code contributor checklist:

nazar-pc commented Aug 25, 2023

rahulksnv commented Aug 25, 2023

nazar-pc 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

nazar-pc commented Aug 30, 2023

rahulksnv commented Aug 25, 2023 •

edited

Loading