Refactor ED25519 curve checker for improved y-coordinate extraction #14
Conversation
…nd modular arithmetic; update token account hash function for strict binary encoding.
AmarildoGrembi
left a comment
AmarildoGrembi
left a comment
There was a problem hiding this comment.
Hey @cienhub, sorry for the delay, and good job! I see only 2 issues that I would like you to improve if possible.
| # Compute x² from the Ed25519 curve equation: x² = (y² - 1) / (d * y² + 1) mod Q | ||
| numerator = (y**2 - 1) % Q | ||
| denominator = (D * y**2 + 1) % Q | ||
| y_squared = y**2 |
There was a problem hiding this comment.
y**2 is computed before modular reduction. This creates unnecessarily large integers and diverges from standard field arithmetic used in Solana / ed25519-dalek.
suggestion:
y = y % Q
y_squared = (y * y) % Q
numerator = (y_squared - 1) % Q
denominator = (D * y_squared + 1) % Q
There was a problem hiding this comment.
Yes this is absolutely right, I'll keep your suggestion
Thanks
| denominator = (D * y_squared + 1) % Q | ||
|
|
||
| # Compute the modular inverse of the denominator | ||
| denominator_inv = OpenSSL::BN.new(denominator).mod_inverse(Q).to_i rescue nil |
There was a problem hiding this comment.
If mod_inverse fails, denominator_inv becomes nil, but execution continues.
This can cause false positives or runtime errors later.
suggestion:
denominator_inv =
OpenSSL::BN.new(denominator).mod_inverse(Q).to_i rescue nil
return false unless denominator_inv
There was a problem hiding this comment.
This is actually another way to write the same thing that is already implemented, wdyt?
denominator_inv = OpenSSL::BN.new(denominator).mod_inverse(Q).to_i rescue nil
return false if denominator_inv.nil? # If inverse doesn't exist, it's off-curve
| # Combine seeds and program ID with the PDA derivation logic | ||
| buffer = seeds.flatten.join + program_id + "ProgramDerivedAddress" | ||
| # strict encoding to binary | ||
| buffer = seeds.join.b + program_id.b + "ProgramDerivedAddress".b |
There was a problem hiding this comment.
This now assumes:
- seeds is a flat array
- each seed is already a binary-safe string
Are we sure about this?
There was a problem hiding this comment.
Good point! In the current implementation, seeds are always binary strings from Base58.base58_to_binary() and pack('C'), so I would say that it's safe.
Also seeds is instanciated as a flat array and even when it is concatened with the nonce still remain a flat array
…ordinate calculations and ensuring correct field operations.
|
Hey @AmarildoGrembi! Thanks for taking the time to review it. Let me know what do you think |
2 participants
Hey! This PR should be a fix for #13.
The bugs that I found are described inside the issue.
Please review these changes and let me know wdyt
We faced a specific problem with ATA derivation and these changes fixed the problem for us, so I'm pretty confident about them but I am more than willing to have your opinion on this @AmarildoGrembi