Fixing minting API by zuzunker · Pull Request #273 · Emurgo/cardano-serialization-lib

zuzunker · 2021-11-28T11:13:57Z

Reworked mint helper function to accept minting script instead of just the hash, and added storing these scripts in the resulting witness set

…t the hash, and added storing these scripts in the resulting witness set

…d updated a test for it

rust/src/tx_builder.rs

ozgrakkurt

looks good to me

Profiling the tx builder showed that even after we removed the fake_key_root construction in #214 due to terrible performance in asm.js (#213) that the cryptoxide calls within `fake_full_tx()` were up a ridiculous amount of the runtime. I investigated this as we had a report that it was taking up to hundreds of milliseconds just to make a tx and I wanted to see if there would be any significant improvement by migrating to an idiomatic rust API as discussed in #276 but from a purely performance perspective in a release build (opt-level=3) this seemed to be pretty minor, even with the unnecessary cloning forced by this inside of the implementation. This will need to be slightly modified once #273 gets merged as there are some conflicts. Previous perf results from building 100,000 simple txs and serializing them: ``` - 53.64% [.] <&cryptoxide::curve25519::Fe as core::ops::arith::Mul>::mul - 53.63% 0xffffffffffffffff + 23.03% cardano_serialization_lib::tx_builder::min_fee - 21.32% cardano_serialization_lib::tx_builder::TransactionBuilder::add_change_if_needed - 15.21% cardano_serialization_lib::tx_builder::TransactionBuilder::fee_for_output cardano_serialization_lib::tx_builder::min_fee + cardano_serialization_lib::tx_builder::fake_full_tx + 6.11% cardano_serialization_lib::tx_builder::TransactionBuilder::min_fee (inlined) - 7.78% ser_lib_perf::main + 6.11% cardano_serialization_lib::tx_builder::TransactionBuilder::build + 1.67% cardano_serialization_lib::tx_builder::TransactionBuilder::add_change_if_needed + 1.46% std::rt::lang_start_internal - 19.40% [.] cryptoxide::curve25519::Fe::square - 0xffffffffffffffff - 8.34% cardano_serialization_lib::tx_builder::min_fee cardano_serialization_lib::tx_builder::TransactionBuilder::build cardano_serialization_lib::tx_builder::TransactionBuilder::build_and_size + cardano_serialization_lib::tx_builder::fake_full_tx - 7.41% cardano_serialization_lib::tx_builder::TransactionBuilder::add_change_if_needed + 5.58% cardano_serialization_lib::tx_builder::TransactionBuilder::fee_for_output + 1.83% cardano_serialization_lib::tx_builder::TransactionBuilder::min_fee (inlined) + 2.77% ser_lib_perf::main + 0.88% std::rt::lang_start_internal + 7.80% [.] cryptoxide::curve25519::GePrecomp::maybe_set + 5.13% [.] cryptoxide::curve25519::Fe::invert + 3.98% [.] <&cryptoxide::curve25519::GeP3 as core::ops::arith::Add<&cryptoxide::curve25519::GePrecomp>>::add + 2.84% [.] cryptoxide::curve25519::GePrecomp::select + 1.22% [.] cryptoxide::curve25519::ge_scalarmult_base + 1.00% [.] cryptoxide::sha2::impl512::reference::digest_block_u64 0.47% [.] cryptoxide::curve25519::GeP2::dbl 0.13% [.] cryptoxide::curve25519::sc_muladd 0.12% [.] cryptoxide::curve25519::Fe::to_bytes 0.12% [.] cryptoxide::curve25519::sc_reduce 0.10% [.] cbor_event::se::Serializer<W>::write_type 0.07% [.] cardano_serialization_lib::tx_builder::fake_full_tx ``` which shows that almost the entire runtime was spent on cryptoxide calls which were only necessary for `fake_full_tx()`. Running it again afterwards there is no overwhelming bottleneck anymore and the remaining runtime was fairly distributed.

# Conflicts: # rust/src/tx_builder.rs

zuzunker

/check

zuzunker · 2021-12-12T23:44:13Z

@rooooooooob @ozgrakkurt , plz review, this one is ready now

zuzunker

/check

rooooooooob

Looks good for the most part. I just think we should better document (or change the name of) TransactionBuilder::get_witness_set()

rust/src/lib.rs

rust/src/tx_builder.rs

rooooooooob · 2022-01-05T20:09:47Z

@vsubhuman what's the status on this PR? It would be nice to get it merged if we can so we can not conflict with other TxBuilder changes, plus I started updating the documentation and wanted to be able to include these changes in that.

