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

improve dust spamming prevention algorithm #1536

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@coblee
Contributor

coblee commented Jun 29, 2012

This code helps with reducing dust spam. The original code will make the transaction fee become the minimum fee if any output is less than a cent. This makes it costly to send dust spam, but there's an edge case. The attacker can send a transaction with 1000 outputs of one satoshi each. The transaction size is huge but the fee is only the base fee. This fix will increase the fee by the base fee amount for each output that's less than a cent.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jun 29, 2012

Contributor

ACK -- the economics seem to make sense, but I am interested in comments from others

Contributor

jgarzik commented Jun 29, 2012

ACK -- the economics seem to make sense, but I am interested in comments from others

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Jun 29, 2012

Contributor

Here's a sample attack:
http://abe.liteco.in/tx/48878749fa2bbad375cc5aa8332aabb262c0a98259e85bbdb20f42ab4fa38966

Only cost him 1 LTC to bloat the blockchain by 50kb

Contributor

coblee commented Jun 29, 2012

Here's a sample attack:
http://abe.liteco.in/tx/48878749fa2bbad375cc5aa8332aabb262c0a98259e85bbdb20f42ab4fa38966

Only cost him 1 LTC to bloat the blockchain by 50kb

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 29, 2012

Member

This doesn't sound bad— although generally I'd like to start talking about how to get the net-change-to-the-open-txn-set included in the free/priority structure.

This is a dual attack in that it both adds static blockchain data but it also adds a ton of data which is probably never going to get redeemed, thus also burdening pruned nodes. This could be discouraged using as the size metric of something like max(size/2,2*size-size_of_txouts_reedemed). This would also discourage this sort of attack, where the out data size is much larger than the redeemed inputs.

Member

gmaxwell commented Jun 29, 2012

This doesn't sound bad— although generally I'd like to start talking about how to get the net-change-to-the-open-txn-set included in the free/priority structure.

This is a dual attack in that it both adds static blockchain data but it also adds a ton of data which is probably never going to get redeemed, thus also burdening pruned nodes. This could be discouraged using as the size metric of something like max(size/2,2*size-size_of_txouts_reedemed). This would also discourage this sort of attack, where the out data size is much larger than the redeemed inputs.

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Jun 29, 2012

Contributor

Yes, once you get a lot of dust spam, select coins can take forever. For Litecoin, I had to implement a workaround for people to ignore the dustspam and make them not count towards coins that they own. It was a huge pain.

Contributor

coblee commented Jun 29, 2012

Yes, once you get a lot of dust spam, select coins can take forever. For Litecoin, I had to implement a workaround for people to ignore the dustspam and make them not count towards coins that they own. It was a huge pain.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jun 29, 2012

Member

Sort of tangent— but wrt the select coins problem you might want to look at adding a post processing step: once you've prepared a txn with change and either a free txn with excess priority, or a with fee which isn't right at the boundary, then add in additional of your smallest inputs in order to get them out of your wallet (and out of pruned chains).

Member

gmaxwell commented Jun 29, 2012

Sort of tangent— but wrt the select coins problem you might want to look at adding a post processing step: once you've prepared a txn with change and either a free txn with excess priority, or a with fee which isn't right at the boundary, then add in additional of your smallest inputs in order to get them out of your wallet (and out of pruned chains).

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 2, 2012

Member

On further consideration: This will break the faucet, it will also remove an incentive to use sendmany.

Member

gmaxwell commented Jul 2, 2012

On further consideration: This will break the faucet, it will also remove an incentive to use sendmany.

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Jul 2, 2012

Contributor

As Bitcoin's value increases, you will probably need to reduce the value in which a output is considered spam. For Litecoin, less than 0.01 LTC is pretty much worthless. Maybe for Bitcoin, we should change it so that we add a fee for any output less than 0.0001 BTC.

Contributor

coblee commented Jul 2, 2012

As Bitcoin's value increases, you will probably need to reduce the value in which a output is considered spam. For Litecoin, less than 0.01 LTC is pretty much worthless. Maybe for Bitcoin, we should change it so that we add a fee for any output less than 0.0001 BTC.

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Jul 2, 2012

Contributor

This doesn't feel like the right fix to me.

Fixing the coin selection algorithm so it works quickly with wallets containing gazillions of tiny transactions is part of the solution. Something simple like "If there are more than X unspent outputs, don't use the fancy knapsack-problem-solver, just use the biggest-value unspent input(s) until you have enough to pay for the transaction+fee."

See https://gist.github.com/2961409 for the direction I think will work. With the right priority algorithm (and a minimum-to-be-relayed-included threshold) I don't think a dust spam fee is needed at all.

Contributor

gavinandresen commented Jul 2, 2012

This doesn't feel like the right fix to me.

Fixing the coin selection algorithm so it works quickly with wallets containing gazillions of tiny transactions is part of the solution. Something simple like "If there are more than X unspent outputs, don't use the fancy knapsack-problem-solver, just use the biggest-value unspent input(s) until you have enough to pay for the transaction+fee."

See https://gist.github.com/2961409 for the direction I think will work. With the right priority algorithm (and a minimum-to-be-relayed-included threshold) I don't think a dust spam fee is needed at all.

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Jul 2, 2012

Contributor

The coin selection fix does not fix the problem that someone can bloat the blockchain with only a small fee like this:
http://abe.liteco.in/tx/48878749fa2bbad375cc5aa8332aabb262c0a98259e85bbdb20f42ab4fa38966

Contributor

coblee commented Jul 2, 2012

The coin selection fix does not fix the problem that someone can bloat the blockchain with only a small fee like this:
http://abe.liteco.in/tx/48878749fa2bbad375cc5aa8332aabb262c0a98259e85bbdb20f42ab4fa38966

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen Jul 2, 2012

Contributor

Right, see the gist for the other part of what I think would be a better fix.

For a very-low-transaction-volume chain like LiteCoin, I think the right fix would involve miners specifying a small maximum block size (say 10K, enough space for 40 or so regular-sized transactions) and a high "forgo this amount of transaction fees" (so block space is dominated by transaction priority, not fee).

Contributor

gavinandresen commented Jul 2, 2012

Right, see the gist for the other part of what I think would be a better fix.

For a very-low-transaction-volume chain like LiteCoin, I think the right fix would involve miners specifying a small maximum block size (say 10K, enough space for 40 or so regular-sized transactions) and a high "forgo this amount of transaction fees" (so block space is dominated by transaction priority, not fee).

@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester commented Aug 9, 2012

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/9b11e48d5a03662333c11c046017f9394b127cd5 for binaries and test log.

@SergioDemianLerner

This comment has been minimized.

Show comment
Hide comment
@SergioDemianLerner

SergioDemianLerner Aug 13, 2012

Contributor

Remember there is a previously reported attack involving one connection sending txs with tiny amounts to increase the GLOBAL limit, thus preventing the other connections from sending free transactions. It's necessary to review how this change may affect that attack, and if it can make it worse. I suggested using per-connection limits, and not a global limit.

Contributor

SergioDemianLerner commented Aug 13, 2012

Remember there is a previously reported attack involving one connection sending txs with tiny amounts to increase the GLOBAL limit, thus preventing the other connections from sending free transactions. It's necessary to review how this change may affect that attack, and if it can make it worse. I suggested using per-connection limits, and not a global limit.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Sep 4, 2012

Contributor

It sounds like consensus is against merging this specific fix at this time.

Contributor

jgarzik commented Sep 4, 2012

It sounds like consensus is against merging this specific fix at this time.

@jgarzik jgarzik closed this Sep 4, 2012

@felipelalli

This comment has been minimized.

Show comment
Hide comment
@felipelalli

felipelalli Jul 8, 2015

It sounds good merging this specific fix at this time, after some attacks.

felipelalli commented Jul 8, 2015

It sounds good merging this specific fix at this time, after some attacks.

@m-schmoock

This comment has been minimized.

Show comment
Hide comment
@m-schmoock

m-schmoock Jul 8, 2015

One only learns from mistakes

m-schmoock commented Jul 8, 2015

One only learns from mistakes

@adv0r

This comment has been minimized.

Show comment
Hide comment

adv0r commented Jul 8, 2015

@mb300sd

This comment has been minimized.

Show comment
Hide comment
@mb300sd

mb300sd Jul 9, 2015

Contributor

Whatever my opinion is worth... +1.

Contributor

mb300sd commented Jul 9, 2015

Whatever my opinion is worth... +1.

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Jul 9, 2015

Consensus has changed. It's back on the table. This could potentially resolve the backlog we have in transactions

ghost commented Jul 9, 2015

Consensus has changed. It's back on the table. This could potentially resolve the backlog we have in transactions

@pentarh

This comment has been minimized.

Show comment
Hide comment
@pentarh

pentarh Jul 9, 2015

Txout should be compared not to CENT, which is very hardcoded value, but to nBaseFee imho. Like this

if (txout.nValue < nBaseFee) nMinFee += nBaseFee;

pentarh commented Jul 9, 2015

Txout should be compared not to CENT, which is very hardcoded value, but to nBaseFee imho. Like this

if (txout.nValue < nBaseFee) nMinFee += nBaseFee;

@ABISprotocol

This comment has been minimized.

Show comment
Hide comment
@ABISprotocol

ABISprotocol Jul 14, 2015

This is certainly not back on the table at all, imho. There were very good reasons this pull request was closed out in 2012. I would encourage people to re-read the remarks (including those of @gavinandresen as shown here) and @gmaxwell's.

It's a bad idea to take a nuclear option with respect to fees and reject the opportunity that can be presented from analyzing microgiving potential. Odd also that people keep referring to this as "dust spam" when they could be reframing the language and the issue... the current "dust problem" (which I would argue should actually be called a "giving opportunity") is an opportunity to re-examine transactions, transforming them into bursts of microgiving. Do not foreclose on the an opportunity to re-examine and re-envision transactions and thus change the system you are using.

In closing, I would also recommend you see issue #6402 as an example (of how this could be handled better, perhaps) and also some of the more recent comments on #6201 (including one that I've entered).

ABISprotocol commented Jul 14, 2015

This is certainly not back on the table at all, imho. There were very good reasons this pull request was closed out in 2012. I would encourage people to re-read the remarks (including those of @gavinandresen as shown here) and @gmaxwell's.

It's a bad idea to take a nuclear option with respect to fees and reject the opportunity that can be presented from analyzing microgiving potential. Odd also that people keep referring to this as "dust spam" when they could be reframing the language and the issue... the current "dust problem" (which I would argue should actually be called a "giving opportunity") is an opportunity to re-examine transactions, transforming them into bursts of microgiving. Do not foreclose on the an opportunity to re-examine and re-envision transactions and thus change the system you are using.

In closing, I would also recommend you see issue #6402 as an example (of how this could be handled better, perhaps) and also some of the more recent comments on #6201 (including one that I've entered).

suprnurd pushed a commit to chaincoin-legacy/chaincoin that referenced this pull request Dec 5, 2017

fixed potential deadlock in CSuperblockManager::IsSuperblockTriggered (
…#1536)

governance.cs -> mempool.cs:
`CGovernanceManager::ProcessMessage - LOCK2(cs_main, governance.cs) ->  GetTransaction ->  lookup - LOCK(mempool.cs)`

mempool.cs -> governance.cs:
`CreateNewBlock - LOCK2(cs_main, mempool.cs)  -> FillBlockPayments ->  IsSuperblockTriggered - LOCK(governance.cs)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment