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

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

Merged
merged 33 commits into from
Apr 7, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
716ffa5
feat(utility): setProposalBlock should be called before apply
deblasis Apr 4, 2023
f7e21f4
refactor(utility): centralized validation in create and apply
deblasis Apr 4, 2023
7d3bf19
fix(utility): fix isProposalBlockSet
deblasis Apr 4, 2023
096470e
refactor(shared): CreateAndApplyProposalBlock & ApplyBlock
deblasis Apr 4, 2023
d6335e4
refactor(consensus): CreateAndApplyProposalBlock & ApplyBlock
deblasis Apr 4, 2023
1ab4d5f
refactor(utility): CreateAndApplyProposalBlock & ApplyBlock
deblasis Apr 4, 2023
54af6c9
refactor(utility): instrumenting with logs
deblasis Apr 4, 2023
88388f8
fix(utility): logging proposer in endBlock
deblasis Apr 4, 2023
f6cf148
docs(consensus): CHANGELOG
deblasis Apr 4, 2023
719c436
docs(shared): CHANGELOG
deblasis Apr 4, 2023
af89d70
docs(utility): CHANGELOG
deblasis Apr 4, 2023
33e19c1
chore(utility): lint
deblasis Apr 4, 2023
f092dcd
chore(utility): lint
deblasis Apr 4, 2023
657841c
chore(shared): nits
deblasis Apr 6, 2023
9a54d76
Update shared/modules/utility_module.go
deblasis Apr 6, 2023
239bc72
Update utility/unit_of_work/module.go
deblasis Apr 6, 2023
c5d07f5
Merge branch 'issue/508-separate-createandapply' of github.com:pokt-n…
deblasis Apr 6, 2023
c80c0fd
Update utility/unit_of_work/uow_leader.go
deblasis Apr 6, 2023
c3828cf
chore(utility): nits
deblasis Apr 6, 2023
757ebff
Merge branch 'issue/508-separate-createandapply' of github.com:pokt-n…
deblasis Apr 6, 2023
a8f80d0
Merge remote-tracking branch 'upstream/main' into issue/508-separate-…
deblasis Apr 6, 2023
9b8c85f
feat(utility): state hash check with feature flag for tests
deblasis Apr 6, 2023
5c625ee
chore(consensus): tidy duplicate import
deblasis Apr 6, 2023
4be8bf7
refactor(consensus): updated to reflect changes in utility
deblasis Apr 6, 2023
391db17
feat(shared): utilityUOW.GetStateHash()
deblasis Apr 6, 2023
9208678
feat(utility): GetStateHash() implementation
deblasis Apr 6, 2023
ed49d93
refactor(utility): processTransactionsFromProposalBlock signature
deblasis Apr 6, 2023
d96bb0e
docs(consensus): CHANGELOG
deblasis Apr 6, 2023
7f5df10
h5law
deblasis Apr 6, 2023
1911fb1
docs(utility): CHANGELOG
deblasis Apr 6, 2023
abbe4e6
chore(utility): CHANGELOG date
deblasis Apr 6, 2023
3ecdc23
Update utility/unit_of_work/module.go
deblasis Apr 6, 2023
d17a865
docs(consensus): CHANGELOG
deblasis Apr 7, 2023
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
8 changes: 6 additions & 2 deletions consensus/doc/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [0.0.0.43] - 2023-04-04

- Renamed `CreateAndApplyProposalBlock` to `CreateProposalBlock`

## [0.0.0.42] - 2023-04-03

- Add `fsm_handler.go` to handle FSM transition events in consensus module
- Update State Machine mock in `utils_test.go`
- Update state_sync module with additional function definitions
- Update state_sync module with additional function definitions

## [0.0.0.41] - 2023-03-30

- Improve & simplify `utilityUnitOfWork` management
Copy link

Choose a reason for hiding this comment

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

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.

Expand Down
2 changes: 1 addition & 1 deletion consensus/e2e_tests/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ func baseLeaderUtilityUnitOfWorkMock(t *testing.T, genesisState *genesis.Genesis
rwContextMock.EXPECT().Release().AnyTimes()

utilityLeaderUnitOfWorkMock.EXPECT().
CreateAndApplyProposalBlock(gomock.Any(), maxTxBytes).
CreateProposalBlock(gomock.Any(), maxTxBytes).
Return(stateHash, make([][]byte, 0), nil).
AnyTimes()
utilityLeaderUnitOfWorkMock.EXPECT().
Expand Down
2 changes: 1 addition & 1 deletion consensus/hotstuff_leader.go
Original file line number Diff line number Diff line change
Expand Up @@ -399,7 +399,7 @@ func (m *consensusModule) prepareBlock(qc *typesCons.QuorumCertificate) (*coreTy
}

// Reap the mempool for transactions to be applied in this block
stateHash, txs, err := leaderUOW.CreateAndApplyProposalBlock(m.privateKey.Address(), maxTxBytes)
stateHash, txs, err := leaderUOW.CreateProposalBlock(m.privateKey.Address(), maxTxBytes)

if err != nil {
return nil, err
Expand Down
4 changes: 4 additions & 0 deletions shared/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

## [0.0.0.48] - 2023-04-06

- Renamed `CreateAndApplyProposalBlock` to `CreateProposalBlock`

## [0.0.0.47] - 2023-04-06

- Updated to reflect pools address changes
Copy link

Choose a reason for hiding this comment

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

The provided code patch shows changes in a project's version history, specifically the addition of version 0.0.0.48 entry with a change made on 2023-04-06. The actual code change consists of renaming a function:

  • Renamed CreateAndApplyProposalBlock to CreateProposalBlock

Here are some considerations for this change:

  1. Check all references: Make sure that all references to the old function name CreateAndApplyProposalBlock have been updated to the new name CreateProposalBlock.

  2. Understand the rationale behind the renaming: Ensure that the new name accurately reflects the intended functionality of this method. Was any functionality removed or changed when renaming it? If so, it's important to review those changes too.

  3. Update documentation and comments: Ensure that any related documentation, comments, or code examples have been updated to use the new function name. This is important to maintain clarity and prevent confusion for users.

  4. Check if there are any tests associated with the renamed function, and update them if necessary. Also, run the tests to ensure that the function still behaves as expected after the renaming.

Copy link
Member

Choose a reason for hiding this comment

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

This actually isn't bad

Expand Down
8 changes: 4 additions & 4 deletions shared/docs/PROTOCOL_STATE_HASH.md
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ sequenceDiagram
When applying the block during the `NEWROUND` message shown above, the majority of the flow is similar between the _leader_ and the _replica_ with one of the major differences being a call to the `Utility` module as seen below.

- `ApplyBlock` - Uses the existing set of transactions to validate & propose
- `CreateAndApplyProposalBlock` - Reaps the mempool for a new set of transaction to validate and propose
- `CreateProposalBlock` - Reaps the mempool for a new set of transaction to validate and propose

```mermaid
graph TD
Expand All @@ -115,14 +115,14 @@ graph TD
I[Is prepareQC.view > lockedQC.view] --> |"No<br>(lockedQC.block)"| Z
I[Is prepareQC.view > lockedQC.view] --> |"Yes<br>(prepareQC.block)"| Z

H[CreateAndApplyProposalBlock]
H[CreateProposalBlock]
Z[ApplyBlock]
```

As either the _leader_ or _replica_, the following steps are followed to apply the proposal transactions in the block.

1. Update the `UtilityUnitOfWork` with the proposed block
2. Call either `ApplyBlock` or `CreateAndApplyProposalBlock` based on the flow above
2. Call either `ApplyBlock` or `CreateProposalBlock` based on the flow above

```mermaid
sequenceDiagram
Expand All @@ -135,7 +135,7 @@ sequenceDiagram
U->>-C: err_code

%% Apply the block to the local proposal state
C->>+U: ApplyBlock / CreateAndApplyProposalBlock
C->>+U: ApplyBlock / CreateProposalBlock
U->>-C: err_code
```

Expand Down
10 changes: 2 additions & 8 deletions shared/modules/utility_module.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,6 @@ type UnstakingActor interface {

// CONSIDERATION: Consider removing `Utility` from `UtilityUnitOfWork` altogether

// TECHDEBT(@deblasis): `CreateAndApplyProposalBlock` and `ApplyBlock` should be be refactored into a
// `GetProposalBlock` and `ApplyProposalBlock` functions

// UtilityUnitOfWork is a unit of work (https://martinfowler.com/eaaCatalog/unitOfWork.html) that allows for atomicity and commit/rollback functionality
type UtilityUnitOfWork interface {
IntegratableModule
Expand All @@ -53,7 +50,6 @@ type UtilityUnitOfWork interface {
// It does not apply, validate or commit the changes.
// For example, it can be use during state sync to set a proposed state transition before validation.
// TODO: Investigate a way to potentially simplify the interface by removing this function.
// TODO: @deblasis: there's still some mix and match between blockHash and stateHash
SetProposalBlock(blockHash string, proposerAddr []byte, txs [][]byte) error

// ApplyBlock applies the context's in-memory proposed state (i.e. the txs in this context).
Expand All @@ -71,10 +67,8 @@ type UtilityUnitOfWork interface {
type LeaderUtilityUnitOfWork interface {
UtilityUnitOfWork

// CreateAndApplyProposalBlock reaps the mempool for txs to be proposed in a new block, and
// applies them to this context after validation.
// TODO: #508 new signature
CreateAndApplyProposalBlock(proposer []byte, maxTxBytes uint64) (stateHash string, txs [][]byte, err error)
// CreateProposalBlock reaps the mempool for txs to be proposed in a new block.
CreateProposalBlock(proposer []byte, maxTxBytes uint64) (stateHash string, txs [][]byte, err error)
}

type ReplicaUtilityUnitOfWork interface {
Expand Down
9 changes: 9 additions & 0 deletions utility/doc/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [0.0.0.34] - 2023-04-06

- 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
Olshansk marked this conversation as resolved.
Show resolved Hide resolved

## [0.0.0.34] - 2023-04-06

- Updated to reflect pools address changes
Copy link

Choose a reason for hiding this comment

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

Overall, the code patch seems to be well-documented and easy to understand. Here are a few suggestions that might improve the patch:

  1. Clearer error descriptions: You could provide more descriptive error names or messages for GetPrevBlockByzantineValidators and ProposalBlockNotSet errors to give more context on their occurrences.
  2. Logging level: Make sure that log statements added in CreateProposalBlock and ApplyBlock have appropriate logging levels (e.g., debug or info). This helps in controlling verbosity when troubleshooting issues.

Since I don't have access to the full code, I cannot point out any functionality bugs, but these suggestions can help improve the code you've provided.

Copy link

Choose a reason for hiding this comment

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

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.


## [0.0.0.33] - 2023-03-30
Copy link

Choose a reason for hiding this comment

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

Your code patch seems to include the following changes:

  1. Renamed a function.
  2. Added new error types.
  3. Added log statements to two functions.
  4. Refactored block creation and application functions for readability/modularity.
  5. Added TODOs for future refactoring.
  6. Renamed a variable for consistency.

Quick code review suggestions:

  1. Make sure you document the reason behind renaming the function (CreateAndApplyProposalBlock to CreateProposalBlock) and confirm that you have updated all instances where the function is called.
  2. Ensure that the new error scenarios (GetPrevBlockByzantineValidators and ProposalBlockNotSet errors) are properly handled in the respective areas of the codebase.
  3. Review the added log statements and make sure they provide enough information without being too verbose, which could negatively impact performance.
  4. Double-check the refactored functions to ensure any optimizations or refactoring haven't introduced bugs or altered functionality.
  5. Consider addressing TODOs before merging the code to maintain best practices and avoid technical debt.
  6. Verify that the renamed variable (u to uow) has been changed across the entire project scope, ensuring consistent naming.

Without the full context of your project, it's not possible to be more detailed about specific risks or improvements. However, always write comprehensive unit tests to mitigate risks in introducing logic problems, and follow good coding practices. Also, consider running static analysis tools to identify any other improvement opportunities.

Expand Down
14 changes: 13 additions & 1 deletion utility/types/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ func NewError(code Code, msg string) Error {
}
}

// NextCode: 133
// NextCode: 135
type Code float64 // CONSIDERATION: Should these be a proto enum or a golang iota?

//nolint:gosec // G101 - Not hard-coded credentials
Expand Down Expand Up @@ -115,6 +115,7 @@ const (
CodeInvalidServiceURLError Code = 70
CodeNotExistsError Code = 71
CodeGetMissedBlocksError Code = 72
CodeGetPrevBlockByzantineValidators Code = 134
CodeEmptyHashError Code = 73
CodeInvalidBlockHeightError Code = 74
CodeUnequalPublicKeysError Code = 75
Expand Down Expand Up @@ -174,6 +175,7 @@ const (
CodeGetHeightError Code = 129
CodeUnknownActorType Code = 130
CodeUnknownMessageType Code = 131
CodeProposalBlockNotSet Code = 133
)

const (
Expand All @@ -184,6 +186,7 @@ const (
UnequalVoteTypesError = "the vote types are not equal"
UnequalPublicKeysError = "the two public keys are not equal"
GetMissedBlocksError = "an error occurred getting the missed blocks field"
GetPrevBlockByzantineValidators = "an error occurred getting the previous block's byzantine validators"
DecodeMessageError = "unable to decode the message"
NotExistsError = "the actor does not exist in the state"
InvalidServiceURLError = "the service url is not valid"
Expand Down Expand Up @@ -284,6 +287,7 @@ const (
InvalidTransactionCountError = "the total transactions are less than the block transactions"
EmptyTimestampError = "the timestamp field is empty"
EmptyProposerError = "the proposer field is empty"
ProposalBlockNotSet = "the proposal block is not set"
EmptyNetworkIDError = "the network id field is empty"
InvalidHashLengthError = "the length of the hash is not the correct size"
NilQuorumCertificateError = "the quorum certificate is nil"
Expand Down Expand Up @@ -366,6 +370,10 @@ func ErrGetMissedBlocks(err error) Error {
return NewError(CodeGetMissedBlocksError, fmt.Sprintf("%s: %s", GetMissedBlocksError, err.Error()))
}

func ErrGetPrevBlockByzantineValidators(err error) Error {
return NewError(CodeGetPrevBlockByzantineValidators, fmt.Sprintf("%s: %s", GetPrevBlockByzantineValidators, err.Error()))
}

func ErrGetStakedTokens(err error) Error {
return NewError(CodeGetStakedAmountError, GetStakedAmountsError)
}
Expand Down Expand Up @@ -767,6 +775,10 @@ func ErrEmptyProposer() Error {
return NewError(CodeEmptyProposerError, EmptyProposerError)
}

func ErrProposalBlockNotSet() Error {
return NewError(CodeProposalBlockNotSet, ProposalBlockNotSet)
}

func ErrEmptyTimestamp() Error {
return NewError(CodeEmptyTimestampError, EmptyTimestampError)
}
Copy link

Choose a reason for hiding this comment

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

Overall, the code patch seems to be mainly adding new error codes and corresponding error messages. Here are a few observations and suggestions:

  1. Consider adding comments to describe the purpose of the newly added error codes for better code maintainability and readability.

  2. Ensure that the CodeGetPrevBlockByzantineValidators and CodeProposalBlockNotSet you're incrementing by follow the same pattern as previous constants, ensuring that each follows their predecessor incrementally. If not, it may lead to confusion or misuse of error codes in future implementations when developers use these constants.

Add comments like the following example for better understanding:

+    CodeGetPrevBlockByzantineValidators   Code = 134  // Add a brief description here
+    CodeProposalBlockNotSet               Code = 133  // Add a brief description here
  1. It appears you have a typo in the constant ErrGetStakedTokens() function, where the error message uses GetStakedAmountsError instead of GetStakedTokensError. To fix this, update the error message used in that function to the correct one.

With these observations and suggestions, your code should be more maintainable and easier to understand.

Expand Down
Loading