Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Synchronously check all transactions to have non-zero length #3885

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from
Open

Synchronously check all transactions to have non-zero length #3885

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e1eaa7f
Synchronously check all `transactions` to have non-zero length
etan-status Aug 14, 2024
c746890
Merge branch 'dev' into bf-emptytxspec
etan-status Sep 19, 2024
1e96d23
Merge branch 'dev' into bf-emptytxspec
etan-status Oct 9, 2024
a6095bf
Update specs/deneb/beacon-chain.md
etan-status Oct 21, 2024
f58b5ba
Merge branch 'dev' into bf-emptytxspec
etan-status Oct 21, 2024
44d5a1b
Sync changes to Electra
etan-status Oct 21, 2024
0f964b0
Change "modified" to "new"
jtraglia Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions specs/bellatrix/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,10 +355,17 @@ def verify_and_notify_new_payload(self: ExecutionEngine,
"""
Return ``True`` if and only if ``new_payload_request`` is valid with respect to ``self.execution_state``.
"""
if not self.is_valid_block_hash(new_payload_request.execution_payload):
execution_payload = new_payload_request.execution_payload

if b'' in execution_payload.transactions:
return False
if not self.notify_new_payload(new_payload_request.execution_payload):

if not self.is_valid_block_hash(execution_payload):
return False

if not self.notify_new_payload(execution_payload):
return False

return True
```

Expand Down
5 changes: 4 additions & 1 deletion specs/deneb/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,7 +294,10 @@ def verify_and_notify_new_payload(self: ExecutionEngine,
Return ``True`` if and only if ``new_payload_request`` is valid with respect to ``self.execution_state``.
"""
execution_payload = new_payload_request.execution_payload
parent_beacon_block_root = new_payload_request.parent_beacon_block_root
parent_beacon_block_root = new_payload_request.parent_beacon_block_root # [Modified in Deneb:EIP4788]
jtraglia marked this conversation as resolved.
Show resolved Hide resolved

if b'' in execution_payload.transactions:
return False
Comment on lines +299 to +300
Copy link
Member

Choose a reason for hiding this comment

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

I think I understand the implications of the PR and agree that zero-length transactions should be rejected, but I'm not certain that verify_and_notify_new_payload is best location for such a check. I feel like notify_new_payload via the Engine API should return false if it sees this.

Copy link
Member

Choose a reason for hiding this comment

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

I think notify_new_payload is not an Engine API thing anymore. In fact, verify_and_notify_new_payload is an Engine API. AFAIU, the reason that we have the logic of verify_and_notify_new_payload in the consensus-specs is because we want to show what the function does, but it doesn't mean that it's an CL thing.

So I think putting it in verify_and_notify_new_payload already makes sense because it's a "verify" thing.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm okay, well in that case I guess it doesn't hurt to add. Let's do it!

@etan-status could you merge in dev & make similar changes to electra?

Copy link
Contributor Author

@etan-status etan-status Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yep, verify_and_notify_new_payload is a mock implementation for the minimal EL checks that have to be done.

This is the matching spec that describes the additional check for zero length transactions.


# [Modified in Deneb:EIP4788]
if not self.is_valid_block_hash(execution_payload, parent_beacon_block_root):
Expand Down
3 changes: 3 additions & 0 deletions specs/electra/beacon-chain.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1015,6 +1015,9 @@ def verify_and_notify_new_payload(self: ExecutionEngine,
parent_beacon_block_root = new_payload_request.parent_beacon_block_root
execution_requests_list = get_execution_requests_list(new_payload_request.execution_requests) # [New in Electra]

if b'' in execution_payload.transactions:
return False

if not self.is_valid_block_hash(execution_payload, parent_beacon_block_root):
return False

Expand Down