Skip to content
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

Bytespersigop #9

Merged
merged 14 commits into from Nov 20, 2016
Merged

Bytespersigop #9

merged 14 commits into from Nov 20, 2016

Conversation

@rubensayshi
Copy link
Owner

rubensayshi commented May 23, 2016

In Bitcoin Core v0.12.1 a PR was merged to add -bytespersigop which limits the ratio of sigops:bytes.
This DOS protection uses a shortcut way of counting sigops which counts OP_CHECKMULTISIG as 20 sigops (the max allowed) and results in CP TXs that use multisig encoding not being relayed and mined by v0.12.1 nodes (when > 80 bytes it no longer fits in opreturn).

this is an edge case, there's been 250 CP TXs with > 80 bytes in the history of CP;

  • of which 129 were of an experimental feature (rock paper scissors) which were only early on.
  • 61 issuances (0.1% of all issuances), depending on the length of the description it sometimes tips over the 80 bytes.
  • 57 broadcasts (0.7% of all broadcasts), with 'random' text, mostly people testing out the feature and Adam testing how much data he could embed in a transaction.

I'm talking to core devs to get this fixed in core, hopefully for the next release, but before v0.12.1 is widely adopted we need a fix for this that we keep in place until v0.12.1 is phased out.

This PR consists of 2 fallbacks to deal with this problem:

  1. it will try to spend from extra UTXOs to tip the bytes:sigops ratio in our favor, 2x as many inputs as data outputs does the trick.
  2. it will fallback to pubkeyhash encoding when the above isn't possible, unfortunately our last resort and a solution that will pollute the UTXO set...
@rubensayshi rubensayshi force-pushed the bytespersigop branch 4 times, most recently from 938febd to 6f7680b May 23, 2016
desired_input_count = 1

if encoding == 'multisig' and data_output and util.enabled('bytespersigop'):
desired_input_count = len(data_output) * 2

This comment has been minimized.

Copy link
@robby-dermody

robby-dermody May 31, 2016

given that data_output = (data_array, data_value), do you mean desired_input_count = len(data_output[0]) * 2 ??

This comment has been minimized.

Copy link
@rubensayshi

rubensayshi Jun 23, 2016

Author Owner

was supposed to be len(data_array)

# in bitcoin core v0.12.1 a -bytespersigop was added that messes with bare multisig transactions,
# as a safeguard we fall back to pubkeyhash encoding when unsure
# when len(inputs) > len(data_outputs) there's more bytes:sigops ratio and we can safely continue
if encoding == 'multisig' and inputs and data_output and len(inputs) < len(data_output) * 2 and util.enabled('bytespersigop'):

This comment has been minimized.

Copy link
@robby-dermody

robby-dermody May 31, 2016

same here -- len(data_output[0]) * 2

"minimum_version_major": 9,
"minimum_version_minor": 54,
"minimum_version_revision": 0,
"block_index": 413083

This comment has been minimized.

Copy link
@robby-dermody

robby-dermody May 31, 2016

make this 500k so that we can safely merge into develop and commit with the next release, as this problem has an impact but not a huge one

@rubensayshi rubensayshi force-pushed the bytespersigop branch 4 times, most recently from 7aa0927 to ee2d1dc Jun 23, 2016
…cast text messages.

the old way had an issue when the string was exactly the switch point (52), because it turned into a 53 long string (+1 from length byte) and that resulted in length byte becoming part of the result.
now always use a varint to specify the length.
@rubensayshi rubensayshi force-pushed the bytespersigop branch 3 times, most recently from 40de8b7 to cd455e2 Jul 10, 2016
@rubensayshi rubensayshi force-pushed the bytespersigop branch from cd455e2 to ce35335 Jul 10, 2016
…ctly-52chars-varstring

Problem with broadcast text exactly 52 chars!  (PROTOCOL CHANGE)
@rubensayshi rubensayshi force-pushed the bytespersigop branch from ce35335 to 8b7aa50 Jul 10, 2016
@rubensayshi rubensayshi force-pushed the bytespersigop branch from 8b7aa50 to 0b947f4 Jul 11, 2016
@rubensayshi rubensayshi merged commit 0b947f4 into develop Nov 20, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.