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

@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.

Concept ACK.

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.

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?

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.

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

@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.

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.

@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.

#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.

#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.

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.

@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.

@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.

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.

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.

Rebased.

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

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

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.

#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.

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).

@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.

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

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.

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.

ACK ab942c1

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

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.

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.

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

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 Change -permitbaremultisig default to off #5231)
• bare multisig or p2pk with impossible parameters: must be spam or attack; addressed by bytespersigop (Fix counting of sigops cost in mempool check #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)

Concept ACK

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?

@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.

fast review ack

concept && utACK ab942c1

This was backported in #8438