Skip to content

Added blockmintxfee setting to BlockDefinitionOptions#133

Merged
dangershony merged 4 commits intoblock-core:masterfrom
x42protocol:x42-transaction-rules
May 28, 2020
Merged

Added blockmintxfee setting to BlockDefinitionOptions#133
dangershony merged 4 commits intoblock-core:masterfrom
x42protocol:x42-transaction-rules

Conversation

@DennisAMenace
Copy link
Copy Markdown
Contributor

  • Added RewardRequired
  • Updated x42 consensus and added MinBlockFeeRate to consensus options.

Added RewardRequired
Updated x42 consensus and added MinBlockFeeRate to consensus options.
/// If the block can have no rewards.
/// </summary>
/// <returns>True - By default require a reward for new blocks</returns>
public virtual bool RewardRequired()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think this is needed once fee is zero in options

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It still needs to be here because reward <= 0 will reject the block otherwise.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah I wonder if that condition is correct if (reward <= 0)

if (reward <= 0)
var checkPosUtxosetRule = this.consensusManager.ConsensusRules.GetRule<CheckPosUtxosetRule>();
long reward = fees + checkPosUtxosetRule.GetProofOfStakeReward(chainTip.Height + 1);
if (reward <= 0 && checkPosUtxosetRule.RewardRequired())
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is interesting, if reward is not required just set it to zero in Network settings why add a flag?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Default is set to reward <= 0 and with x42 a block might have no rewards, and this fails, so I need the flag here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can add another network consensus item similar to the blockmintxfee, is that what you are thinking here?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we need this then yes a network consensus is better then changing the rule.
However I am wondering if it should not be reward < 0

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yes I think we should change this condition to be reward < 0 it's left over code form stratisx I believe and it does not make sense anymore

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

[BuriedDeployments.BIP66] = 0
};

var bip9Deployments = new x42BIP9Deployments
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

good very important

Copy link
Copy Markdown
Member

@dangershony dangershony left a comment

Choose a reason for hiding this comment

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

Good job

@dangershony dangershony merged commit b83d16f into block-core:master May 28, 2020
@DennisAMenace DennisAMenace deleted the x42-transaction-rules branch May 28, 2020 13:21
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.

2 participants