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

feat: brioche hard fork; halving block reward #75

Merged
merged 40 commits into from
May 29, 2024
Merged

feat: brioche hard fork; halving block reward #75

merged 40 commits into from
May 29, 2024

Conversation

egonspace
Copy link
Collaborator

@egonspace egonspace commented May 14, 2024

Brioche hard fork includes the following.

  • block reward halving system
  • not use RewardAmount from the gov contract for block reward any more
  • brioche config includes brioche hard fork block, first halving block, halving period, halving rate, finish halving block, block reward amount

And added some refactoring about validation for Fees and Reward fields of a block.

The following tested manually.

  • Hard fork test with a private local chain.
    • 2 nodes
    • start with master version and upgrade it with brioche binary
    • brioche hard fork starting
    • brioche reward amount
    • starting brioche halving
    • finishing brioche halving
    • halving period
    • halving rate
  • Mainnet syncing with brioche version

@egonspace egonspace requested a review from a team May 14, 2024 07:47
@egonspace egonspace self-assigned this May 14, 2024
core/state_processor.go Outdated Show resolved Hide resolved
core/state_processor.go Outdated Show resolved Hide resolved
core/state_processor.go Outdated Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
params/config.go Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
@egonspace egonspace marked this pull request as ready for review May 20, 2024 23:35
consensus/ethash/consensus.go Show resolved Hide resolved
params/config.go Outdated Show resolved Hide resolved
wemix/admin.go Outdated Show resolved Hide resolved
@egonspace egonspace changed the base branch from master to dev May 22, 2024 07:47
@jed-wemade
Copy link

There is no Brioche config validation whether configs are same among the nodes.
Without the validation, the below test vector would be passed.

func TestRewardValidation(t *testing.T) {
	...
	byzantineConfig := &params.ChainConfig{
		ChainID:      gspec.Config.ChainID,
		LondonBlock:  gspec.Config.LondonBlock,
		BriocheBlock: gspec.Config.BriocheBlock,
		Brioche: &params.BriocheConfig{
			BlockReward:       gspec.Config.Brioche.BlockReward,
			FirstHalvingBlock: gspec.Config.Brioche.FirstHalvingBlock,
			HalvingPeriod:     new(big.Int).Div(gspec.Config.Brioche.HalvingPeriod, big.NewInt(2)), // different period!!
			FinishRewardBlock: gspec.Config.Brioche.FinishRewardBlock,
			HalvingTimes:      gspec.Config.Brioche.HalvingTimes,
			HalvingRate:       gspec.Config.Brioche.HalvingRate,
		}
	}

Node cannot catch the difference before applying config from correct nodes.
Hence, nodes set by wrong config params would compromise the safety.

@egonspace
Copy link
Collaborator Author

There is no Brioche config validation whether configs are same among the nodes. Without the validation, the below test vector would be passed.

func TestRewardValidation(t *testing.T) {
	...
	byzantineConfig := &params.ChainConfig{
		ChainID:      gspec.Config.ChainID,
		LondonBlock:  gspec.Config.LondonBlock,
		BriocheBlock: gspec.Config.BriocheBlock,
		Brioche: &params.BriocheConfig{
			BlockReward:       gspec.Config.Brioche.BlockReward,
			FirstHalvingBlock: gspec.Config.Brioche.FirstHalvingBlock,
			HalvingPeriod:     new(big.Int).Div(gspec.Config.Brioche.HalvingPeriod, big.NewInt(2)), // different period!!
			FinishRewardBlock: gspec.Config.Brioche.FinishRewardBlock,
			HalvingTimes:      gspec.Config.Brioche.HalvingTimes,
			HalvingRate:       gspec.Config.Brioche.HalvingRate,
		}
	}

Node cannot catch the difference before applying config from correct nodes. Hence, nodes set by wrong config params would compromise the safety.

Unfortunately, we don't have a mechanism to compare configurations with other nodes. But if you think about it, you can see that synchronizing configurations between nodes is not conceptually correct. This is because, for example, different config settings are indispensable for nodes during the hard fork upgrade.

@jed-wemade
Copy link

Unfortunately, we don't have a mechanism to compare configurations with other nodes. But if you think about it, you can see that synchronizing configurations between nodes is not conceptually correct. This is because, for example, different config settings are indispensable for nodes during the hard fork upgrade.

I got it. It is general hard fork situation.
I was concern different configs make same state among nodes for a while.
The config affects on future halving blocks, and fork will be done by major nodes.

core/state_processor.go Show resolved Hide resolved
core/state_processor.go Outdated Show resolved Hide resolved
core/state_processor.go Outdated Show resolved Hide resolved
core/chain_makers.go Show resolved Hide resolved
wemix/admin.go Outdated Show resolved Hide resolved
@egonspace
Copy link
Collaborator Author

egonspace commented May 28, 2024

Unfortunately, we don't have a mechanism to compare configurations with other nodes. But if you think about it, you can see that synchronizing configurations between nodes is not conceptually correct. This is because, for example, different config settings are indispensable for nodes during the hard fork upgrade.

I got it. It is general hard fork situation. I was concern different configs make same state among nodes for a while. The config affects on future halving blocks, and fork will be done by major nodes.

According to your good advice, if the next fork information is different when handshaking with peer, I left a logging so that we could prepare for it in advance.

WARN [05-28|17:28:03.345] Checking ForkID: different next fork     peer=f6215791327f1b5a52a763297a5263407a92ea2fa586a7b887410ef9e5b6800d peerForkID="{Hash:[44 77 205 215] Next:0}" myForkID="{Hash:[44 77 205 215] Next:607147}" error=nil

@egonspace egonspace merged commit 4fabf50 into dev May 29, 2024
5 checks passed
@egonspace egonspace deleted the feat/brioche branch June 3, 2024 06:55
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

6 participants