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.

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

Avoid multiple calls to process_proposal #3473

Merged
merged 12 commits into from
Jul 24, 2024
Merged

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Jul 2, 2024

Describe your changes

This PR tries to improve the extra calls to process proposal requested by Namada (nothing changes for the requests coming from Comet). More specifically:

  • The more complete process_proposal is now called instead of process_txs
  • The InMemory struct has been expanded with an extra field that caches the requests to process_proposal for a given block so that future calls can be avoided. This cache is cleared when handling a FinalizeBlock request

Indicate on which release or other PRs this topic is based on

v0.40.0

Checklist before merging to draft

  • I have added a changelog
  • Git history is in acceptable state

Copy link

codecov bot commented Jul 2, 2024

Codecov Report

Attention: Patch coverage is 10.78838% with 215 lines in your changes missing coverage. Please review.

Project coverage is 53.41%. Comparing base (8479d38) to head (5523e05).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/node/src/shims/abcipp_shim_types.rs 0.00% 78 Missing ⚠️
crates/node/src/shims/abcipp_shim.rs 0.00% 72 Missing ⚠️
crates/node/src/lib.rs 0.00% 36 Missing ⚠️
crates/node/src/shell/testing/node.rs 0.00% 12 Missing ⚠️
crates/core/src/hash.rs 0.00% 5 Missing ⚠️
...node/src/shell/vote_extensions/bridge_pool_vext.rs 0.00% 4 Missing ⚠️
...rates/node/src/shell/vote_extensions/eth_events.rs 0.00% 4 Missing ⚠️
...s/node/src/shell/vote_extensions/val_set_update.rs 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3473      +/-   ##
==========================================
- Coverage   53.48%   53.41%   -0.08%     
==========================================
  Files         320      320              
  Lines      110000   110160     +160     
==========================================
+ Hits        58832    58840       +8     
- Misses      51168    51320     +152     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

grarco added a commit that referenced this pull request Jul 2, 2024
@grarco grarco marked this pull request as ready for review July 2, 2024 17:07
@grarco grarco requested review from tzemanovic and sug0 July 2, 2024 17:07
crates/node/src/shell/finalize_block.rs Show resolved Hide resolved
crates/node/src/lib.rs Outdated Show resolved Hide resolved
crates/node/src/shims/abcipp_shim.rs Outdated Show resolved Hide resolved
crates/node/src/shims/abcipp_shim.rs Outdated Show resolved Hide resolved
crates/node/src/shims/abcipp_shim.rs Outdated Show resolved Hide resolved
@grarco
Copy link
Contributor Author

grarco commented Jul 4, 2024

@sug0 I've pushed a few more commits to change some things on top of your recommendations:

  • We know have a custom type for the cached elements to avoid caching the txs results when the block is rejected (since we don't need them)
  • The cache is not more a HashMap but a CLruCache to avoid consuming too much memory in cases where the network struggles to make progress and starts producing many block proposals

Let me know if they are ok or it's better to revert the last commits

@sug0 sug0 self-requested a review July 8, 2024 08:25
sug0
sug0 previously approved these changes Jul 8, 2024
Copy link
Contributor

@sug0 sug0 left a comment

Choose a reason for hiding this comment

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

lgtm!

@grarco grarco dismissed sug0’s stale review July 8, 2024 08:25

The merge-base changed after approval.

@grarco grarco force-pushed the grarco/opt-validation-calls branch from 8674109 to 5523e05 Compare July 8, 2024 09:39
@grarco grarco mentioned this pull request Jul 8, 2024
brentstone pushed a commit that referenced this pull request Jul 9, 2024
brentstone added a commit that referenced this pull request Jul 10, 2024
* grarco/opt-validation-calls:
  Changelog #3473
  Renames process proposal cache
  LRU process proposal cache
  Custom type for process proposal result cache
  Cache of process proposal uses results
  Refactors process proposal check in a separate function
  Avoids full clone of the entire block in process proposal
  Cache check in `try_recheck_process_proposal`
  Cache result of `process_proposal`
  New type for process proposal rechecks
  Removes redundant `votes` field from `FinalizeBlock` request
  Rechecks with `process_proposal` instead of `process_txs`
@brentstone brentstone merged commit f77a23e into main Jul 24, 2024
16 of 19 checks passed
@brentstone brentstone deleted the grarco/opt-validation-calls branch July 24, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants