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

Fix counting of sigops cost in mempool check #8364

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@f139975

f139975 commented Jul 18, 2016

The new "bytespersigop" limit introduced the side effect of making most multisig transactions non-standard.

This fixes #8079 by accurately counting the sigops cost, solely for the newly introduced check.

As alternative I see two options:

  • revert #7081 which introduced this bug
  • raise DEFAULT_BYTES_PER_SIGOP significally so that there is no longer a side effect

This should be backported to earlier versions, and especially for 0.13, as soon as possible the path is clear.

Fix counting of sigops cost in mempool check
The new "bytespersigop" limit introduced the side effect of making most multisig transactions non-standard.

This fixes the issue by accurately counting the sigops cost, solely for the newly introduced check.
@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 18, 2016

Member

This does not prevent the vulnerability, I think? Someone can still create a large number of high-(legacy)sigops transactions, and the mining code will include them, resulting in a very small block.

AFAIK the only workable solution is to treat transactions in the mempool as if their size was max(size, sigops * MAX_BLOCK_SIZE / MAX_SIGOPS).

Member

sipa commented Jul 18, 2016

This does not prevent the vulnerability, I think? Someone can still create a large number of high-(legacy)sigops transactions, and the mining code will include them, resulting in a very small block.

AFAIK the only workable solution is to treat transactions in the mempool as if their size was max(size, sigops * MAX_BLOCK_SIZE / MAX_SIGOPS).

@f139975

This comment has been minimized.

Show comment
Hide comment
@f139975

f139975 Jul 18, 2016

This does not prevent the vulnerability, I think?

That's not the objective of this submission, but to remove the side effect of making multisig transactions non-standard, which was introduced by #7081, while leaving #7081 as intact as possible.

AFAIK the only workable solution is to scale up the mempool transaction size to max(size, sigops * MAX_BLOCK_SIZE / MAX_SIGOPS).

I'd be happy, if there is a better solution, and I'd be very glad, if you could push one. My primary goal was to present something that may still be merged into 0.13.

f139975 commented Jul 18, 2016

This does not prevent the vulnerability, I think?

That's not the objective of this submission, but to remove the side effect of making multisig transactions non-standard, which was introduced by #7081, while leaving #7081 as intact as possible.

AFAIK the only workable solution is to scale up the mempool transaction size to max(size, sigops * MAX_BLOCK_SIZE / MAX_SIGOPS).

I'd be happy, if there is a better solution, and I'd be very glad, if you could push one. My primary goal was to present something that may still be merged into 0.13.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 18, 2016

Member

@f139975 I'll see what I can do. I understand the desire to have #8079 fixed, but not by reintroducing the attack that #7081 prevented.

Member

sipa commented Jul 18, 2016

@f139975 I'll see what I can do. I understand the desire to have #8079 fixed, but not by reintroducing the attack that #7081 prevented.

@f139975

This comment has been minimized.

Show comment
Hide comment
@f139975

f139975 Jul 18, 2016

Thank you!

f139975 commented Jul 18, 2016

Thank you!

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak
Member

btcdrak commented Jul 18, 2016

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 18, 2016

Member

@sipa The DoS being addressed here was based on actual verification time, not the miner limit. So this fix (at least as described; have not reviewed the code) should be okay. Whether the mining code wants to demand a higher fee or not, is a separate (and perhaps reasonable) issue.

Concept ACK. Would be nice if someone reopened and/or merged #5231 at the same time.

Member

luke-jr commented Jul 18, 2016

@sipa The DoS being addressed here was based on actual verification time, not the miner limit. So this fix (at least as described; have not reviewed the code) should be okay. Whether the mining code wants to demand a higher fee or not, is a separate (and perhaps reasonable) issue.

Concept ACK. Would be nice if someone reopened and/or merged #5231 at the same time.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 18, 2016

Member

@luke-jr 20000 verification operations are allowed, and that is not currently changed by any of the proposals (neither #7081, this PR, or #8365). If that is a problem, we should propose a softfork to reduce the sigops limit, not change relay policy (which only prevents it in an easily-avoided case).

Member

sipa commented Jul 18, 2016

@luke-jr 20000 verification operations are allowed, and that is not currently changed by any of the proposals (neither #7081, this PR, or #8365). If that is a problem, we should propose a softfork to reduce the sigops limit, not change relay policy (which only prevents it in an easily-avoided case).

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 18, 2016

Member

I don't see what use this serves. It makes the code more complex and doesn't constrain anyone's ability to cause computation. #8365 is sensible.

Member

gmaxwell commented Jul 18, 2016

I don't see what use this serves. It makes the code more complex and doesn't constrain anyone's ability to cause computation. #8365 is sensible.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 26, 2016

Member

Closing in favor of #8365, which was merged today

Member

laanwj commented Jul 26, 2016

Closing in favor of #8365, which was merged today

@laanwj laanwj closed this Jul 26, 2016

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