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

Introduce minimum fees configured by machines #1827

Merged
merged 4 commits into from Nov 1, 2019

Conversation

HoOngEe
Copy link
Contributor

@HoOngEe HoOngEe commented Oct 25, 2019

It resolves #1758
It enables validators to customize fee thresholds for transactions to be included in their mem pools.

@HoOngEe HoOngEe force-pushed the feature/customized-min-fee branch 3 times, most recently from 2d5b4ed to 6bb3025 Compare October 28, 2019 07:25
@HoOngEe HoOngEe changed the title [WIP] Introduce the concept of mem pool minimum fee Introduce the concept of mem pool minimum fee Oct 28, 2019
@HoOngEe HoOngEe changed the title Introduce the concept of mem pool minimum fee Introduce machine defined minimum fees Oct 28, 2019
@HoOngEe HoOngEe changed the title Introduce machine defined minimum fees Introduce minimum fees configured by machines Oct 28, 2019
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

Let's add an integration test with the scenario below:

  1. Node A's minimum fee for a pay transaction is 100CCC
  2. Node B's minimum fee for a pay transaction is 200CCC
  3. Node A creates a Block with a transaction whose fee is 140CCC.
  4. Node B verifies the block successfully.

@HoOngEe
Copy link
Contributor Author

HoOngEe commented Oct 28, 2019

I implemented the integration test you commented.

Copy link
Contributor

@foriequal0 foriequal0 left a comment

Choose a reason for hiding this comment

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

You've added codechain binary here: remove it from the repository. 811ab50

@HoOngEe HoOngEe force-pushed the feature/customized-min-fee branch 2 times, most recently from a9461e6 to 0cd8de2 Compare October 29, 2019 02:19
@HoOngEe
Copy link
Contributor Author

HoOngEe commented Oct 29, 2019

You've added codechain binary here: remove it from the repository. 811ab50

Thank you, I removed it from the commit.

@@ -781,17 +770,18 @@ impl MemPool {
origin: TxOrigin,
client_account: &AccountDetails,
) -> Result<(), Error> {
if origin != TxOrigin::Local && tx.fee < self.minimal_fee {
let action_min_fee = self.minimum_fees.min_cost(&tx.action);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why origin != TxOrigin::Local condition is dropped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because I think that's the correct behavior. Some services let users send their transactions via RPC directly to the nodes. In this case, I think the mem pool minimum fees should be applied to such transactions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh. The spec is changed. okay.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we should remain the original behavior. RPC should not be open to the internet. We didn't consider any safety in RPC. All the people who want to send a transaction should have its own CodeChain instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

How about discussing this with other team members?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok then, I'll summary it and make a conversation. Until now, the mem pool's minimal fee is always set to zero.

Copy link
Contributor Author

@HoOngEe HoOngEe Oct 30, 2019

Choose a reason for hiding this comment

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

I changed the behavior not to reject local transactions despite low fees.

test/src/e2e.long/mempoolMinfee.test.ts Outdated Show resolved Hide resolved
test/src/e2e.long/mempoolMinfee.test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@majecty majecty left a comment

Choose a reason for hiding this comment

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

IMO, local transaction should be mined if its fee is low.

@HoOngEe HoOngEe force-pushed the feature/customized-min-fee branch 4 times, most recently from 5806f5e to 1dbc363 Compare October 30, 2019 09:01
majecty
majecty previously approved these changes Oct 30, 2019
@HoOngEe HoOngEe force-pushed the feature/customized-min-fee branch 2 times, most recently from d697014 to f510c01 Compare October 31, 2019 02:53
It enables validators to customize fee thresholds for transactions
to be included in their mempools.
core/src/miner/mem_pool.rs Outdated Show resolved Hide resolved
Transactions whose fees are below the minimum fees configured by the mem pool should be rejected.
foriequal0
foriequal0 previously approved these changes Nov 1, 2019
The mem pool minimum fees should not affect to the block verifications.
@mergify mergify bot merged commit 27ceb2a into CodeChain-io:master Nov 1, 2019
@HoOngEe HoOngEe deleted the feature/customized-min-fee branch November 1, 2019 05:25
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.

Configure minimum fee in a node
3 participants