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

Transaction size plugin #10

Open
wants to merge 13 commits into
base: master
from

Conversation

6 participants
@belane
Member

belane commented Sep 13, 2018

Plugin to filter Transactions by size. It does not allow large free Tx and requires a proportional fee for large Tx (current values are indicative).

  • Not Allow free TX bigger than MaxFreeTransactionSize.
  • Not Allow TX bigger than MaxTransactionSize.
  • For TX bigger than TransactionExtraSize require proportional fee.
@erikzhang

Why create a new project instead of merging it into SimplePolicy?

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 14, 2018

Member

You're right, I'm gonna merge it with SimplePolicy.

Member

belane commented Sep 14, 2018

You're right, I'm gonna merge it with SimplePolicy.

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Sep 14, 2018

Member

Please review this first: neo-project/neo#381

Member

erikzhang commented Sep 14, 2018

Please review this first: neo-project/neo#381

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 14, 2018

Can we set MaxFreeTransactionSize to 400 bytes instead? That will cover a 2-signature transaction.

RavenXce commented Sep 14, 2018

Can we set MaxFreeTransactionSize to 400 bytes instead? That will cover a 2-signature transaction.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 14, 2018

Member

Can we set MaxFreeTransactionSize to 400 bytes instead? That will cover a 2-signature transaction.

Do you think we need to increase to 400 free Tx? most of normal Tx fit in 200-300 bytes.

Member

belane commented Sep 14, 2018

Can we set MaxFreeTransactionSize to 400 bytes instead? That will cover a 2-signature transaction.

Do you think we need to increase to 400 free Tx? most of normal Tx fit in 200-300 bytes.

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 15, 2018

Can we set MaxFreeTransactionSize to 400 bytes instead? That will cover a 2-signature transaction.

Do you think we need to increase to 400 free Tx? most of normal Tx fit in 200-300 bytes.

Once two signatures are required, it is slightly above 300 bytes. I think it is pretty common and we use that quite a lot.

RavenXce commented Sep 15, 2018

Can we set MaxFreeTransactionSize to 400 bytes instead? That will cover a 2-signature transaction.

Do you think we need to increase to 400 free Tx? most of normal Tx fit in 200-300 bytes.

Once two signatures are required, it is slightly above 300 bytes. I think it is pretty common and we use that quite a lot.

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 15, 2018

Actually thinking about it, is there a point for MaxFreeTransactionSize? It seems TransactionExtraSize would be sufficient to prevent useless large transactions.

RavenXce commented Sep 15, 2018

Actually thinking about it, is there a point for MaxFreeTransactionSize? It seems TransactionExtraSize would be sufficient to prevent useless large transactions.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 16, 2018

Member

@RavenXce I've increased MaxFreeTransactionSize to 400.

The approach is:

  1. Allow Transactions without fee below MaxFreeTransactionSize in order to maintain free Tx on Neo but without it can cause any troubles.
  2. Require 'any' fee for the rest Tx, between MaxFreeTransactionSize and TransactionExtraSize.
  3. Require proportional fee for large transactions.

Current values are not very restrictive, so they can be revised in the future if needed.

I think the change can be merged if you agree @erikzhang.

Member

belane commented Sep 16, 2018

@RavenXce I've increased MaxFreeTransactionSize to 400.

The approach is:

  1. Allow Transactions without fee below MaxFreeTransactionSize in order to maintain free Tx on Neo but without it can cause any troubles.
  2. Require 'any' fee for the rest Tx, between MaxFreeTransactionSize and TransactionExtraSize.
  3. Require proportional fee for large transactions.

Current values are not very restrictive, so they can be revised in the future if needed.

I think the change can be merged if you agree @erikzhang.

Show resolved Hide resolved SimplePolicy/SimplePolicy/config.json Outdated
Show outdated Hide outdated SimplePolicy/SimplePolicyPlugin.cs Outdated
Show resolved Hide resolved SimplePolicy/SimplePolicyPlugin.cs Outdated
Show resolved Hide resolved SimplePolicy/SimplePolicyPlugin.cs Outdated
@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 16, 2018

It seems that only requiring any fee for MaxFreeTransactionSize to TransactionExtraSize serves to be more of an annoyance than anything else, as only 1 sat of gas is forced to be paid, which is insignificant.

Users who want fast transactions will already be paying an appropriate amount of fees, and spammers can still increase the blockchain size with very little cost - so nothing much is improved within that size range.

Also I'm concerned about putting out minimum gas fees in general without lots of warnings and announcements to developers and the NEO community. A policy for consensus nodes to not include certain transactions should be treated as non-backward compatible change. Some contracts may not have been built with fee payment in mind, and may not allow withdrawals with fees, for example:

https://github.com/neo-ngd/CGAS-Contract/blob/master/NeoContract/CGAS.cs#L46
https://github.com/ConjurTech/switcheo/blob/v1/switcheo/BrokerContract.cs#L208

RavenXce commented Sep 16, 2018

It seems that only requiring any fee for MaxFreeTransactionSize to TransactionExtraSize serves to be more of an annoyance than anything else, as only 1 sat of gas is forced to be paid, which is insignificant.

Users who want fast transactions will already be paying an appropriate amount of fees, and spammers can still increase the blockchain size with very little cost - so nothing much is improved within that size range.

Also I'm concerned about putting out minimum gas fees in general without lots of warnings and announcements to developers and the NEO community. A policy for consensus nodes to not include certain transactions should be treated as non-backward compatible change. Some contracts may not have been built with fee payment in mind, and may not allow withdrawals with fees, for example:

https://github.com/neo-ngd/CGAS-Contract/blob/master/NeoContract/CGAS.cs#L46
https://github.com/ConjurTech/switcheo/blob/v1/switcheo/BrokerContract.cs#L208

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Sep 20, 2018

Member

Agree with @RavenXce, we can remove TransactionExtraSize and use MaxFreeTransactionSize.

Member

erikzhang commented Sep 20, 2018

Agree with @RavenXce, we can remove TransactionExtraSize and use MaxFreeTransactionSize.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 20, 2018

Member

@RavenXce I'm totally agree that any change should be put in knowledge of all parties involved.

As your and @erikzhang suggestion, removed TransactionExtraSize window and now we require proportional fee for any Tx over MaxFreeTransactionSize.

Member

belane commented Sep 20, 2018

@RavenXce I'm totally agree that any change should be put in knowledge of all parties involved.

As your and @erikzhang suggestion, removed TransactionExtraSize window and now we require proportional fee for any Tx over MaxFreeTransactionSize.

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Sep 21, 2018

Member

@jsolman Can we merge this PR now?

Member

erikzhang commented Sep 21, 2018

@jsolman Can we merge this PR now?

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 21, 2018

@erikzhang I think this will cause some problems on Switcheo DEX contracts. I went through the transactions and now saw that txns with 2 inputs and 2 output can exceed 400 bytes. If we need to merge this into mainnet can we increase the MaxFreeTransactionSize further first?

https://neotracker.io/tx/e33587a7474386305877220168713f2cff9ff21eaea0cd74caf2083a4803b195

RavenXce commented Sep 21, 2018

@erikzhang I think this will cause some problems on Switcheo DEX contracts. I went through the transactions and now saw that txns with 2 inputs and 2 output can exceed 400 bytes. If we need to merge this into mainnet can we increase the MaxFreeTransactionSize further first?

https://neotracker.io/tx/e33587a7474386305877220168713f2cff9ff21eaea0cd74caf2083a4803b195

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Sep 21, 2018

Member

Do you think 512 is a good value for MaxFreeTransactionSize?

Member

erikzhang commented Sep 21, 2018

Do you think 512 is a good value for MaxFreeTransactionSize?

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 21, 2018

I think to be safe we can start with 2048 or 1024. We can then lower it further if spammers are abusing the allowed size. On Switcheo's end, the max inputs allowed will be limited.

RavenXce commented Sep 21, 2018

I think to be safe we can start with 2048 or 1024. We can then lower it further if spammers are abusing the allowed size. On Switcheo's end, the max inputs allowed will be limited.

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 21, 2018

RavenXce commented Sep 21, 2018

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 22, 2018

I agree with the concern about deposits with multiple utxo’s and withdraw transactions that’s need to mark multiple inputs needing to have potentially larger transaction sizes; however, why shouldn’t those transactions require fees since they put more load on the network?

jsolman commented Sep 22, 2018

I agree with the concern about deposits with multiple utxo’s and withdraw transactions that’s need to mark multiple inputs needing to have potentially larger transaction sizes; however, why shouldn’t those transactions require fees since they put more load on the network?

@RavenXce

This comment has been minimized.

Show comment
Hide comment
@RavenXce

RavenXce Sep 22, 2018

RavenXce commented Sep 22, 2018

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 22, 2018

I don’t think this should be merged then until a specific timeline is outlined. I’d like to hear more discussion around allowing the natural ordering of transactions confirmation by consensus nodes to cause transaction fees be required. The issue with anything that requires fees now effectively denies service to contracts such as @RavenXce is referring to from operating since they don’t properly support fees for certain operations like withdraw. At least with allowing natural fees based on network usage doesn’t completely deny service to contract operations not supporting fees — it just makes them only not work during high volume.

IMO it’s best to announce and update any contract immediately that has critical operations not supporting network fees or you are at risk of denial of service to customers.

jsolman commented Sep 22, 2018

I don’t think this should be merged then until a specific timeline is outlined. I’d like to hear more discussion around allowing the natural ordering of transactions confirmation by consensus nodes to cause transaction fees be required. The issue with anything that requires fees now effectively denies service to contracts such as @RavenXce is referring to from operating since they don’t properly support fees for certain operations like withdraw. At least with allowing natural fees based on network usage doesn’t completely deny service to contract operations not supporting fees — it just makes them only not work during high volume.

IMO it’s best to announce and update any contract immediately that has critical operations not supporting network fees or you are at risk of denial of service to customers.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 22, 2018

Member

@RavenXce I understand your point, let's agree between all the most appropriate values, that help protect the network and guarantee the use of legacy contracts.

Member

belane commented Sep 22, 2018

@RavenXce I understand your point, let's agree between all the most appropriate values, that help protect the network and guarantee the use of legacy contracts.

@igormcoelho

This comment has been minimized.

Show comment
Hide comment
@igormcoelho

igormcoelho Sep 24, 2018

Contributor

I got a little late in this conversation, but I hope I can also contribute a little bit... in my opinion (and I guess it's common sense) we must protect all big business currently running on Neo network, even from major/unexpected price changes. And since their contracts are old (as early adopters of the platform), they may be the most inneficient for many reasons... however, I see these changes proposed here with good eyes, in a way that avoids abuse on the network (and increases prices systematically on tx use, which is good). Like @RavenXce commented with me in another post (neo-project/proposals#66), if we implement the staking idea, changes like this will be much easier to apply, because big business will perhaps be able to run with zero or near-zero cost fees for some time, as long it is effective to preventing global abuse (which I believe). But I'm still not fully sure if there is a community agreement on it... so please, if you could take a look in this proposal (neo-project/proposals#67) or even co-author if you like it, because in the next week I'll try to move to a prototype implementation (help will be very welcome too :D). Anyway, good work here!

Contributor

igormcoelho commented Sep 24, 2018

I got a little late in this conversation, but I hope I can also contribute a little bit... in my opinion (and I guess it's common sense) we must protect all big business currently running on Neo network, even from major/unexpected price changes. And since their contracts are old (as early adopters of the platform), they may be the most inneficient for many reasons... however, I see these changes proposed here with good eyes, in a way that avoids abuse on the network (and increases prices systematically on tx use, which is good). Like @RavenXce commented with me in another post (neo-project/proposals#66), if we implement the staking idea, changes like this will be much easier to apply, because big business will perhaps be able to run with zero or near-zero cost fees for some time, as long it is effective to preventing global abuse (which I believe). But I'm still not fully sure if there is a community agreement on it... so please, if you could take a look in this proposal (neo-project/proposals#67) or even co-author if you like it, because in the next week I'll try to move to a prototype implementation (help will be very welcome too :D). Anyway, good work here!

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Sep 25, 2018

Member

We should protect the early supporters of NEO. Maybe we can set the limit to 1024 first, and if it is abused later, we can adjust the value again.

Member

erikzhang commented Sep 25, 2018

We should protect the early supporters of NEO. Maybe we can set the limit to 1024 first, and if it is abused later, we can adjust the value again.

@igormcoelho

This comment has been minimized.

Show comment
Hide comment
@igormcoelho

igormcoelho Sep 25, 2018

Contributor

@shargon think about this, that NEP is related... the idea is to avoid unnecessary space and computation on smart script field.

Contributor

igormcoelho commented Sep 25, 2018

@shargon think about this, that NEP is related... the idea is to avoid unnecessary space and computation on smart script field.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 25, 2018

Member

I have set MaxFreeTransactionSize to 1024. Let's see if we all agree.

Member

belane commented Sep 25, 2018

I have set MaxFreeTransactionSize to 1024. Let's see if we all agree.

Show resolved Hide resolved SimplePolicy/Settings.cs
Show resolved Hide resolved SimplePolicy/SimplePolicy/config.json
@igormcoelho

This comment has been minimized.

Show comment
Hide comment
@igormcoelho

igormcoelho Sep 26, 2018

Contributor

@belane is this plugin valid only when building the block? I guess this tx policy should act on every relay node to not even relay such messages (big and not paying enough), otherwise mempool will be filled anyway.

Contributor

igormcoelho commented Sep 26, 2018

@belane is this plugin valid only when building the block? I guess this tx policy should act on every relay node to not even relay such messages (big and not paying enough), otherwise mempool will be filled anyway.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 26, 2018

Member

@igormcoelho it uses FilterForMemoryPool that is applied in OnNewTransaction https://github.com/neo-project/neo/blob/master/neo/Ledger/Blockchain.cs#L357 so it won't even enter in the pool.

Member

belane commented Sep 26, 2018

@igormcoelho it uses FilterForMemoryPool that is applied in OnNewTransaction https://github.com/neo-project/neo/blob/master/neo/Ledger/Blockchain.cs#L357 so it won't even enter in the pool.

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 27, 2018

So all nodes need to try to align on the same policy as consensus nodes or their mem_pool will get filled with transactions that will never be confirmed, right? If this policy (dictating fee requirements) changes on consensus nodes and other nodes are slow to update, then their normal running behavior will likely be to have a full mem_pool. We are all aware of this, right?

I guess it will give them some incentive to update their policy quickly.

jsolman commented Sep 27, 2018

So all nodes need to try to align on the same policy as consensus nodes or their mem_pool will get filled with transactions that will never be confirmed, right? If this policy (dictating fee requirements) changes on consensus nodes and other nodes are slow to update, then their normal running behavior will likely be to have a full mem_pool. We are all aware of this, right?

I guess it will give them some incentive to update their policy quickly.

@igormcoelho

This comment has been minimized.

Show comment
Hide comment
@igormcoelho

igormcoelho Sep 27, 2018

Contributor

Man, you're a genious! I haven't thought of that before, but I guess two things may fix that even when different policies apply: (1) expire transactions after a given number of blocks (or time), as canesin mentioned before (2) as I mentioned in another PR, users should start to attach "expiration time scripts" in their smart transactions, so they know for sure they will execute in at most 5 blocks(or never execute), for example. I'm suggesting this to Neon wallet guys and that will work soon, but one thing I havent thought is that nodes should reexecute pending transactions from time to time go guarantee they are cut off from mempool(user is safe anyway, because it will never execute anyway even if they try in the future).

Contributor

igormcoelho commented Sep 27, 2018

Man, you're a genious! I haven't thought of that before, but I guess two things may fix that even when different policies apply: (1) expire transactions after a given number of blocks (or time), as canesin mentioned before (2) as I mentioned in another PR, users should start to attach "expiration time scripts" in their smart transactions, so they know for sure they will execute in at most 5 blocks(or never execute), for example. I'm suggesting this to Neon wallet guys and that will work soon, but one thing I havent thought is that nodes should reexecute pending transactions from time to time go guarantee they are cut off from mempool(user is safe anyway, because it will never execute anyway even if they try in the future).

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 27, 2018

@igormcoelho Your solution (2) is not a solution.
Not all users will attach expiration time scripts; what if someone malicious adds them for instance?

jsolman commented Sep 27, 2018

@igormcoelho Your solution (2) is not a solution.
Not all users will attach expiration time scripts; what if someone malicious adds them for instance?

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 27, 2018

@igormcoelho Your solution (1) of expiring transactions after a given number of blocks or time will not work, since the transaction itself doesn't have information about time it was first verified when it is relayed. It will just get relayed again, and will end up back in the node's mem_pool (for node that has a differing policy from consensus nodes).

jsolman commented Sep 27, 2018

@igormcoelho Your solution (1) of expiring transactions after a given number of blocks or time will not work, since the transaction itself doesn't have information about time it was first verified when it is relayed. It will just get relayed again, and will end up back in the node's mem_pool (for node that has a differing policy from consensus nodes).

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 27, 2018

I have a NEP in progress that is meant to address this issue in another way.

Here is an excerpt from it's first motivation section:

Without a deterministic throttling mechanism in place aggreed upon by
the vast majority of nodes, the network will continue to be bogged down 
by spam, causing timeouts from Rpc calls that serve end users, and 
making the end user applications unavailable. 

This issue deserves a NEP since it is important that the solution not be 
one that causes censorship, but that does effectively mitigate DDOS 
attacks. The changes can be made without changing the network protocol
itself, but nodes must keep track of additional metrics for recently
active addresses in order to properly throttle bad acting addresses.
It is advantageous to have this criteria in a NEP that can be agreed
upon by various client implementations. 

Maybe I can up the priority of completing it.

jsolman commented Sep 27, 2018

I have a NEP in progress that is meant to address this issue in another way.

Here is an excerpt from it's first motivation section:

Without a deterministic throttling mechanism in place aggreed upon by
the vast majority of nodes, the network will continue to be bogged down 
by spam, causing timeouts from Rpc calls that serve end users, and 
making the end user applications unavailable. 

This issue deserves a NEP since it is important that the solution not be 
one that causes censorship, but that does effectively mitigate DDOS 
attacks. The changes can be made without changing the network protocol
itself, but nodes must keep track of additional metrics for recently
active addresses in order to properly throttle bad acting addresses.
It is advantageous to have this criteria in a NEP that can be agreed
upon by various client implementations. 

Maybe I can up the priority of completing it.

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 27, 2018

Even MaxFreeTransactionSize is a value that nodes needs to have set the same as on consensus nodes. I suppose I can support this change, if there is an official place where it will be publicly published what plugin settings are being used by each of the consensus nodes running on the network. Is there such a place? Whichever consensus node is the most lenient will determine which transactions will be forever stuck in the mem_pool by nodes with a more lenient policy.

jsolman commented Sep 27, 2018

Even MaxFreeTransactionSize is a value that nodes needs to have set the same as on consensus nodes. I suppose I can support this change, if there is an official place where it will be publicly published what plugin settings are being used by each of the consensus nodes running on the network. Is there such a place? Whichever consensus node is the most lenient will determine which transactions will be forever stuck in the mem_pool by nodes with a more lenient policy.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 27, 2018

Member

@jsolman I think that at the moment there is no centralized source with this information.
One solution could be to create a simple smart contract in which the consensus nodes will publish their parameters regularly, so the rest of the nodes can always consult the current policy.
This opens the door for consensus nodes to periodically determine values based on statistics.

Member

belane commented Sep 27, 2018

@jsolman I think that at the moment there is no centralized source with this information.
One solution could be to create a simple smart contract in which the consensus nodes will publish their parameters regularly, so the rest of the nodes can always consult the current policy.
This opens the door for consensus nodes to periodically determine values based on statistics.

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Sep 28, 2018

Member

Based on the concerns mentioned by @jsolman, I wonder if these features should be moved to neo-project/neo, not to plugins?

Member

erikzhang commented Sep 28, 2018

Based on the concerns mentioned by @jsolman, I wonder if these features should be moved to neo-project/neo, not to plugins?

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 28, 2018

Yes, put it in the core if you will have it.

jsolman commented Sep 28, 2018

Yes, put it in the core if you will have it.

@jsolman

This comment has been minimized.

Show comment
Hide comment
@jsolman

jsolman Sep 28, 2018

I’m still not that excited about this fixed solution. Bumping versions of Neo to adjust these values is not great. Maybe the idea of having a contract that consensus nodes can use to publish the current limits is a good idea though.

jsolman commented Sep 28, 2018

I’m still not that excited about this fixed solution. Bumping versions of Neo to adjust these values is not great. Maybe the idea of having a contract that consensus nodes can use to publish the current limits is a good idea though.

@belane

This comment has been minimized.

Show comment
Hide comment
@belane

belane Sep 30, 2018

Member

Moving this logic to the core may have some advantages, would help to have more homogenized values between nodes. We can evaluate later the smart contract option, but I really like it.

On the other hand, exchanges could have the benefit of having these functions in the node, such as filtering the MemPool from their nodes to only their addresses.

Member

belane commented Sep 30, 2018

Moving this logic to the core may have some advantages, would help to have more homogenized values between nodes. We can evaluate later the smart contract option, but I really like it.

On the other hand, exchanges could have the benefit of having these functions in the node, such as filtering the MemPool from their nodes to only their addresses.

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Oct 11, 2018

Member

I have moved the MaxTransactionSize to core: neo-project/neo#409

Member

erikzhang commented Oct 11, 2018

I have moved the MaxTransactionSize to core: neo-project/neo#409

@erikzhang

This comment has been minimized.

Show comment
Hide comment
@erikzhang

erikzhang Oct 11, 2018

Member

I made a new proposal: neo-project/neo#410

I think the policy settings can be broadcast to the network by P2P message alert.

Member

erikzhang commented Oct 11, 2018

I made a new proposal: neo-project/neo#410

I think the policy settings can be broadcast to the network by P2P message alert.

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