Page MenuHomePhabricator

Add a flag that update default datacarriersize to 220 byte
ClosedPublic

Authored by deadalnix on Mar 5 2018, 12:17.

Details

Summary

To disincentivize the use of other methods to embed data into the chain, in particular via P2SH, the default datacarriersize is raised from 80 byte to 220 byte, so it becomes the "cheapest" way of embedding data into the chain. The default value is changed when a flag is passed to IsStandardTx

Test Plan
  1. Check, if a script with max custom provided payload size is considered standard, with and without the flag
  2. Check, if a script with max custom provided payload size + 1 is considered non-standard, with and without the flag
  3. Check, if a script with max old default payload size (83 byte total) is considered standard, when the HF is not active
  4. Check, if a script with max old default payload size + 1 (84 byte total) is considered non-standard, when the HF is not active
  5. Check, if a script with max new default payload size (223 byte total) is considered standard, when the HF is active
  6. Check, if a script with max new default payload size + 1 (224 byte total) is considered non-standard, when the HF is active

Via: make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
opreturnsize
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 2089
Build 2324: Bitcoin ABC Buildbot (legacy)
Build 2323: arc lint + arc unit

Event Timeline

Owners added a reviewer: Restricted Owners Package.Mar 5 2018, 12:17
deadalnix requested changes to this revision.Mar 5 2018, 14:42
deadalnix added a subscriber: deadalnix.

Activation code and test ?

This revision now requires changes to proceed.Mar 5 2018, 14:42

Activation code and test ?

This is simply a policy update and doesn't need an activation.

The tests are updated. There is one to check, whether the max. payload size is still considered standard, and another one, which ensures the max. payload size+1 is not considered as standard.

This is important anyway. If mempools are not mostly uniform, then fast block propagation techniques such as compact block and XThin do not work anymore.

This is important anyway. If mempools are not mostly uniform, then fast block propagation techniques such as compact block and XThin do not work anymore.

Setting a different default value, which depends on the chain state (e.g. after block xxx, or when May HF is active), during startup is straight forward, but this doesn't really help, because miners would need to restart the client after the activation time for the update to kick in.

So we need a switch during runtime and there are three options:

  1. User set a custom size with -datacarriersize
  2. The policy update is not active and thus the default is 83 byte
  3. The policy update is active and thus the default is 223 byte

Right now IsStandard and IsStandardTx are stateless though.

In D1158#21917, @dexX7 wrote:

This is important anyway. If mempools are not mostly uniform, then fast block propagation techniques such as compact block and XThin do not work anymore.

Setting a different default value, which depends on the chain state (e.g. after block xxx, or when May HF is active), during startup is straight forward, but this doesn't really help, because miners would need to restart the client after the activation time for the update to kick in.

So we need a switch during runtime and there are three options:

  1. User set a custom size with -datacarriersize
  2. The policy update is not active and thus the default is 83 byte
  3. The policy update is active and thus the default is 223 byte

Right now IsStandard and IsStandardTx are stateless though.

You mean if they've already configured their client to accept non-standard transactions? Yes, we could check if the current configured value is the default of 83 and then swap to 223 at the time of the hardfork. This seems to be what you are suggesting?

IsStandard and IsStandardTx probably need to become stateful for future upgrades anyways. Might as well fix them now.

You mean if they've already configured their client to accept non-standard transactions? Yes, we could check if the current configured value is the default of 83 and then swap to 223 at the time of the hardfork. This seems to be what you are suggesting?

IsStandard and IsStandardTx probably need to become stateful for future upgrades anyways. Might as well fix them now.

Essentially, yes.

During startup nMaxDatacarrierBytes is initialized to default value of MAX_OP_RETURN_RELAY, which is currently 83 byte, or if the user provided a -datacarriersize=n, then that one is chosen.

However, IsStandard() now needs to be modified so that:

  1. If a user provided a custom value, then don't change anything
  2. Else, if the HF isn't active yet, use the default value of 83 byte
  3. Else, if the HF is active, use the new default value of 223 byte

This could be done by providing IsStandard() (and as consequence also IsStandardTx()) with Config and CBlockIndex objects, to do a check, whether the HF is active.

I spoke with dex off-diff. He is going to have IsStandardTx take a flag to enable the new behavior and check the chainstate externally (passing in the flag). We can remove it after the hardfork in the next release.

Also, he's going to add a beautiful test :)

After May HF update default datacarriersize to 220 byte

Summary: To disincentivize the use of other methods to embed data into the chain, in particular via P2SH, the default datacarriersize is raised from 80 byte to 220 byte, so it becomes the "cheapest" way of embedding data into the chain. The default value is updated after the May hard fork was activated. If there is a user provided -datacarriersize, nothing changes.

