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

Preventing 0 fee Tx spamming - Consensus Min Fee #4527

Closed
4 tasks
RiccardoM opened this issue Jun 10, 2019 · 25 comments
Closed
4 tasks

Preventing 0 fee Tx spamming - Consensus Min Fee #4527

RiccardoM opened this issue Jun 10, 2019 · 25 comments

Comments

@RiccardoM
Copy link
Contributor

RiccardoM commented Jun 10, 2019

Summary of Bug

Currently inside the MsgSend#ValidateBasic method implementation, the sender's balance is not checked for validity.

This is currently allowing a specific address (cosmos1pv6s6z542kyudwq8zxznkeu6hm6yv4fgvpzvd3) to spam invalid transactions trying to send 1.0 ATOM while having only 0.9 ATOMs inside his wallet.

Some transactions' hashes are the following.

  • 9B0064B69F3C49ECA35DDE48D8FCE2378A638E0385DB489E12FF265D9018C825
  • EA4230A264C5CCD84EAD767F5FDB5172B72483428E0B85A899A684F768B77098
  • 1DF213731B007438E8DC749C9207264BD88F59D7CA2DDFF7F16B8F93122CC9EC
  • Many many others

Version

028bbef

Steps to Reproduce

  1. Create a send transaction having the following data
    • Send amount greater than current balance
    • Fee amount set to zero
  2. Try broadcasting the tx

The tx will be broadcasted but the send won't be effective.

Possible solutions

Improve validate check

A plausible solution in order to avoid this would be to implement a check based on the current balance of the sender, and a higher end estimate of the gas that he is going to pay inside the `ValidateBasic` method. The algorithm could be something like the following
  1. Get the sender_current_balance
  2. Compute a higher end estimate of the gas that he is going to pay
  3. Get the amount of fee that he is going to pay
  4. If sender_current_balance < gas + fee + send_amount invalidate the transaction

I don't know if a higher estimate of the fees and gas is possibile of if there is a smarter way of implementing this.

After a discussion presented below, this has been judged the worst option due to the fact that ValidateBasic has to perform basic checks and should not have access to the keeper.

Use a new tx_fee_floor param

Another option could be to implement a tx_fee_floor param inside the SDK in order to set a minimum tx fee that all validators must have set. This could be the only way to force validators to set a non-zero minimum-tx-fee value and prevent spam.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@RiccardoM RiccardoM changed the title Current wallet amount is not checked while validating a Tx Current sender balance is not checked while validating a MsgSend Jun 10, 2019
@fedekunze
Copy link
Collaborator

fedekunze commented Jun 10, 2019

On the tx hashes that you mentioned above. If you check the log, it says "success":false. The ValidateBasic() function from messages only validates the fields of the message. The rest is checked on the ante handler.

@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jun 10, 2019

It doesn't matter if the sender doesn't have enough balance, this is not a requirement for a transaction to be pass the ante-handler, but a requirement of running the transaction through checkTx (which does more than validate basic as it has store lookup capabilities, validate-basic shouldnot/cannot). Once the ante-handler has been run if the transaction fails, it's okay because the fees have been irrevocably been paid to run the transaction - if the ante-handler fails then the transaction should be failing at the ante-handler and checkTx shouldn't even be run. If no fees are provided then the ante-handler should fail.

If any of the above statements are broken and somehow the zero-fees transactions are making it past the ante-handler then there is bug in the antehandler.

related: #4528


edit. validators set the min fee parameter - think the reason we haven’t had this is because we don’t have global params which is where a “global” floor-bottom parameter should live (as some private chains may want to allow zero fee transactions).

@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 11, 2019

Just to reiterate what has already been expressed:

  • ValidateBasic checks are meant to be, well, basic. They do not perform any state reads or writes. Hence any revisions to the protocol resulting from this discussion would not change any ValidateBasic calls (e.g. balance checks).
  • There isn't inherently anything broken or wrong in the protocol with regards to what has been expressed in the issue. It is completely up to the proposing validator's discretion to accept 0 fee txs.
  • As long as CheckTx passes in the proposing validator, the tx will execute in all validators regardless of the fee provided.

Now, generally I'm not immediately in favor of having a tx fee floor param in x/auth because I believe it should be up to the validator's sole discretion to accept what they wish. However, as it's been described in this issue, spamming can still occur with 0 fee txs so long as there is a validator to propose/accept them.

The simple solution would be to signal to these validator(s) that they should stop doing so (increase their min fees)! In fact, @sunnya97 already did this. However, it may still be advantageous to have a tx fee floor param. In such a case, I believe this must go through governance and should be relatively low. We must also carefully think about all crypto-economic impacts.

@RiccardoM
Copy link
Contributor Author

Thank you all for taking part into the discussion of this problem.

I now understand what you all are saying about the validation of the transaction and I am going to change the title of this issue in order to make it more generic.

About the tx fee floor param, what could the cypto-economic impacts of such param?

@RiccardoM RiccardoM changed the title Current sender balance is not checked while validating a MsgSend Preventing 0 fee Tx spamming Jun 12, 2019
@alexanderbez
Copy link
Contributor

alexanderbez commented Jun 12, 2019

Thanks @RiccardoM. Mostly around picking the appropriate floor.

We should also update the proposal in the issue body to use the tx fee floor param as that seems like the best alternative.

@RiccardoM
Copy link
Contributor Author

@alexanderbez I've updated the description with the new possible solution

@alexanderbez
Copy link
Contributor

Thanks @RiccardoM! Something like this, I believe, will have to go through a governance proposal. Also thinking further on this, it would be probably in the network's best interest to have what I'll call a "consensus min fee" to be dynamic based on some formula instead of static. There has been discussion by Vitalik on the zcash repo.

@alexanderbez alexanderbez changed the title Preventing 0 fee Tx spamming Preventing 0 fee Tx spamming - Consensus Min Fee Jun 14, 2019
@rigelrozanski
Copy link
Contributor

An idea of how we could make it dynamic is by simply taking the median value of validators' min fee as the network min fee

@alexanderbez
Copy link
Contributor

An idea of how we could make it dynamic is by simply taking the median value of validators' min fee as the network min fee

Interesting. This would require the ability of validators being able to know about other validator's min fees.

Building on this, and borrowing from convos with @ValarDragon and discussions in the zcash thread, how about the following parameterized and relatively simple consensus min fee:

Have some initial consensus min fee F and a target block fulfillment size S (e.g. 75%). Every N blocks check to see if the average block fulfillment size is below or above S. If below, reduce F by some factor. If above, increase F by some factor. Otherwise, leave unchanged. We can probably ignore empty blocks too.

@alexanderbez alexanderbez mentioned this issue Jul 19, 2019
4 tasks
@rigelrozanski
Copy link
Contributor

rigelrozanski commented Jul 25, 2019

hmm should we really ignore empty blocks though? I don't see a reason why empty blocks shouldn't just function in the same way any other block does in that proposed mechanism - empty blocks represent unused capacity of the blockchain

So this mechanism is interesting because as soon as a blockchain started getting spammed the mechanism would react to create a large fee.

My only comment on this mechanism I don't think that the multiplicative factors would necessarily be appropriate... for instance if the blockchain has little use for a long time dragging the min fees to the lower possible limit, it should still be able to react to be being spammed quickly, I'd almost want the factor to be based on a long term historical average... the greater the spike/drop in usage the disproportionately faster the rate would change. make sense?

@alexanderbez
Copy link
Contributor

Yeah on second thought, it may be advantageous to not avoid empty blocks. Wrt to drastic spikes in spam, we can possibly tune N to be small enough. Also, there is room to play with the multiplicative factor -- maybe we can parameterize this factor based on some recent historic input.

@rigelrozanski
Copy link
Contributor

love it

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jul 13, 2020
@sahith-narahari
Copy link
Contributor

@alexanderbez do you think this can be added to sdk, and set as true or false in genesis. Few zones may be intrested to use this

@alexanderbez
Copy link
Contributor

Of course, that's why the issue is still open, just not enough bandwidth to tackle it. We also need to implement this in such a way that allows applications to opt-in this functionality, because for example, in Gaia, this would need to go through a gov proposal I think. Let's slate this for 0.41.

@aaronc
Copy link
Member

aaronc commented Feb 10, 2021

We would really like to get this done. I'm marking as 0.42 nice-to-have and will start on a draft PR.

@aaronc
Copy link
Member

aaronc commented Feb 11, 2021

So the basic algorithm described here is pretty straightforward with just a few parameters.

My only comment on this mechanism I don't think that the multiplicative factors would necessarily be appropriate... for instance if the blockchain has little use for a long time dragging the min fees to the lower possible limit, it should still be able to react to be being spammed quickly, I'd almost want the factor to be based on a long term historical average... the greater the spike/drop in usage the disproportionately faster the rate would change. make sense?

I do think this is an important thing to keep in mind that we doesn't necessarily have an obvious solution to. I think the general idea would be to increase the fee quicker when gas usage spikes quicker so we might look at rate of change. But this would make one part of EIP 1559 quoted below more difficult:

When the network exceeds the target per-block gas usage, the base fee increases slightly and when capacity is below the target, it decreases slightly. Because these base fee changes are constrained, the maximum difference in base fee from block to block is predictable. This then allows wallets to auto-set the gas fees for users in a highly reliable fashion. It is expected that most users will not have to manually adjust gas fees, even in periods of high network activity.

So if we have an algorithm that increases fees quickly to respond to spikes, it will make fee estimation in wallets harder which I think is an important UX consideration. But if we don't do these quick fee ramp ups, it leaves newer, less mature zones vulnerable to cheap spam attacks at periods of lower activity. Any thoughts on how we reconcile these concerns?

@hxrts
Copy link
Contributor

hxrts commented Feb 11, 2021

One consideration is there may be private/POA networks that don't want fees at all, a Cosmos Cash chain for instance. So we may want to have an override parameter.

I linked to a few resources here, the escalator algorithm has the ramp-up property you describe.

@clevinson
Copy link
Contributor

There was a lot of active discussion on this issue today during our architecture call. Notes here.

@alexanderbez
Copy link
Contributor

Happy to help out in either implementation or review here 👍

@robert-zaremba
Copy link
Collaborator

Seams to be related to: Preventing 0 fee Tx spamming - Consensus Min Fee #4527

@tac0turtle
Copy link
Member

closing in favor of #8917

@DesMarie
Copy link

DesMarie commented Jan 29, 2023

Summary of Bug

Currently inside the MsgSend#ValidateBasic method implementation, the sender's balance is not checked for validity.

This is currently allowing a specific address (cosmos1pv6s6z542kyudwq8zxznkeu6hm6yv4fgvpzvd3) to spam invalid transactions trying to send 1.0 ATOM while having only 0.9 ATOMs inside his wallet.

Some transactions' hashes are the following.

  • 9B0064B69F3C49ECA35DDE48D8FCE2378A638E0385DB489E12FF265D9018C825
  • EA4230A264C5CCD84EAD767F5FDB5172B72483428E0B85A899A684F768B77098
  • 1DF213731B007438E8DC749C9207264BD88F59D7CA2DDFF7F16B8F93122CC9EC
  • Many many others

Version

028bbef

Steps to Reproduce

  1. Create a send transaction having the following data

    • Send amount greater than current balance
    • Fee amount set to zero
  2. Try broadcasting the tx

The tx will be broadcasted but the send won't be effective.

Possible solutions

Improve validate check

~ A plausible solution in order to avoid this would be to implement a check based on the current balance of the sender, and a higher end estimate of the gas that he is going to pay inside the ValidateBasic method. The algorithm could be something like the following 1. Get the sender_current_balance 2. Compute a higher end estimate of the gas that he is going to pay 3. Get the amount of fee that he is going to pay 4. If sender_current_balance < gas + fee + send_amount invalidate the transaction I don't know if a higher estimate of the fees and gas is possibile of if there is a smarter way of implementing this. ~
After a discussion presented below, this has been judged the worst option due to the fact that ValidateBasic has to perform basic checks and should not have access to the keeper.

Use a new tx_fee_floor param

Another option could be to implement a tx_fee_floor param inside the SDK in order to set a minimum tx fee that all validators must have set. This could be the only way to force validators to set a non-zero minimum-tx-fee value and prevent spam.

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned

@DesMarie
Copy link

``

@alexanderbez
Copy link
Contributor

@DesMarie there is a globalfee module that chains use to have a consensus-wide min fee. The thing is, when the fee is transferred it should fail, thus CheckTx should fail and the tx should never enter the mempool. So having a balance check doesn't buy you anything.

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

No branches or pull requests