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

rfc(0039): Add cheque lock rfc #308

Merged
merged 37 commits into from
Sep 30, 2022
Merged

Conversation

duanyytop
Copy link
Contributor

No description provided.

@duanyytop duanyytop requested a review from a team as a code owner January 27, 2022 06:57
@janx janx changed the title Add cheque lock rfc Assigned RFC number 0038: Add cheque lock rfc Feb 8, 2022
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
rfcs/0038-cheque/0038-cheque.md Outdated Show resolved Hide resolved
duanyytop and others added 20 commits February 9, 2022 13:44
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
Co-authored-by: busyforking <5958+janx@users.noreply.github.com>
@jordanmack
Copy link
Contributor

I think the CI configure is enough and @jordanmack how do you feel?

Yes, that is adequate. Just add a line that links to this and describes what it is.

@duanyytop
Copy link
Contributor Author

I think the CI configure is enough and @jordanmack how do you feel?

Yes, that is adequate. Just add a line that links to this and describes what it is.

#308

XuJiandong
XuJiandong previously approved these changes Sep 14, 2022
janx
janx previously approved these changes Sep 14, 2022
@jordanmack
Copy link
Contributor

@duanyytop I will be submitting a pull request with some additional suggestions for readability, but first I have some questions.

1.b.i. It loops through all input cells using the current cheque lock script, if any of the inputs' since is not zero, the cheque lock returns with an error state.

Unlock Rules 1.b.i will error if the receiver is valid and a since was used on one of the inputs. It seems like the transaction would still work if a since was included. What is the reasoning for disallowing the since here?

2.b. If the matching input cells with the receiver lock hash are found, it performs the same check as in 1.b.i and 1.b.ii.
2.b.i. It loops through all input cells using the receiver lock hash, if the matching inputs are not found, the cheque lock returns with an error state

Unlock Rules 2.b checks if any input cells are using the receiver lock and then performs validations from steps 1.b.i and 1.b.ii. Then in step 2.b.i it loops through these same input cells using the receiver lock, and checks for what? It sounds like it checks for the receiver lock cells, then loops through these cells to check if it has the receiver lock again. I do not understand this step.

2.b.ii It loops through all input cells using the receiver lock hash, if the first witness of the cheque cell group is empty(the witness is not WitnessArgs or the lock of WitnessArgs is empty)

Unlock Rules 2.b.ii appears to be incomplete. I do not understand this step.

@duanyytop
Copy link
Contributor Author

Unlock Rules 1.b.i will error if the receiver is valid and a since was used on one of the inputs. It seems like the transaction would still work if a since was included. What is the reasoning for disallowing the since here?

For the receiver, when since is equal to 0, he/she can claim the asset without waiting for a period of time.

Unlock Rules 2.b checks if any input cells are using the receiver lock and then performs validations from steps 1.b.i and 1.b.ii. Then in step 2.b.i it loops through these same input cells using the receiver lock, and checks for what? It sounds like it checks for the receiver lock cells, then loops through these cells to check if it has the receiver lock again. I do not understand this step.

This rule does seem redundant and can be deleted.

Unlock Rules 2.b.ii appears to be incomplete. I do not understand this step.

Indeed, a paragraph should be added at the end: “the cheque lock returns with an error state“

@duanyytop duanyytop dismissed stale reviews from janx and XuJiandong via 38b8291 September 19, 2022 01:57
@jordanmack
Copy link
Contributor

@duanyytop I'm rewriting part of the description for clarity. I want to make sure I understand the functionality and intent correctly.

The Cheque Lock can match against a secp256k1-blake2b-sighash-all signature, which means the default lock must be used when a signature is provided to unlock.

The Cheque Lock can also match against a lock hash from the inputs, in which case it can be any form of lock script since it is just a hash that is being checked.

Is the intent here to encourage the use of the default lock only, or should it be stated that smart contracts could also use the Cheque Lock when signatures are not provided?

@duanyytop
Copy link
Contributor Author

duanyytop commented Sep 22, 2022

@duanyytop I'm rewriting part of the description for clarity. I want to make sure I understand the functionality and intent correctly.

The Cheque Lock can match against a secp256k1-blake2b-sighash-all signature, which means the default lock must be used when a signature is provided to unlock.

The Cheque Lock can also match against a lock hash from the inputs, in which case it can be any form of lock script since it is just a hash that is being checked.

Is the intent here to encourage the use of the default lock only, or should it be stated that smart contracts could also use the Cheque Lock when signatures are not provided?

The Cheque Lock only supports secp256k1-blake2b-sighash-all signature , so the sender and receiver can only use secp256k1-blake2b-sighash-all lock script to withdraw and claim the assets.

@jordanmack
Copy link
Contributor

The Cheque Lock only supports secp256k1-blake2b-sighash-all signature , so the sender and receiver can only use secp256k1-blake2b-sighash-all lock script to withdraw and claim the assets.

When no witness is provided, it attempts to match against the input lock hashes, as shown here. I do not know if we have an official name for this pattern. I've been calling it an "authorization piggyback". Since it is just matching the lock hash, it could be a lock of any type, including a lock script that is part of a smart contract.

  1. Is my understanding of this correct?
  2. Should I indicate that Cheque Lock only be used with secp256k1-blake2b-sighash-all to avoid complications, even if it could technically be used with different locks?

@duanyytop
Copy link
Contributor Author

The Cheque Lock only supports secp256k1-blake2b-sighash-all signature , so the sender and receiver can only use secp256k1-blake2b-sighash-all lock script to withdraw and claim the assets.

When no witness is provided, it attempts to match against the input lock hashes, as shown here. I do not know if we have an official name for this pattern. I've been calling it an "authorization piggyback". Since it is just matching the lock hash, it could be a lock of any type, including a lock script that is part of a smart contract.

  1. Is my understanding of this correct?
  2. Should I indicate that Cheque Lock only be used with secp256k1-blake2b-sighash-all to avoid complications, even if it could technically be used with different locks?
lock:
    code_hash: cheque lock script
    args: <20 byte receiver secp256k1-blake2b-sighash-all lock hash> <20 byte sender secp256k1-blake2b-sighash-all lock hash>

The Cheque Lock script data structure determines that the lock hash you are talking about can only be secp256k1-blake2b-sighash-all lock hash. And the reason why only secp256k1-blake2b-sighash-all is supported is determined by the requirements at that time.

@jordanmack
Copy link
Contributor

jordanmack commented Sep 22, 2022

@duanyytop A few more questions in relation to Unlock Rule 2.b.ii:

I see that within claim::validate() it locates the witness for the receiver input cell, but then within helpers::check_witness_args() it does not validate the signature at all, and just ensures that any signature exists.

  1. What is the reason for checking that a signature exists, but not actually validating the signature?
    2. Why check for a signature at all when doing an authorization piggyback since you are relying on the logic of the input's lock, and you cannot be certain that the lock is designed to require a signature to unlock?

Edit: I wrote these questions before I saw your previous reply posted. If only secp256k1-blake2b-sighash-all is supported, then question 2 is irrelevant.

@duanyytop
Copy link
Contributor Author

duanyytop commented Sep 22, 2022

  1. What is the reason for checking that a signature exists, but not actually validating the signature?

If the WitnessArgs is not empty for secp256k1-blake2b-sighash-all, the signature will be verified by the receiver lock script and Cheque Lock does not need to do repetitive things.

@jordanmack
Copy link
Contributor

If the WitnessArgs is not empty for secp256k1-blake2b-sighash-all, the signature will be verified by the receiver lock script and Cheque Lock does not need to do repetitive things.

Yes, this is the crux of my question. The Cheque Lock does not need to do a repetitive signature validation that is already done by the receiver lock script. So why does it check that WitnessArgs is not empty? What is ensured by doing this extra step?

@duanyytop
Copy link
Contributor Author

duanyytop commented Sep 22, 2022

If the WitnessArgs is not empty for secp256k1-blake2b-sighash-all, the signature will be verified by the receiver lock script and Cheque Lock does not need to do repetitive things.

Yes, this is the crux of my question. The Cheque Lock does not need to do a repetitive signature validation that is already done by the receiver lock script. So why does it check that WitnessArgs is not empty? What is ensured by doing this extra step?

Because there is no hard-coded secp256k1-blake2b-sighash-all lock information in the contract, that is to say, we cannot guarantee that the user must use the secp256k1-blake2b-sighash-all lock. Checking whether the WitnessArgs is empty can avoid the risk of evil to a certain extent (although it cannot be fundamentally eliminated)

@jordanmack
Copy link
Contributor

@duanyytop
Copy link
Contributor Author

@jordanmack any questions?

@jordanmack
Copy link
Contributor

@jordanmack any questions?

No more questions. Everything should be included in the PR.

@janx janx merged commit 06dbcb8 into nervosnetwork:master Sep 30, 2022
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