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

Soft-fork boilerplate #2199

Merged
merged 14 commits into from
Aug 17, 2020
Merged

Soft-fork boilerplate #2199

merged 14 commits into from
Aug 17, 2020

Conversation

iulianpascalau
Copy link
Contributor

@iulianpascalau iulianpascalau commented Aug 3, 2020

  • implemented epoch notifier for the soft fork process and changed some options from bool to uint32 representing the epoch number when will become active

this PR should and will not cause breaking changes with the launched mainnet

Testing steps:

  1. start a testnet, with non 0 values for start epoch numbers
  2. for the first epochs (when the the options are inactive) each call to those 3 functions (deploy SC, relayed tx and builtin function call) will not be executed (they will error)
  3. starting from the epoch provided as input in the config.toml, the transactions will succeed.

system tested using current version of erd-py for deploy SC and builtin functions

iulianpascalau and others added 2 commits August 3, 2020 22:22
…me options from bool to uint32 representing the epoch number when will become active
@iulianpascalau iulianpascalau self-assigned this Aug 3, 2020
@iulianpascalau iulianpascalau added the type:feature New feature or request label Aug 3, 2020
@sasurobert sasurobert self-requested a review August 4, 2020 06:49
cmd/node/config/economics.toml Outdated Show resolved Hide resolved
}

// NotifyAllPrepare does nothing
func (gen *genericEpochNotifier) NotifyAllPrepare(_ data.HeaderHandler, _ data.BodyHandler) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think Prepare is needed. only notifyAll - but let's have a talk about 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.

I had to implement the interface in order to get this component notified by the epochStartNotifier

Copy link
Contributor

Choose a reason for hiding this comment

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

ok. we have to see the edge cases when changing epoch. I think we need it in NotifyAllPrepare - as the code is right now the activation of the new Header version, and the activation of certain features.

Do not forget to add the activation of a new header version when is needed ?

Copy link
Contributor

Choose a reason for hiding this comment

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

new epoch block (start of epoch block from meta, then shard should have the new header version and to have the required features already activated at block creation and at block processing time.

core/forking/genericEpochNotifier.go Outdated Show resolved Hide resolved
core/interface.go Outdated Show resolved Hide resolved
process/smartContract/process.go Outdated Show resolved Hide resolved
process/transaction/shardProcess.go Outdated Show resolved Hide resolved
Copy link
Contributor

@AdoAdoAdo AdoAdoAdo left a comment

Choose a reason for hiding this comment

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

Can you please add for each manual testing scenario the following:

a) Prerequisites
b) Test steps
c) Expected results
d) Actual results

If there are parameters involved in testing what are the parameters boundaries and the expected results and actual results for testing inside boundaries, on boundaries and outside boundaries.

sasurobert
sasurobert previously approved these changes Aug 4, 2020
sasurobert
sasurobert previously approved these changes Aug 5, 2020
@raduchis raduchis self-requested a review August 6, 2020 07:26
core/forking/genericEpochNotifier.go Outdated Show resolved Hide resolved
core/forking/genericEpochNotifier_test.go Outdated Show resolved Hide resolved
core/forking/genericEpochNotifier_test.go Outdated Show resolved Hide resolved
core/interface.go Outdated Show resolved Hide resolved
core/mock/epochNotifierStub.go Outdated Show resolved Hide resolved
genesis/process/disabled/epochNotifier.go Outdated Show resolved Hide resolved
…epoch notifier and taking care of the hardfork processing
raduchis
raduchis previously approved these changes Aug 6, 2020
sasurobert
sasurobert previously approved these changes Aug 7, 2020
# Conflicts:
#	genesis/errors.go
sasurobert
sasurobert previously approved these changes Aug 15, 2020
raduchis
raduchis previously approved these changes Aug 15, 2020
Copy link
Contributor

@LucianMincu LucianMincu left a comment

Choose a reason for hiding this comment

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

System tests passed.

@LucianMincu LucianMincu merged commit cf9762c into development Aug 17, 2020
@LucianMincu LucianMincu deleted the EN-7230-softfork branch August 17, 2020 02:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants