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

Automatic reference script #449

Merged
merged 95 commits into from
Sep 12, 2024
Merged

Automatic reference script #449

merged 95 commits into from
Sep 12, 2024

Conversation

mmontin
Copy link
Collaborator

@mmontin mmontin commented Aug 30, 2024

This PR will close #447 but allowing to implicit search for correct reference scripts in known utxos. In the process, this will also close #438.

In practice, this means that putting any script on utxos will be sufficient for cooked to find those scripts and use them as reference whenever required. In particular, putting reference scripts in the initial distribution will be even more useful.

A drawback of this approach is that there will be no visual difference between redeeming something with of without a reference script. To compensate, a new log entry is generated whenever cooked assigns a reference script by itself.

This automated addition of reference scripts can be turned off and it will still be possible to manually provide a reference input with a script attached when redeeming a script.

This is rebase on #450

@mmontin mmontin changed the base branch from main to mm/tests-on-logging September 10, 2024 13:15
Base automatically changed from mm/tests-on-logging to main September 11, 2024 15:14
@mmontin mmontin marked this pull request as ready for review September 11, 2024 15:28
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Show resolved Hide resolved
src/Cooked/Pretty/Cooked.hs Outdated Show resolved Hide resolved
-- reference scripts and compare their hashes, thus will slightly reduce
-- performance, especially when handling a lot of utxos.
--
-- Defaut is 'True'.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it be False by default if this really slows down things by default?
Or is there an optimization to be found such as setting a flag or adding an entry in a map of known ref scripts (script hash to tx out ref) in the chain state when a utxo with a ref script is produced? Could this improve performance by not going through everything?

Copy link
Collaborator Author

@mmontin mmontin Sep 12, 2024

Choose a reason for hiding this comment

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

From our current test suite, the overhead is minimal, also it takes a slightly bit more time. I have thought about such optimization, but it has drawbacks:

  • it duplicates information
  • This map would be hard to maintain because utxos disappear when consumed. So whenever a transaction is validated, we would have to check whether utxos from this map have been consumed to remove them, but only do that in case of successful validation of the transaction. We would also have to manually add them to the map when they are created.

Overall I believe the current solution is better (because simpler) unless we discover at some point that is really hurts performance.
Also, if we want to address possible future performance issues, we know that this is an area that can offer some improvement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

All of this is debatable. This optimization is clearly doable in practice.

Copy link
Member

@florentc florentc Sep 12, 2024

Choose a reason for hiding this comment

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

In my opinion, duplicated information is not a problem. The overhead in terms of space is minimal (there will never be more than a handful of entries in that map and even if we had millions, what is a few MB of ram). However we already often find execution time to be a bit long. Not on one single trace, but in a test suite / audit test suite it quickly adds up to annoying seconds.

On the one hand, space is not really an issue, and there are never a lot of ref scripts anyways so the map will always be small. On the other hand, having a lot of utxos is not uncommon at all: big initial distributions, or sometimes we even do stress tests with a huge number of utxos during certain specific attack attempts.

Therefore I think in terms of scaling, the alternative implementation with a map of known ref scripts and their location is way better.

I would be in favor of either:

  • switch to the second implementation, even if it involves a few additions during validation to update the map
  • OR keep the current implementation for the time being but turn it off by default and explain the tradeoff in the documentation. I don't think this feature is needed right away. It could be an experimental feature that is disabled by default until we implement the optimized version.

src/Cooked/Skeleton.hs Outdated Show resolved Hide resolved
src/Cooked/MockChain/AutoReferenceScripts.hs Outdated Show resolved Hide resolved
src/Cooked/MockChain/AutoReferenceScripts.hs Show resolved Hide resolved
src/Cooked/MockChain/AutoReferenceScripts.hs Outdated Show resolved Hide resolved
@mmontin mmontin merged commit 7b4aadd into main Sep 12, 2024
6 checks passed
@mmontin mmontin deleted the mm/auto-ref-scripts branch September 12, 2024 15:21
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.

Improving the use of reference scripts Improve naming of redeemers smart constructors in skeleton
2 participants