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

Secp operations during txid computation #202

Closed
RCasatta opened this issue May 9, 2024 · 8 comments
Closed

Secp operations during txid computation #202

RCasatta opened this issue May 9, 2024 · 8 comments

Comments

@RCasatta
Copy link
Collaborator

RCasatta commented May 9, 2024

In a performance profile in a downstream project I noticed txid() is taking too much time even considering it's an expensive computation, in the flame graph there were calls to secp function which I don't think should happen?

Can share the performance graph if can help understand

@apoelstra
Copy link
Member

Sure. What secp function was it?

@RCasatta
Copy link
Collaborator Author

RCasatta commented May 9, 2024

I had to leave the PC... I remember fe_sqrt and others, will post tomorrow

@RCasatta
Copy link
Collaborator Author

Here are a convenient screenshot and the profiler json (to be loaded from chrome dev tools, performance tab)

For what I understand is how the PedersenCommitment is serialized with secp256k1_pedersen_commitment_serialize

image

Trace-20240510T085107.json

@apoelstra
Copy link
Member

Ok, yeah, the issue is in libsecp256k1-zkp, not here. For some bizarre reason secp256k1_pedersen_commitment_serialize decompresses the point then recompresses it, when all it needs to do is a memcmp (I think). So this function is wasting a lot of time for no reason.

@RCasatta
Copy link
Collaborator Author

RCasatta commented Jul 1, 2024

Issue has been fixed in the upstream lib, I think we need

  • secp256k1-zkp-sys upgrade to catch the changes
  • rust-secp256k1-zkp release
  • upgrade dep here (if rust-secp256k1-zkp not released as patch version)

@apoelstra
Copy link
Member

Bumping secp-zkp requires bumping secp, which requires bumping bitcoin, which requires bumping bitcoind, which requires bumping elementsd.

I am almost done this. Opened RCasatta/elementsd#18

@apoelstra
Copy link
Member

The next version of rust-bitcoin will break out the "types that never change" into a new primitives crate which should be sufficient for most of the other crates in the ecosystem, and which should rarely have major version bumps, which will make this all way less painful.

@RCasatta
Copy link
Collaborator Author

Fixed with #209
next release will take this

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

No branches or pull requests

2 participants