Skip to content

Recover envelope sse events#8347

Merged
tersec merged 5 commits into
unstablefrom
sam/lQp
May 8, 2026
Merged

Recover envelope sse events#8347
tersec merged 5 commits into
unstablefrom
sam/lQp

Conversation

@ahshum
Copy link
Copy Markdown
Contributor

@ahshum ahshum commented Apr 27, 2026 •
edited
Loading

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 27, 2026 •
edited
Loading

Unit Test Results

       12 files  ±0    2 836 suites  ±0   1h 19m 8s ⏱️ - 1m 34s
15 934 tests ±0  14 369 ✔️ ±0  1 565 💤 ±0  0 ❌ ±0 
76 696 runs  ±0  74 956 ✔️ ±0  1 740 💤 ±0  0 ❌ ±0 

Results for commit cd33408. ± Comparison against base commit 00c6d5b.

♻️ This comment has been updated with latest results.

tersec
tersec reviewed Apr 28, 2026
View reviewed changes
Comment thread beacon_chain/consensus_object_pools/block_clearance.nim
# Put the envelope into db and update optimistic status for the block.
dag.db.putExecutionPayloadEnvelope(signedEnvelope)

# https://github.com/ethereum/beacon-APIs/blob/v5.0.0-alpha.1/apis/eventstream/index.yaml
Copy link
Copy Markdown
Contributor

@tersec tersec Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why remove this comment? The idea is to identify which of the now-3 envelope SSE events triggers on specific conditions, and the dag.onEnvelopeFoo name doesn't make it immediately obvious what the mapping to/from the SSE event name is.

Copy link
Copy Markdown
Contributor Author

@ahshum ahshum Apr 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Beacon block also has 3 SSE events and there is not references/comments so I thought it is fine to not have comment. Are you suggesting to add them to all 3 places and also to block's?

Copy link
Copy Markdown
Contributor Author

@ahshum ahshum Apr 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also confusing to add on trigger instead of the event topic, likes how we define the data types of forks.

Copy link
Copy Markdown
Contributor

@tersec tersec May 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's partly because getting the triggers right is the most subtle part for these, and the part where the spec verbiage most directly applies.

tersec
tersec reviewed May 7, 2026
View reviewed changes
Comment thread beacon_chain/gossip_processing/block_processor.nim Outdated
tersec
tersec reviewed May 7, 2026
View reviewed changes
Comment thread beacon_chain/gossip_processing/block_processor.nim Outdated
@tersec tersec merged commit e0ba839 into unstable May 8, 2026
11 checks passed
@tersec tersec deleted the sam/lQp branch May 8, 2026 12:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tersec tersec tersec left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ahshum @tersec