Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
TradableKitty
piece #171base: main
Are you sure you want to change the base?
TradableKitty
piece #171Changes from 7 commits
b522b1b
2e0885d
80d2e99
5461173
eea8241
1f5855b
4d4501b
32be05f
7b0a12e
389576b
c77120b
d57f9b1
db5748d
02aaf78
9c4f903
9e2c908
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Also, I think the fields should follow a more logical order, such as:
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.
It is generally better to add new members at the end of a struct. This practice aligns with the principle of maintaining backward compatibility. When we add a new member at the end of the struct, existing code that uses the struct won't be affected, as the layout of the existing members remains unchanged.
If you add a new member in the middle of a struct, it can break existing code that relies on the order and size of the struct members. This is because the memory layout of the struct may change, leading to potential issues with code that assumes a specific order or size.
By appending new members at the end, we follow a practice commonly referred to as "struct versioning" or "extensible struct pattern," where you ensure that new fields are added without affecting the existing layout. This helps in maintaining compatibility and minimizes the risk of introducing errors in the existing codebase.
As of now, I don't see any code which is relying on the layout of the structure.
If it is a strong request, I will update it. Otherwise, I want to keep it as it is.
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.
Although you are not wrong, at this stage of the development we don't need to care about this, and we should prioritize doing stuff that makes sense and is clear and understandable.
And sometimes, you do want to break compatibility.
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.
Ok. I updated the struct as you suggested.
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.
It seems like you're spending a lot of effort making sure that no two kitties have the same DNA here. But isn't that already guaranteed (assuming polynomial time advarsaries) by the hash function itself?
I recommend you remove this check entirely. Here you should just make sure that the output kitties are the same as the input kitties except for the names. If you aren't convinced, I challenge you to think of a case where two kitties would have the same DNA.
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.
@JoshOrndorff, Thank you.
I can remove those checks but I thought of below cases:
Good wallets don't abuse the blockchain apis, but rouge wallets can abuse the blockchain API as below :
Case1:
I have 1 kitty in my wallet
I will send the same kitty 2 times as below :
let inputs: Vec<Input> = vec![input_kitty_ref.clone(),input_kitty_ref]; // I am just sending same kitty 2 times
Then I will create the 2 outputs as below in the wallet :
Below is o/p when I tested.
Blockchain logs: I truncated the logs manually for easy reading :
INFO tokio-runtime-worker tuxedo_template_runtime: runtime-> validate_transaction TransactionSource = TransactionSource::InBlock tx = Transaction { inputs: [Input { output_ref: OutputRef { tx_hash: 0x928ff2975393c8466ef76121f98692a3500f1a4579372799eaffb27deaf2b387, index: 0 }, redeemer: [184, ---] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }, Output { payload: DynamicallyTypedData { data: [0, --- 116, 116] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }], checker: FreeKitty(UpdateKittyName) } block_hash = 0x37040850b24d9dad724f1657ea572b1536df1c7fdf52670090dd54b4a0b6c537
You can see I could create 2 kitties with 1 kitty by abusing update name API .
So thought of preventing that with the Line no 647 to 649 i.e below :
if utxo_input_kitty.dna != utxo_output_kitty.dna { return Err(ConstraintCheckerError::DnaMismatchBetweenInputAndOutput); }
With the above check the transaction will fail as below: I removed some logs for easy reading .
TransactionSource = TransactionSource::External tx = Transaction { inputs: [Input { output_ref: OutputRef { tx_hash: 0x45ce7924543b5e60490c93e25ff8b379589290d90a46df1725bf4e57d9325fb2, index: 0 }, redeemer: [---0, 108, 105, 108, 121], type_id: [75, 105, 116, 116] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }], checker: FreeKitty(UpdateKittyName) } block_hash = 0x86ed44adb9742d6cebea1538bab2645812a00e3b11249dc44379ec957fcd1629 2024-03-02 11:22:04.243 WARN tokio-runtime-worker tuxedo-core: Tuxedo Transaction did not validate (in the pool): ConstraintCheckerError(FreeKitty(DnaMismatchBetweenInputAndOutput))
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.
Case 2:
If I send 2 output kittyes with same DNA i.e without below line in wallet:
updated_kitty_fake.dna = KittyDNA(H256::from_slice(b"mom_kitty_1asdfasdfasdfasdfasdfz")); // Here I am updating the kitty DNA to some random value.
I did the below steps
1. The code is the same as 1st case except for the DNA update of updated_kitty_fake i.e I didn't do a DNA update.
2. Update the kitty name from vina to Tina.
3. Then I have a new kitty lily which has the same DNA as the updated kitty tina . Then also transaction validation is passed without those checks;
Below are the wallet logs
Blockchain log:
2024-03-02 11:38:54.004 INFO tokio-runtime-worker tuxedo_template_runtime: runtime-> apply_extrinsic Transaction { inputs: [Input { output_ref: OutputRef { tx_hash: 0xbd3f0a8bb0596f336c82122cd373b2743717d28d82bdf3ee48a0b86d458c0f71, index: 0 }, redeemer: [56, 5, 230, 157, 51, 235, 0, 59, 119, 144, 251, 58, 29, 247, 215, 194, 194, 241, 15, 174, 193, 227, 100, 185, 51, 92, 123, 148, 120, 6, 81, 31, 243, 141, 56, 37, 230, 43, 233, 254, 225, 60, 25, 119, 206, 8, 84, 229, 145, 92, 46, 128, 123, 90, 234, 63, 48, 98, 198, 238, 7, 178, 77, 129] }, Input { output_ref: OutputRef { tx_hash: 0xbd3f0a8bb0596f336c82122cd373b2743717d28d82bdf3ee48a0b86d458c0f71, index: 0 }, redeemer: [172, 171, 253, 111, 104, 241, 143, 143, 59, 122, 103, 39, 113, 118, 69, 21, 56, 136, 207, 121, 143, 84, 229, 76, 146, 230, 46, 106, 240, 146, 252, 69, 149, 254, 28, 98, 163, 118, 28, 100, 185, 176, 226, 204, 235, 182, 67, 31, 83, 211, 10, 230, 200, 86, 142, 38, 176, 239, 255, 252, 170, 164, 128, 141] }], peeks: [], outputs: [Output { payload: DynamicallyTypedData { data: [0, -- 121], type_id: [75, 105, 116, 116] }, verifier: Sr25519Signature(Sr25519Signature { owner_pubkey: 0xd2bf4b844dfefd6772a8843e669f943408966a977e3ae2af1dd78e0f55f4df67 }) }], checker: FreeKitty(UpdateKittyName) } 2024-03-02 11:38:54.006 INFO tokio-runtime-worker tuxedo_template_runtime: runtime-> finalize_block
If I have the below line I can avoid this situation as inputs are checked.
if dna_to_kitty_set.contains(&utxo_input_kitty.dna) { return Err(ConstraintCheckerError::DuplicateKittyFound);
I checked the kitties via verifykitty which will get the kitties from the blockchain db, Then I also could fine 2 kitties with the same DNA.
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 I am doing something wrong in the above tests, Please suggest.
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.
@JoshOrndorff
Hi Joshy,
I have delved deeper into the issues and identified the root cause of why duplicate DNA can occur in kitties.
But basically I think the issue is common to any UTXOs.
Issue 1: Duplicate UTXO Checks
Presently, our UTXO validation primarily focuses on the existing UTXOs within the blockchain database i.e 'TransparentUtxoSet', detecting any duplicate output UTXOs from the transaction output sets.
If a duplicate UTXO is detected, we trigger the error 'PreExistingOutput.' However, it seems we do not perform checks for duplicity within the outputs of the transaction itself. I think it may be valid in the case of coin/money.
Tuxedo/tuxedo-core/src/executive.rs
Lines 110 to 125 in 1ad2343
Perhaps originally, the design intended to have a single output of the same type.
Due to this, I can create identical kitties by cloning in the 'create_kitty' transaction, and all the outputs are validated and stored in the database as valid UTXOs.
Possible Solutions:
Solution 1: Consider supporting only one output of the same type at a time or per transaction to address the issue of allowing multiple cloned outputs as valid transactions.
Solution 2: Implement a mechanism in each piece to verify uniqueness within the output UTXO, preventing the inclusion of duplicate outputs in a transaction.
Issue 2: DNA Calculation and Uniqueness
This issue predominantly concerns kitties, and it remains unclear if it also applies to other UTXOs. Currently,
we calculate DNA based on the 'dna_preimage' supplied by the client. While we can identify duplicate DNA if the 'dna_preimage' is the same and the other information is identical, considering the hash of the UTXO as a whole, this becomes challenging when parameters like Parents (gender, name, etc.) can be updated, altering the hash of the UTXO and compromising DNA uniqueness.
This issues is exploited in any of the transactions like updateName , UpdatePrice, listKittyForSAle,etc
Possible Solution:
To address this, we can reconsider the DNA calculation process it self or check the uniqueness of DNA within the Kitty piece, as we are currently doing.
I think it is better to create new issues for this. I will do it
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've replied in your issue #190. I think you are not distinguishing between the concepts of two kitties with the same DNA vs a single kitty. You cannot input the same kitty twice because tuxedo core already checks it.
The question here is how would you ever get two identical kitties to begin with. The only way is if you mint them directly. Up to you whether that is valid or not. If it is not, use a universal creator to ensure they are always unique. If it is valid to mint kitties with identical dna, then it should be no surprise that their offspring can also have identical DNA.
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.
Thanks @JoshOrndorff, I removed the DNA check.