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

ComputeEcdsaSecp256k1Key is never used #1087

Closed
graydon opened this issue Sep 27, 2023 · 1 comment · Fixed by #1105
Closed

ComputeEcdsaSecp256k1Key is never used #1087

graydon opened this issue Sep 27, 2023 · 1 comment · Fixed by #1105
Assignees
Labels
bug Something isn't working

Comments

@graydon
Copy link
Contributor

graydon commented Sep 27, 2023

The interface we settled on for Secp256k1 is the "key recovery" interface which doesn't involve a user ever converting a bytes object directly to a public key. This cost is therefore never used (and the codepaths that do it are only used to calibrate the cost, never actually do it).

It should probably be removed from the XDR cost types.

@graydon graydon added the bug Something isn't working label Sep 27, 2023
@graydon graydon assigned graydon and jayz22 and unassigned graydon Sep 27, 2023
github-merge-queue bot pushed a commit that referenced this issue Oct 11, 2023
Today I didn't get as much done -- had some errands and a doctor
appointment -- but my goal was to get through the residual bits of
host.rs and I did that at least. Along the way I removed the `allows`
that enabled un-warned dead code and unused variables, and found some
stuff:

- Two missing error checks on linear-memory "unpack" vector lengths.
These don't represent a _major_ vulnerability, but they do allow users
to do an unpack to a mismatched vector length, which could hide a bug. A
footgun at least. Fixed.
- A dead cost type, based on a previous iteration of the secp256k1
interface. Filed #1087 to track removing it.
- Some diagnositcs weren't using the names we so meticulously pass down
to them. Just throwing them out. Fixed.
  - There was a bunch of dead code and over-public interfaces. Fixed.
@jayz22
Copy link
Contributor

jayz22 commented Oct 11, 2023

Xdr change has been merged. Env change will be made with #1105.

github-merge-queue bot pushed a commit that referenced this issue Oct 12, 2023
### What

- Update to the latest XDR, consolidating a few memory-related
`ContractCostType` -- resolves
#1020
- Resolve #1087
- Move some no longer used cost type calibrations (VecEntry, MapEntry)
to an `experimental` directory, they are currently not used anywhere but
would be useful for experimental purpose. Will do a followup to make
them usable.
- Fixed a bug in `memory_grow` function, where we were checking the
limit against the wrong input
- Add test helpers to make wasm memory alloc accessible
- Various test fixes, clearifications

### Why

[TODO: Why this change is being made. Include any context required to
understand the why.]

### Known limitations

Follow ups:
- Refactor the analytical models
- Make `experimental` directory usable
- Recalibrate numbers on an x86 machine and update the parameters
- Write a more complex test than the currently `complex`, use that for
metering benchmark

---------

Co-authored-by: Graydon Hoare <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants