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
Details
Details
- Reviewers
dexX7 schancel matiu - Group Reviewers
Restricted Owners Package (Owns No Changed Paths) Restricted Project - Maniphest Tasks
- T302: Update default data carrier size
- Commits
- rSTAGINGcbf4410912f6: Add a flag that update default datacarriersize to 220 byte
rABCcbf4410912f6: Add a flag that update default datacarriersize to 220 byte
- Check, if a script with max custom provided payload size is considered standard, with and without the flag
- Check, if a script with max custom provided payload size + 1 is considered non-standard, with and without the flag
- Check, if a script with max old default payload size (83 byte total) is considered standard, when the HF is not active
- Check, if a script with max old default payload size + 1 (84 byte total) is considered non-standard, when the HF is not active
- Check, if a script with max new default payload size (223 byte total) is considered standard, when the HF is active
- 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
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
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:
- User set a custom size with -datacarriersize
- The policy update is not active and thus the default is 83 byte
- The policy update is active and thus the default is 223 byte
Right now IsStandard and IsStandardTx are stateless though.
Comment Actions
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.
Comment Actions
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:
- If a user provided a custom value, then don't change anything
- Else, if the HF isn't active yet, use the default value of 83 byte
- 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.
Comment Actions
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 :)
Comment Actions
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:
- Check, if a script with max custom provided payload size is considered standard
- Check, if a script with max custom provided payload size + 1 is considered non-standard
- Check, if a script with max old default payload size (83 byte total) is considered standard, when the HF is not active
- Check, if a script with max old default payload size + 1 (84 byte total) is considered non-standard, when the HF is not active
- Check, if a script with max new default payload size (223 byte total) is considered standard, when the HF is active
- 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
Comment Actions
This looks really good on the high level review. If someone else wants to dig into the details I would appreciate it.
Comment Actions
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. |
| src/init.cpp | ||
|---|---|---|
| 828 ↗ | (On Diff #3187) | In my mind, there are two reasonable and simple options for this:
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. |
Comment Actions
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:
- Check, if a script with max custom provided payload size is considered standard
- Check, if a script with max custom provided payload size + 1 is considered non-standard
- Check, if a script with max old default payload size (83 byte total) is considered standard, when the HF is not active
- Check, if a script with max old default payload size + 1 (84 byte total) is considered non-standard, when the HF is not active
- Check, if a script with max new default payload size (223 byte total) is considered standard, when the HF is active
- 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
Comment Actions
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. |
Comment Actions
Test overriding the value of the command line parameter and ensure this work with and without the large opreturn flag
