Treat high-sigop transactions as larger rather than rejecting them

sipa commented at 7:01 pm on July 18, 2016: member

This is an alternative to #8364, and should fix #8079 without reintroducing the high-sigop attack fixed by #7081.

When a transaction’s sigops * bytespersigop exceeds its size size, use that value as its size instead (for fee purposes and mempool sorting). This means that high-sigop transactions are no longer prevented, but they need to pay a fee corresponding to the maximally-used resource.

All currently acceptable transactions should remain acceptable and there should be no effect on their fee/sorting/prioritization.

luke-jr commented at 7:55 pm on July 18, 2016: member

This solves a different concern than #8364. Concept ACK, but without the removal of bytespersigop.

sipa commented at 7:57 pm on July 18, 2016: member

@luke-jr It was always my assumption that the bytespersigop default was chosen to be equal to MAX_SIZE / MAX_SIGOPS. The fact that it isn’t (20, instead of 50) means that someone can pay only 40% of the price for a full block’s worth of space, and fill it up with small-size high-sigops transactions.

I see no reason why the number would ever be other than 50, so I’m removing the parameter.

gmaxwell commented at 8:02 pm on July 18, 2016: contributor

Concept ACK.

in src/txmempool.cpp: in 5f514d7320 outdated

27@@ -28,7 +28,7 @@ CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
28     hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue),
29     spendsCoinbase(_spendsCoinbase), sigOpCost(_sigOpsCost), lockPoints(lp)
30 {
31-    nTxCost = GetTransactionCost(_tx);
32+    nTxCost = std::max(GetTransactionCost(_tx), (MAX_BLOCK_COST * sigOpCost + MAX_BLOCK_SIGOPS_COST - 1) / MAX_BLOCK_SIGOPS_COST);

sdaftuar commented at 8:50 pm on July 18, 2016:

Rather than lose the caching of the cost, how about we just change GetTxSize() to return the max of these two quantities? This way we have access to the actual cost so we can write smarter code in the future, while I believe GetTxSize() is already used everywhere (but we should confirm) for feerate calculations, included size/feerate of ancestors and descendants, both in the mempool and in the mining code.

sdaftuar commented at 9:12 pm on July 18, 2016: member

Concept ACK, but to be clear I don’t think we should merge a policy change like this for 0.13. As it applies to all transactions, and in particular not just the ones being rejected under the current -bytespersigop policy, we should give users more notice on the proposed change. (In the worst case, some transactions that are relayed under current policy of 20 bytes per sigop could conceivably see their effective feerates cut by up to 60%. I’m analyzing some historical data to get a sense of how many transactions would be affected by this policy; current estimate: 0.8%, no stats yet on how much of an effect it has overall on those transactions.)

Moreover I can imagine that downstream software might want to consider updating their notion of feerate to match what we do here (wallets, block explorers, fee estimators, etc), so we should give people time for that.

One nit: putting aside any non-linear optimization issues, there’s another slightly suboptimal behavior here in how the mining package selection would work. If a package is too big to fit into the block using the sigops-inflated size, but would fit using its actual size, then we should add it (as we’re sorting based on feerate-using-inflated size, it’s clearly the best thing for us to add). However we’re getting rid of the caching of ancestor package actual sizes, so we don’t have a good way to do this. I expect that this is a small enough effect that we don’t need to worry about it now, but we should document it for the future.

gmaxwell commented at 9:51 pm on July 18, 2016: contributor

I think it could be temporarily adjusted to only impact transactions currently precluded by the bytes per sigop policy. Would that make it more attractive for a more immediate deployment?

sipa force-pushed on Jul 18, 2016

sipa commented at 11:33 pm on July 18, 2016: member

Reworked the patch.

It no longer removes -bytespersigop (as that constitutes a policy change we shouldn’t be introducing this late in the release process), and does not kill the cost caching in mining. As a result, the existing tests also remain valuable.

This is hopefully less controversial, fewer code changes, and more efficient.

luke-jr commented at 11:42 pm on July 18, 2016: member

The changes do not address my concern. bytespersigop is not about miner fee optimisation.

sipa commented at 11:52 pm on July 18, 2016: member

@luke-jr I’m sorry to have misunderstood the original intent of the #7081 then, but that’s the only thing it does in my opinion.

The code we had before #7081 produces nonsensical blocks when high-sigop low-size transactions are present in the mempool, resulting in lower fees than a miner could claim, and reduced transaction capacity on the network for a low cost to the attacker. Our code was broken, to the extent that both miners and the network would be better off patching their code.

#7081 outlaws low-size high-sigop transactions, but it does not prevent hard-to-validate transactions or high overall validation cost. Those situations can still be produced by simply stuffing the high-sigops transactions until they are big enough to pass. This PR just treats such high-sigop transactions as if they were that big already. As a minor side effect, it removes the incentive for such an attack to in addition to disrupting capacity also bloat the block chain.

luke-jr commented at 11:59 pm on July 18, 2016: member

If there are not 20 bytes per (properly counted; ie, with the fix in #8364) sigop, then the transaction is clearly misusing the sigop for spam and/or attack. The goal of bytespersigop is to prevent such transactions from being mined or relayed at all.

I’m fine with miner optimisation as in this PR, but it’s a separate matter.

sipa commented at 0:04 am on July 19, 2016: member

@luke-jr I tend to agree there, but that’s a policy change that should be addressed by something like #5231 after public discussion.

This PR is a bug fix for the unintentional effect of making some formerly standard transactions nonacceptable. That’s not something that should happen without discussion. Furthermore, this does not make anything worse: the spam you were trying to prevent in #7081 was never made impossible - just more expensive. This PR does not change that.

#8364 however makes a related attack worse. With that you can have a low-accurate-sigop high-consensus-sigop small-size transaction attack that removes network capacity for an even lower price, as you only need to pay proportional to the accurate sigop count.

luke-jr commented at 0:12 am on July 19, 2016: member

#5231 is a policy change, sure, but I’m not talking about blocking that spam. bytespersigop (as it stands today, and also when fixed by #8364) blocks a different category of spam.

sipa commented at 0:13 am on July 19, 2016: member

#7081 and #8364 do not block any spam. They just force attackers to also bloat the size of the transactions to bypass the check, and pay a higher fee.

This PR lets them still pay the higher fee, but removes the need to bloat.

luke-jr commented at 0:28 am on July 19, 2016: member

They don’t force attackers to do anything. They block the spam, which may lead attackers to consider bloating the size, waiting for (or running) a miner that doesn’t have such a policy, or perhaps just not attacking. Just asking for a higher fee sends the message that it’s acceptable behaviour and should be expected to be reliable.

sipa commented at 0:30 am on July 19, 2016: member

@luke-jr With #8364 they don’t even need to pay a higher fee. Use 1-of-1 multisig, and you get a factor 20 reduction in fee.

luke-jr commented at 0:45 am on July 19, 2016: member

@sipa #8364 fixes a bug. Nothing more. Like I said, I’m happy to have this concept merged in addition to the (fixed) existing behaviour.

sipa commented at 0:53 am on July 19, 2016: member

Oh, you mean reject based on accurate sigoos, but charge based on consensus sigops? That sounds fine, but I think the extra filtering compared to just this PR remains easily bypassable, so I don’t like the extra complexity.

dexX7 commented at 9:17 am on July 19, 2016: contributor

They just force attackers to also bloat the size of the transactions to bypass the check.

While I don’t consider them as attacker, counterparty-lib and omnicore both create bare-multisig transactions in some rare cases, and as temorary fix to get around the limit introduced by #7081, transactions are indeed bloated.

Treat high-sigop transactions as larger rather than rejecting them ab942c15bd

sipa force-pushed on Jul 19, 2016

sipa commented at 10:35 am on July 19, 2016: member

Rebased.

I think we should include this (with or without #8364) to get #8079 fixed in 0.13.

sdaftuar commented at 11:00 am on July 19, 2016: member

Concept ack new approach, will review and test later today.

jonasschnelli added the label Mempool on Jul 19, 2016

in src/policy/policy.h: in ab942c15bd

67@@ -66,8 +68,10 @@ bool IsStandardTx(const CTransaction& tx, std::string& reason);
68      */
69 bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
70 
71+extern unsigned int nBytesPerSigOp;
72+
73 /** Compute the virtual transaction size (weight reinterpreted as bytes). */
74-int64_t GetVirtualTransactionSize(int64_t nWeight);
75-int64_t GetVirtualTransactionSize(const CTransaction& tx);
76+int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost);

sdaftuar commented at 3:38 pm on July 19, 2016:

nit: Could you add a comment to these functions, indicating the role of sigops here?

sdaftuar commented at 3:48 pm on July 19, 2016: member

One nit, apart from the one I left already: I think it would be nice if in AcceptToMemoryPoolWorker, we had a debugging print to indicate when a transaction was being processed using a higher effective size, as sigops are not something we always have a record of when debugging.

Code review ACK apart from those two nits, will test.

One thing I noticed is that if we were to increase the bytesPerSigOp default value in the future (from 20 to 50 – I assume we’ll propose that for a future release), then I think we should also reduce MAX_STANDARD_TX_SIGOPS_COST (currently 16000), because the IsStandard size checks operate on the transaction’s actual weight without regard to sigop adjustments, and with the current value and at 50 bytes per sigop, it’d be possible to effectively exceed the IsStandard weight limit (400000) by crafting a smaller transaction that used the maximum number of sigops.

This isn’t an issue now, because at bytesPerSigOp=20, the effective maximum weight that is achievable by sigops alone is still under 400000.

It seems that extremely few transactions are accepted for relay under current policy with sig op cost greater than 8000 (I counted 135 out of 8.7M in a sampling of historical data from February/March, the max sigop cost I encountered was 9644), so I don’t think this would be an unreasonable change to propose, just wanted to flag it.

luke-jr commented at 5:56 pm on July 19, 2016: member

#8364 fixes #8079, and should be included in 0.13. The current code here merely removes bytespersigop and replaces it with something entirely different. I don’t care if the new miner policy code concept here is added to 0.13 or not, but it’s not related to #8079 IMO.

sipa commented at 6:03 pm on July 19, 2016: member

#8364 technically fixes the problem, but introduces an actually exploitable bug for miners and the network (cheap attack that results in low-fee nearly-empty blocks). It should not be included on its own.

Combined with this PR that problem disappears, but I don’t see the advantage over just this PR (which also fixes the bug, and in addition does not incentivize bloating transactions to bypass the protection).

sdaftuar commented at 6:25 pm on July 19, 2016: member

Tested ACK.

I agree with @sipa – there is no need for #8364, we should merge this instead. If users are bloating their transactions to get around a policy limit, we should take that as a sign that the policy limit is poorly designed.

After this is merged, we should document in the release notes the changed policy as well as the changed RPC output (the raw transaction rpc’s are returning an unmodified-by-sigops vsize, while the mempool RPC’s are outputting the modified-by-sigops vsize).

luke-jr commented at 7:44 pm on July 19, 2016: member

@sdaftuar Please understand the issues before taking a position. The bloating is in response to the OP_RETURN limit, which is entirely unrelated to either this or bytespersigop. @sipa is speculating that attackers (not users) might bloat abusive transactions to get around the anti-spam limit of bytespersigop.

bytespersigop is a spam filter. This PR is an economic fee rate adjustment. Two entirely different things.

sdaftuar commented at 7:58 pm on July 19, 2016: member

and as temorary fix to get around the limit introduced by #7081, transactions are indeed bloated.

dexX7 commented at 8:18 pm on July 19, 2016: contributor

The bloating is in response to the OP_RETURN limit, which is entirely unrelated to either this or bytespersigop.

Actually, while counterparty-lib and omnicore use bare-multisig scripts to transport data that doesn’t fit into OP_RETURN scripts, both projects started to bloat transactions to get around the sigops limit, so the bloating is indeed the response to #7081. And just FYI: Counterparty even goes one step further and falls back to data embedding via P2PKH, in case the transaction can’t be bloated sufficently, e.g. because an user has not enough inputs available. I believe Dropzone also reverted back to embedding data via P2PKH as response to #7081.

bytespersigop is a spam filter. This PR is an economic fee rate adjustment. Two entirely different things.

It results in a similar outcome.

luke-jr commented at 8:34 pm on July 19, 2016: member

That’s not multisig, it’s data abusively encoded as multisig. For Bitcoin to have any chance of succeeding, it must be stopped, not accommodated.

f139975 commented at 10:11 pm on July 19, 2016: none

ACK ab942c15bd3854650afa810d7c22d7fd30d346c1

This is a very clever solution. Will close #8364, once this one is merged.

wizkid057 commented at 11:43 pm on July 19, 2016: none

I’m against any sort of spam in reaching the blockchain. As a miner and a pool operator I have to object to mining anything that could encourage attackers or spammers in any way. This behavior appears to encourage spammers and has the appearance of condoning their behavior. While I agree that some tweaking could be done to allow more legitimate transactions with more sigops through normally, removing this limit entirely done not seem like a sane solution.

With a spammer who actually cares about bloat, perhaps this could encourage them to lessen it slightly with this approach. But first, they have no real incentive to do so if the end result is still the same fee. Plus, an actual attacker has zero incentive to reduce the bloat as a result of this PR either way. This still ignores the point that none of the spam or attacks should be getting mined anyway. Again, as a miner, we should not be condoning spammers by giving the appearance that we’re willing to accept their spam for a fee. There should be no fee high enough for accepting a spam/attack transaction.

NACK as this stands. Concept ACK If implemented with the bytespersigop filter retained, since then I think this would have the desired effect of reducing/discouraging spam/attacks and also allows legitimate transactions that need more sigops to pay a higher fee.

dexX7 commented at 0:03 am on July 20, 2016: contributor

If you want to discourage the use of bare-multisig, then please start the process to revive #5231, which was closed back then due to it’s controversy. Seemingly neither the submitter of #7081, nor it’s reviewers, were aware of the side effect #8079. Arguing against this pull request, or #8364, based on the opinion that the side effect is actually nice to have seems wrong to me, as this just shows that the whole process of review and consensus can be bypassed.

luke-jr commented at 0:24 am on July 20, 2016: member

@dexX7 You are the only one here I see talking about bare multisig.

luke-jr commented at 0:32 am on July 20, 2016: member

To help clarify things, the four different topics:

OP_RETURN data carrier: debatably-legit use cases exist (Counterparty); currently limited to 80 bytes per tx, allegedly “forcing” some “use cases” to resort to abusing bare multisig
bare multisig: legit use cases exist, no known legit use in practice (?), often abused for spam and attacks (policy filtered by default in #5231)
bare multisig or p2pk with impossible parameters: must be spam or attack; addressed by bytespersigop (#8364 fixes a bug in this)
bare multisig with possible parameters that consume the block sigop limit disproportionately: legit use cases exist, no known legit use in practice (?), often abused for spam and attacks, potential DoS risk if miners sort before better-fee’d transactions (what @sipa is trying to address in this PR)

petertodd commented at 2:42 am on July 20, 2016: contributor

Concept ACK

morcos commented at 8:20 pm on July 20, 2016: member

Concept ACK.
I actually think we should make the “policy change” of changing this back to 50 bytes per sigop and removing the option entirely. It’s not really about getting your transaction relayed but about having miners construct more optimal blocks. It seems silly to me to have the mining code be purposefully less good just because users aren’t expecting miners to have more intelligent algorithms. How do you defend that decision to miners? We’re not giving you better code because some users might be confused and not estimate their fees properly (including Bitcoin Core for that matter)? Maybe you could make that argument if there was a huge impact, but sounds like its not?

morcos commented at 2:56 pm on July 21, 2016: member

@sdaftuar points out to me that under normal conditions, you will mine more optimal blocks with 20 bytes per sig op (or even no limit) than with 50. So I withdraw my recommendation to change it 50, I think leaving it at 20 and making it configurable (as in this pull) is ideal.

laanwj added this to the milestone 0.13.0 on Jul 21, 2016

laanwj added the label Needs backport on Jul 21, 2016

jtimon commented at 1:35 pm on July 23, 2016: contributor

fast review ack

dcousens commented at 3:49 pm on July 23, 2016: contributor

concept && utACK ab942c1

laanwj merged this on Jul 26, 2016

laanwj closed this on Jul 26, 2016

laanwj referenced this in commit 618c9dd8c6 on Jul 26, 2016

MarcoFalke removed the label Needs backport on Oct 3, 2016

MarcoFalke commented at 8:52 am on October 3, 2016: member

This was backported in #8438

DrahtBot locked this on Sep 8, 2021

Treat high-sigop transactions as larger rather than rejecting them #8365