Profiling the tx builder showed that even after we removed the fake_key_root construction in #214 due to terrible performance in asm.js (#213) that the cryptoxide calls within `fake_full_tx()` were up a ridiculous amount of the runtime. I investigated this as we had a report that it was taking up to hundreds of milliseconds just to make a tx and I wanted to see if there would be any significant improvement by migrating to an idiomatic rust API as discussed in #276 but from a purely performance perspective in a release build (opt-level=3) this seemed to be pretty minor, even with the unnecessary cloning forced by this inside of the implementation. This will need to be slightly modified once #273 gets merged as there are some conflicts. Previous perf results from building 100,000 simple txs and serializing them: ``` - 53.64% [.] <&cryptoxide::curve25519::Fe as core::ops::arith::Mul>::mul - 53.63% 0xffffffffffffffff + 23.03% cardano_serialization_lib::tx_builder::min_fee - 21.32% cardano_serialization_lib::tx_builder::TransactionBuilder::add_change_if_needed - 15.21% cardano_serialization_lib::tx_builder::TransactionBuilder::fee_for_output cardano_serialization_lib::tx_builder::min_fee + cardano_serialization_lib::tx_builder::fake_full_tx + 6.11% cardano_serialization_lib::tx_builder::TransactionBuilder::min_fee (inlined) - 7.78% ser_lib_perf::main + 6.11% cardano_serialization_lib::tx_builder::TransactionBuilder::build + 1.67% cardano_serialization_lib::tx_builder::TransactionBuilder::add_change_if_needed + 1.46% std::rt::lang_start_internal - 19.40% [.] cryptoxide::curve25519::Fe::square - 0xffffffffffffffff - 8.34% cardano_serialization_lib::tx_builder::min_fee cardano_serialization_lib::tx_builder::TransactionBuilder::build cardano_serialization_lib::tx_builder::TransactionBuilder::build_and_size + cardano_serialization_lib::tx_builder::fake_full_tx - 7.41% cardano_serialization_lib::tx_builder::TransactionBuilder::add_change_if_needed + 5.58% cardano_serialization_lib::tx_builder::TransactionBuilder::fee_for_output + 1.83% cardano_serialization_lib::tx_builder::TransactionBuilder::min_fee (inlined) + 2.77% ser_lib_perf::main + 0.88% std::rt::lang_start_internal + 7.80% [.] cryptoxide::curve25519::GePrecomp::maybe_set + 5.13% [.] cryptoxide::curve25519::Fe::invert + 3.98% [.] <&cryptoxide::curve25519::GeP3 as core::ops::arith::Add<&cryptoxide::curve25519::GePrecomp>>::add + 2.84% [.] cryptoxide::curve25519::GePrecomp::select + 1.22% [.] cryptoxide::curve25519::ge_scalarmult_base + 1.00% [.] cryptoxide::sha2::impl512::reference::digest_block_u64 0.47% [.] cryptoxide::curve25519::GeP2::dbl 0.13% [.] cryptoxide::curve25519::sc_muladd 0.12% [.] cryptoxide::curve25519::Fe::to_bytes 0.12% [.] cryptoxide::curve25519::sc_reduce 0.10% [.] cbor_event::se::Serializer<W>::write_type 0.07% [.] cardano_serialization_lib::tx_builder::fake_full_tx ``` which shows that almost the entire runtime was spent on cryptoxide calls which were only necessary for `fake_full_tx()`. Running it again afterwards there is no overwhelming bottleneck anymore and the remaining runtime was fairly distributed. (cherry picked from commit e0b33c6)

zuzunker · 2022-01-12T11:29:40Z

@vsubhuman what's the status on this PR? It would be nice to get it merged if we can so we can not conflict with other TxBuilder changes, plus I started updating the documentation and wanted to be able to include these changes in that.

I would be really happy to get that in and can prioritize it this week. Let me look into the comments and see if I can make any required changes quickly. And then we can get the coin-selection in as well

rooooooooob · 2022-01-12T22:35:54Z

That would be great. My comment about documenting what is in the returned witness set isn't quite as important anyway since it looks like we'll be working to integrate in a second-phase witness builder combining this + #307 and hooking the two together later on, so that function return will likely change soon anyway.

# Conflicts: # package-lock.json # package.json # rust/Cargo.lock # rust/Cargo.toml

zuzunker · 2022-01-15T07:19:57Z

Looks good for the most part. I just think we should better document (or change the name of) TransactionBuilder::get_witness_set()

Added comment to this function. Note that it's internal, so not a part of the public API, but good point that it was missing any clarity. Idea is that this function should be updated when new functionality is added to the builder, e.g. when it supports more different scripts.

zuzunker

/check

zuzunker

/check

rooooooooob

LGTM now

zuzunker

/check

Reworked mint helper function to accept minting script instead of jus…

3618506

…t the hash, and added storing these scripts in the resulting witness set

zuzunker added this to the 10.0.0 milestone Nov 28, 2021

zuzunker linked an issue Nov 28, 2021 that may be closed by this pull request

FeeTooSmallUTxO when used with ScriptAll, TimelockExpiry & transaction metadata #272

Closed

zuzunker added 3 commits November 28, 2021 14:26

Fixed test for fee estimation

edbab5f

Added test that fee-estimation fails when mint scripts are not provided

b6de84d

Fixed fee-estimation to add vkey witnesses for each minting policy an…

276334a

…d updated a test for it

zuzunker marked this pull request as ready for review November 29, 2021 12:04

zuzunker requested review from ozgrakkurt, pedromtcosta, rooooooooob and twwu123 November 29, 2021 12:05

rooooooooob reviewed Nov 30, 2021

View reviewed changes

rust/src/tx_builder.rs Outdated Show resolved Hide resolved

zuzunker added 2 commits November 30, 2021 11:57

Flowgen update. Warnings fixed. Version bump to beta7

91bbbb8

Fixed unique vkey signatures for minting. Version bump to beta8

94460c2

ozgrakkurt reviewed Dec 2, 2021

View reviewed changes

Started changing API for mint-scripts

ed86ae8

zuzunker mentioned this pull request Dec 7, 2021

BETA Release / 10.0.0-beta.8 #283

Merged

rooooooooob mentioned this pull request Dec 10, 2021

Remove all cryptoxide calls from fake_full_tx() #286

Merged

Fixed tests for new minting API

d968823

zuzunker added 2 commits December 13, 2021 02:22

Merge remote-tracking branch 'origin/master' into ruslan/mint-fix

3db7400

# Conflicts: # rust/src/tx_builder.rs

Removed unused functions and variables (after merge)

f1eb5fd

zuzunker requested review from ozgrakkurt and rooooooooob December 12, 2021 23:26

zuzunker added 2 commits December 13, 2021 02:31

Flowgen update

2bbe1c4

Replaced functions with a new type and From implementation

4a65faf

zuzunker commented Dec 12, 2021

View reviewed changes

Removed unused field

15bd42e

zuzunker commented Dec 13, 2021

View reviewed changes

rooooooooob reviewed Dec 15, 2021

View reviewed changes

rust/src/lib.rs Show resolved Hide resolved

rust/src/tx_builder.rs Show resolved Hide resolved

rust/src/tx_builder.rs Outdated Show resolved Hide resolved

rust/src/tx_builder.rs Show resolved Hide resolved

rooooooooob reviewed Dec 16, 2021

View reviewed changes

rust/src/tx_builder.rs Show resolved Hide resolved

SebastienGllmt mentioned this pull request Dec 25, 2021

Add Witness builder #307

Open

zuzunker added 4 commits January 14, 2022 13:10

Merge remote-tracking branch 'origin/master' into ruslan/mint-fix

4daf87d

# Conflicts: # package-lock.json # package.json # rust/Cargo.lock # rust/Cargo.toml

lock file updates

1409301

Merge remote-tracking branch 'origin/master' into ruslan/mint-fix

f4aef74

Added comment to get_witness_set()

4ac439a

zuzunker added 2 commits January 15, 2022 10:21

Removed unnecessary clone

2b93da3

Commented out a non-critical expensive assert in a critical function

97995ed

zuzunker commented Jan 15, 2022

View reviewed changes

Merge branch 'master' into ruslan/mint-fix

f4e68c8

zuzunker commented Jan 15, 2022

View reviewed changes

rooooooooob approved these changes Jan 15, 2022

View reviewed changes

Disabled some tests for the disabled functionality

3021134

zuzunker commented Jan 17, 2022

View reviewed changes

zuzunker merged commit 6a8c67a into master Jan 17, 2022

zuzunker deleted the ruslan/mint-fix branch January 17, 2022 09:13

zuzunker mentioned this pull request Feb 6, 2022

Release / 10.0.0 #358

Merged

40 tasks

Conversation

zuzunker commented Nov 28, 2021

Uh oh!

Uh oh!

ozgrakkurt left a comment

Choose a reason for hiding this comment

Uh oh!

zuzunker left a comment

Choose a reason for hiding this comment

Uh oh!

zuzunker commented Dec 12, 2021

Uh oh!

zuzunker left a comment

Choose a reason for hiding this comment

Uh oh!

rooooooooob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rooooooooob commented Jan 5, 2022

Uh oh!

zuzunker commented Jan 12, 2022

Uh oh!

rooooooooob commented Jan 12, 2022

Uh oh!

zuzunker commented Jan 15, 2022

Uh oh!

zuzunker left a comment

Choose a reason for hiding this comment

Uh oh!

zuzunker left a comment

Choose a reason for hiding this comment

Uh oh!

rooooooooob left a comment

Choose a reason for hiding this comment

Uh oh!

zuzunker left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants