Prevent Spam Txs

[AdityaSripal]

Summary of Bug

Brought up by @gamarin2 from user on Telegram.

checkState does not reflect any account balance updates from msg processing. This can be exploited to cheaply spam tx's in full block.

Example provided by @gamarin2:

Account1: 101 atoms (initial balance)

TxList of a single Block:

Tx1- Transfer 100 Atoms from Account1 to Account2 (owned by same person)
checkState: {Account1: 101 atoms - fee}  (passed)
deliverState: {Account1: 101 atoms - fee - 100 atoms}  (passed)

Tx2 - GarbageTx with fee2 > 1atom
checkState: {Account1: 101 atoms - fee - fee2}  (passed)
deliverState: {Account1: 101 atoms - fee - 100 atoms - fee2}  (failed)

repeat Tx2 to fill rest of block

checkTx(Tx1) only updated Account1's balance by deducting feeAmount rather than deducting by feeAmount plus the 100 atoms that would have been deducted by msg processing.

This allows future tx's within the same block to pass CheckTx so long as the fee is not greater than 101atoms - fee. Even though Account1's true balance at that point is 1atom - fee.

This can be exploited by submitting an arbitrary amount of garbage tx's like Tx2 that all have fees that are larger than Account1's balance. They will still pass CheckTx so long as they are proposed in same block as Tx1, however the fees in Tx2 cannot be collected by validators because the Account doesn't actually hold those funds.

Thus, entire block can be filled by spammer for the cost of the fee submitted in Tx1.

Version

master: 1c9a188

Steps to Reproduce

Start gaiad node with slow block time and 2 genesis accounts {Acc1: 101 atoms, Acc2: 1atoms}

Submit Tx1 (fee = 0.1atom) and Tx2 (fee = 1.1 atom) such that they get committed in same block. CheckTx of Tx2 will not fail. However deliverState will.

Querying Balances after block:
{Acc1: 0.9atom, Acc2: 101atoms}

Suggested Solution

This is an issue for any msg that removes coins from an account. Thus, checkState must reflect all account updates from msg.

Use modular ante-handler (#4572) to fix the problem:
Each module knows which module msgs update account balances.
Thus, each module AnteHandler checks to see if any of these msgs are embedded inside the tx. If that msg is in tx and ctx.IsCheckTx is true, then antehandler removes coins from account.

This allows checkState to be updated with account balances from msg processing without making CheckTx do any unnecessary state updates that msg handler might perform.

---

For Admin Use

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

[alexanderbez]

Indeed this is exploitable. But I'm not sure the suggested solution is viable: then antehandler removes coins from account. While this may make sense for MsgSend and MsgMultiSend, it doesn't make sense for other messages as balance modifications can be much more complicated. It would also be ideal if we could abstract this necessary implementation away from module implementors.

I would strive to think of a way we can handle this automatically in the BaseApp. This sounds like a flaw/poor design choice in the check and deliver context state separation. Perhaps we can think of a way to reconcile those states.

[AdityaSripal]

Yea agreed, if the coins that a specific msg is going to deduct is obviously calculatable the above solution isn't a problem.

If there is complicated state logic involved in trying to figure out the funds to deduct from an account, this solution becomes quite ugly.

[anagnoresis]

If trying to deduct both the tx business funds and corresponding fees from an account in CheckTx to preclude spam txs, the msg handler should be called to get that fund amounts(duplicate workload in CheckTx and DeliverTx) or some additional business logic is required to run against different msg types.

[anagnoresis]

Spam txs can also cheaply take up the full block with fees set to zero if any bonded validator configs its MinGasPrice to the default empty string.

[alexanderbez]

If trying to deduct both the tx business funds and corresponding fees from an account in CheckTx to preclude spam txs, the msg handler should be called to get that fund amounts(duplicate workload in CheckTx and DeliverTx) or some additional business logic is required to run against different msg types.

We want to keep the BaseApp semantics the same more or less -- i.e. we want to avoid having to execute the messages in CheckTx. I was thinking more along the lines of having CheckTx state be updated to reflect the changes in DeliverTx after a tx's execution...somehow.

Spam txs can also cheaply take up the full block with fees set to zero if any bonded validator configs its MinGasPrice to the default empty string.

This is a different matter entirely. It's being discussed in #4527.

[b00f]

I would like to share my idea to address this issue (Which might make real troubles)
We don't need to add new stage for CheckTx. AFAIK we have two phases: antehandler and module handler.
If the DB version in block N is V(n) we can let all modules to update keepers during antehandler call and then reset the DB version exactly when TXs are going to be commited. This change will happens= inside core and need to be tested carefully.

[anagnoresis]

Some msg-specific logic is required to compute the exact tx funds either in anteHandler or somewhere before handler in runTx.
Would rather define a new funds field in each msg and verify the computing logic in its ValidateBasic in anteHandler. Then deduct both the funds and fees to preclude sequential spam txs.

We want to keep the BaseApp semantics the same more or less -- i.e. we want to avoid having to execute the messages in CheckTx.

[alexanderbez]

@anagnoresis I'm not so sure it'll be that simple. For starters, that would add an additional field and complicate the UX. Secondly, not all messages are so simple that having a funds field would solve this. There could be business logic internally that results in other funds being moved from accounts to other accounts. But maybe this actually OK as we only care about the senders funds.

Just spitballing here, but what if we set the CheckTx state to the DeliverTx state after successful execution?

[AdityaSripal]

@alexanderbez I think that will work. So after any successful DeliverTx, we set the CheckState = DeliverState correct?

This would make sure that previous spends in the same block will cause fee-deduction to fail on future tx's which is the problem we want to solve.

I like this solution 👍

[anagnoresis]

I think it will be useful but not that much, setting the CheckTx state to the DeliverTx state after any successful execution of txs. As it will take effect for only a short time between the start of DeliverTx and the start of state.Commit(commit will lock mempool) at each block height, spam txs still pass CheckTx into mempool the same way as before all the time except the short time.

Moreover, spam txs will be reaped from mempool into proposals before CheckTx state gets updated from DeliverTx state, which leaves the problem unsolved.

[ethanfrey]

Two points,

TL;DR design of cosmos-sdk was that fees were sufficient to remove spam

CheckTx is generally rerun after every commit. Using the check state (which is a throw-away buffer from the last commit with the checktx applied). This is good. The very conscious decision in the cosmos-sdk design was to make CheckTx as fast as possible, which was only to ensure there were enough fees, nothing about whether the tx would succeed. Even if the tx fails, they must pay fees, and the fees and nonce are updated in the CheckTx state before checking the next transactions. Thus, fees is supposed to be enough to limit spam.

TL;DR there are other approaches, easy to run full tx every check

In weave we allow every handler to register separate Check and Deliver methods, to allow execution of anything in Check. Nonce and fees are handled by the middleware (ante handler), but the handler may just check the message is valid, or update the check state. For most messages we do update the state, and thus multiple sendtx would not make it into mempool unless there were enough tokens in the account for all.

In cosmos-sdk, this could be done easily as well, but removing the short-circuit (return on Check) in the antehandler/runTx, and passing all check and deliver tx to the handlers. By default, they would update the check state, and any handler would have to explicitly add separate logic for ctx.IsCheck() if they don't want to do full execution on check.

I personally agree with this change, but the main question is the design direction - do we want heavy checktx and clean blocks, or just allow anyone who can pay fees. I will also note that the heaviest sections of the transaction execution are signature verification (which is currently performed on check) and committing the iavl to disk (never done on check). Reading and writing to an in-memory cache for the changes to state should be a relatively minor cost (at least that was my basic results from benchmarking on weave, which is a similar design and also golang sdk using the iavl storage library).

[ethanfrey]

I think it would be great if there was a process for external requests to the sdk (that have passed some threshold) to enter design discussions with core sdk engineers. Many times these small changes have much larger implications in the design. And the sdk needs a consistent overarching design philosophy... all changes must fit inside it or open conversations to modify the design philosophy (also for other issues moving forward)

[AdityaSripal]

Even if the tx fails, they must pay fees, and the fees and nonce are updated in the CheckTx state before checking the next transactions. Thus, fees is supposed to be enough to limit spam.

This is the intended behavior, but it won't happen in the example i posted above.

I will also note that the heaviest sections of the transaction execution are signature verification (which is currently performed on check)

This should be addressed in the future by checking if CheckTx is a Recheck (ABCI now allows this). This would make us only verify the signature on the first CheckTx for a given tx, and not every single time a Commit occurs

[anagnoresis]

Yes, the spam txs won't pay any fees to pass CheckTx except the first valid tx.

The worst case is that only the first send tx pays its fee and all the others (thousands of spam txs) will get reaped into blocks without any costs.

And it turns out to be one valid transaction and thousands of failed spam txs per block with only a very small amount of fee deducted from the sender's account when the account of the receiver also belongs to the same sender.

Recheck in the state.Commit, enabled by default, would remove a few spam txs following the first valid send tx, but it cannot protect the mempool from the new launched sequences of valid send tx and its coupled spam txs.

This is the intended behavior, but it won't happen in the example i posted above.

[ethanfrey]

Okay, fees are not deducted from the check state sent to the next checktx?
Seems like a bug.

I would actually recommend taking a step back and looking at the whole runTx/runMsg/anteHandler pipeline and seeing if this can be cleaned up to resolve this issue and others (especially extensibility and error reporting)

[alexanderbez]

Okay, fees are not deducted from the check state sent to the next checktx?
Seems like a bug.

Yes, the spam txs won't pay any fees to pass CheckTx except the first valid tx.

This is not true. Fees are deducted in CheckTx states.

---

Here is a test case that demonstrates current behavior in how you can spam:

func TestSpamTxs(t *testing.T) {
	mapp := getMockApp(t)
	origCoins := sdk.NewCoins(sdk.NewInt64Coin("stake", 150))
	acc1 := &auth.BaseAccount{
		Address: addr1,
		Coins:   origCoins,
	}
	acc2 := &auth.BaseAccount{
		Address: addr2,
		Coins:   sdk.NewCoins(),
	}

	// initialize genesis
	mock.SetGenesis(mapp, []auth.Account{acc1, acc2})

	// get first account
	ctxCheck := mapp.BaseApp.NewContext(true, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 := mapp.AccountKeeper.GetAccount(ctxCheck, addr1)

	// generate valid tx
	origAcc1Num := _acc1.GetAccountNumber()
	origAcc2Seq := _acc1.GetSequence()
	fee := auth.StdFee{
		Amount: sdk.NewCoins(sdk.NewInt64Coin("stake", 1)),
		Gas:    100000,
	}
	sendMsg := types.NewMsgSend(addr1, addr2, sdk.NewCoins(sdk.NewInt64Coin("stake", 100)))
	validTx := mock.GenTxWithFee(
		[]sdk.Msg{sendMsg},
		fee,
		[]uint64{origAcc1Num},
		[]uint64{origAcc2Seq},
		priv1,
	)

	// begin block
	header := abci.Header{Height: mapp.LastBlockHeight() + 1}
	mapp.BeginBlock(abci.RequestBeginBlock{Header: header})

	// check valid tx (CheckTx)
	checkTxRes := mapp.Check(validTx)
	require.Equal(t, sdk.CodeOK, checkTxRes.Code, checkTxRes.Log)

	// deliver valid tx (DeliverTx)
	deliverTxRes := mapp.Deliver(validTx)
	require.Equal(t, sdk.CodeOK, deliverTxRes.Code, deliverTxRes.Log)

	// validate CheckTx state
	ctxCheck = mapp.BaseApp.NewContext(true, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 = mapp.AccountKeeper.GetAccount(ctxCheck, addr1)
	require.Equal(t, _acc1.GetSequence(), origAcc2Seq+1)                            // => 0 + 1 = 1
	require.Equal(t, _acc1.GetCoins().String(), origCoins.Sub(fee.Amount).String()) // => 150 - 1 (fee) = 149

	// validate DeliverTx state
	ctxDeliver := mapp.BaseApp.NewContext(false, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 = mapp.AccountKeeper.GetAccount(ctxDeliver, addr1)
	require.Equal(t, _acc1.GetSequence(), origAcc2Seq+1)                                              // => 0 + 1 = 1
	require.Equal(t, _acc1.GetCoins().String(), sdk.NewCoins(sdk.NewInt64Coin("stake", 49)).String()) // 150 - 1 (fee) - 100 (send) = 49

	for i := 1; i < 49; i++ {
		// generate garbage (spam) tx
		spamTx := mock.GenTxWithFee(
			[]sdk.Msg{sendMsg},
			fee,
			[]uint64{origAcc1Num},
			[]uint64{uint64(i)},
			priv1,
		)

		// check valid tx (CheckTx)
		checkTxRes = mapp.Check(spamTx)
		require.Equal(t, sdk.CodeOK, checkTxRes.Code, checkTxRes.Log)

		// deliver valid tx (DeliverTx)
		deliverTxRes = mapp.Deliver(spamTx)
		require.NotEqual(t, sdk.CodeOK, deliverTxRes.Code, deliverTxRes.Log)

		// validate CheckTx state
		ctxCheck = mapp.BaseApp.NewContext(true, abci.Header{Height: mapp.LastBlockHeight() + 1})
		_acc1 = mapp.AccountKeeper.GetAccount(ctxCheck, addr1)
		require.Equal(t, _acc1.GetSequence(), uint64(i+1))
		require.Equal(t, _acc1.GetCoins().String(), sdk.NewCoins(sdk.NewInt64Coin("stake", int64(149-i))).String())
	}

	// At this point, the block has been filled with 1 valid tx and 48 spam txs.
	// However, each spam tx still does pay fees!!!
	ctxDeliver = mapp.BaseApp.NewContext(false, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 = mapp.AccountKeeper.GetAccount(ctxDeliver, addr1)
	require.Equal(t, _acc1.GetCoins().String(), sdk.NewCoins(sdk.NewInt64Coin("stake", 1)).String())

	mapp.EndBlock(abci.RequestEndBlock{})
	mapp.Commit()
}

So while it may be true that you are able to fill up blocks with spam txs, you do it at a cost -- paying fees.

[ethanfrey]

So while it may be true that you are able to fill up blocks with spam txs, you do it at a cost -- paying fees.

Great investigation, Bez, and how I thought the sdk was designed. We can debate the design, but the implementation lives up to spec 😄

There are two options:

• Quick CheckTx, allow some bad tx in block, but FEEs are main anti-spam mechanism
• Slower CheckTx, full validation. Less bad tx in block, but any malicious validator can still chose to include them

The second one is more computation cost for more protection from malicious users not running a validator. In either case, a validator may include any tx (even unparseable ones) in the block. They will all return errors in DeliverTx, but will make it into the history. (Note that a validator doesn't even need to pay fees to include spam in any block they propose)

It would be good to look at the threat model and if deeper validation in CheckTx is worth it. But at least we are certain that fees are functioning as an anti-spam mechanism and the response should be to raise the min gas price if you don't like garbage tx.

[AdityaSripal]

@bez @ethanfrey the issue is that the fees are being deducted from a stale balance. So in bez's example above, this isn't a problem since the current balance still happens to have enough funds to pay the subsequent fees. Here's a test case that showcases the issue a bit more clearly imo:

func TestSpamTxs(t *testing.T) {
	mapp := getMockApp(t)
	origCoins := sdk.NewCoins(sdk.NewInt64Coin("stake", 101))
	acc1 := &auth.BaseAccount{
		Address: addr1,
		Coins:   origCoins,
	}
	acc2 := &auth.BaseAccount{
		Address: addr2,
		Coins:   sdk.NewCoins(),
	}

	// initialize genesis
	mock.SetGenesis(mapp, []auth.Account{acc1, acc2})

	// get first account
	ctxCheck := mapp.BaseApp.NewContext(true, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 := mapp.AccountKeeper.GetAccount(ctxCheck, addr1)

	// generate valid tx
	origAcc1Num := _acc1.GetAccountNumber()
	origAcc2Seq := _acc1.GetSequence()
	fee := auth.StdFee{
		Amount: sdk.NewCoins(sdk.NewInt64Coin("stake", 1)),
		Gas:    100000,
	}
	sendMsg := types.NewMsgSend(addr1, addr2, sdk.NewCoins(sdk.NewInt64Coin("stake", 100)))
	validTx := mock.GenTxWithFee(
		[]sdk.Msg{sendMsg},
		fee,
		[]uint64{origAcc1Num},
		[]uint64{origAcc2Seq},
		priv1,
	)

	// begin block
	header := abci.Header{Height: mapp.LastBlockHeight() + 1}
	mapp.BeginBlock(abci.RequestBeginBlock{Header: header})

	// check valid tx (CheckTx)
	checkTxRes := mapp.Check(validTx)
	require.Equal(t, sdk.CodeOK, checkTxRes.Code, checkTxRes.Log)

	// deliver valid tx (DeliverTx)
	deliverTxRes := mapp.Deliver(validTx)
	require.Equal(t, sdk.CodeOK, deliverTxRes.Code, deliverTxRes.Log)

	// validate CheckTx state
	ctxCheck = mapp.BaseApp.NewContext(true, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 = mapp.AccountKeeper.GetAccount(ctxCheck, addr1)
	require.Equal(t, _acc1.GetSequence(), origAcc2Seq+1)                            // => 0 + 1 = 1
	require.Equal(t, _acc1.GetCoins().String(), origCoins.Sub(fee.Amount).String()) // => 101 - 1 (fee) = 100

	// validate DeliverTx state
	ctxDeliver := mapp.BaseApp.NewContext(false, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 = mapp.AccountKeeper.GetAccount(ctxDeliver, addr1)
	require.Equal(t, _acc1.GetSequence(), origAcc2Seq+1)                 // => 0 + 1 = 1
	require.Equal(t, _acc1.GetCoins().String(), sdk.NewCoins().String()) // 101 - 1 (fee) - 100 (send) = 0

	for i := 1; i < 100; i++ {
		// generate garbage (spam) tx
		spamTx := mock.GenTxWithFee(
			[]sdk.Msg{sendMsg},
			fee,
			[]uint64{origAcc1Num},
			[]uint64{uint64(i)},
			priv1,
		)

		// check valid tx (CheckTx)
		checkTxRes = mapp.Check(spamTx) // 101 - 1 (fee1) - i * fee => PASSES
		require.Equal(t, sdk.CodeOK, checkTxRes.Code, checkTxRes.Log)

		// deliver valid tx (DeliverTx)
		deliverTxRes = mapp.Deliver(spamTx) // 0 - fee => FAILS
		require.NotEqual(t, sdk.CodeOK, deliverTxRes.Code, deliverTxRes.Log)
	}

	// At this point, the block has been filled with 1 valid tx and 99 spam txs.
        // I could spam with arbitrary number of spamTx's from an empty account so long as it is in same block as first tx
	// Each SpamTx has failed DeliverTx but passes CheckTx.
	// Thus, a proposer may unintentionally fill up a block with these spam txs.
	// Spammer doesn't pay for all the spam tx's because there isn't any funds in his account after the first DeliverTx anyway
	ctxDeliver = mapp.BaseApp.NewContext(false, abci.Header{Height: mapp.LastBlockHeight() + 1})
	_acc1 = mapp.AccountKeeper.GetAccount(ctxDeliver, addr1)
	require.Equal(t, _acc1.GetCoins().String(), sdk.NewCoins().String())

	mapp.EndBlock(abci.RequestEndBlock{})
	mapp.Commit()
}

[alexanderbez]

Indeed, you are correct @AdityaSripal. I wonder if simply setting the CheckTx state to DeliverTx state will work (since we currently process them in sequence)?

[AdityaSripal]

@alexanderbez why do we even need to maintain two separate states anymore? Let's just have a single state that we maintain, just don't write at the end of the CheckTx. At the end of DeliverTx we update the state and future CheckTxs and DeliverTxs will rely on the new state

Will try implementing to see if it will work

[anagnoresis]

@AdityaSripal If having just a single state without persisting the CheckState at the end, there would be duplicate workloads in CheckTxs the same as DeliverTxs except the last writing. I think it will work but not that light-weighted.

[ethanfrey]

DeliverTx and CheckTx have wildly different lifetimes.

Starting at commit of block H.

• All tx in mempool that have not been included get passed to CheckTx (using state after H as a base)
• Any new Tx received on p2p or rpc goes to CheckTx
• Some seconds pass like this... (5?)
  ...
  -> Tendermint reaches consensus on a block
  • BeginBlock
  • DeliverTx
  • EndBlock
  • Commit

It is true that a few random CheckTx may be called in parallel with DeliverTx (but not during the commit phase, which has a lock). But the main execution of CheckTx is AFTER Commit and BEFORE BeginBlock.

Before removing things like the two states (we need that so the 50 CheckTx called before BeginBlock have state that build upon each other), I think it would be good to draw out the lifecycle of the Tx.

github.com/iov-core/weave has this working without issues for a long time. and a relatively simple solution using two states Deliver and Check. We just write state updates in CheckTx if the Tx doesn't error. If it does error, it doesn't enter mempool.

I mention this, as I think many cosmos-sdk devs can learn from a simpler weave to understand the flows. And I am concerned that in a rush to address on bug, some important architecture will be broken.

[ethanfrey]

@AdityaSripal To really convince me with your example above...

Use this line:
origCoins := sdk.NewCoins(sdk.NewInt64Coin("stake", 10))

And if it can still get 100 CheckTx with a few of 1stake to pass, then the problem is demonstrated. Right now you have plenty to pay them all

[b00f]

Quick CheckTx, allow some bad tx in block, but FEEs are main anti-spam mechanism

I do agree with this option. Specially if you are in case going to run smart contracts in future. Then running smart contracts in check-tx phase is troublesome. Let it be as it is now. Fee is a good mechanism for preventing spams.

[sunnya97]

I think the proper solution to this is that we should be doing CheckTxs in order, and then the DeliverTxs.

There is an open issue on Tendermint or the SDK that in fact, validators should run CheckTx before pre-voting on a block. Then we don't have to run the ante-handler in the DeliverTx.

@ebuchman do you know where this issue is?

[anagnoresis]

@sunnya97 If CheckTx before pre-voting on a block detected spam txs, should the validator stop pre-voting on the block?

If so, running CheckTx before pre-voting on a block may result in a DoS attack on the whole chain, and no blocks would be committed when leaving CheckTx for the mempool untouched.

There is an open issue on Tendermint or the SDK that in fact, validators should run CheckTx before pre-voting on a block. Then we don't have to run the ante-handler in the DeliverTx.

[alexanderbez]

@ethanfrey we've had folks work pretty hard on a tx lifecycle doc found here.

That being said, I'd recommend we not jump to any immediate proposed solutions as there are some major potential changes upcoming to ABCI/mempool design that we should take into consideration, namely:

• DeliverBlock (batch/parallel tx execution)
• Moving the mempool to the application side

I think the proper solution to this is that we should be doing CheckTxs in order, and then the DeliverTxs.

Not sure I follow...Do you mean executing all the CheckTxs in a block first and then DeliverTxs?

[wangkui0508]

When using cosmos-sdk to develop our blockchain, we use a quick patch to alleviate this problem. It is not a final and perfect solution, but it is effective and easy to implement.

We add a cache to record the most recent unconfirmed transactions (i.e. within 15 minutes) which passed CheckTx and have not be included in a committed block. Using this cache, we can require that one account can have at most one unconfirmed transaction, during a given period (for example, 1 minute). If extra transaction comes, it will be rejected by CheckTx.

Such a cache can be implemented totally in the upper-level application, without modifying the ABCI protocol or the code in cosmos-sdk&terdermint. And this cache is not part of the consensus, so validators can choose to disable or enable it according to their own opinion.

With cosmos-sdk, multiple Msg within one transaction is easier to use than multiple unconfirmed transactions. So forbiding multiple unconfirmed transactions will not hurt user experience.

[alexanderbez]

I can see how that works @wangkui0508 but I don't think that's a viable solution, at least in the long term. It doesn't seem like the best idea to introduce these kinds of limitations in CheckTx.

[wangkui0508]

It is a temporary patch to alleviate the problem, in the short term. I think DeliverBlock would be a better solution in the long term.

[b00f]

DeliverBlock is a good idea but bring lots of changes in Tendermint and it might come with unseen circumstances and issues. I think it's more viable to think solving this issue with current design.

I have an idea and I like to share with you. Merging AnteHandler and ModuleHandler. Anytime a new block is going to create, we need to create a snapshot of current state and execute transactions on this snapshot.
Maybe we can just change the context to get snapshot (or cached) kvstore for CheckTx phase. Do you think it's feasible?

func (c Context) KVStore(key StoreKey) KVStore {
   if c.CheckTx {
     return c.CachedMultiStore()...
   } else {
     return c.MultiStore()...
  }
} 

[alexanderbez]

@b00f the current design already works on a cached-view. How does your proposal work insight of that and how does it help with spam?

[b00f]

Maybe cache is not a good term here. I try to explain my though.
Consider we have two different kvstors which they are identical at init_block phase. But the second kvstore is like a sandbox for executing txs at check_tx phase. Txs need to fully execute (not only deducting fee). So the sandbox has the state of blokchain online.
By delivering_tx we need to just re-execute all txs but this time apply on main kvstore.

Fully executing tx will help to address this issue. I tried to find an answer for this question:
How we can fully execute txs in check_tx phase but at the same time don't change the state of blockchain.

@alexanderbez Do you think this can fix this issue?

[wangkui0508]

How we can fully execute txs in check_tx phase but at the same time don't change the state of blockchain. —— Since the current design already works on a cached-view, this target can be easily achieved.

The problem is, we want check_tx to be light-weighted and run fast. So "fully execute txs in check_tx" is a bad idea.

[alexanderbez]

Correct, like @wangkui0508 pointed out, we intentionally avoid executing the entire tx during CheckTx.

[b00f]

If you don't fully execute a tx, how do you want to prevent spamming?

A spam transaction will be validated/executed by one node and since it's invalid it will be dropped from mempool.

I don't say this is the best solution, but compare to other solutions it is more practical without bringing more complexity to the code.

[alexanderbez]

Spam is currently prevented using the (min) fee mechanism. Although, this only holds if all the validators have some sane minimum fee set.

[b00f]

I don't like to debate but minimum-fee has not solved the issue. It just mitigated it.

[alexanderbez]

I don't like to debate but minimum-fee has not solved the issue. It just mitigated it.

To some extent, correct, hence we're having this discussion to discuss ideas.

[anagnoresis]

@b00f Agreed. To compute the accurate fund deduction, fully execution of a tx is required. And minimum-fee only does a little favor to prevent this type of spam txs.

Since a light-weighted CheckTx is wanted currently, is it possible to add a field Debts into the BaseAccount to record the missing fees that should be paid but lost? In @AdityaSripal 's example, the 1st valid tx will clear the true balance of the spammer's account, and all 99 spam txs could sum their unpaid fees in the Debts. The spammer has to pay all the debts until he/she is able to send txs. Of course, the spammer can create new accounts to send spam txs.

[ebuchman]

There is an open issue on Tendermint or the SDK that in fact, validators should run CheckTx before pre-voting on a block. Then we don't have to run the ante-handler in the DeliverTx.

@ebuchman do you know where this issue is?

Actually not sure we have a good dedicated issue for this, but related are https://github.com/tendermint/tendermint/issues/2639 and tendermint/tendermint#3322 (comment)

[github-actions]

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.

[tac0turtle]

Still applicable

[robert-zaremba]

Digging out - seams to be more relevant now

[tac0turtle]

closing in favor of #8917