Reject duplicate sidecars in BlobSidecarsByRoot

[jtraglia]

PR Description

After sending a BlobSidecarsByRoot request to a peer, it would be possible for the peer to send the same blob sidecar multiple times. Not necessarily a problem, but I think it would be a good idea to disallow this. We expect peers to send unique sidecars like they're supposed to.

I was initially concerned with this function because I thought it would be possible to send the same sidecar forever. This isn't possible though because Eth2OutgoingRequestHandler handles this properly. Nice.

teku/networking/eth2/src/main/java/tech/pegasys/teku/networking/eth2/rpc/core/Eth2OutgoingRequestHandler.java

Lines 131 to 133 in f31b239

	if (chunksReceived > maximumResponseChunks) {
	throw new ExtraDataAppendedException();
	}

Documentation

• I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

• I thought about adding a changelog entry, and added one if I deemed necessary.

[zilm13]

LGTM

[zilm13]

Nice catch!
I was also thinking on checking the order, but decided it's too paranoid for now. And it's a bit complicated while delivering low value because we allow gaps in the response.

[tbenr]

not super super about synchronization though

[jtraglia]

@zilm13: I was also thinking on checking the order, but decided it's too paranoid for now.

I also had this thought. I checked the specs and didn't see anything about ordering. I can imagine a situation where a peer returns the sidecars in a different order due to some optimizations or something.

@tbenr: not super super about synchronization though

Yeah I agree. Is there a better way to do this?

[tbenr]

Since AsyncResponseProcessor is designed to run the response processing strictly one by one, we can avoid synchronization and just have a thread safe Set that can be used by different threads (threads can still change from one chunk to the other). I pushed a commit. WDYT @zilm13 @jtraglia ?

[jtraglia]

I think that looks good! But maybe we do need to check order after all. Chatting with colleagues some about it and it seems that it does matter. Checking order might simplify this too, as we wouldn't need to delete entries.

[jtraglia]

Made a PR to the consensus specs to bring this up:

• Add sentences about order of ByRoot responses ethereum/consensus-specs#3544

[zilm13]

Ok let's freeze it for a while (until spec resolution) and we could do order check without any synchronization. volatile Optional<> previous could be enough

[rolfyone]

@StefanBratanov are we circling back on this one?

[StefanBratanov]

@StefanBratanov are we circling back on this one?

There is a spec PR ethereum/consensus-specs#3544 that we would like to be closed/merged, before circling back on this one, so will leave this PR open for now.

[zilm13]

Btw I've checked our code and if we get a response in a wrong order we will fail on import because for BlobSidecars it could mean that you request [b0s0,b0s1,b0s2] and receive [b0s2, b0s0, b0s1]. So if MUST is not added to the spec, we need to add sorting somewhere.

[jtraglia]

I've pulled in changes from master & addressed Stefan's review comments. If the team is still interested in merging this PR, it should be ready now. If not, happy to close it. I don't think we're going to make this change in the consensus-specs.

[StefanBratanov]

Hi @jtraglia rejection of duplicates makes sense to me and I think it's a good one. @tbenr @zilm13 what's your take?

[zilm13]

It's better than nothing as otherwise we will add it twice here

teku/beacon/sync/src/main/java/tech/pegasys/teku/beacon/sync/historical/HistoricalBatchFetcher.java

Lines 250 to 254 in 94d3782

	private void processBlobSidecar(final BlobSidecar blobSidecar) {
	blobSidecarsBySlotToImport
	.computeIfAbsent(blobSidecar.getSlotAndBlockRoot(), __ -> new ArrayList<>())
	.add(blobSidecar);
	}

But I'm also concerning on this #7658 (comment) though we could address it separately

[zilm13]

By spec we cannot require what we really want, so we should do it in our HistoricalBatchFetcher instead, but I'm thinking on something like this, which is more or less up to the spec and looks like covering all needed validations
What do you think @StefanBratanov @jtraglia zilm13@7b140b7

Or we shouldn't require the same order?
If we cannot require the same order, existing PR by @jtraglia is good

[zilm13]

okay, I've reread your discussion in consensus-specs, looks like there is no consensus about the order, so I'm happy to merge this one PR
We just need extra sorting in HistoricalBatchFetcher, we cannot put all our needs in the spec.  Actually I see there is an area of possible improvements in the fetcher, so we will do it in some separate PR

[tbenr]

LGTM