-
Notifications
You must be signed in to change notification settings - Fork 155
Add CKB_TX_MESSAGE_ALL specification proposal #446
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
base: master
Are you sure you want to change the base?
Conversation
| Historically, a particular `message` calculation algorithm has been [introduced](https://github.com/nervosnetwork/ckb-system-scripts/blob/934166406fafb33e299f5688a904cadb99b7d518/c/secp256k1_blake160_sighash_all.c#L149-L219) by lock scripts included in CKB's genesis blocks, and used since then. Many other locks from the community have also adopted a similar workflow. However, this workflow has only since existed in part of a script's implementation. It has never been properly documented. On the other hand, certain pitfalls of this very workflow arise as we learn more about coding for CKB's particular environment: | ||
|
|
||
| * While the current workflow assumes the first witness of current executed input cells [script group](https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0022-transaction-structure/0022-transaction-structure.md) is a [WitnessArgs](https://github.com/nervosnetwork/ckb/blob/a6733e6af5bb0da7e34fb99ddf98b03054fa9d4a/util/types/schemas/blockchain.mol#L104-L108) structure serialized in the [molecule](https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0008-serialization/0008-serialization.md) serialization format, this particular assumption is not enforced, and there is code that [exploits](https://github.com/cryptape/quantum-resistant-lock-script/blob/22de5369b60b1e59bb698927c143d9efbe8527a9/c/ckb-sphincsplus-lock.c#L67-L80) this oversight for certain gains. We do believe this can be a problem as future standards arise. | ||
| * The current workflow covers the whole [Transaction](https://github.com/nervosnetwork/rfcs/blob/master/rfcs/0022-transaction-structure/0022-transaction-structure.md) structure, as well as all witnesses from the current script group. However, the `Transaction` structure only contains pointer to all the consumed input cells, it does not cover any contents of the input cells, e.g., the CKBytes store in each input cell, or any input cell's data. This makes it harder to design a proper offline signing protocol. If we dig through the literature, the Bitcoin community actually made the same choice early, but later [came up](https://en.bitcoin.it/wiki/BIP_0143) with an updated design, that signs actual contents of each input UTXOs as well. We do believe a message that covers all input cells' contents can definitely bring merits to future CKB wallets & appliations. |
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.
typo appliations
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.
However, the
Transactionstructure only contains pointer to all the consumed input cells, it does not cover any contents of the input cells, e.g., the CKBytes store in each input cell, or any input cell's data
This part explains what the current transaction hash captures, am i 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.
If yes, with a replace by fee feature enabled, and since transaction hash doesn't capture the CKBytes, can the following happen?
Alice sends Bob 100 ckb using her 500ckb cell, get 400 in change.
Nothing stops Bob to take that same signature and add to a new transaction that says:
Alice sends Bob 400ckb using her 500ckb cell, get 100 in change.
Bob broadcast + prioritize it and get 400, instead of 100?
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 you slightly misunderstand the following lines:
the Transaction structure only contains pointer to all the consumed input cells
Yes a transaction hash does not cover the actual contents of input cells(e.g., CKBytes of each input cell), however transaction hash does cover OutPoint in CellInput structure, which can be viewed as a pointer to an input cell. This pointer, already helps guard against manipulating input cells in a signed CKB transactions:
Your described attacks won't happen due to several reasons:
- First of all,
Alice sends Bob 100 ckborAlice sends Bob 400 ckbwill be represented as an output cell to Bob. This means the new transaction has an output cell changed, the transaction hash naturally changed, the old signature won't work - Even if we fit the output cell different, say a transaction
Alice sends Bob 400 ckb using her 500 ckb cell, get 99 in change, using 1 ckb as fees, then someone(most likely a miner) changes it toAlice sends Bob 400 ckb using her 1000 ckb cell, get 99 in change, using 501 ckb as fees, now all output cells stay the same, so the problem described in the first bullet point won't happen. However, we still need to perform the attack, by swaping one input cells to a different one, 2 solutions exist:
** We could simply change oneOutPointin one ofCellInputstructure from the transaction to point to a new cell which holds 1000 ckb from Alice, however, this means oneCellInputstructure is changed in the old transaction, the transaction hash in the new transaction changes as well. The signature validating flow changes
** If we cannot change anything from the signed transaction, the question now is shifted to: can we increase the CKBytes stored in a cell on-chain, while also keeping theOutPointused to reference this on-chain cell the same? To me this is an even harder task, I don't have a way to make it happen now, let me know if you believe such attacks exist.
To summarize here, I don't personally believe the old, current way of signing transactions in CKB has any weak points so one can manipulate a transaction after it is signed. However, being secure on chain is not enough sometimes, CIGHASH_ALL enhances the workflow by signing contents from input cells as well, simply to make offline signing easier.
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.
thank you for clarifying. You're right i misunderstood the line
will be represented as an output cell to Bob. This means the new transaction has an output cell changed, the transaction hash naturally changed
This should clear things up.
|
@XuJiandong All typos have been fixed |
|
|
||
| As a result, this document aims to propose `CIGHASH_ALL`, a properly defined message calculation scheme used by CKB lock scripts to ensure transactions are not malleable. | ||
|
|
||
| The name `CIGHASH_ALL` comes from `CKB's Signature Hash All`. In many places, including [filename](https://github.com/nervosnetwork/ckb-system-scripts/blob/master/c/secp256k1_blake160_sighash_all.c) from CKB's system script code, `SIGHASH_ALL` has been used to represent the old workflow to calculate a signing message. Here we explicitly pick a different name, so as to distinguish between the two. |
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 choosing a better name. CIGHASH_ALL can easily cause confusion, as it sounds the same as SIGHASH_ALL and a single char typo could turn it into SIGHASH_ALL.
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 other names I can think of is CKB_HASH_ALL, CKB_SIGHASH_ALL, CKB_MSGHASH_ALL, or any other suggestions are welcome.
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.
Nervos's sighash_all uses an algorithm that aligns with Bitcoin's pre-SegWit SIGHASH, so inputs metadata is not included in the message to sign.
Bitcoin tackled this in BIP143: https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki
Maybe we can directly or indirectly reference BIP143/SegWit in the name 🤔
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 honest, I personally don't care about what the name is, I just care about the fact that a commonly-agreed name is decided here. So I will leave this comment as it is, and will always be happy to modify the name to whatever is chosen.
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.
After some thoughts, I suggest the name CKB_SIGHASH_ALL. I will leave this name here for a few days, if no further questions arise, I will make the actual changes here.
EDIT: personally, I feel it necessary to acknowledge BIP143 in the RFC spec definition, but I think it is not appropriate to include BIP143 in the name of the new spec.
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 combine the above comments, I would suggest a different name here: CKB_TX_MESSAGE_ALL, it consists of 3 parts:
- The
CKB_TX_prefix works as a namespace denoting that we are building a specification for CKB transactions. Let's be nice to the whole blockchain community, to avoid a name that is too general MESSAGEhas 2 advantages: first, it helps avoid confusion withSIGHASH; second, if you think about the above defined specification and the reference implementation, we are not really designing a hash here, we are designing a spec which is a series of concatenated bytes, you can keep the bytes as it is, or you can feed them into a hash. So what we have here, really is a message, not necessarily a hash.- The final suffix defines the range of the transaction to include in the message. Personally, I think
allandfullare both fine but semantically speaking I fellallis slightly better when I checked the dictionary. But I'm not a native English speaker, @Matt-RUN-CKB @jordanmack care to weigh in here?
So my current suggestion would be CKB_TX_MESSAGE_ALL. Like the previous one, I'm gonna keep it here for a few days, and will actually make the change later if no further comments are received.
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.
Even CKB_MESSAGE_FULL sounds good 👍 (I'm not a native English speaker tho)
About that TX, what's the reason for its inclusion? Can you foresee a possible future where messages are created from non-TX entities?
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.
About that TX, what's the reason for its inclusion? Can you foresee a possible future where messages are created from non-TX entities?
CKB is more than just transactions and scripting part, there might be also messages in other parts, such as p2p layers and others. Adding TX as part of suffix make it more precise and future proof.
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 we haven't heard more on the suggestions, I've renamed the specification from CIGHASH_ALL to CKB_TX_MESSAGE_ALL
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.
Makes sense, for sure it's an improvement over CIGHASH_ALL 👍
|
CKB_SIGHASH143 in homage to the king |
|
Congratulations @xxuejie on the proposal!! I fully support the idea 🔥 Are we able to tackle also another problem, possibly within this proposal? (if makes sense) During the internal review iCKB was rightfully slapped with Malleability of unsigned lock witnesses. This is not really an iCKB problem, but a The underlying issue is that in As more and more protocols start using non-signature-based locks and also start using the witness to store data, this issue is gonna become more and more evident. Are we able to do something about the malleability of unsigned lock witnesses with this proposal? Love & Peace, Phroi |
|
PS: maybe it's already fixed in its current proposal specification, I still have to study the implementation details. I just want to be sure that the malleability of unsigned lock witnesses is fully considered before this proposal moves to the next steps 🙏 |
I personally question the necessity of non-signature based locks at all. If I were doing the design, I would use types solely for non-signature based locks. In addition, this design is specifically addressed to locks that do require a signature, and want the signature to guard enough entities. A separate design will be needed, even if we want to have non-signature based locks. |
This is indeed the best-practice, but sometimes I cannot see a way to apply it. Please, let's continue this topic on Nervos Talk!! 🤗
while keeping all the rest as unchanged as possible: backward compatibility at its finest, gotcha!! I don't know, it feels a bit like a missed opportunity for laying the bases for future work, so for forward compatibility. Well, if anybody else feels the same, feel free to pitch-in! Love & Peace, Phroi |
In certain scenarios, the length of the lock field, might only be decided after the signing. Assuming a multisig setup where different parties employ different signature validation algorithms(which is totally possible on CKB), it will only when we have gathered all the signatures, will we know the final length of the lock field, and the total witness length.
|
Note the specification here is slightly modified: in a previous version, the first 16 bytes of the first witness from the running script group were included in the signing message. However, in certain setups, the length of the lock field of this witness, and the length of the total witness as well cannot be decided beforehand, and could only be calculated after signatures have been gathered. One such case is a multisig setup where each parties use a different signature verification algorithm(by design this is totally doable on CKB), one party using secp256k1 uses 65 bytes of signatures, while another party using SPHINC+ might use signature of certain kilobytes. In a voting process represented via a multisig setup, it is unclear who will generate a signature, thus we cannot beforehand infer the length of signatures we will gathered. To cope with this scenario, the specification has been adjusted to only sign the length of |
| * The first witness of current running script group must be a valid [WitnessArgs](https://github.com/nervosnetwork/ckb/blob/81a1b9a1491edca0bc42c12d8bf0f715a055a93f/util/gen-types/schemas/blockchain.mol#L114-L118) structure serialized in the molecule serialization format. This has now become an enforced rule, it is not an assumption that can be exploited or ignored. | ||
| * The content of all input cells are covered by the message calculation workflow, making it much easier to design an offline signing scheme. | ||
| * Witness length is packed in 32-bit unsigned integers, while 64-bit unsigned integers were used in older workflow. Notice that all CKB data structures, including `CellOutput`, cell data, witness, etc., will first be serialized in molecule serialization format. Note molecule uses 32-bit integer to denote the length of a structure, this means that we will never have a `CellOutput` / cell data / witness structure that is bigger than 4GB, and there is no point in representing the length in 64-bit integers. | ||
| * A different concatenation/hashing design is introduced for the first witness of the current script group, discarding the original zero-filled design. We believe this new solution can contribute to a more optimized implementation, both in terms of runtime cycles and binary 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.
This sounds weird to me since:
-
Bad compatibility: Considering our attempt on CoBuild, having a different witness layout seems possible in the future. This enforcement brings a serious problem to compatibility.
-
Confusion of abstraction levels: For me, the zero-filled design comes with the algorithm instead of the message specification. Almost all signing algorithms in the past used
SIGHASH_ALL, but they filled thelockwith different dummy values (65 0s for Secp256k1, 85 0s for EVM, 572 0s for Nostr), and of course, we can leave thelockempty for performance.
So ifCKB_TX_MESSAGE_ALLis at the same abstraction level asSIGHASH_ALL, I don't think it should force the algorithm to put what dummy value here (even more, if the algorithm wanted to put some meaningful value to thelockto aid validation, it would not be able to do so.) -
Uncertain optimization: Check a possible implementation. If a program wants to process this logic in a relatively readable manner, it may introduce additional decoding work when computing the message. Even if we merge this decoding step with the decoding necessary for signature verification, I don't think it will be a performance optimization compared with the original method of calculating the message without decoding. Notice that replacing the zero-fill design with an always-empty design already leads to substantial performance gains.
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 fully agree on the bad [future] compatibility of the proposed witness layout, important things that are not accounted for:
(weird but good for brain-storming) create a type/type script composing standard, e.g. abandon lock script completely, use type script for everything, every cell uses a 'meta' type script which can read cell data to determine what script/logic should be invoked and invoke them in turn.
- Non-signature based locks protocols such as the protocol proposed by @janx (which would use
always-successfullocks) or the ones implemented in iCKB.
While I really appreciate the inclusion of input cell metadata in the message, it also seems like a missed opportunity for an improved witness layout.
Love & Peace, Phroi
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 there is some misunderstanding here.
First of all, CKB_TX_MESSAGE_ALL DOES NOT introduce any incompatible changes whatsoever. It it compatible with all existing locks & types, it uses WitnessArgs structure exactly in the same way as all lock scripts I know of today, it also is perfectly compatible with cobuild. There are no breaking changes. In fact, I don't think I can move forward with any new CKB design that is not compatible with existing CKB designs, a powerful killer app might be able to do so, but I just can't.
What this paragraph describes, is simply a chicken-egg problem: the first witness in the script group of a lock script by convention is a WitnessArgs structure, the lock field of this WitnessArgs, is a signature. However, prior to generating the signature, we need to either skip the contents in this lock field, or use zeros to fill the space occupied by the signature.
We could definitely debate which is the majority design choice, but historically there are certain lock scripts which will simply skip the lock field, an example can be found here. There was a time where Rust based lock script favors this workflow due to poorly-designed public APIs in the molecule libraries.
On the other hand, even if we stick to a zero-filled design, it is not always as simple as "fill everything in lock field as zeros", lock field can be variable in length(making it harder to do the filling in Rust using constant memory consumption). In addition, lock field can contain more than signatures, what if a lock field contains multisig paramters or other user request parameters for lock script? In this sense, the lock field might mix signatures with other parameter values together. It might be quite troublesome to pinpoint the exact bytes for all the signatures, and zero-fill them instead. I personally consider the following 2 as 2 separate tasks:
- Generating a signing message from a single CKB transaction
- Validate a signature given a signing message
With spawn deployed in mainnet, I expect there are lock scripts(and potentially type scripts) which simply generates a signing message, and relies on other scripts on chain for the actual signature validation. Assuming a zero-filled design, there might be leaky abstraction, where a signing message algorithm must know signature formats to know what exact bytes it should zero-fill in the lock field.
Given all those past concerns, I'm proposing a different solution here: instead of zero-filling signatures, CKB_TX_MESSAGE_ALL simply do not sign lock field at all, it leaves the task to a lock script to fully validate the data in lock field, whether it is a plain signature, or contains more user request parameters.
Again, the changed solution only applies to how a signature is calculated. It DOES NOT change witness layout at all. We continue to build WitnessArgs structure in the first witness of a script group, a signature continues to live in the lock field of the WitnessArgs structure. This new design is indeed compatible with cobuild and all existing designs I personally know of. CKB_TX_MESSAGE_ALL simply states the following requirements in the specification level:
- The first witness in the current script group must be a valid WitnessArgs structure.
- The
input_typefield of the first witness in the current script group will be signed in the signature - The
output_typefield of the first witness in the current script group will be signed in the signature
The lock field will simply be ignored by the signature calculation process. However all data will continue to be placed in the position where they were placed before. No incompatible changes are introduced.
Finally, let's talk performance.
I must say that personally, I consider the performance of on-chain CKB scripts to be of more importance than off-chain servers. On-chain CKB scripts are running in a much more constraint place,and the performance metrics of those scripts are directly connected to fees paid by each CKB transaction. On the other hand it is a much simpler task to scale off-chain servers, people have been saying for almost 20 years that developers cost more than machines, when it is possible it makes more sense just to add more machines.
I would argue that the new design here, where lock field is ignored rather than zero-filled, is more performant in terms of on-chain CKB scripts. Ignoring lock field leads to super optimal on-chain script implementation, right now a C implementation does this, assuming molecule's Rust implementation introduces lazy reader based validator(which I assume is really a general feature request), we could do the exact same optimal implementation in Rust code as well.
For zero-filled implementation, I can only think of 2 possible implementations:
- We read the full witness into a CKB script's memory space, and zero fill lock field stored in memory. However, this solution could run into issues very quickly. For example, a buggy implementation can be found here, this code assumes that an entire witness could fit in a CKB script's heap space. Theoretically, a witness can be almost as large as a CKB block, which will be ~600KB, however, ckb-std in current implementation, does not allocate enough heap space for such big a block. We can certainly debate the likelyhood, but there will be cases where current CKB script cannot handle a CKB transaction which should run perfectly fine. Even such code is fixed, reading full witness all at once can waste quite a lot of memory in a precious on-chain memory space. I doubt if this is a good idea.
- We use a lazy reader based solution, we also divide the full lock field in chunks, we load each chunk at a time, and do zero-filling as requested. However, I have yet to see one implementation which is willing to go through all those troubles.
Given those thoughts, I do consider current design in this proposal an optimization in on-chain scripts. As for off-chain servers, I'm willing to bet that decoding one more WitnessArgs when signing a transaction does not affect performance in any noticeable ways.
And just to recap: I do believe the newly introduced design here tackles 3 potential issues:
- Different implementation workflows (some ignore lock field, some do zero-filling) due to missing specification
- A leaky abstraction would be introduced since signing message calculation algorithms might need to learn about signature formats
- Zero-filling has performance concerns for on-chain scripts.
In the meantime, no breaking change is introduced, the new design is perfectly compatible will all existing proposals.
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.
Thank you so much for the explanation, now I understand better about the sentence We believe this new solution can contribute to a more optimized implementation, both in terms of runtime cycles and binary size.. Also, the spawn example is a good example of wanting to compute a message without knowing the signature algorithm, which makes everything more reasonable.
Let me sort this out - we actually have three ways of message hashing:
- SIGHASH_ALL + (Usually)
WitnessArgs.lockZero-Fill - This method cannot correctly handle dynamic length signatures. - CoBuild's
SighashAll- I think it's more likeCoBuild.TxMessageAll, since it also requires all input info. - TX_MESSAGE_ALL - A minor upgrade to SIGHASH_ALL.
Here, both 1 and 3 are designed specifically for WitnessArgs. If we want a CoBuild compatible lock script, we should just use 2.
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.
Yep, I think you understand them correctly, tho I do want to mention that for 1, we actually have a series of slightly-different-but-generally-the-same workflow, not just one. Some might not do zero-fill(such as the example I mentioned earlier).
I was hoping that in the future we can all converge on 2 and 3, and gradually deprecate 1.
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 comment makes everything clear, so much so that you may consider providing this context in the RFC Rationale Section.
The vibe I get is that gently pushing the ecosystem from SIGHASH_ALL to CKB_TX_MESSAGE_ALL may also very well be what enables CoBuild adoption, thanks to CCC likely support for both and that, once you start making changes in your codebase, you are open to make more.
I just wonder if it is possible to streamline more 2 and 3.
Love & Peace, Phroi
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.
Personally, I think the adoption of CKB_TX_MESSAGE_ALL is much easier than CoBuild, since the scope is much smaller and easier for integration. In addition, it touches almost no UX part at all, in most cases it's just developers switching to a new set of APIs, which can be quickly picked up.
CoBuild, on the other hand, is a heavy component that requires all parties to participate. And it really is unclear when / if there will be enough adoption in current CKB community.
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 are right!! That said, I also forecast that the synergy between CoBuild, SSRI and spawn will drive adoption 🤗
|
Question: If we think data behind inputs should be hashed, should we also hash data behind cell deps and header deps? e.g., The reward from Nervos DAO might be affected by the header deps. |
I don't have an answer, it might make sense for header deps. But cell deps might cover actual code(secp256k1 in genesis also has that 1 MB multiplication table), it could potential be a huge chunk of data to hash. |
Yes, cell deps almost always have some code inside, not to mention we also have to deal with dep groups. Meanwhile, cell inputs are usually lighter but may still contain a lot of data at times. Actually, I might doubt if hashing everything directly before signing makes sense. For me, those data is already specified by hashes e.g., |
|
For input cells, the original requirement comes from offline signing, such as a hardware wallet: those devices aren't powerful enough, or lack the Internet access to run a full CKB node. So when the signature only covers One alternative solution is that for each input cell you can include the full transaction denoted by And now we end up at a tradeoff where contents in input cells are signed in signatures, this way a small hardware can verify the stored CKBytes / UDTs in input cells, and signatures guarentee that malicious inputs will not have a chance to land on chain. I do agree that there are other solutions to the problem, everyone will definitely have their likes, I just want to explain the path that ends up to current design in this RFC. |
I see. I do think it's acceptable to only hash data behind
I'm curious why they don't like this. Or if they don't like this, will they like the current design? They still need extra code/way to pass extra data besides the |
|
Relaying an opinion from Cryptape internals: recall that the spec now do sign And the differentiator is: headers are small and constant in size. To verify input & dep cells in the same way, full transactions are required, which can be quite large, making it hard to verify on smaller devices. So for now it is proposed that only input cells are signed in current spec. |
PREVIEW LINK