rust-elements icon indicating copy to clipboard operation
rust-elements copied to clipboard

Secp operations during txid computation

Open RCasatta opened this issue 1 year ago • 7 comments

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

RCasatta avatar May 09 '24 20:05 RCasatta

Sure. What secp function was it?

apoelstra avatar May 09 '24 20:05 apoelstra

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

RCasatta avatar May 09 '24 20:05 RCasatta

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

RCasatta avatar May 10 '24 07:05 RCasatta

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.

apoelstra avatar May 10 '24 12:05 apoelstra

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)

RCasatta avatar Jul 01 '24 12:07 RCasatta

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

I am almost done this. Opened https://github.com/RCasatta/elementsd/pull/18

apoelstra avatar Jul 09 '24 22:07 apoelstra

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.

apoelstra avatar Jul 09 '24 22:07 apoelstra

Fixed with https://github.com/ElementsProject/rust-elements/pull/209 next release will take this

RCasatta avatar Sep 23 '24 09:09 RCasatta