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

feat: use recoverable signature to reduce tx size #15

Merged
merged 3 commits into from
Jun 14, 2019

Conversation

jjyr
Copy link
Contributor

@jjyr jjyr commented Jun 5, 2019

BREAKING CHANGE: witness data is changed

This PR replace normal signature with recoverable signature. So instead of reading pubkey from witness, the contract recovers a pubkey from the signature, this change can reduce 33 bytes tx size for each witness.

However, due to the additional recover signature operation, the total cycles increased from 2660252 to 2739437 for a typical 2-in-2-out tx.

The secp256k1_blake160_sighash_all binary size increased from 1190704 bytes to 1193224 bytes.

I think it worth the change, Please leave your opinion in comments.

Changes:

  1. The original witness data [<pubkey>, <sig>] changed to [<sig>].
  2. The sig must be signed with secp256k1_ecdsa_sign_recoverable method.

BREAKING CHANGE: witness data is changed
@nervos-bot nervos-bot bot added the breaking change The feature breaks consensus, database, message schema or RPC interface. label Jun 5, 2019
@jjyr jjyr requested a review from a team June 5, 2019 15:39
@janx
Copy link
Member

janx commented Jun 5, 2019

Edit: After a second thought, this change won't affect security as we still use pubkey hash as address and only reveal pubkey on spending.

This change affects security.

https://bitcoin.stackexchange.com/questions/3600/why-are-bitcoin-addresses-hashes-of-public-keys

@doitian
Copy link
Member

doitian commented Jun 5, 2019

@ashchan please assign issues to SDKs.

@doitian
Copy link
Member

doitian commented Jun 5, 2019

Can we add test here? It's insane that we don't have a test on such important code.

cc @xxuejie

@jjyr jjyr requested a review from a team June 10, 2019 03:54
@doitian
Copy link
Member

doitian commented Jun 10, 2019

I reject adding the binary into repo. Ask developers to build them. Build them before test in CI.

@xxuejie
Copy link
Collaborator

xxuejie commented Jun 11, 2019

See here for a way to build the scripts.

@jjyr jjyr force-pushed the try-sign-with-recoverable branch from 77049ab to d6d811c Compare June 11, 2019 03:01
@jjyr
Copy link
Contributor Author

jjyr commented Jun 11, 2019

Updated, use make ci to compile and run tests.

@doitian
Copy link
Member

doitian commented Jun 11, 2019

Could you setup Travis to run the tests?

@jjyr jjyr force-pushed the try-sign-with-recoverable branch 5 times, most recently from 09ed346 to afbfc2d Compare June 11, 2019 08:06
@doitian
Copy link
Member

doitian commented Jun 11, 2019

As @quake suggested, we can port the existing test suites for secp256k1 recoverable signature. But it does not block this issue, we can start a new PR for this.

@jjyr jjyr force-pushed the try-sign-with-recoverable branch from afbfc2d to f889685 Compare June 12, 2019 03:05
@jjyr jjyr requested a review from xxuejie June 12, 2019 03:06
language: rust
rust: 1.34.2
dist: xenial
sudo: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need sudo here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, I just copy the .travis.yml template from CKB

@xxuejie xxuejie merged commit bb76263 into nervosnetwork:master Jun 14, 2019
@jjyr jjyr deleted the try-sign-with-recoverable branch June 14, 2019 02:59
jjyr added a commit to jjyr/ckb that referenced this pull request Jun 14, 2019
By using secp256k1 recoverable signing, we can significantly reduce tx size by removing the pubkey from tx witness. The total cycles for verification increased from 2660252 to 2739437 for a typical 2-in-2-out tx. discussion is on nervosnetwork/ckb-system-scripts#15

The default secp256k1_blake160_sighash_all lock script is compiled
from the following source:

https://github.com/nervosnetwork/ckb-system-scripts/blob/517e667a3ce496b58ba948e22b34d1e5c9e79710/c/secp256k1_blake160_sighash_all.c

The exact gcc version used in the compilation can be located in the
following docker image:

xxuejie/riscv-gnu-toolchain-rv64imac: 20190606
driftluo pushed a commit to driftluo/ckb that referenced this pull request Jun 18, 2019
By using secp256k1 recoverable signing, we can significantly reduce tx size by removing the pubkey from tx witness. The total cycles for verification increased from 2660252 to 2739437 for a typical 2-in-2-out tx. discussion is on nervosnetwork/ckb-system-scripts#15

The default secp256k1_blake160_sighash_all lock script is compiled
from the following source:

https://github.com/nervosnetwork/ckb-system-scripts/blob/517e667a3ce496b58ba948e22b34d1e5c9e79710/c/secp256k1_blake160_sighash_all.c

The exact gcc version used in the compilation can be located in the
following docker image:

xxuejie/riscv-gnu-toolchain-rv64imac: 20190606
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change The feature breaks consensus, database, message schema or RPC interface.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants