Skip to content
This repository was archived by the owner on Nov 10, 2025. It is now read-only.

Fix MaxBlockSystemFee#774

Merged
erikzhang merged 1 commit intomasterfrom
Fix-MaxBlockSystemFee
Dec 2, 2022
Merged

Fix MaxBlockSystemFee#774
erikzhang merged 1 commit intomasterfrom
Fix-MaxBlockSystemFee

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants