Lenient duplicate checks on HTTP API for block publication#5574
Merged
mergify[bot] merged 35 commits intosigp:unstablefrom Sep 24, 2024
Merged
Lenient duplicate checks on HTTP API for block publication#5574mergify[bot] merged 35 commits intosigp:unstablefrom
mergify[bot] merged 35 commits intosigp:unstablefrom
Conversation
* save * save * make ProvenancedBlock concrete * delete into gossip verified block contents * get rid of IntoBlobSidecar trait * remove IntoGossipVerified trait * get tests compiling * don't check sidecar slashability in publish * remove second publish closure * drop blob bool. also prefer using message index over index of position in list
Member
Author
|
Ready for review, but I wouldn't recommend that relays start testing this PR until the issues with |
michaelsproul
commented
Jul 16, 2024
jimmygchen
reviewed
Jul 23, 2024
jimmygchen
reviewed
Jul 23, 2024
jimmygchen
reviewed
Jul 23, 2024
jimmygchen
reviewed
Jul 23, 2024
jimmygchen
requested changes
Jul 23, 2024
Member
jimmygchen
left a comment
There was a problem hiding this comment.
I've added a few more comments. Looks great!
I wasn't expecting that much change, but looks like we do need to cover all the scenarios you've identified here. Nice tests you've added here - it helps me understand what we're trying to achieve here! 👍
Looks like I will have to rework quite a bit in PeerDAS 💀
jimmygchen
reviewed
Sep 23, 2024
jimmygchen
requested changes
Sep 23, 2024
Member
jimmygchen
left a comment
There was a problem hiding this comment.
Just a few more minor comments, otherwise looks good to me. Would be good to get another pair of eyes on this.
There's probably some potential refactor we can do in publish_block, but perhaps we can do it in a separate PR, as this PR has a few simplifications that a few other changes depend on, e.g. PeerDAS publish block optimisations made in b0e6283. The added tests in this PR should make future refactoring easier.
Member
Author
|
@mergify queue |
✅ The pull request has been merged automaticallyDetailsThe pull request has been merged automatically at 2792705 |
Member
Author
|
tasty insta merge finally |
chong-he
pushed a commit
to chong-he/lighthouse
that referenced
this pull request
Nov 26, 2024
* start splitting gossip verification * WIP * Gossip verify separate (#7) * save * save * make ProvenancedBlock concrete * delete into gossip verified block contents * get rid of IntoBlobSidecar trait * remove IntoGossipVerified trait * get tests compiling * don't check sidecar slashability in publish * remove second publish closure * drop blob bool. also prefer using message index over index of position in list * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Fix low-hanging tests * Fix tests and clean up * Clean up imports * more cleanup * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Further refine behaviour and add tests * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Remove empty line * Fix test (block is not fully imported just gossip verified) * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Update for unstable & use empty blob list * Update comment * Add test for duplicate block case * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Clarify unreachable case * Fix another publish_block case * Remove unreachable case in filter chain segment * Revert unrelated blob optimisation * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Fix merge conflicts * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Fix some compilation issues. Impl is fucked though * Support peerDAS * Fix tests * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Fix conflict * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate * Address review comments * Merge remote-tracking branch 'origin/unstable' into gossip-verify-separate
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue Addressed
Fix the much-talked-about bug in Lighthouse's HTTP API which prevents blocks with blobs from being published if the block has already been seen on gossip
See: https://gist.github.com/benhenryhunter/7b7d9c9e3218aad52f75e3647b83a6cc
Proposed Changes
process_blobso that it takes apublish_fnwhich can be used to publish the block in the case where it is imported during blob processing.BlockIsAlreadyKnownerror into two cases so we can differentiate duplicate blocks that have already been fully imported from ones that are being imported/failed import.publish_blockso that it publishes all unseen blocks and blobs on gossip.