stacks-core icon indicating copy to clipboard operation
stacks-core copied to clipboard

Costs-3 contract

Open pavitthrap opened this issue 3 years ago • 5 comments

Description

This PR adds new cost functions for the Clarity2 functions, and adds the costs-3 contract.

For the most part, the new costs contract does not alter old costs. However, as part of this benchmarking effort, I revamped the mocked data used (database and MARF accesses are now much more realistic). I also caught a few errors related to the computation of the input size (which only significantly altered one cost function). These changes led to a few cost changes. I provide a list of the altered costs below:

  • cost_stx_balance (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access)
  • cost_stx_transfer (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access)
  • cost_at_block (increased 10x) - using a real block now, which explains cost increase
  • cost_ast_parse (decreased 6x) (improvements in v2)
  • cost_concat (decreased 2x). (fixed input size computation)

We are now using version 0.21.0 of the Rust secp256k1 library. When we benchmarked the costs-2 contract, the version of this library was at 0.19.0. Due to improvements in these libraries, the runtimes for the following functions changed.

  • cost_secp256k1recover (decreased 2x)
  • cost_secp256k1verify (decreased 2x)
  • cost_poison_microblock (decreased 2x) - this function recovers the public key (so it essentially does the same operation as secp256k1recover)

Applicable issues

  • fixes #3115

Additional info (benefits, drawbacks, caveats)

  • Not currently using new cost functions in the code base as that requires additional code change of loading the costs-3 contract (#3266) - I can also address that in this PR if people feel that is appropriate.

pavitthrap avatar Aug 29 '22 15:08 pavitthrap

Codecov Report

Merging #3267 (4156983) into next (557f184) will decrease coverage by 0.03%. The diff coverage is 19.47%.

@@            Coverage Diff             @@
##             next    #3267      +/-   ##
==========================================
- Coverage   29.60%   29.56%   -0.04%     
==========================================
  Files         279      284       +5     
  Lines      251259   258262    +7003     
==========================================
+ Hits        74378    76354    +1976     
- Misses     176881   181908    +5027     
Impacted Files Coverage Δ
clarity/src/vm/functions/assets.rs 26.96% <0.00%> (ø)
clarity/src/vm/functions/conversions.rs 14.56% <0.00%> (-0.40%) :arrow_down:
clarity/src/vm/functions/principals.rs 0.00% <0.00%> (ø)
clarity/src/vm/functions/sequences.rs 50.98% <0.00%> (-2.77%) :arrow_down:
clarity/src/vm/types/mod.rs 50.41% <0.00%> (-0.28%) :arrow_down:
src/burnchains/tests/affirmation.rs 0.00% <0.00%> (ø)
src/burnchains/tests/burnchain.rs 0.00% <0.00%> (ø)
src/burnchains/tests/db.rs 0.00% <0.00%> (ø)
src/burnchains/tests/mod.rs 0.00% <0.00%> (ø)
.../chainstate/burn/operations/leader_key_register.rs 15.39% <0.00%> (-0.03%) :arrow_down:
... and 92 more

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

codecov[bot] avatar Aug 29 '22 15:08 codecov[bot]

cost_stx_balance (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access) cost_stx_transfer (increased 7x) - 99% of the runtime is in get_stx_balance_snapshot (DB access) cost_at_block (increased 10x) - using a real block now, which explains cost increase

Wow! :exploding_head: Do we know if there's a way to speed these operations up? Curious if this is due to a natural average MARF key-path lengthening that will naturally occur at a logarithmic scale as more data gets added, or if this is due to an artifact / regression of the reference implementation.

jcnelson avatar Aug 29 '22 19:08 jcnelson

Notes from the blockchain meeting:

  • This PR tests runtimes against a real DB, so the runtime cost includes the I/O costs of hitting a real MARF. Before, we had determined the read_count and write_count values with a real MARF, but determined the runtime values with a mocked MARF.

jcnelson avatar Sep 12 '22 15:09 jcnelson

@pavitthrap raised a good point about the advantages of using the real backend versus the mocked backend -- this kind of relates to a question about the size of the MARF used in the benchmarks.

Do you know approximately how many keys were in the MARF, and how many MARF "blocks" there are in the database?

kantai avatar Sep 12 '22 15:09 kantai

Update: will replace the MARF with a smaller one (up to 10 blocks). Will re-benchmark the functions with the new MARF once the PR for the function replace-at is done.

pavitthrap avatar Sep 19 '22 16:09 pavitthrap

Need a function for replace-at and the new analysis function for trait-checking

jcnelson avatar Oct 17 '22 15:10 jcnelson

This PR tests runtimes against a real DB, so the runtime cost includes the I/O costs of hitting a real MARF. Before, we had determined the read_count and write_count values with a real MARF, but determined the runtime values with a mocked MARF.

@pavitthrap does the benchmark with real MARF use HDD or SSD?

igorsyl avatar Oct 19 '22 15:10 igorsyl

  • It appears https://github.com/stacks-network/stacks-blockchain/issues/3266 is needed for 2.1. Could we include it to this PR?
  • Do we have a programmatic check that the ClarityCostFunction enum is complete and not missing clarity functions?

igorsyl avatar Oct 19 '22 15:10 igorsyl

This PR tests runtimes against a real DB, so the runtime cost includes the I/O costs of hitting a real MARF. Before, we had determined the read_count and write_count values with a real MARF, but determined the runtime values with a mocked MARF.

@pavitthrap does the benchmark with real MARF use HDD or SSD?

It uses a standard persistent disk, so HDD.

pavitthrap avatar Nov 04 '22 14:11 pavitthrap

This should also address #3380 and #3381.

jcnelson avatar Nov 04 '22 17:11 jcnelson

  • It appears Load costs-3 contract  #3266 is needed for 2.1. Could we include it to this PR?
  • Do we have a programmatic check that the ClarityCostFunction enum is complete and not missing clarity functions?

I'll be addressing #3380 and #3381, so I'll see about adding the contract when I do that. I added a programmatic check now.

pavitthrap avatar Nov 04 '22 22:11 pavitthrap

N.B. the benchmarks for I/O should use an SSD, because we don't want seeks and filesystem implementation details to interfere with benchmarks.

jcnelson avatar Nov 07 '22 16:11 jcnelson

Okay @gregorycoppola @obycode @igorsyl @a3slade, I've made a few optimizations to this PR so that the cost functions for the stx-* functions are now only about 20% slower instead of over 8x slower. The reason for this is because we have to determine the current burnchain tip height in order to calculate the consolidated balance, which incurs some extra CPU cycles and I/O for loading up the data from the headers DB and parsing it out.

Once #3389 merges, I'll go and add cost functions for the bitwise operators.

This PR also addresses #3266. costs-3 is loaded as part of the epoch 2.1 instantiation.

In addition, this PR addresses #3381, #3380, and #3311.

jcnelson avatar Nov 15 '22 04:11 jcnelson

Okay, had to revert the implementation of ClarityDB::get_current_burnchain_block_height() because the new one was buggy. But, the other optimizations can remain in place. The stx-* methods are now 2x slower than they were in 2.05, due to the additional burnchain DB MARF query.

jcnelson avatar Nov 15 '22 15:11 jcnelson

transition_empty_blocks test failing

igorsyl avatar Nov 15 '22 20:11 igorsyl