Skip to content

perf(core): remove newly added body validation until we know its needed outside of syncing#2970

Merged
rodrigo-o merged 4 commits into
mainfrom
remove-additional-validation
May 30, 2025
Merged

perf(core): remove newly added body validation until we know its needed outside of syncing#2970
rodrigo-o merged 4 commits into
mainfrom
remove-additional-validation

Conversation

@rodrigo-o

@rodrigo-o rodrigo-o commented May 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Motivation

This removes an additional validation done on the pre-execution of blocks as part of this PR

Description

Remove the call to the validate_block_body in validate_block

Closes #2973

@github-actions

github-actions Bot commented May 29, 2025 •
edited
Loading

Copy link
Copy Markdown

Lines of code report

Total lines added: 0
Total lines removed: 2
Total lines changed: 2

Detailed view 
+----------------------------------------+-------+------+
| File                                   | Lines | Diff |
+----------------------------------------+-------+------+
| ethrex/crates/blockchain/blockchain.rs | 491   | -2   |
+----------------------------------------+-------+------+

@rodrigo-o rodrigo-o marked this pull request as ready for review May 29, 2025 19:54
@rodrigo-o rodrigo-o requested a review from a team as a code owner May 29, 2025 19:54
@rodrigo-o

rodrigo-o commented May 29, 2025

Copy link
Copy Markdown
Contributor Author

Running the benchmark showed a 10% improved performance after removing the function call from validate_block:

main: test Block import/Block import ERC20 transfers ... bench: 202550917686 ns/iter (+/- 2862677651)
this branch: test Block import/Block import ERC20 transfers ... bench: 177127830858 ns/iter (+/- 2722864879)

I'm going to create an issue for checking possible missing validations and also to take a look at how #2658 affected syncing performance.

@rodrigo-o rodrigo-o self-assigned this May 29, 2025
@rodrigo-o rodrigo-o mentioned this pull request May 29, 2025
Open
fmoletta
fmoletta reviewed May 29, 2025
View reviewed changes

@fmoletta fmoletta left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we add validate_block_body to where validate_block is used in syncing?

@rodrigo-o

rodrigo-o commented May 29, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Shouldn't we add validate_block_body to where validate_block is used in syncing?

As far as I can tell, the validate block body added here was just an addition, we are already checking bodies on request here:

ethrex/crates/networking/p2p/peer_handler.rs

Line 248 in ff28c6c

.find_map(|block| validate_block_body(block).err())

Or am I missing something?

@rodrigo-o

rodrigo-o commented May 29, 2025

Copy link
Copy Markdown
Contributor Author

Issue #2977 created

@fmoletta

fmoletta commented May 29, 2025

Copy link
Copy Markdown
Contributor

Shouldn't we add validate_block_body to where validate_block is used in syncing?

As far as I can tell, the validate block body added here was just an addition, we are already checking bodies on request here:

ethrex/crates/networking/p2p/peer_handler.rs

Line 248 in ff28c6c

.find_map(|block| validate_block_body(block).err())

Or am I be missing something?

My bad, I thought it was added because we were using validate_block on the peer handler, ignore my previous comment

@rodrigo-o rodrigo-o added this pull request to the merge queue May 30, 2025
Merged via the queue into main with commit d202d1d May 30, 2025
20 checks passed
@rodrigo-o rodrigo-o deleted the remove-additional-validation branch May 30, 2025 15:35
pedrobergamini pushed a commit to pedrobergamini/ethrex that referenced this pull request Aug 24, 2025
@rodrigo-o
perf(core): remove newly added body validation until we know its need…
8fc81bf 
…ed outside of syncing (lambdaclass#2970)

**Motivation**

This removes an additional validation done on the pre-execution of
blocks as part of [this
PR](lambdaclass#2658)

**Description**

Remove the call to the `validate_block_body` in `validate_block`

Closes lambdaclass#2973
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JereSalo JereSalo JereSalo approved these changes

+1 more reviewer

@fmoletta fmoletta fmoletta approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@rodrigo-o rodrigo-o

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Performance degrade after #2658

3 participants

@rodrigo-o @fmoletta @JereSalo