Test Plan:

  1. Check, if a script with max custom provided payload size is considered standard
  2. Check, if a script with max custom provided payload size + 1 is considered non-standard
  3. Check, if a script with max old default payload size (83 byte total) is considered standard, when the HF is not active
  4. Check, if a script with max old default payload size + 1 (84 byte total) is considered non-standard, when the HF is not active
  5. Check, if a script with max new default payload size (223 byte total) is considered standard, when the HF is active
  6. Check, if a script with max new default payload size + 1 (224 byte total) is considered non-standard, when the HF is active

    Via: make check
dexX7 retitled this revision from Update default datacarriersize to 220 byte to After May HF update default datacarriersize to 220 byte.Mar 14 2018, 18:02
dexX7 edited the summary of this revision. (Show Details)
dexX7 edited the test plan for this revision. (Show Details)

This looks really good on the high level review. If someone else wants to dig into the details I would appreciate it.

Left some questions, thanks!

src/init.cpp
828 ↗(On Diff #3187)

Even after the HF, this will prints '83', right? It should be updated

src/policy/policy.cpp
49 ↗(On Diff #3187)

what would happen is someone sets -datacarriersize = 83 after the HF?

It seems the param will be ignored and 223 will be used, is that on purpose?

src/init.cpp
828 ↗(On Diff #3187)

After the HF most of this submission can be removed, because it's no longer necessary to switch between default values.

The help text is unfortunately not blockchain aware and may even be accessed before the chain is initialized, so adding a switch here seems difficult.

src/policy/policy.cpp
49 ↗(On Diff #3187)

Good catch. Yes, in this case the user value will be overwritten.

This could be addressed by getting and checking the actual configuration, but this comes at the cost of performance, given that it's done for every transaction entering the mempool.

schancel rescinded a token.
schancel awarded a token.
jasonbcox added inline comments.
src/init.cpp
828 ↗(On Diff #3187)

In my mind, there are two reasonable and simple options for this:

  1. Reword the help text to be something similar to: "... (pre-fork default: %u, post-fork default: %u)"
  2. Leave it as-is and cleanup this code immediately post-fork. The cleanup would include changing this to DEFAULT_OP_RETURN_RELAY_LARGE.

Either one is fine by me. If people feel strongly about keeping the help text accurate, I lean to option #1.

src/policy/policy.cpp
49 ↗(On Diff #3187)

imo, it should be fine to leave this artifact in the code since we can clean a lot of this code up post-fork. After cleanup, this won't be an issue.

deadalnix requested changes to this revision.Mar 15 2018, 00:03

Please use clang-format 4.0 to format these files.

This revision now requires changes to proceed.Mar 15 2018, 00:03
dexX7 edited the test plan for this revision. (Show Details)

After May HF update default datacarriersize to 220 byte

Summary: To disincentivize the use of other methods to embed data into the chain, in particular via P2SH, the default datacarriersize is raised from 80 byte to 220 byte, so it becomes the "cheapest" way of embedding data into the chain. The default value is updated after the May hard fork was activated. If there is a user provided -datacarriersize, nothing changes.

Test Plan:

  1. Check, if a script with max custom provided payload size is considered standard
  2. Check, if a script with max custom provided payload size + 1 is considered non-standard
  3. Check, if a script with max old default payload size (83 byte total) is considered standard, when the HF is not active
  4. Check, if a script with max old default payload size + 1 (84 byte total) is considered non-standard, when the HF is not active
  5. Check, if a script with max new default payload size (223 byte total) is considered standard, when the HF is active
  6. Check, if a script with max new default payload size + 1 (224 byte total) is considered non-standard, when the HF is active

    Via: make check

Please use clang-format 4.0 to format these files.

When using arc diff formating suggestions are made, but the diff becomes very, very large, because all touched files seem to be formatted.

Is this really something that should be done in this submission?

src/init.cpp
828 ↗(On Diff #3187)

I like the first option and I updated the submission accordingly. I also agree that most of it can be reverted or removed after the HF.

deadalnix edited reviewers, added: dexX7; removed: deadalnix.

Apply various nits, simplify testing.

Remove activation code (as it was untested) and rebase on top of D1201

Test overriding the value of the command line parameter and ensure this work with and without the large opreturn flag

deadalnix retitled this revision from After May HF update default datacarriersize to 220 byte to Add a flag that update default datacarriersize to 220 byte.Mar 16 2018, 00:14
deadalnix edited the summary of this revision. (Show Details)
deadalnix edited the test plan for this revision. (Show Details)
This revision is now accepted and ready to land.Mar 16 2018, 00:14

Looks good to me! I like the changes.

This revision was automatically updated to reflect the committed changes.