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

Fixes scheduled execution errors on rollback and some refactoring #3357

Merged
merged 7 commits into from
Aug 20, 2021

Conversation

AdoAdoAdo
Copy link
Contributor

No description provided.

@AdoAdoAdo AdoAdoAdo self-assigned this Aug 19, 2021
@@ -365,7 +365,8 @@ func CreateMockArguments(
Version: "softwareVersion",
HistoryRepository: &dblookupext.HistoryRepositoryStub{},
EpochNotifier: &epochNotifier.EpochNotifierStub{},
ScheduledTxsExecutionHandler: &testscommon.ScheduledTxsExecutionStub{},
ScheduledTxsExecutionHandler: &testscommon.ScheduledTxsExecutionStub{
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be on the same line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return mapScheduledSCRs
}

func (ste *scheduledTxsExecution) createCopyScheduledSCRs() (map[block.Type][]data.TransactionHandler, int) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method is used only in GetScheduledSCRs method above, which has only two lines of code. I think that this code could be merged there and also for the debugging scope we could keep the logs as they were before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, previously I was using it in two places, removed, and added back the logs

scheduledRootHash, mapScheduledSCRs, err := ste.getScheduledRootHashAndSCRsForHeader(headerHash)
ste.SetScheduledRootHashAndSCRs(scheduledRootHash, mapScheduledSCRs)

return err
Copy link
Contributor

Choose a reason for hiding this comment

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

return nil here and if err != nil { return err } before L319 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

func (ste *scheduledTxsExecution) SaveState(headerHash []byte) {
scheduledRootHash := ste.GetScheduledRootHash()
mapScheduledSCRs := ste.GetScheduledSCRs()
if ste.HaveScheduledTxs() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this method could be removed from interface and replaced here with the check done inside it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

return scheduledRootHash, txHandlersMap, nil
}

func (ste *scheduledTxsExecution) getMarshalizedScheduledRootHashAndSCRs(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this used anywhere? Maybe replace L328-L342 with the call of this method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes forgot to use it in SaveState

SetScheduledRootHashAndSCRs(rootHash []byte, mapSCRs map[block.Type][]data.TransactionHandler)
GetScheduledRootHashForHeader(headerHash []byte) ([]byte, error)
RollBackToBlock(headerHash []byte) error
SaveState(headerHash []byte)
GetScheduledRootHash() []byte
Copy link
Contributor

Choose a reason for hiding this comment

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

Are GetScheduledRootHash and SetScheduledRootHash still used outside the component? If not we can remove them from interface and make them private there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, still used from baseProcess and baseSync

} else {
boot.scheduledTxsExecutionHandler.SetScheduledRootHash(scheduledRootHash)
boot.scheduledTxsExecutionHandler.SetScheduledSCRs(mapScheduledSCRs)
boot.scheduledTxsExecutionHandler.SetScheduledRootHashAndSCRs(prevHeader.GetRootHash(),make(map[block.Type][]data.TransactionHandler))
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a space after ,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@AdoAdoAdo AdoAdoAdo merged commit b759829 into feat/scheduled-sc-execution Aug 20, 2021
@AdoAdoAdo AdoAdoAdo deleted the fixes-scheduled-revert branch August 20, 2021 07:28
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.

None yet

3 participants