-
Notifications
You must be signed in to change notification settings - Fork 283
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
Reject duplicate sidecars in BlobSidecarsByRoot #7658
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Nice catch! |
not super super about synchronization though |
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.
Yeah I agree. Is there a better way to do this? |
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. |
Made a PR to the consensus specs to bring this up: |
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 |
...a/tech/pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRootValidator.java
Outdated
Show resolved
Hide resolved
...a/tech/pegasys/teku/networking/eth2/rpc/beaconchain/methods/BlobSidecarsByRootValidator.java
Outdated
Show resolved
Hide resolved
@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. |
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. |
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. |
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
But I'm also concerning on this #7658 (comment) though we could address it separately |
By spec we cannot require what we really want, so we should do it in our Or we shouldn't require the same order? |
okay, I've reread your discussion in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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
Documentation
doc-change-required
label to this PR if updates are required.Changelog