Skip to content
This repository was archived by the owner on Feb 26, 2020. It is now read-only.

update c-lib to upstream#19

Merged
ordian merged 7 commits into
masterfrom
ao-update-c-lib-to-upstream
Sep 13, 2019
Merged

update c-lib to upstream#19
ordian merged 7 commits into
masterfrom
ao-update-c-lib-to-upstream

Conversation

@ordian
Copy link
Copy Markdown

@ordian ordian commented Sep 13, 2019

I've updated the c-lib to f5c8a00, build.rs to upstream except for disabling USE_EXTERNAL_DEFAULT_CALLBACKS (https://github.com/paritytech/rust-secp256k1/compare/master...paritytech:ao-update-c-lib-to-upstream?expand=1#diff-a7b0a2dee0126cddf994326e705a91eaR103) (link time errors otherwise).

The benchmarks on parity-ethereum shows 33% improvement for verify_block_unordered

verify_block_unordered  time:   [17.404 ms 17.558 ms 17.720 ms]                                    
                        change: [-35.612% -35.041% -34.470%] (p = 0.00 < 0.05)
                        Performance has improved.

else
{
secp256k1_ecmult_const(&res, &pt, &s);
secp256k1_ecmult_const(&res, &pt, &s, 256);
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be verified

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ext.c is not present in upstream (our extention), so we need to make sure 256 is the right size here

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/cc @svyatonik PTAL?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, if I am the right person to ask, but imo that's the correct change :)

Comment thread src/lib.rs

impl error::Error for Error {
fn cause(&self) -> Option<&error::Error> { None }
fn cause(&self) -> Option<&dyn error::Error> { None }
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

warning fix

Comment thread build.rs Outdated
Comment thread build.rs Outdated
Comment thread build.rs

base_config.flag("-g")
.include("depend/secp256k1/src")
.debug(true)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't mean the same thing as in Rust I take it? But what does it mean? Includes symbols?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we used "-g" flag previously, I guess this should mean the same

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so what does -gmean?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the same as debug = true in rust, adds debug symbols to the build (doesn't downgrade the perf much)

Comment thread build.rs Outdated
else
{
secp256k1_ecmult_const(&res, &pt, &s);
secp256k1_ecmult_const(&res, &pt, &s, 256);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate?

include src/modules/ecdh/Makefile.am.include
endif

if ENABLE_MODULE_SCHNORR
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like schnorr support was removed, do we use it though?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afaik we do not, but maybe @svyatonik can help with that too?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehhh...The only thing I can guarantee is that we don't use it in blockchain clients - parity-ethereum (and specifically in SecretStore), parity-bitcoin, parity-zcash (and obviously in substrate). Can't tell about the the rest...

Comment thread build.rs Outdated
@ordian ordian merged commit a96ad75 into master Sep 13, 2019
@ordian ordian deleted the ao-update-c-lib-to-upstream branch September 13, 2019 15:15
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants