-
Notifications
You must be signed in to change notification settings - Fork 64
Conversation
may-2018-hardfork.md
Outdated
@@ -0,0 +1,33 @@ | |||
# March 2018 Hardfork Specification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this March or May Hardfork Specification ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May. Thanks
The following are not consensus changes, but are recommended changes for Bitcoin Cash implementations: | ||
|
||
* Automatic replay protection for future hardforks | ||
* Increase OP_RETURN relay size to 223 total bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be mad explicit that it means a payload of 220 bytes, 223 being the total size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be pedantic: the actual payload size really depends on the number of push operations used, which also count in terms of this limit. The +3 overhead comes from the opcode and two pushes and is inherited from bitcoin/bitcoin#6424.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new limit should be stated so as to be directly comparable to the current 80-byte limit. Notes regarding overhead should just clarify.
I'm somewhat surprised about any need to raise to 32mb already so soon...
and isn't replay-protection of future hardforks something that future forks
should have to deal with?
…On Sat, Feb 3, 2018 at 8:14 PM, dexX7 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In may-2018-hardfork.md
<#53 (comment)>:
> @@ -0,0 +1,33 @@
+# May 2018 Hardfork Specification
+
+Version Draft 0.2, 2018-02-01
+
+## Summary
+
+When the median time past[1] of the most recent 11 blocks (MTP-11) is greater than or equal to UNIX timestamp 1526400000 Bitcoin Cash will execute a hardfork according to this specification. Starting from the next block these consensus rules changes will take effect:
+
+* Blocksize increase to 32 MB
+* Re-enabling of several opcodes
+
+The following are not consensus changes, but are recommended changes for Bitcoin Cash implementations:
+
+* Automatic replay protection for future hardforks
+* Increase OP_RETURN relay size to 223 total bytes
To be pedantic: the actual payload size really depends on the number of
push operations used, which also count in terms of this limit. The +3
overhead comes from the opcode and two pushes and is inherited from
bitcoin/bitcoin#6424 <bitcoin/bitcoin#6424>.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#53 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AFAwsthSMJ4dxEKtayRk9pbYK7UkRTabks5tRLAHgaJpZM4R2NFc>
.
|
may-2018-hardfork.md
Outdated
|
||
## OpCodes | ||
|
||
Several opcodes will be re-enabled per [may-2018-opcodes](may-2018-opcodes.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when rendered this will become a broken url cause there's no may-2018-opcodes.md
included in the repo
@sickpig Yeah, the opcodes spec should be merged with this PR prior to going in, as well as the OP_DATASIGVERIFY |
@avl42 Why is this surprising? We have quite a lot of work to do to get to terabyte blocks... |
may-2018-reenabled-opcodes.md
Outdated
|OP_CAT |126 |0x7e|x1 x2 |out |Concatenates two byte strings | | ||
|OP_SUBSTR |127 |0x7f|in begin size |out |Returns a section of a string | | ||
|OP_LEFT |128 |0x80|in size |out |Keeps only characters left of the specified point in the string | | ||
|OP_RIGHT |129 |0x81|in size |out |Keeps only characters right of the specified point in the string | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't reflect the discussions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weren't left and right dropped as they could be done in other ways already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the top section only lists the opcodes that were removed in 2011, they are not all to be re-enabled. This section has caused confusion several times, I will reword it.
may-2018-reenabled-opcodes.md
Outdated
|OP_DIV |150 |0x96|a b |out |*a* is divided by *b* | | ||
|OP_MOD |151 |0x97|a b |out |return the remainder after *a* is divided by *b* | | ||
|OP_LSHIFT |152 |0x98|a b |out |shifts *a* left by *b* bits, preserving sign | | ||
|OP_RSHIFT |152 |0x99|a b |out |shifts *a* right by *b* bits, preserving sign | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mul and shift are not part of the package. I get that there are technically disabled opcodes, but this is supposed to be a spec for people to be able to implement the new opcodes.
may-2018-reenabled-opcodes.md
Outdated
|OP_RSHIFT |152 |0x99|a b |out |shifts *a* right by *b* bits, preserving sign | | ||
|
||
|
||
### Proposed op codes to reenable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also not the tone you'd expect from a spec. This is good for a document explaining what we want to do and why, but as far as the spec is, things are or are not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, you are quite right. this was a proposal, now it is a spec, still proposed but should be written in its final form.
may-2018-reenabled-opcodes.md
Outdated
* `Ox11 0x2233 OP_CAT -> 0x112233` | ||
|
||
The operator must fail if: | ||
* `0 <= len(out) <= MAX_SCRIPT_ELEMENT_SIZE`. The operation cannot output elements that violate the constraint on the element size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the condition is reversed here. The is the valid range, so it doesn't fail when things are in that range. they do fail when they are not in that range.
may-2018-reenabled-opcodes.md
Outdated
2. `a 0 OP_DIV -> failure`. Division by positive zero (all sizes), negative zero (all sizes), `OP_0` | ||
3. `a b OP_DIV -> out` where `a < 0` then the result must be negative or any form of zero. | ||
4. check valid results for operands of different lengths `1..4` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide examples of expected results so people can verify they round the right way.
may-2018-reenabled-opcodes.md
Outdated
1. `a b OP_MOD -> failure` where `!isnum(a)` or `!isnum(b)`. Both operands must be valid numbers. | ||
2. `a 0 OP_MOD -> failure`. Division by positive zero (all sizes), negative zero (all sizes), `OP_0` | ||
4. check valid results for operands of different lengths `1..4` | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dito, this absolutely needs example values, especially since it is not obvious what the implication of a or b being negative can be.
may-2018-reenabled-opcodes.md
Outdated
1. `0x00 OP_BIN2NUM -> OP_0`. Arrays of zero bytes, of various lengths, should produce an OP_0 (zero length array). | ||
2. `0x00000000000001 OP_BIN2NUM -> 0x01`. A large binary array, whose numeric value would fit in the numeric type, is a valid operand. | ||
1. `0x80000000000001 OP_BIN2NUM -> 0x81`. Same as above, for negative values. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to specify what happen in case of negative zero.
may-2018-reenabled-opcodes.md
Outdated
1. `n m OP_NUM2BIN -> failure` where `!isnum(n)` or `!isnum(m)`. Both operands must be valid numbers. | ||
2. `0x0100 1 OP_NUM2BIN -> failure`. Trying to produce a binary array which is smaller than the minimum size needed to contain the number. | ||
3. `0x01 (MAX_SCRIPT_ELEMENT_SIZE+1) OP_NUM2BIN -> failure`. Trying to produce an array which is too large. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if the number cannot be represented in the specified number of bytes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's covered by failure condition 2 - m < len(n). this should be explained.
may-2018-hardfork.md
Outdated
|
||
When the median time past[1] of the most recent 11 blocks (MTP-11) is greater than or equal to UNIX timestamp 1542300000 Bitcoin Cash full nodes implementing the May 2018 consensus rules SHOULD enforce the following change: | ||
|
||
* Update `forkid`[1] to be equal a non-zero unique constant chosen by each implementation. It is explicit that this specification intentionally omits a value, as it should be different for each implementation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should specify the forkId for old clients to continue to communicate on.
may-2018-hardfork.md
Outdated
|
||
When the median time past[1] of the most recent 11 blocks (MTP-11) is greater than or equal to UNIX timestamp 1526400000 Bitcoin Cash will execute a hardfork according to this specification. Starting from the next block these consensus rules changes will take effect: | ||
|
||
* Blocksize increase to 32 MB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is clarification here needed w/r/t mebibytes vs megabytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codebase seems to be using megabytes proper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I searched for any omissions but the spec is thorough and complete.
I think it needs more language consistency though. I have left very pedantic comments. Feel free to ignore as you see fit.
may-2018-reenabled-opcodes.md
Outdated
|
||
## <a name="data-types"></a>Script data types | ||
|
||
It should be noted that in script operation data values on the stack are interpreted as either binary strings (i.e. an array of bytes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"binary strings", "binary array", "array of bytes", "byte array" are all mixed in the document.
IMO this should be unified to "byte array", (or even better "byte sequence" which implicates variable length). A "binary array" not a correct formal term for spec. Binary is representation, not a data type.
may-2018-reenabled-opcodes.md
Outdated
3. There is one exception to rule 3: if there is more than one byte and the most significant bit of the | ||
second-most-significant-byte is set it would conflict with the sign bit. In this case a single 0x00 or 0x80 byte is allowed | ||
to the left. | ||
4. Negative zero is not allowed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest clarifying positive zero representation as well.
"Zero is encoded as a zero length byte sequence. Single byte positive or negative zero (0x00 or 0x80) are not allowed."
may-2018-reenabled-opcodes.md
Outdated
1. `b == 0`. Fail if `b` is equal to any type of zero. | ||
|
||
Impact of successful execution: | ||
* stack memory use reduced (one element removed) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(one element removed) is redundant with next line
may-2018-reenabled-opcodes.md
Outdated
2. `a 0 OP_DIV -> failure`. Division by positive zero (all sizes), negative zero (all sizes), `OP_0` | ||
3. `a b OP_DIV -> out` where `a < 0` then the result must be negative or any form of zero. | ||
4. `27 7 OP_DIV -> 3`, `27 -7 OP_DIV -> -3`, `-27 7 OP_DIV -> -3`, `-27 -7 OP_DIV -> 3`. Check negative operands. | ||
5. check valid results for operands of different lengths `1..4` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be 0..4?
may-2018-reenabled-opcodes.md
Outdated
|
||
Returns the remainder after dividing a by b. The output will be represented using the least number of bytes required. | ||
|
||
a b OP_MOD → out |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using two different arrows throughout doc.
may-2018-reenabled-opcodes.md
Outdated
Arithmetic: `OP_DIV`, `OP_MOD` | ||
|
||
New operations: | ||
* `x OP_BIN2NUM -> n`, convert a binary array `x` into a valid (canonical) numeric element |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(canonical) is confusing here. It is implicated by "numeric element"
Also "number value", "numeric value", "numeric element", "number" are mixed throughout. I would suggest using one ("numeric value"), and only clarify that a number value is a canonical signed representation in "data types"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also "valid number", "valid numeric value" are used in the doc.
I think it is also clearer to remove "valid" as it is implicated by consistent use of "numeric value". There is no such thing as an "invalid numeric value".
may-2018-reenabled-opcodes.md
Outdated
whilst preserving the sign bit. | ||
|
||
**Endian notation** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing. I don't think hex strings are big endian for byte sequences. Also it doesn't seem to be used for numeric values.
I would suggest removing it, use actual (little endian) when needed and clarify in place: 0xFF80 (-255)
may-2018-reenabled-opcodes.md
Outdated
|
||
The new opcode `x n OP_NUM2BIN` can be used to convert a number into a zero padded binary array of length `n` | ||
whilst preserving the sign bit. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also clarify that there are compatible/identical with the semantics for existing arithmetic operations? Are they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
um, I'm not sure I understand what you mean by this.
may-2018-reenabled-opcodes.md
Outdated
* *Operand order*. In keeping with convention where multiple operands are specified the top most stack item is the | ||
last operand. e.g. `x1 x2 OP_CAT` --> `x2` is the top stack item and `x1` is the next from the top. | ||
* *empty byte array*. Throughout this document `OP_0` is used as a convenient representation of an empty byte array. Whilst it is | ||
a push data op code it's effect is to push an empty byte array to the stack. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*its
may-2018-reenabled-opcodes.md
Outdated
|
||
Unit tests: | ||
|
||
1. `x1 x2 OP_AND -> failure`, where `len(x1) != len(x2)`. Operation fails when length of operands not equal. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest s/Operation fails when length of operands not equal./The two operands must be the same size./
as it now oddly alternates between "length" and "size."
may-2018-reenabled-opcodes.md
Outdated
Unit tests: | ||
1. `a b OP_DIV -> failure` where `!isnum(a)` or `!isnum(b)`. Both operands must be valid numbers | ||
2. `a 0 OP_DIV -> failure`. Division by positive zero (all sizes), negative zero (all sizes), `OP_0` | ||
3. `a b OP_DIV -> out` where `a < 0` then the result must be negative or any form of zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...where a < 0
and b > 0
...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, although there are more conditions. Removed, covered by the next clause.
may-2018-reenabled-opcodes.md
Outdated
The numeric type has specific limitations: | ||
1. The used encoding is little endian with an explicit sign bit (the highest bit of the last byte). | ||
2. They cannot exceed 4 bytes in length. | ||
3. They must be encoded using the shortest possible byte length (no zero padding) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the OP_MOD, OP_DIV code this still seems to be dependent on SCRIPT_VERIFY_MINIMALDATA, which determines standardness? Or did I miss it?
According to the draft specification for the upcoming May 15, 2018 Bitcoin Cash HF (bitcoincashorg/spec#53) the activation time is 1526400000, i.e. Tue May 15 18:00:00 CEST 2018
* Add May 15, 2018 HF activation time to consensus params According to the draft specification for the upcoming May 15, 2018 Bitcoin Cash HF (bitcoincashorg/spec#53) the activation time is 1526400000, i.e. Tue May 15 18:00:00 CEST 2018
@avl42 The purpose of the replay protection is to give exchanges and other community participants assurance that if there are hostile forks that they will be replay protected against. Irrespective of any NEED to raise the max block size to 32mb, the code is capable of doing so. The miners set the block size limits always, if developers are limiting it, it is because development has not found a way to address the market with the bitcoin protocol. Getting anywhere close to the developer limits is a failure of development. The actual limits are not set by developers any more since the beginning of Bitcoin Cash. The 32mb is just what the software is capable of managing. Mining limits are currently mostly at 2, 4, and 8mb depending on the miner, and this will likely remain so unless it becomes profitable to increase these (based on actual usage). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My approval is not contingent on the changes I suggested.
|
||
When the median time past[1] of the most recent 11 blocks (MTP-11) is greater than or equal to UNIX timestamp 1526400000 Bitcoin Cash will execute a hardfork according to this specification. Starting from the next block these consensus rules changes will take effect: | ||
|
||
* Blocksize increase to 32,000,000 bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Block size limit increases"
* Automatic replay protection for future hardforks | ||
* Increase OP_RETURN relay size to 223 total bytes | ||
|
||
## Blocksize increase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Block size limit increase
|
||
The blocksize hard capacity limit will be increased to 32MB (32000000 bytes). | ||
|
||
## OpCodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Opcodes"
Capital C is not used elsewhere in the doc.
The following are not consensus changes, but are recommended changes for Bitcoin Cash implementations: | ||
|
||
* Automatic replay protection for future hardforks | ||
* Increase OP_RETURN relay size to 223 total bytes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new limit should be stated so as to be directly comparable to the current 80-byte limit. Notes regarding overhead should just clarify.
may-2018-reenabled-opcodes.md
Outdated
operations that is preferred over a set of more complex op codes. Input conditions that create ambiguous or undefined | ||
behaviour should fail fast. | ||
|
||
Each op code should be examined for the following risk conditions and mitigating behaviour defined explicitly: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be? How about "Mitigating behaviour is defined explicitly for the following risk conditions"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a case where words like "should" and "must" ought to have meanings like Internet RFCs. Given that this code will form the foundation of a potentially mind-boggling financial system (among many other uses), ambiguity cannot be allowed. This is a case where "must" needs to be used. Explicit fail conditions must be defined, with expected failure behaviors for each one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The words "should" and "must" are used with those connotations throughout the rest of the document. However, this is an introductory paragraph that describes the approach used during the production of this specification. I dont think this level of precision is required here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the "risk and philosophical approach" paragraph. This is useful guidance but belongs in a guidance document and rather than the actual specification.
* `OP_0 0 OP_SPLIT -> OP_0 OP_0`. Execution of OP_SPLIT on empty array results in two empty arrays. | ||
* `x 0 OP_SPLIT -> OP_0 x` | ||
* `x len(x) OP_SPLIT -> x OP_0` | ||
* `x (len(x) + 1) OP_SPLIT -> FAIL` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a test where 0 < n < len(x) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. A test case for the basic success scenario should be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
may-2018-reenabled-opcodes.md
Outdated
type, is a valid operand. | ||
1. `0x80000000000001 OP_BIN2NUM -> 0x81`. Same as above, for negative values. | ||
1. `0x80 OP_BIN2NUM -> OP_0`. Negative zero, in a byte sequence, should produce zero. | ||
1. `0x80000000000000 OP_BIN2NUM -> OP_0`. Large negative zero, in a byte sequence, should produce zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a test with value other than 1 or 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, added, thanks
may-2018-reenabled-opcodes.md
Outdated
type, for both positive and negative values. | ||
1. `0x00 OP_BIN2NUM -> OP_0`. Byte sequences, of various lengths, consisting only of zeros should produce an OP_0 (zero | ||
length array). | ||
2. `0x00000000000001 OP_BIN2NUM -> 0x01`. A large byte sequence, whose numeric value would fit in the numeric value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify a test with input sequence length of MAX_SCRIPT_ELEMENT_SIZE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, might be useful, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My review focuses on clarity in the document, including technical (code) issues, grammar and formatting.
may-2018-reenabled-opcodes.md
Outdated
1. The used encoding is little endian with an explicit sign bit (the highest bit of the last byte). | ||
2. They cannot exceed 4 bytes in length. | ||
3. They must be encoded using the shortest possible byte length (no zero padding) | ||
3. There is one exception to rule 3: if there is more than one byte and the most significant bit of the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be numbered as 3.1? Just for clarity. It looks odd to see a second, indented "item 3."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.
may-2018-reenabled-opcodes.md
Outdated
operations that is preferred over a set of more complex op codes. Input conditions that create ambiguous or undefined | ||
behaviour should fail fast. | ||
|
||
Each op code should be examined for the following risk conditions and mitigating behaviour defined explicitly: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a case where words like "should" and "must" ought to have meanings like Internet RFCs. Given that this code will form the foundation of a potentially mind-boggling financial system (among many other uses), ambiguity cannot be allowed. This is a case where "must" needs to be used. Explicit fail conditions must be defined, with expected failure behaviors for each one.
may-2018-reenabled-opcodes.md
Outdated
|
||
Each op code should be examined for the following risk conditions and mitigating behaviour defined explicitly: | ||
* Operand byte length mismatch. Where it would be normally expected that two operands would be of matching byte lengths | ||
the resultant behaviour should be defined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example where "should" versus "must" ambiguity is a problem. Let's say an operator requires two operands of equal length, but doesn't specify a default behavior if they're not. What happens if for some reason it gets operands of unequal lengths? Client A defines its result to use the shorter length and truncate the longer one's length. Client B defines its result to pad or otherwise expand the shorter operand to match the longer length. Client C simply fails the operation. And now we have a mess.
I realize that each operator below has detailed test cases for each such scenario. This paragraph is discussing the philosophy behind implementing the opcodes. It should be clear in the policy that such scenarios must, not should, have a defined behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section removed
may-2018-reenabled-opcodes.md
Outdated
* Overflows. Defined behaviour in the instance that result of the operation exceeds MAX_SCRIPT_ELEMENT_SIZE | ||
* Empty byte sequence operands. Whether empty byte sequences should be allowed as a representation of zero. | ||
* Empty byte sequence output. Note that an operation that outputs an empty byte sequence has effectively pushed `false` | ||
onto the stack. If this is the last operation in a script or if a conditional operator immediately follows the script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For grammatical clarity, I would rephrase this sentence.
"The script author must consider the possibility that this is the last operation in a script, and the possibility that a conditional operator immediately follows the script."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
section removed
may-2018-reenabled-opcodes.md
Outdated
x1 x2 OP_CAT -> out | ||
|
||
Examples: | ||
* `Ox11 0x2233 OP_CAT -> 0x112233` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the prior comment about little-endian versus big-endian and how the docs use a different byte order than the actual data, is this example literally correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Endianness only applies to the interpretation of numbers, please be more specific as to what you mean. This example should likely be written as
{0x11} {0x22, 0x33} OP_CAT -> {0x11, 0x22, 0x33}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think your clarification would help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree, will do
may-2018-reenabled-opcodes.md
Outdated
|
||
**Floor division** | ||
|
||
Note: that when considering integer division and modulo operations with negative operands the rules applied in the C language and most |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a grammar thing, the word "that" should not be here, or else the colon should be removed.
Also, there should be a comma after "operands".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done, thanks
may-2018-reenabled-opcodes.md
Outdated
Unit tests: | ||
1. `a b OP_DIV -> failure` where `!isnum(a)` or `!isnum(b)`. Both operands must be numeric values | ||
2. `a 0 OP_DIV -> failure`. Division by positive zero (all sizes), negative zero (all sizes), `OP_0` | ||
4. `27 7 OP_DIV -> 3`, `27 -7 OP_DIV -> -3`, `-27 7 OP_DIV -> -3`, `-27 -7 OP_DIV -> 3`. Check negative operands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test Case 3 is missing. It should probably be a case for correct integer division without remainders. For example:
27 3 OP_DIV -> 9
, 27 -3 OP_DIV -> -9
, -27 3 OP_DIV -> -9
, -27 -3 OP_DIV -> 9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
markdown renumbers as required, but updated numbers. successful tests covered by case 5 (really 4) below
may-2018-reenabled-opcodes.md
Outdated
Unit tests: | ||
1. `a b OP_MOD -> failure` where `!isnum(a)` or `!isnum(b)`. Both operands must be numeric values. | ||
2. `a 0 OP_MOD -> failure`. Division by positive zero (all sizes), negative zero (all sizes), `OP_0` | ||
4. `27 7 OP_MOD -> 6`, `27 -7 OP_MOD -> 6`, `-27 7 OP_MOD -> -6`, `-27 -7 OP_MOD -> -6`. Check negative operands. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case 3 is missing. Either renumber the subsequent ones, or add a test case for zero-result cases (e.g., 27 3 OP_MOD -> 0
, etc.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
markdown renumbers automatically, but updated. Zero result cases added, thanks.
may-2018-reenabled-opcodes.md
Outdated
* `0x0000000002 OP_BIN2NUM -> 0x02` | ||
* `0x800005 OP_BIN2NUM -> 0x85` | ||
|
||
The operator must fail if: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no section here for "Impact of successful execution" as with the other opcodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, added.
may-2018-reenabled-opcodes.md
Outdated
Unit tests: | ||
1. `a OP_BIN2NUM -> failure`, when `a` is a byte sequence whose numeric value is too large to fit into the numeric value | ||
type, for both positive and negative values. | ||
1. `0x00 OP_BIN2NUM -> OP_0`. Byte sequences, of various lengths, consisting only of zeros should produce an OP_0 (zero |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case numbering is not correct. It should be sequential. See also the subsequent cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
markdown renumbers automatically.
What is the situation with historical appearances of the re-enabled opcodes? Presumably the chain still validates but are historical appearances listed anywhere? Some of these have changed semantics. |
may-2018-reenabled-opcodes.md
Outdated
length array). | ||
2. `0x00000000000001 OP_BIN2NUM -> 0x01`. A large byte sequence, whose numeric value would fit in the numeric value | ||
type, is a valid operand. | ||
1. `0x80000000000001 OP_BIN2NUM -> 0x81`. Same as above, for negative values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these tests fail because 0x80000000000001 is longer than the maximum 4-byte integer allowed by numeric opcodes (see CScriptNum).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the point of this test was that BIN2NUM should correctly translate a large byte sequence into a numeric if the result will be valid. In this case, the result is -1, which is a valid numeric.
The numeric value of 0x80000000000001 is -1. The point of this test was that this number can fit in a minimally encoded integer. EDIT: I think there was some confusion here from me about endianess. Please check the latest version. |
may-2018-reenabled-opcodes.md
Outdated
|
||
## Reference implementation | ||
|
||
OP_AND, OP_OR, OP_XOR: https://reviews.bitcoinabc.org/D1090 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all wrong now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
may-2018-reenabled-opcodes.md
Outdated
|
||
## New operations | ||
|
||
### OP_BIN2NUM |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These need to specify the endianness of the input. They should interpret the input as little endian so that so that BIN2NUM is idempotent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, good point
|
I was wondering the same as @DesWurstes. It seemed like we all agreed Tom Harding's inverted simple form of Jacob Eliosoff's EMA was the best thing to do. I only objected (as before) that the value of N=100 was too large. That would be like WT-144 with an N=230 in the speed of response. The following graphs compares Simple EMA N=50, SMA N=144, LWMA N=60 (my version of WT-144), and Digishield v3 (which is like an improved version of SMA N=68). You have to use N~=50 to 60 to make the long-term speed of EMA equal to SMA=144. The EMA will have a lot faster in short term response. N=100 for EMA is as fast as SMA N=144 only in short term response. Digishield and LWMA N=60 are a lot faster. The second chart shows how speed comes at a price in stability. I've been guiding Monero micro clones to use LWMA N=60 and it's working great on 5 of them, attracting fewer hash attacks and usually fewer delays than BCH's DAA. Not counting the startup, the DAA has had 6 episodes of attracting excessive mining resulting > 11 solvetimes averages to be less than 2.5 minutes which is the result of difficulty being a lot slower than price changes. In 7 months of data on these micro coins, there haven't been any instances of this on 4 of them despite having much larger attackers. For EMA to act similarly, it's N would need to be 60/2.3 = 26. But N=50 is safer since SMA N=144 is not broken. All of these Monero clones were using N=720 which for their T=120 is the same as BCH's DAA N=144. They're both daily averages. The small coins have the advantage getting 4x more sample points. But the daily SMA average reliably results in a disaster for them. Using N=144 is the same as hoping BCH's price does not fall too much. That seems like a security risk based on a presumption. SMA N=144 is working fine so far, as long as the coin stays big and the miners stay nice. EMA N=100 will probably work better. SMA N=100 or EMA N=50 will work better. The Simple EMA is |
If Bobtail is used, N can be reduced a lot to get a faster response to hash rate changes without a loss in stability (equally smooth). If solvetime goes to 2.5 minutes, then N=144 will work a lot faster and better. EMA may need changing if Bobtail is used. Bobtail by itself will not allow SMA N=144 to work any better (faster in response), only a little more stable. |
@josephNLD I apologize for not anwering earlier, but here is my reply:
Why would anyone care for a hostile fork? If the hostile fork happened
I can't imagine that exchanges ask for something like that, but if they
I see this slightly differently (and still not in the way bitcoin-core does, to be clear) I'd much more prefer if the maximum blocksize would be auto-adjusted by the average |
Also, there is still the unanswered question about the motivation to add those new opcodes. Does anyone have a usecase for it? Some new challenge/response patterns? I'd like to read about at least one pair of output- and input-scripts using the new |
@DesWurstes I appreciate your reply, but it doesn't yet satisfy me. If it were about a real scripting language, I'd certainly not raise the same This script language has a special purpose: allowing to specify a challenge With this premise, anything that isn't explicitly useful for the specific purpose really
That serendipitously did a nice Ockham's shave. That was my opinion about "why does even anyone ask for a reason". Now for the single opcodes' reasonings: OP_CAT: where would the arguments to be CAT'd come from? If all the arguments OP_SPLIT: This seems to be for tx's that move funds to "whoever wastes some CPU to for the other ones it essentially just: "it's so basic it just needs to be there and the uses It's not like I could stop this train, but I think it's good to have a discussion. Even if this |
@avl42, I appreciate your concern. I think most people would agree that Bitcoin Cash's primary purpose is to enable the transfer of value through simple transactions. However, it is capable of many more functions, including sophisticated transaction types. It is a challenge to enable more sophisticated functionality without impacting the primary purpose and it must be done carefully, but I think we have achieved that with this update. The question of whether Bitcoin Cash should be capable of supporting additional functionality has been discussed at length in other places. I think those other places, such as the Bitcoin Unlimited forums, are a more appropriate place for that discussion. |
may-2018-hardfork.md
Outdated
|
||
* Update `forkid`[1] to be equal to 0xFF0001. ForkIDs beginning with 0xFF will be reserved for future protocol upgrades. | ||
|
||
This particular consensus rule MUST NOT be implemented by Bitcoin Cash wallet software. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to know more about the intended meaning of this 'MUST NOT' here.
At first glance, it seems inconsistent with the preceding 'SHOULD' in l.27.
It would be consistent if interpreting 'MUST NOT' as 'does not have to', but 'MUST NOT' means absolutely prohibited according to RFC2119.
If wallet software is absolutely prohibited from implementing a future consensus rule, then it will diverge from consensus guaranteed, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intent here is that wallets should not implement this rule, only nodes relaying transactions. That way wallets will stay using the main chain, rather than any future minority fork from Bitcoin Cash.
may-2018-hardfork.md
Outdated
|
||
## Automatic Replay Protection | ||
|
||
When the median time past[1] of the most recent 11 blocks (MTP-11) is greater than or equal to UNIX timestamp 1542300000 Bitcoin Cash full nodes implementing the May 2018 consensus rules SHOULD enforce the following change: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would seem helpful to me to mark this timestamp so that the reader clearly notices that this is not the 15 May fork time, but the proposed November hard fork time.
As an example of how it could be marked more clearly:
UNIX timestamp 1542300000 (November 2018 hardfork)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, why is it even in a document named "may-2018-hardfork.md" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it pertains to replay protection of future hardforks.
Co-author: Daniel Connolly <daniel@dconnolly.com>
I've aggregated all the feedback and updates and will be merging this now. We should open new pull requests for future changes so history is properly recorded. |
There was a new exploit conducted on Graft. All cryptonote coins who might find themselves facing a miner with >50% hash power need to immediately apply the non-fork Jagerman MTP Patch. It has background discussed here and summarized here. BCH should do something similar if its nodes create block templates with the current time even when the MTP is ahead of current time. Otherwise, they will be creating a template that they are not going to accept, if it is solved before the MTP goes into the past (relative to that template time). The MTP can get ahead of current time by a >57% miner assigning timestamps into the future (when MTP=11 and FTL =~ 11. FTL= future time limit which I think is 7200 seconds (FTL =12) in BCH as in BTC. The result is that no one but those with forwarded timestamps on their template can mine. The attacker will be the only one getting blocks until current time passes his fake MTP. He can do it selfishly so that even when the MTP passes current time and blocks start getting accepted, he can continue to mine privately until the public blockchain's work starts approaching his. It enables makes selfish mining easier. If BCH does not have code to stop this already in place (from BTC) my calculations indicates a 68% miner can get 22 blocks out in the open, or use them to jump-start a selfish mining attack. ( @jagerman ) |
Bitcoin ABC already ensures that the timestamp chosen in a block template
is ahead of the MTP.
2018-04-28 11:15 GMT+02:00 zawy12 <notifications@github.com>:
… There was a new exploit conducted on Graft. All cryptonote coins who might
find themselves facing a miner with >50% hash power need to immediately
apply the non-fork *Jagerman MTP Patch
<https://github.com/graft-project/GraftNetwork/pull/118/commits/5b349daf08993c694ff9a05022dee432852832a2>*.
It has background discussed here
<graft-project/GraftNetwork#118> and summarized
here <oxen-io/oxen-core#26>.
BCH should do something similar if its nodes create block templates with
the current time even when the MTP is ahead of current time. Otherwise,
they will be creating a template that they are not going to accept, if
solved before the MTP goes into the past (relative to that template time).
The MTP can get ahead of current time by a >57% miner assigning timestamps
into the future (when MTP=11 and FTL =~ 11. FTL= future time limit which I
think is 7200 seconds (FTL =12) in BCH as in BTC. The result is that no one
but those with forwarded timestamps on their template can mine. The
attacker will be the only one getting blocks until current time passes his
fake MTP. He can do it selfishly so that even when the MTP passes current
time and blocks start getting accepted, he can continue to mine privately
until the public blockchain's work starts approaching his. It enables makes
selfish mining easier. If BCH does not have code to stop this already in
place (from BTC) my calculations indicates a 68% miner can get 22 blocks
out in the open, or use them to jump-start a selfish mining attack. (
@jagerman <https://github.com/jagerman> )
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#53 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA0IaWv8ONwPaFrJB2C5jgGavCIjJ6wKks5ttDMigaJpZM4R2NFc>
.
|
No description provided.