[Persistence][Utility] Separate all CreateAndApply functions into more functional components - Issue #508

[deblasis]

Description

This PR tends to the deliverables in the parent issue

Issue

Fixes #508

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

Consensus

• Updated to reflect the new ApplyBlock signature from utilityUnitOfWork
• Updated to use utilityUnitOfWork.GetStateHash()
• Removed duplicate import
• Renamed CreateAndApplyProposalBlock to CreateProposalBlock

Shared

• Renamed CreateAndApplyProposalBlock to CreateProposalBlock
• Added GetStateHash to UtilityUnitOfWork

Utility

• Renamed CreateAndApplyProposalBlock to CreateProposalBlock
• Added GetPrevBlockByzantineValidators and ProposalBlockNotSet errors
• Instrumented CreateProposalBlock and ApplyBlock with log statements
• Refactored functions for block creation and application to be more readable/modular
• Added TODOs for future refactoring
• Renamed u to uow for consistency
• Added stateHash validation against proposalBlock
• Added feature flag for stateHash validation (for testing purposes)
• Updated tests accordingly

Testing

• make develop_test; if any code changes were made
• Docker Compose LocalNet; if any major functionality was changed or introduced
• k8s LocalNet; if any infrastructure or configuration changes were made

Required Checklist

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
• I have tested my changes using the available tooling
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

[github-actions]

The changelog validation failed with the following output:

Missing changelog in module: consensus/

Missing changelog in module: shared/

Missing changelog in module: utility/

Changelog verification failed. See error messages for more detail.

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

[reviewpad]

AI-Generated Pull Request Summary: This pull request introduces various changes to improve code consistency, readability, error handling, and logging across multiple files. The primary modification is refactoring the CreateAndApplyProposalBlock function into a new function called CreateProposalBlock, along with separating the mempool reaping logic into another function called reapMempool. Additional changes include updating the block.go file to handle Byzantine validators, renaming several functions and method calls, updating comments and TODOs, and introducing new error codes, messages, and error creation functions. Furthermore, there are updates to flow diagrams, sequence diagrams, and documentation in the PROTOCOL_STATE_HASH.md file. Finally, test cases have been modified, and new test cases have been added to ensure proper error handling.

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

[github-actions]

The changelog validation failed with the following output:

Missing changelog in module: consensus/

Missing changelog in module: shared/

Missing changelog in module: utility/

Changelog verification failed. See error messages for more detail.

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

[reviewpad]

AI-Generated Pull Request Summary: This pull request introduces updates and improvements to several files, primarily focusing on renaming the CreateAndApplyProposalBlock function to CreateProposalBlock and improving the transaction processing and error handling involved in creating a proposal block. Key changes include:

1. Documentation updates in PROTOCOL_STATE_HASH.md, making flowcharts and descriptions consistent with the updated naming.
2. Modifications to block_test.go, utility_module.go, utility/types/error.go, consensus/hotstuff_leader.go, consensus/e2e_tests/utils_test.go, and uow_leader.go primarily to accommodate the renaming of CreateAndApplyProposalBlock to CreateProposalBlock and handle error cases more gracefully.
3. Introduction of several new functions for better modularity and readability, such as processTransactionsFromProposalBlock, reapMempool, and prevBlockByzantineValidators.
4. Refactoring and restructuring of the ApplyBlock function and the addition of a new helper function for better transaction processing.
5. Improvements in readability and maintainability within the block.go file, including renaming variables, adding log statements, and handling error cases more gracefully.

These changes help improve transaction processing, error handling, and maintainability in the creation of a proposal block and contribute to the overall robustness and efficiency of the system.

[deblasis]

Sweet

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

The check succeeded, dismissing the review comment.

[github-actions]

Here's a brief review of the provided code patch:

1. It seems that you are updating the project's changelog with a new version (0.0.0.35) and its main changes.
2. You have renamed a function CreateAndApplyProposalBlock to CreateProposalBlock, which is clearer if it only creates proposal blocks without applying them.
3. Two new error types were added: GetPrevBlockByzantineValidators and ProposalBlockNotSet. Make sure they are caught and handled correctly.
4. Several log statements were added for instrumentation, this will help in debugging and understanding the application flow.
5. Refactoring was done to improve readability and modularity of the code. This suggests a cleaner codebase but requires proper testing to ensure functionality hasn't been compromised.
6. TODOs were added indicating additional refactoring plans. Be sure to follow up on these to continue improving the codebase.
7. A variable was renamed u to uow for consistency. Ensure all references to the old name have been updated.
8. stateHash validation against proposalBlock has been added, which should improve security for the block. However, make sure that no incorrect validations throw unexpected errors.
9. A feature flag was introduced for stateHash validation for testing purposes. Remember to remove this when appropriate or decide whether to toggle it in production.
10. Lastly, tests have been updated accordingly. Confirm that all tests still pass after the above changes have been made.

