-
Notifications
You must be signed in to change notification settings - Fork 6
Refactor the whole lock script #14
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
Conversation
Several noticeable points: * Change the directory structure to mimic one generated from ckb-script-templates * Adjust CFLAGS & LDFLAGS to properly minimize scripts * Remove unneeded global variables in C * Rearchitect the script so root script does transaction valdiation, and leaf scripts do SPHINCS+ signature verification * Change signing message to CKB_TX_MESSAGE_ALL spec * Adjust blake2b personalizations
Note this is still lacking tests(new tests shall be added, old tests shall be adjusted), however the code structure and the docs covering usage are mostly complete, so I'm sending it out for review early |
CKB fits detached message mode of SPHINCS+, since the signing messages will be calculated based on enclosing CKB transactions.
As of now, I've adjusted the code structure to IMHO a more proper state, all the tests & tools have been fixed to be usable on the updated structure. I still want to add more tests(e.g., multisig testing, cross validationg from another signing library), but as the change is already big enough, I consider this change here to be ready for review. I will use another PR to submit more tests |
Is there any guide about how to review the C code? All the files are renamed and moved. |
|
* Add Rust based script following the same behavior as the C script, but use fips205 crate to implement NIST FIPS205 * Add a new crate for signing/verifying in CKB's context * Add positive(meaning valid signatures are used) validation tests where signatures are generated by fips205 crate, and then verified by both C and Rust scripts
For on-chain script this change has no effects(actually, one optimization due to one less memcpy), it prepares us to run NIST test vectors in CKB-VM
Hey @xxuejie, I was wondering, have you considered adding an UDT-compatible ACP variant? |
Unfortunately with current state of toolings for CKB, ACP to me is a lost battle. In a different universe where people are used that one identity could map to multiple addresses on CKB, ACP might be acceptable, but for current state of the art, I don't think ACP will be a thing. |
Appreciated the fast reply!! Agreed, it creates a user friction which in general is not handled well enough by wallets. That said, in most applications of ACP are not user facing, so it may still find some usage in other contexts where the cell is operated by a professional team, like for example to receive UDT payments from users. On this note, I'm considering developing an ACP-alike, compatible with the Quantum Resistant Lock, by leveraging the ability to In this regard, I just noticed the latest commit: Fix script locating bug when called via spawn / exec as a callee. In my opinion the adopted solution may create future composability issues. Imagine that there are three scripts spawning each other under determinate conditions:
A may indirectly spawn C, so with the proposed solution A needs to pass down to B the index of C in the My questions are:
Love & Peace, Phroi %27 |
PS: I woke up in the middle of the night with a nightmare about the security implications of this design choice!! 🤣 For starters, I understand that C ( Clarified this, I'd like to examine together the security implications passing the index of CellDeps:
This can happen either:
While external reviewers would point out that the chain of trust was broken outside Quantum Resistant Lock code, from my perspective is this QRL implementation detail allowing for such an exploit. A different design would not suffer from such an attack. I'm not sure about the assumptions on the deployment of QRL, so I assume the least flexible: Cryptape will choose to deploy immutably and reference both C In this scenario, you can still hard-code inside A solution to this issue that I found in dCKB Source code was to deploy the code immutably and reference it by It would work in this way:
This way the code deployment itself is immutable, but at the same time it allows for flexibility in the algorithm and the previously proposed attack vector is not applicable. Alternatively, you could directly embed a know cell Only downside I can see is that these cells cannot be embedded inside DepGroups, but it doesn't change much from the transaction size perspective as QRL transactions have already a massive witness. @xxuejie what's your take on this? Can you point out additional downsides? Love & Peace, Phroi %91 |
Just to provide some more background on latest fixes in xxuejie@27041c7. This is uncovered during our QA process: the quantum lock is actually a combination of multiple scripts together: there is a root script C and a set of leaf scripts D. So when C figures out the parameter set to use for current address / identity, C will use spawn or exec to call D dynamically. What's unique about the quantum lock, is that C and D are actually combined together in a single deployed script, stored in a single dep cell. So ideally, C can just use exec / spawn to call itself with different But there is a catch: in current CKB syscall design, there is no way for a spawn / exec callee script to figure out the cell dep containing itself. It is a neglect of CKB syscall design, in the next hardfork we might want to fix it. But for the moment, we will have to deal with what we have, and the simplest solution I have here, is that caller must pass the cell dep index of callee script via It might indeed cause complications: assuming a script B will dynamically use spawn / exec to call quantum lock script C, and possibly other lock scripts for different types of signature verifications. For quantum lock script C, B will need to pass cell dep index, but another lock script might not need such an index. It is not perfect, but I would bet this is something that can be dealt with using proper abstractions. In fact I do believe most script calling logic in B can and should be captured with proper abstractions. The abstraction layer can perfectly handle differences in scripts. In fact I do believe different kinds of quirks might exist for other scripts at all. One painful lesson I learned, is that CKB is far from a state where common protocols & conventions can be built, it's likely we will have to deal with those quirks for quite a while. Given those thought, personally I feel fine to introduce cell dep index to quantum lock. Different people might have different opinions of course, for me, I'm not so worried about the security of the introduced cell dep index. One observation first: I don't believe that any spawn / exec callee script alone can determine the security of the whole lifecycle of CKB script executions. In the presence of spawn / exec syscalls, the security of the whole script lifecycle, will always be the intersection of all scripts executed following spawn / exec chains. Assuming script A spawns B, B execs C, it will never be enough when we say C is secure, but we don't know about A and B. The full chain will only be secure when all of A and B and C are secure enough. When a script B calls quantum lock C via spawn / exec. It might happen that B uses an incorrect cell dep index. I personally consider this to be a fault of B, not a fault of C. C can only deals with the information it gets. Assuming B could also call another lock E, and possibly passes the public key hash required to E. What if B swaps the public key hash to a fault one or worse, a malicious one? Do we consider E to be faulty? No, I would personally consider B to be faulty. In this sense, I'm personally consider current implementation of quantum lock to be secure enough. Only given the correct cell dep index, will the quantum lock C be able to locate the correct leaf quantum script D. To me this requirement makes sense. That being said, I learned enough lessons that people might just come from different backgrounds holding different opinions. The current version of quantum resistant lock, is really about building a safe solution covering the most basic cases where people want to simply HODL their tokens for decades. I'm not so keen on introducing more features to this particular quantum lock. We might well have other quantum locks in the future with more features, so if a consensus can be reached that being the callee of spawn / exec syscall is not required, we can just removed the added feature about cell dep index. I personally don't think cell dep index introduces any implications whatsoever, but really I'm fine either ways, as long as an agreement on the feature set can be reached. |
Hey @xxuejie, I again appreciate the fast reply 🙏
I didn't realize this detail, this is so cool!! I always wondered if such a design was achievable! I even found right away the tool you created for this purpose and it's even mentioned in the docs 👍👍
That indeed is quite a catch. The solution you adopted is simple and, who knows, could be widely adopted as a meta-protocol on top of spawn/exec. But, what about an even simpler solution?
With this solution, QRL to find its own CellDep OutPoint just make a linear search over CellDeps, comparing cells type. Usually the lock CellDep is added as last one, so an optimization would be to iterate CellDeps in reverse order. @xxuejie what's your take on this? Love & Peace, Phroi %27 |
Nope, if you ask me, I don't consider that to be simpler. |
Appreciated the fast and honest reply! Just one last question: Why? |
I don't know how to explain that. One solution simply adds a new parameter to a place that is supposed to hold parameters. I guess different people just have different ideas on what is considered to be simple. |
I see I see, so we are weighting easy to deploy against easy to integrate. Let's make a hypothetical real world usage example. In February @janx proposed the following meta-protocol to address the fact that output locks do not validate:
Which to me boils down to: if an output cell has a lock which do not appear as type nor input lock, spawn it. Now, let's assume that every upcoming lock script adopts the solution adopted here: how can every type script know which is the right CellDep index for every possible output lock script being spawned? Conversely, with the proposed solution, this standard boils down to:
Love & Peace, Phroi %26 |
I'm not gonna debating on which one is a more favorable one. It's just comparing apples with oranges. These days I tend to avoid such discussions. I just want to commment on one thing:
This is actually extremely easy. When you are using raw exec / spawn syscalls:
You already have the cell dep index. However, most typically, people would use the higher level ones:
In this case only In fact if you look at the actual implementation, both I'm pulling out Rust code as examples here, but a similar design could also easily be implemented in C and in TypeScript. This is really no big deal. |
I don't know. With this it feels like we are gonna usher in a era of middleware. To spawn a third party script, you first spawn a middleware script which (given a specific target script) knows the needed args and how to format them. Even if we have different viewpoints, I appreciate the discussion! Love & Peace, Phroi %5 |
ping @chenyukang The background is explained here. Right now I see 3 options:
|
Hey @phroi After deploying the quantum lock, you can still create a lightweight wrapper script to invoke it via the type id, then lock A → wrapper script, without needing to pass the |
Thank for letting me know, appreciated!! A couple more questions to understand how to develop this wrapper:
Phroi |
Assuming quantum resistant lock is deployed in cell A, you can have a cell E which is a dep group aggregating cell A, cell B, cell C, cell D together. You can only add cell E as a cell dep to your transaction, this is perfectly fine. That being said, if a use case requires calling into the quantum resistant lock via spawn / exec syscalls. The caller script must use |
We're going to deploy it in TypeId way. |
Hey @chenyukang, thank you for letting me know!! 🤗 Since you are gonna deploy by TypeID, then I would also suggest to use Multi-Sig QRL for the lock on the TypeID QRL binary. Let's assume you use a non Quantum Resistant Multi-Sig on the TypeID QRL binary, then an attacker armed with a Quantum Computer can bypass QRL on an Asset easily:
Love & Peace, Phroi |
@phroi Seems impossible? CKB doesn't have such Multi-Sig QRL in existence before the QRL in this repo. What I'm perceiving from this move is that Cryptape will use the legacy SECP256K1/blake160 Multisig to lock the typeID cell then move it to a dead lock (no one can spend even a QC) after some time? |
Remember that this is not safe from a CKB-VM perspective: references by type always run on the latest VM and compatibility between VM versions is not assured:
The trick is that a Cell can be used as both Input and as CellDeps in a valid transaction. With that in mind, we have at least three ways to achieve this:
As a reviewer, 1, 2 and 3 have the same result with one small exception: 3. is not Quantum Resistant until the lock of the binary is updated to be Quantum Resistant itself Love & Peace, Phroi %42 |
yes, you're right. |
Thanks for sharing your thoughts! @phroi We will set the lock to all zeros after the code for Quantum Resistant is stabilized (we expect it to be reached before a Quantum Computer comes out). As for your concern, in theory, secp256k1 remains secure against quantum computers as long as the public keys are not reused (i.e., not exposed). We may leak public keys when upgrading the lock and consuming the old cells, so if we want to upgrade the binary, we will replace all previously used public keys in the multi-sig with new ones. This practice will keeps the system secure. The current deployment strategy isn’t perfect, it represents a balanced trade-off under the present conditions. |
Compatibility issues may only be introduced when we find a bug in an old version and fix it in newer version. We will try our best not to break anything unintentionally. In practice, we will rerun all the old transactions to ensure the new VM's behavior is as expected. |
Please, make sure to inform the multi-sig members of this crucial detail.
Pretty different wording from the latest mention of this detail in a RFC:
I will make some PRs to the VM1 and VM2 RFCs with this additional wording, let's see Love & Peace, Phroi %23 |
Several noticeable points: