refactor(standards): clean up signature.masm and optimize verify_signatures#3230
refactor(standards): clean up signature.masm and optimize verify_signatures#3230onurinanc wants to merge 7 commits into
signature.masm and optimize verify_signatures#3230Conversation
PhilippGackstatter
left a comment
PhilippGackstatter
left a comment
There was a problem hiding this comment.
Looks good, left a few suggestions.
| # Duplicate the public key and message so that SIG_KEY can be computed by `merge` while a | ||
| # copy of the public key is kept on the stack for the signature verification below. This | ||
| # avoids round-tripping the public key through local memory. | ||
| dupw.1 dupw.1 | ||
| # => [OWNER_PUB_KEY, MSG, OWNER_PUB_KEY, MSG, i-1] |
There was a problem hiding this comment.
| # Duplicate the public key and message so that SIG_KEY can be computed by `merge` while a | |
| # copy of the public key is kept on the stack for the signature verification below. This | |
| # avoids round-tripping the public key through local memory. | |
| dupw.1 dupw.1 | |
| # => [OWNER_PUB_KEY, MSG, OWNER_PUB_KEY, MSG, i-1] | |
| dupw.1 dupw.1 | |
| # => [OWNER_PUB_KEY, MSG, OWNER_PUB_KEY, MSG, i-1] |
nit: unnecessary explanation of code
| adv.has_mapkey | ||
| # => [SIG_KEY, MSG, i-1] | ||
| # => [SIG_KEY, OWNER_PUB_KEY, MSG, i-1] |
There was a problem hiding this comment.
Looks like neither branch uses SIG_KEY below, so we can drop it right after this instruction.
| exec.active_account::get_initial_map_item | ||
| # => [OWNER_PUB_KEY, i-1, MSG] |
There was a problem hiding this comment.
Btw. why do we call this OWNER_PUB_KEY? Why "owner"? Should we call it APPROVER_PUB_KEY, which would be consistent with APPROVER_MAP_KEY above?
There was a problem hiding this comment.
OWNER_PUB_KEY doesn't make sense to me as well. APPROVER_PUB_KEY seems better.
| # SIG_KEY is no longer needed; drop it and duplicate the message so one copy survives | ||
| # verification. | ||
| dropw | ||
| # => [OWNER_PUB_KEY, MSG, i-1] |
There was a problem hiding this comment.
nit: In general, I would avoid commenting drops or duplications since it's typically not hard to figure out why we need these and adding a comment to these adds unnecessary verbosity.
| # Build map key from the signer index, still held on the stack as the loop counter. | ||
| dup.12 exec.create_approver_map_key | ||
| # => [APPROVER_MAP_KEY, OWNER_PUB_KEY, MSG, MSG, i-1] |
There was a problem hiding this comment.
nit: another improvement here would be giving i-1 a more precise name like signer_idx, so we wouldn't need to explain that the signer index is the loop counter.
2 participants
This PR cleans up
signature.masmand optimizesverify_signatures:authenticate_transaction, relying solely on the internal trap inverify_signature_by_scheme.scheme_idduplication inverify_signature_by_schemeso the value is consumed cleanly on each branch.neq.0double-negation prefixes inassert_supported_scheme_word, saving six VM cycles per call.NUM_OF_APPROVERS_LOClocal slot and shrinksverify_signatureslocals accordingly.SIGNER_INDEX_LOCstore/load by reusing the signer index already held on the stack viadup.12.CURRENT_PK_LOC, removing one word store and one word load per loop iteration and dropping the now-unused local.