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

Feat: update pessimistic cost estimator to differentiate by contract sender #2984

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

kantai
Copy link
Member

@kantai kantai commented Jan 6, 2022

This change adds a field to the pessimistic cost estimator's lookup key. It doesn't change the table schema, so it's compatible with existing cost estimator tables, though it will end up resetting cost estimates. This change allows the cost estimator to differentiate between contracts with different contract senders. In most respects, this PR is a fix for incorrect behavior on the part of the pessimistic estimator, but I'm not sure this rises to the level of necessitating a hotfix, as the cost estimator's current behavior still allows miners to progress the network, albeit slightly less efficiently. If necessary, though, this could be rebased as a hotfix very easily (the patch is essentially a one line change).

@gregorycoppola
Copy link
Contributor

So you are adding the address of the user calling the contract?

What motivates this change? What problem is it solving and what data suggests it is working?

@codecov
Copy link

codecov bot commented Jan 6, 2022

Codecov Report

Merging #2984 (8750b69) into develop (dc34f9e) will decrease coverage by 0.15%.
The diff coverage is 78.69%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2984      +/-   ##
===========================================
- Coverage    82.79%   82.63%   -0.16%     
===========================================
  Files          235      237       +2     
  Lines       190437   193149    +2712     
===========================================
+ Hits        157672   159616    +1944     
- Misses       32765    33533     +768     
Impacted Files Coverage Δ
src/chainstate/stacks/db/mod.rs 86.78% <0.00%> (-0.06%) ⬇️
src/codec/macros.rs 100.00% <ø> (ø)
src/core/mod.rs 98.01% <ø> (ø)
src/lib.rs 100.00% <ø> (ø)
src/libclarity.rs 0.00% <ø> (ø)
src/net/neighbors.rs 38.31% <0.00%> (-0.99%) ⬇️
src/util/mod.rs 87.80% <ø> (ø)
src/net/rpc.rs 30.60% <18.85%> (-0.44%) ⬇️
src/net/mod.rs 67.42% <33.33%> (-0.34%) ⬇️
src/net/p2p.rs 61.34% <35.79%> (-3.78%) ⬇️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd3f28d...8750b69. Read the comment docs.

@kantai
Copy link
Member Author

kantai commented Jan 6, 2022

So you are adding the address of the user calling the contract?

No -- this is the contract publisher address.

What motivates this change? What problem is it solving and what data suggests it is working?

Without differentiating by contract publisher, two contracts with the same name, but different publishers, would share an estimate, even though the contracts are actually different contracts. This isn't rooted in data, but it is definitely incorrect behavior (which is why this is isn't necessarily a hotfix), and the added unit test demonstrates the correct behavior (separation between different contracts).

Copy link
Contributor

@gregorycoppola gregorycoppola left a comment

Choose a reason for hiding this comment

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

Oh I see. yes this would seem to be a logical bug.

Copy link
Contributor

@pavitthrap pavitthrap left a comment

Choose a reason for hiding this comment

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

lgtm!

@kantai kantai merged commit 98cdf3d into develop Jan 7, 2022
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

Successfully merging this pull request may close these issues.

4 participants