Overall, the patch seems focused on increasing code readability and improving functionality. Testing the updated code thoroughly, both automatically and manually, is critical to ensure correctness and stability. Additionally, prompt action on the TODOs mentioned would be ideal for continuous improvement.

[reviewpad]

AI-Generated Summary: This pull request includes changes across multiple files, refactoring and updating the codebase for better readability, maintainability, and functionality. Key updates include:

1. Splitting the CreateAndApplyProposalBlock function into separate CreateProposalBlock and ApplyBlock functions.
2. Introducing a new method GetStateHash in the UtilityUnitOfWork interface, and updating various functions and tests to use it.
3. Adding new error codes, error messages, and error handling to the codebase.
4. Improved logging and variable naming throughout.
5. Updating CHANGELOG.md and other documentation files to reflect the changes made in this pull request.
6. Changes in import paths and refactoring of some functions for better organization.

Overall, this pull request enhances the code clarity, error handling, and modularization in the codebase, making it more maintainable and better for future development.

[deblasis]

@Olshansk I believe I tackled everything

[Olshansk]

Can you just rerun tests & localnet on your machine to make sure that the failure in consensus/e2e_tests/state_sync_test.go was a one-off flaky issue?

Otherwise, just see my suggestion and

[Olshansk]

This actually isn't bad

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

[github-actions]

The changelog validation failed with the following output:

Latest date in consensus/doc/CHANGELOG.md is incorrect.
Latest: 2023-04-07, Current: 2023-04-06

Latest date in shared/CHANGELOG.md is incorrect.
Latest: 2023-04-07, Current: 2023-04-06

Latest date in utility/doc/CHANGELOG.md is incorrect.
Latest: 2023-04-07, Current: 2023-04-06

Changelog verification failed. See error messages for more detail.

Please update the relevant CHANGELOG.md files and ensure they follow the correct format.

[reviewpad]

AI-Generated Summary: This pull request includes changes in multiple files related to the refactoring of blockchain and consensus code, improving readability and consistency. Key changes involve renaming the function CreateAndApplyProposalBlock to CreateProposalBlock, updating error codes, implementing new functions like GetStateHash and processTransactionsFromProposalBlock, and modifying tests to adapt to these changes. Additional updates include changes to the CHANGELOG.md, logging improvements, import updates, and renaming of an imported package alias. Overall, these modifications aim to enhance the code structure, testing scenarios, and error handling, while ensuring better debugging and tracing execution.

[cr-gpt]

Seems you are using me but didn't get OPENAI_API_KEY seted in Variables for this repo. you could follow readme for more information

[github-actions]

Based on the given code patch, here's a brief code review:

1. The changes in versioning and release notes are well-documented and clear.

2. Renaming CreateAndApplyProposalBlock to CreateProposalBlock potentially reflects better separation of concerns in your application.

3. Updating to use the new ApplyBlock signature and GetStateHash() function from utilityUnitOfWork ensures consistency across the project.

4. Removing the duplicate import means the code will be cleaner and more efficient.

5. Formatting inconsistencies are fixed by adding line breaks before and after listing updates for [0.0.0.42].

Overall, the changes appear beneficial and I don't see any obvious bugs or risks. However, it's important to ensure these changes are covered by appropriate tests and don't introduce unintended behavior throughout the project.

The check succeeded, dismissing the review comment.

[reviewpad]

AI-Generated Summary: This pull request introduces various changes across multiple files, primarily focusing on refactoring the CreateAndApplyProposalBlock function into two separate functions, CreateProposalBlock and ApplyBlock, as well as adding a new method GetStateHash() to the UtilityUnitOfWork interface. Other changes include updating tests and error handling, renaming variables and functions for consistency, enhancing logging, and updating the documentation and CHANGELOG.md file accordingly. Additionally, the pull request improves code organization, readability, and maintainability in various areas of the codebase.

[deblasis]

Can you just rerun tests & localnet on your machine to make sure that the failure in consensus/e2e_tests/state_sync_test.go was a one-off flaky issue?

Otherwise, just see my suggestion and

test.mp4

👍

[Olshansk]