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

Streamline validation #3448

Merged
merged 10 commits into from
Jul 5, 2024
Merged

Streamline validation #3448

merged 10 commits into from
Jul 5, 2024

Conversation

grarco
Copy link
Contributor

@grarco grarco commented Jun 27, 2024

Describe your changes

Closes #3190.
Closes #3193.

This PR fixes some wrong documentation for the finalize_block and check_proposal_tx_function.

It also introduces a new local configuration parameter to allows a node to rerun the process_proposal checks before finalize block in case they don't want to blindly trust the majority of validators. This is implemented for both full nodes and validators since the latter could also skip the process_proposal call in some cases.

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

#3192 (diffs for review: https://github.com/anoma/namada/pull/3448/files/176754fbd523ec7d17a1dbbd10f59900b846aeb1..1a704b679b122656aa4ff6d0baa947054c326271)

Checklist before merging to draft

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

Copy link

codecov bot commented Jun 27, 2024

Codecov Report

Attention: Patch coverage is 30.48780% with 114 lines in your changes missing coverage. Please review.

Project coverage is 53.87%. Comparing base (879a326) to head (fc3241d).
Report is 4 commits behind head on main.

Files Patch % Lines
crates/node/src/lib.rs 0.00% 49 Missing ⚠️
crates/apps_lib/src/cli.rs 0.00% 28 Missing ⚠️
crates/node/src/shell/mod.rs 31.70% 28 Missing ⚠️
crates/node/src/shell/process_proposal.rs 46.66% 8 Missing ⚠️
crates/apps_lib/src/config/mod.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3448      +/-   ##
==========================================
- Coverage   53.92%   53.87%   -0.05%     
==========================================
  Files         317      317              
  Lines      107575   107664      +89     
==========================================
- Hits        58011    58007       -4     
- Misses      49564    49657      +93     

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

@grarco
Copy link
Contributor Author

grarco commented Jun 28, 2024

NOTE: currently, the recheck is done calling process_txs but I believe it would be slightly better to call process_proposal (which is the handler of the cometbft's call), so that if we changed anything in the handler that would immediately translate to the recheck logic too, while, at the moment, if the modification lies outside of process_txs this would not translate to the recheck. On top of that, because of the shim, nodes can currently call process_txs twice for the same block proposal. With the addition of the recheck logic this can go up to 3 calls to this function: would be nice if we could keep track of the whether the function already ran so that we can ensure that it gets called at most once.

I'll try to address both of these in another branch

@grarco grarco marked this pull request as ready for review June 28, 2024 11:27
@grarco grarco requested a review from tzemanovic June 28, 2024 11:27
Copy link
Member

@tzemanovic tzemanovic left a comment

Choose a reason for hiding this comment

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

1 small style recommendation - other than that LGTM

@tzemanovic tzemanovic mentioned this pull request Jul 1, 2024
brentstone added a commit that referenced this pull request Jul 3, 2024
* grarco/streamline-validation:
  Refactors recheck of `process_proposal` before `finalize_block`
  Changelog #3448
  Clippy + fmt
  Removes redundant check in `finalize_block` and implements recheck
  Updates docs of `finalize_block` and `process_proposal`
  Moves allowed txs check in `process_proposal` before the check on fees
  Removes duplicated validation in `prepare_proposal`
  Anticipates check on tx allowlist in mempool
  Changelog #3192
  Minor code optimizations
brentstone added a commit that referenced this pull request Jul 3, 2024
* grarco/streamline-validation:
  Refactors recheck of `process_proposal` before `finalize_block`
  Changelog #3448
  Clippy + fmt
  Removes redundant check in `finalize_block` and implements recheck
  Updates docs of `finalize_block` and `process_proposal`
  Moves allowed txs check in `process_proposal` before the check on fees
  Removes duplicated validation in `prepare_proposal`
  Anticipates check on tx allowlist in mempool
brentstone added a commit that referenced this pull request Jul 4, 2024
* origin/grarco/streamline-validation:
  Refactors recheck of `process_proposal` before `finalize_block`
  Changelog #3448
  Clippy + fmt
  Removes redundant check in `finalize_block` and implements recheck
  Updates docs of `finalize_block` and `process_proposal`
  Moves allowed txs check in `process_proposal` before the check on fees
  Removes duplicated validation in `prepare_proposal`
  Anticipates check on tx allowlist in mempool
@brentstone brentstone merged commit 91d82c9 into main Jul 5, 2024
16 of 19 checks passed
@brentstone brentstone deleted the grarco/streamline-validation branch July 5, 2024 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants