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

Add Specification for 2018-11-15 Upgrade #94

Merged
merged 3 commits into from Sep 27, 2018
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+65 −0
Diff settings

Always

Just for now

Copy path View file
@@ -0,0 +1,65 @@
---
layout: specification
title: 2018 November 15 Network Upgrade Specification

This comment has been minimized.

@jtoomim

jtoomim Aug 10, 2018

Contributor

Title should be:

2018 November 15 Proposed Network Upgrade Specification

Since the Bitcoin Cash community has not shown overwhelming support for this specification yet, it should be clearly described as a proposal.

This comment has been minimized.

@Mengerian

Mengerian Aug 11, 2018

Author Contributor

That's a good point.

I don't expect this pull request to get merged for at least a few more weeks, so maybe we should monitor community sentiment, and decide at that point how to characterize it. I'll try to characterize it in a way that seems reasonable.

This comment has been minimized.

@ftrader

ftrader Aug 23, 2018

Contributor

@Mengerian , could you explain what happened since then, that caused this specification to be published without a 'proposed' tag?

I find myself agreeing with people who are reasonably critical of this, such as to publish on 'bitcoincash.org' without pointing out that the specification for the upcoming upgrade is still contended.

date: 2018-08-24
activation: 1542300000
version: 0.3
---

## Summary

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 will execute an upgrade of the network consensus rules according to this specification. Starting from the next block these consensus rules changes will take effect:

* Remove topological transaction order constraint, and enforce canonical transaction order

This comment has been minimized.

@zander

zander Aug 9, 2018

Contributor

The removal of topological transaction order is not an improvement.

You want to remove information gathered during the mempool-fill time for practically free. This information is used during validation to make sure we can parallelize the validation of those transactions not in 'topological order'.

When you remove this information then it takes an exponential amount of time to find it again (based on number of transactions inputs+outputs in the block). Which is a huge negative.
A draft (designed on paper) solution to do the inserts first and the deletes later doesn't make any sense in ANY UTXO implementation because the inserts will end up being 100% serialized (due to locking). So similarly there, good bye parallel validation.

A simple solution is to sort the transactions that actually are spent in the same block in a separate, topologically sorted, section. And then sort the rest "canonical".

The Graphene developers have already agreed that this approach would be great for them, giving all the advantages of sorting.

That solution also has the benefit of not being a protocol fork.

Bottom line; Please don't remove topological transaction order, it breaks known implementations.

This comment has been minimized.

@jtoomim

jtoomim Aug 9, 2018

Contributor

Why do you say that any UTXO implementation will end up with inserts serialized? Concurrent hashtables are a thing; you can set one up with a mutex on each bucket, for example. Lock-free hashtables are also possible using atomic swaps.

This comment has been minimized.

@zander

zander Aug 9, 2018

Contributor

jt, that doesn't really address the problem of removing the ordering information Satoshi put in the protocol.

But, concurrent hashtables have no implementation in the standard library or in boost (according to google), and while theoretically people could build a new container for this, that should not be required to make a well-performing implementation for the consensus rules.

This comment has been minimized.

@jtoomim

jtoomim Aug 9, 2018

Contributor

It doesn't seem to be important information. The transaction set naturally has the structure of a DAG, not a list. Encoding transactions in blocks as a list adds unnecessary information content to the block encoding. Getting rid of unnecessary information seems like a good idea. Not needing to check that transactions are topologically ordered seems like a win, too -- it's a lot easier to check that transactions are sorted by hash than it is to check that the transaction sorting is a valid topological sorting, especially for parallel algorithms.

By the way, it does not take exponential time to (re)generate a topological sorting of elements. It's actually O(n) versus transaction count for serial algorithms, and O(log^2(n)) versus processor count for parallel implementations.

We're already using a library outside of boost and std:: to store our UTXOs: LevelDB. It seems within reason to include a concurrent hashtable library. Many such libraries exist in single header form with APIs identical or similar to std::unordered_map, so replacement should be pretty easy. I did that once with sparsepp a few weeks ago, and it wasn't hard. Moreover, std::unordered_map is relatively slow and memory-inefficient, so switching to a more modern hashtable implementation can improve performance even without parallelization.

This comment has been minimized.

@zander

zander Aug 9, 2018

Contributor

