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

Fix MaxBlockSystemFee #774

Merged
merged 1 commit into from
Dec 2, 2022
Merged

Fix MaxBlockSystemFee #774

merged 1 commit into from
Dec 2, 2022

Conversation

erikzhang
Copy link
Member

No description provided.

consensus?.Tell(message.Payload);
{
Transaction tx = (Transaction)message.Payload;
if (tx.SystemFee > settings.MaxBlockSystemFee)
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it change anything at all? Consensus process only cares about a specific list of transactions when it expects them in a particular stage of the process. CheckPrepareResponse does the overall check anyway for the consensus process. Then non-consensus nodes won't notice this change at all and could still have such a transaction in their mempools.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see this setting (MaxBlockSystemFee) is only in the dBFT plugin now in C# implementation, so ordinary nodes can't do anything wrt this limit.

@roman-khimov
Copy link
Contributor

The best way solve this is to move MaxBlockSystemFee to ProtocolConfiguration again (it was there way back when) and have all nodes do this check before accepting transaction into the memory pool. If we're not doing this (and keeping plugins/configs the way they are now), maybe this patch is the only way to at least make the consensus process work correctly.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I see. Perhaps it should be here as you included, but also in the TCP P2P layer.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

@superboyiii tested?

@superboyiii
Copy link
Member

@superboyiii tested?

Yes, consensus throwed these over-MaxBlockSystemFee txs out of mempool. Ordinary nodes still keep them in their mempool. Anyway, it's better than nothing, at least ensured the safety of consensus. We can merge this and release v3.5.0 ASAP. Improvement can be done next time.

@erikzhang erikzhang merged commit ee0c02b into master Dec 2, 2022
@erikzhang erikzhang deleted the Fix-MaxBlockSystemFee branch December 2, 2022 03:43
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.

6 participants