Skip to content

Create transactions to replace the implicit state change.#324

Merged
sgkim126 merged 6 commits intoCodeChain-io:masterfrom
sgkim126:ap
Apr 29, 2020
Merged

Create transactions to replace the implicit state change.#324
sgkim126 merged 6 commits intoCodeChain-io:masterfrom
sgkim126:ap

Conversation

@sgkim126
Copy link
Contributor

@sgkim126 sgkim126 commented Apr 12, 2020

It closes #40, #124 and #125.

I still implement a verification that this transaction must be the first transaction of every block.

@sgkim126 sgkim126 added the do-not-merge Do not merge this PR label Apr 12, 2020
@byeongjee
Copy link
Contributor

@HoOngEe @somniumism I think you should take into account this change when you are working on Distribution and Staking modules. I remember that we talked about this last week.

@sgkim126 sgkim126 changed the title [WIP] Add the UpdateValidators transaction instead of implicit state change on open block [WIP] Create a transaction to replace the implicit state change. Apr 18, 2020
@sgkim126 sgkim126 changed the title [WIP] Create a transaction to replace the implicit state change. [WIP] Create transactions to replace the implicit state change. Apr 18, 2020
@sgkim126 sgkim126 force-pushed the ap branch 16 times, most recently from a4b80e7 to 316d036 Compare April 19, 2020 16:15
This was linked to issues Apr 20, 2020
@sgkim126
Copy link
Contributor Author

One of the shutdown test fails yet, but I think it's because the test is fragile. It depends on the timing very hard to meet. I'm considering disabling the test temporarily if I cannot find the fix.

@sgkim126 sgkim126 changed the title [WIP] Create transactions to replace the implicit state change. Create transactions to replace the implicit state change. Apr 21, 2020
@sgkim126 sgkim126 requested a review from majecty April 21, 2020 00:41
@sgkim126
Copy link
Contributor Author

I disabled one of the e2e test. The test assumes that the network executes all setup transactions(ex. sending ccc, distributing stakes...) in a term. But the assumption is hard to meet because this PR makes block generation slower.

@sgkim126
Copy link
Contributor Author

The transaction is not complete yet. This PR made an Elect transaction without fields, but it should describe how to change the validators to let others know who are the next validators without an execution. But, calculating it to create a block makes block generation slower than now, so it makes the more test fail.
And, this PR doesn't implement the order verification.
I'll try them in other PR.
I want to merge this PR first, to introduce a logic without implicit state change.

.into())
}
NextValidators::elect(self)?.save_to_state(self)?;
return self.update_term_params()
Copy link

Choose a reason for hiding this comment

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

Calling update_term_params in the Action::Elect feels awkward. update_term_params changes the value used in election, jail, nomination. How about creating UpdateTermParams transaction?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I added the transaction, but it made block generation slow and more tests became fragile.
So I merged the transaction to reduce the number of transactions.
I still wonder why one more signing makes the node slow, but it did. I'll try again. Maybe it was a problem of the Travis.

Copy link

Choose a reason for hiding this comment

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

I'm so sad to hear that.

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 tested it in several ways. It seems that adding one more transaction would not harm the performance significantly, but it seems that it's the last step which makes the tests fail.

The test assumes the test setup is done in one term. But, the previous
patches makes the block generation slow, and it's hard to meet this
condition.
@sgkim126 sgkim126 merged commit 12cdbf2 into CodeChain-io:master Apr 29, 2020
@sgkim126 sgkim126 deleted the ap branch June 6, 2020 11:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge Do not merge this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CloseBlock transaction Add OpenTerm transaction Proposal to remove implicit state changes

3 participants