It doesn't seem to be important information.

You should at minimum show the courtesy to read other peoples posts, I explained in detail above why it is...

The transaction set naturally has the structure of a DAG, not a list

Yes, as I said elsewhere there are two types of nodes. One type is the 99% which can be reordered at will since in graph language, they are not related to anyone. The rest of the nodes are, and those need we need to know the ordering for.

Getting rid of unnecessary information seems like a good idea.

Tx 1 spends Tx2, thus you need to process TX1 before Tx2 or you will report that Tx2 spends non-existing funds. That ordering is really quite important.

it's a lot easier to check that transactions are sorted by hash than it is to check that the transaction sorting is a valid topological sorting, especially for parallel algorithms.

This misses the fact that no implementation spends even a single cycle today doing this check. So adding a validation rule about sorting makes things slower. Because we don't check sorting today.

Bottom line;

  • this rule adds validation for ordering. There is no such validation today.
  • this rule removes data needed for parallel validation which makes PV much more costly.
  • this rule doesn't help ANY software or technology.

JToomim may have a point that technology could be used to make this not a big issue, but that software should be written, tested and seen to be better than the PV software we have today before a protocol change is made.

Doing a change based on what some developers theorize should be better. without any actual practical experience to back it up, is really irresponsible. This is not how you deal with a 10 billion dollar industry.

This comment has been minimized.

@jtoomim

jtoomim Aug 10, 2018

Contributor

Doing a change based on what some developers theorize should be better. without any actual practical experience to back it up, is really irresponsible.

While I may be more optimistic than you about the performance and security of the canonical block idea, I definitely agree with this sentence. No plans should be made to enact a change like this until it has been shown, with a sample implementation, to be an improvement.

That said, it's acceptable to write a specification for the proposed change in advance of the implementation, which is what this document is supposed to be. We just need to make sure that we are aware that the proposed and specified changes are not yet approved.

This comment has been minimized.

@jtoomim

jtoomim Aug 10, 2018

Contributor

A simple solution is to sort the transactions that actually are spent in the same block in a separate, topologically sorted, section. And then sort the rest "canonical".

The Graphene developers have already agreed that this approach would be great for them, giving all the advantages of sorting.

By the way, a while ago I implemented an encoding scheme for Graphene which gives approximately the same efficiency on the current CreateNewBlock results that you'd get from the separation of topological and canonical ordering sections. So yes, canonical ordering is not needed for Graphene to work, and probably isn't needed at all. It would certainly make Graphene simpler and more efficient, though.

This comment has been minimized.

@awemany

awemany Aug 15, 2018

I like to take the opportunity to point out this very preliminary analysis of one aspect of this change that I did:

https://github.com/awemany/valorder

@deadalnix: Have you done a deeper analysis of this aspect? Other aspects?

Is there a spec for the change to lexicographic ordering? There's quite a few things to consider. One obvious one is the encoding of TXID order when doing the sorting...

I have to admit that it is my fault that I should have been much more sceptical earlier. The fact that the positives of this change are emphasized whereas the negatives (like the above) are dropped under the table tells me that there might be some group think in action. No offense meant.

* Enable OP_CHECKDATASIG and OP_CHECKDATASIGVERIFY opcodes
* Enforce minimum transaction size
* Enforce "push only" rule for scriptSig
* Enforce "clean stack" rule

The following are not consensus changes, but are recommended changes for Bitcoin Cash implementations:

* Automatic replay protection for future upgrade

## Canonical Transaction Order

With the exception of the coinbase transaction, transactions within a block shall be sorted by Transaction ID in ascending order. The coinbase transaction shall be the first transaction in a block.

## OpCodes

New opcodes OP_CHECKDATASIG and OP_CHECKDATASIGVERIFY will be enabled as specified in [op_checkdatasig.md](op_checkdatasig.md) [2].

## Minimum Transaction Size

This comment has been minimized.

@zquestz

zquestz Sep 7, 2018

Contributor

Can someone enlighten me on why this is 100 bytes and not 64? All the literature I read about this Merkle tree vulnerability is for 64 byte transactions. Would like to know why this is 100. Thanks in advance.

This comment has been minimized.

@sickpig

sickpig Sep 13, 2018

Collaborator

Guess @deadalnix, @jasonbcox or @schancel could chime in. We were in agreement to fix it but we had different idea on the details, then ABC in the process of respecting releases timeline included the 100 byte limit in the code.

As a matter of fact we could have put a different constraint, i.e. != 64 or used Sergio Demian Lerner's proposal to fix it w/o the need of a fork.

This comment has been minimized.

@markblundeberg

markblundeberg Sep 14, 2018

Collaborator

I am thinking of an application using BCH that would make use of very short anyone-can-spend redeem scripts as 'notification addresses' for lite clients. For example the redeem script would be PUSH(8 bytes) or even PUSH(4 bytes), spendable with empty scriptsig. The idea with these addresses is that anyone (miners included) can grab the dust.

Ideally miners could spend these utxos using one-input, zero-output transactions that just add them to the collected fees. Such transactions would be only 61 or 57 bytes long. So, I think the != 64 constraint would make most sense.

This comment has been minimized.

@zquestz

zquestz Sep 18, 2018

Contributor

Thank you @sickpig and @markblundeberg for actually answering me. Greatly appreciated. =)

This comment has been minimized.

@awemany

awemany Sep 20, 2018

@markblundeberg: I have stumbled over the 100tx limit while doing my "Zero Conf Forfeit" transaction as well. @schancel told me it should be easy to remove again (and technically, I agree).

I also think we should definitely allow small transactions in the future, even if there are plans to reserve space in the merkle / to-be-merklix tree.

A P2PKH -> P2SH transaction is impacted by the 100 byte limit.

EDIT: Correction: No it isn't. It is only < 100 bytes without the scriptSigs. I have to figure out where I stumbled over the limit with my work on Zero Conf Forfeits but I was annoyed.

As I compromise, I'd like to put onto the agenda to remove this limit again when Sergio Demian Lerner's proposal is implemented (or similar) and make this the goal for the next HF.

This comment has been minimized.

@SubbieMasse

This comment has been minimized.

@markblundeberg

markblundeberg Sep 28, 2018

Collaborator

There have been 29000 transactions under 64 bytes so far:
https://blockchair.com/bitcoin-cash/transactions?q=size(..63)#

Only five have been 64 bytes exactly:
https://blockchair.com/bitcoin-cash/transactions?q=size(64)#

Proposal for next May fork: fix the size limit so it is != 64.


Transactions that are smaller than 100 bytes shall be considered invalid. This protects against a Merkle tree vulnerability that allows attackers to spoof transactions against SPV wallets [3].

## Push Only

Transactions shall be considered invalid if an opcode with number greater than 96 (hex encoding 0x60) appears in a scriptSig. This is the same as Bitcoin BIP 62 rule #3 [4].

## Clean Stack

For a transaction to be valid, only a single non-zero item must remaing on the stack upon completion of Script evaluation. If any extra data elements remain on the stack, the script evaluates to false. This is the same as Bitcoin BIP 62 rule #6 [4].

## Automatic Replay Protection

When the median time past [2] of the most recent 11 blocks (MTP-11) is less than UNIX timestamp 1557921600 (May 2019 upgrade) Bitcoin Cash full nodes MUST enforce the following rule:

* `forkid` [5] to be equal to 0.

When the median time past [1] of the most recent 11 blocks (MTP-11) is greater than or equal to UNIX timestamp 1557921600 (May 2019 upgrade) Bitcoin Cash full nodes implementing the November 2018 consensus rules SHOULD enforce the following change:

* Update `forkid` [5] 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. Wallets that follow the upgrade should not have to change anything.

## References

[1] Median Time Past is described in [bitcoin.it wiki](https://en.bitcoin.it/wiki/Block_timestamp). It is guaranteed by consensus trules to be monotonically increasing.

[2] https://github.com/bitcoincashorg/bitcoincash.org/blob/master/spec/op_checkdatasig.md

[3] [Leaf-Node weakness in Bitcoin Merkle Tree Design](https://bitslog.wordpress.com/2018/06/09/leaf-node-weakness-in-bitcoin-merkle-tree-design/)

[4] [BIP 62](https://github.com/bitcoin/bips/blob/master/bip-0062.mediawiki)

[5] The `forkId` is defined as per the [replay protected sighash](replay-protected-sighash.md) specification.
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.