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

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

Merged
merged 1 commit into from Jul 26, 2016

Conversation

Projects
None yet
@sipa
Member

sipa commented Jul 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 18, 2016

Member

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

Member

luke-jr commented Jul 18, 2016

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 18, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 18, 2016

Member

Concept ACK.

Member

gmaxwell commented Jul 18, 2016

Concept ACK.

Show outdated Hide outdated src/txmempool.cpp Outdated
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 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.

Member

sdaftuar commented Jul 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 18, 2016

Member

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?

Member

gmaxwell commented Jul 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 18, 2016

Member

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

Member

luke-jr commented Jul 18, 2016

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 18, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 18, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 19, 2016

#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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 19, 2016

#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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.

Member

sipa commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jul 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.

Contributor

dexX7 commented Jul 19, 2016

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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 19, 2016

Member

Rebased.

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

Member

sipa commented Jul 19, 2016

Rebased.

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

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 19, 2016

Member

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

Member

sdaftuar commented Jul 19, 2016

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

/** Compute the virtual transaction size (weight reinterpreted as bytes). */
int64_t GetVirtualTransactionSize(int64_t nWeight);
int64_t GetVirtualTransactionSize(const CTransaction& tx);
int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost);

This comment has been minimized.

@sdaftuar

sdaftuar Jul 19, 2016

Member

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

@sdaftuar

sdaftuar Jul 19, 2016

Member

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

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 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.

Member

sdaftuar commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 19, 2016

#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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 19, 2016

Member
Member

sipa commented Jul 19, 2016

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

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

Member

sdaftuar commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 19, 2016

@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

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 19, 2016

Member

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

Member

sdaftuar commented Jul 19, 2016

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

@dexX7

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jul 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.

Contributor

dexX7 commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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.

Member

luke-jr commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@f139975

f139975 Jul 19, 2016

ACK ab942c1

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

f139975 commented Jul 19, 2016

ACK ab942c1

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

@wizkid057

This comment has been minimized.

Show comment
Hide comment
@wizkid057

wizkid057 Jul 19, 2016

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.

wizkid057 commented Jul 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@dexX7

dexX7 Jul 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.

Contributor

dexX7 commented Jul 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 20, 2016

Member

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

Member

luke-jr commented Jul 20, 2016

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 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)
Member

luke-jr commented Jul 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Jul 20, 2016

Contributor

Concept ACK

Contributor

petertodd commented Jul 20, 2016

Concept ACK

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 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?

Member

morcos commented Jul 20, 2016

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Jul 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.

Member

morcos commented Jul 21, 2016

@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 laanwj added this to the 0.13.0 milestone Jul 21, 2016

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 23, 2016

Member

fast review ack

Member

jtimon commented Jul 23, 2016

fast review ack

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Jul 23, 2016

Contributor

concept && utACK ab942c1

Contributor

dcousens commented Jul 23, 2016

concept && utACK ab942c1

@laanwj laanwj merged commit ab942c1 into bitcoin:master Jul 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 26, 2016

Merge #8365: Treat high-sigop transactions as larger rather than reje…
…cting them

ab942c1 Treat high-sigop transactions as larger rather than rejecting them (Pieter Wuille)
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Oct 3, 2016

Member

This was backported in #8438

Member

MarcoFalke commented Oct 3, 2016

This was backported in #8438

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