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

Implement Cobuild integration and Solana support #3

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

XuJiandong
Copy link
Contributor

@XuJiandong XuJiandong commented Jan 22, 2024

For the detail of Solana support, please see XuJiandong#48

Comments for Cobuild reviewing:

  • The main implementation is in cobuild.h.
  • Otx(open transaction) implementation is included.
  • The words "tested by" in cobuild.c means the branch is covered by corresponding test case.
  • The lazy reader is implemented in cobuild.c. No matter how large the transaction is, the total memory consumption is fixed(~2K for global and ~4K for stack). We have test case for it.
  • The precomputed table for secp256k1 is removed. It can save a lot of memory (~1M).
  • molecule data structure is verified with lazy reader(See verfiy_WitnessLayout)
  • There is message calculation flow in omnilock which doesn't exist in cobuild spec. One extra byte at start of seal is for further use.

migrate PR from: nervosnetwork/ckb-production-scripts#75

including
* eos
* tron
* bitcoin(Support UniSat and OKX wallet)
* dogecoin

Special feature for btc/etc, now they can display meaningful messages.

* BTC(UniSat/OKX)
You're signing:
CKB (Bitcoin Layer-2) transaction: 0x{sighash_all in hex}

* ETH(Metamask)
You're signing:
CKB transaction: 0x{sighash_all in hex}
@XuJiandong XuJiandong changed the title [WIP] Cobuild support Cobuild support Jan 22, 2024
* The main implementation is in cobuild.c.
* The lazy reader is implemented in cobuild.c.

Other changes:
* Add test cases for cobuild
* Update ckb-* to 0.113.0 and rust-toolchain
* Update ckb-c-stdlib
* Add test vectors
c/omni_lock.c Show resolved Hide resolved
XuJiandong and others added 2 commits January 30, 2024 13:22
* support message calculation flow in seal
* Add testcase
* Change to 1st hardfork
Cobuild otx implementation and test cases.

Other major changes:
* Minimize secp256k1 precomputed table
* Support llvm toolchain

Other minor changes:
* Update gcc toolchain
* Update ckb-c-stdlib
* Add rust format rules

---------

Co-authored-by: Mohanson <mohanson@outlook.com>
Co-authored-by: joii2020 <87224197+joii2020@users.noreply.github.com>
@XuJiandong XuJiandong requested review from xxuejie and jjyr and removed request for doitian February 23, 2024 01:47
Copy link
Contributor

@xxuejie xxuejie left a comment

Choose a reason for hiding this comment

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

I haven't finished checking the correctness part of the code.

But the more I look at this code, the more I think we failed to deliver a proper set of API in molecule-c2, and the accompanying Rust API at nervosnetwork/molecule#77.

Personally, I would picture 2 clearly defined, distinct eras of APIs where CKB smart contracts use:

  • Initially, we had a bunch of APIs, like load_input, load_witness, load_cell, load_cell_by_field, etc. And you are expected to be experienced enough to mix the use of them to achieve validation
  • With lazy loader, we should be able to reduce the effort to only a selected few APIs: load_script for loading current script, and load_transaction for accessing anything related to the enclosing transaction.

In the second era, I would expect a smart contract only needs to build a single data source containing the entire transaction, and in 99% of the case, CKB transaction will very small, meaning a single syscall can be used by the data source to load the whole transaction into a memory buffer owned by the data source. Later the smart contract only needs to deal with a single cursor built from the data source containing the full transaction. The smart contract can then slice the cursor into smaller cursors, like one cursor for an input cell, and another cursor for an output cell lock, and yet another cursor for a witness. A cursor representing a witness can be further reassembly into a cursor for a WitnessLayout structure, or a WitnessArgs structure for legacy layout.

To me, that would bring us a much cleaner and a much performant design.

However, in current omnilock, we are still mixing numerous APIs together, littered with different patterns. get_witness_layout and ckb_fetch_otx_start are 2 examples here. We are manually coding against many hidden jargons, making the code harder to read nor maintain. Combining with the fact that molecule-c2 does no verification on the data structure, I must say I wouldn't personally agree with the way this lock is organized now.

Yes there is a chance the lock script might still be correct mostly and secure, but to me this lock is moving into a uncertain direction, and I'm afraid it's gonna bite us one day.

So if I were doing this, I would go back to molecule-c2, if the API is not good enough for usage, we should alter molecule-c2 with better APIs. After that, omnilock should only need to construct a single cursor containing the full CKB transaction, and continue to validate from there.

Makefile Outdated Show resolved Hide resolved
c/dump_secp256k1_data_20210801.c Outdated Show resolved Hide resolved
c/omni_lock.c Outdated Show resolved Hide resolved
c/cobuild.c Outdated Show resolved Hide resolved
c/cobuild.c Outdated Show resolved Hide resolved
@XuJiandong
Copy link
Contributor Author

In the second era, I would expect a smart contract only needs to build a single data source containing the entire transaction

A single data source is not working for following data structure:

  1. cell_deps
  2. header
  3. inputs

@xxuejie
Copy link
Contributor

xxuejie commented Feb 26, 2024

In the second era, I would expect a smart contract only needs to build a single data source containing the entire transaction

A single data source is not working for following data structure:

  1. cell_deps
  2. header
  3. inputs

Correct, those syscalls are just like load_script, which is outside of a single transaction, and should be loaded via designated syscalls.

But that does not void the reasoning here: there are simply too many code pieces in the implementation now to load a part of the data which indeed belong to the enclosing transaction, causing enough confusions and chaos.

joii2020 and others added 3 commits February 27, 2024 09:35
* Remove all "20210801". Include: submodules(ckb-c-stdlib, secp256k1), secp256k1_data
* Remove clang makefile
* Use compiled __builtin_ctzl
* Fix check_since
* Add test cases for since
* Refactor to leverage molecule-c2 as much as possible
@xxuejie
Copy link
Contributor

xxuejie commented Feb 29, 2024

This can of course be a latter task, but I believe we can move cobuild.c / cobuild.h (ideally I would personally just keep a cobuild.h file containing both declaration and implementation for the ease of integration, but that a question of preference) to ckb-c-stdlib for reuse in other contracts.

joii2020 and others added 2 commits March 1, 2024 16:53
Apply verify_WitnessLayout and verify_WitnessArgs on molecule data.

---------

Co-authored-by: xjd <xjd@cryptape.com>
c/basic.mol Outdated Show resolved Hide resolved
Add test cases:
* The witnesses is none
* The output data is big enough(700K)
* Add a 700K witness
* Args is big enough (700K)
* Test case Otx support header_dep
* Add test vectors for cobuild
* Remove deps secp256k1_data on testcase
@homura
Copy link

homura commented Mar 11, 2024

Hi there, it's Lumos. We're planning to integrate with the CoBuild protocol through Omnilock and would like to ask a few questions before proceeding:

  • Is there any documentation or example that demonstrates how Omnilock supports the CoBuild protocol
  • Is the PR stable and unlikely to undergo major changes that could affect the integration process
  • Do you have any other suggestions for SDKs that would be useful in integrating with this PR

@XuJiandong
Copy link
Contributor Author

Hi there, it's Lumos. We're planning to integrate with the CoBuild protocol through Omnilock and would like to ask a few questions before proceeding:

  • Is there any documentation or example that demonstrates how Omnilock supports the CoBuild protocol
  • Is the PR stable and unlikely to undergo major changes that could affect the integration process
  • Do you have any other suggestions for SDKs that would be useful in integrating with this PR

issue 2: yes, it's stable.
issue 3: We have test vectors in this PR. It will be useful.

@EthanYuan
Copy link

Hello, CKB SDK Rust is also planning to support CoBuild , it would be very helpful to have a synchronized PR for updating 0042-omnilock.md.

@@ -0,0 +1,8 @@
import basic;
Copy link

Choose a reason for hiding this comment

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

Contract usually only needs WitnessLayout, I would recommend spliting the schema into two files:

  • witness_layout.mol: Contains WitnessLayout and its dependencies
  • building_packet.mol: This is for offchain, it imports witness_layout and includes the definition for BuildingPacket as well as other dependencies.

c/omni_lock_time_lock.h Show resolved Hide resolved
@homura
Copy link

homura commented Mar 24, 2024

I have a question regarding the script built by make all-via-docker

I remember that the printf instructions used to be ignored or erased by the compiler. However, the compiled binary from this PR still contains the printf instructions. I am not sure if this is the expected behavior.

I received the following output from the ckb-debugger, where the log No otx detected may have been generated from this line:

No otx detected
message(len=12): 0c0000000800000004000000
seal(len=86): 00550000001000000055000000550000004100000016c86c5dfed70f32cf0b62e31517b7b376de7453a4e1637d7d4300f374a6ef7061183fef1f116073e2b235496ca96a2c2eb8b9c03e76cb0b5b88b427bfe0953a01
ckb_generate_smh total hashed 200 bytes
smh(len=32): 55009e27578c58aaf1255a713d525770f0992a9c2e09604fba4221c44fe2dded
cobuild_activated = 1
Run result: 0
Total cycles consumed: 1368092(1.3M)
Transfer cycles: 30437(29.7K), running cycles: 1337655(1.3M)

another possibility is that the printf instruction is not erased, but that the debugger I used before may not set an output printer, whereas the current debugger has one

@XuJiandong
Copy link
Contributor Author

XuJiandong commented Mar 25, 2024

I have a question regarding the script built by make all-via-docker

I remember that the printf instructions used to be ignored or erased by the compiler. However, the compiled binary from this PR still contains the printf instructions. I am not sure if this is the expected behavior.

I received the following output from the ckb-debugger, where the log No otx detected may have been generated from this line:

No otx detected
message(len=12): 0c0000000800000004000000
seal(len=86): 00550000001000000055000000550000004100000016c86c5dfed70f32cf0b62e31517b7b376de7453a4e1637d7d4300f374a6ef7061183fef1f116073e2b235496ca96a2c2eb8b9c03e76cb0b5b88b427bfe0953a01
ckb_generate_smh total hashed 200 bytes
smh(len=32): 55009e27578c58aaf1255a713d525770f0992a9c2e09604fba4221c44fe2dded
cobuild_activated = 1
Run result: 0
Total cycles consumed: 1368092(1.3M)
Transfer cycles: 30437(29.7K), running cycles: 1337655(1.3M)

another possibility is that the printf instruction is not erased, but that the debugger I used before may not set an output printer, whereas the current debugger has one

It is expected behavior. Will be removed when it's ready. It's very important for troubleshooting now.

@homura
Copy link

homura commented Mar 25, 2024

It is expected behavior. Will be removed when it's ready. It's very important for troubleshooting now.

I see, it does helpful for debugging

XuJiandong added a commit to XuJiandong/ckb-transaction-cobuild-poc that referenced this pull request Mar 25, 2024
Reference C implementation:
cryptape/omnilock#3

Changes:

- add lazy reader support
- API changed
- add testing
- add documents. make it ready for publishing
- fix according to updated spec
- add logs
* refactor cobuild into a standalone library
* fix simulator bug on witness
* cleanup comments
* update clang-format docker
@homura
Copy link

homura commented Mar 31, 2024

I noticed that during the scanning process, the contract checks for the presence of a WitnessLayout in witnesses to determine if CoBuild mode is activated. However, the current implementation seems incomplete in scenarios where an NFT can be minted while unlocking a long-term Nervos DAO. Such transactions may look like this:

inputs:
  - input#0:
      lock: omnilock
      type: dao
  - input#1 # mint lock cell
      lock: nft-mint-lock # to check if a Nervos DAO has been locked for a particular period
outputs:
  - output#0:
      lock: omnilock
      type: dao-withdraw
  - output#1 # mint lock cell
  - output#2 # NFT cell
witnesses:
  - witness#0: WitnessArgs # dao deposit header witness
  - witness#1: WitnessLayout.SighashAll # with mint NFT action

* Solana (phantom wallet) draft support
* Check consumed cycles
* Optimize get_witness_layout
* Add macro DISABLE_MESSAGE_CALCULATION_FLOW
@XuJiandong XuJiandong changed the title Cobuild support Implemente Cobuild integration and Solana support Apr 8, 2024
Add checking to generated binary
@XuJiandong
Copy link
Contributor Author

Pudge testnet deployment information:

parameter value
code_hash 0x533cbfca1aba7b6286d07c23292f80b89f690786bc1cc07c96f0b6643849fc5d
hash_type type
tx_hash 0xbf5be776ebc34bf681606a197116eaf5f4b23ada55cc5fc32f7abbda081d0918
index 0x0
dep_type code

* Add exec test case
@XuJiandong XuJiandong changed the title Implemente Cobuild integration and Solana support Implement Cobuild integration and Solana support Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants