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

Add flag isUsingBlockchainContext to ErgoTree #929

Merged

Conversation

megatron00999
Copy link
Contributor

Fixes #928

Add a flag isUsingBlockchainContext to ErgoTree similar to the existing hasDeserialize.

@megatron00999 megatron00999 changed the title [WIP] Add flag isUsingBlockchainContext to ErgoTree Add flag isUsingBlockchainContext to ErgoTree Nov 5, 2023
@megatron00999 megatron00999 marked this pull request as ready for review November 5, 2023 09:50
@megatron00999
Copy link
Contributor Author

@kushti @aslesarenko Please review and let me know if there is anything else I need to do in this PR. Thank you.

@aslesarenko aslesarenko changed the base branch from develop to v5.0.13-RC November 7, 2023 10:49
@aslesarenko
Copy link
Member

@megatron00999, I targeted the next release branch, please resolve the conflicts.

Copy link
Member

@aslesarenko aslesarenko left a comment

Choose a reason for hiding this comment

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

Please look how _hasDeserialize is initialized
var _hasDeserialize: Option[Boolean] = givenDeserialize
and were givenDeserialize comes from.

The key point is here where ErgoTree is created in the serializer with Some(...) value.
This way no additional traverse is necessary after deserialization, which is the main use case during tx validation and is critical for performance.

@megatron00999
Copy link
Contributor Author

@aslesarenko Updated now. Added the detection of blockchain context during parsing in 44844ac

aslesarenko
aslesarenko previously approved these changes Nov 22, 2023
Copy link
Member

@aslesarenko aslesarenko left a comment

Choose a reason for hiding this comment

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

LGTM, but please fix the tests

@aslesarenko aslesarenko self-requested a review November 22, 2023 13:52
@aslesarenko aslesarenko dismissed their stale review November 22, 2023 13:54

tests need to be fixed first, otherwise LGTM

@megatron00999
Copy link
Contributor Author

Strangely, CalcSha256Specification test failed locally on my system with the following error:

[info] CalcSha256Specification:
[info] - CalcSha256: Should pass standard tests. *** FAILED ***
[info]   AssertionError was thrown during property evaluation. (CalcSha256Specification.scala:35)
[info]     Message: assertion failed: Inconsistent stack at List(sigmastate.eval.CProfiler$OpStat@64a05def, sigmastate.eval.CProfiler$OpStat@42d7f5a7, sigmastate.eval.CProfiler$OpStat@643956ec, sigmastate.eval.CProfiler$OpStat@1a3afd0f)
[info]     Occurred at table row 3 (zero based, not counting headings), which had values (
[info]       string = abcdefghbcdefghicdefghijdefghijkefghijklfghijklmghijklmnhijklmnoijklmnopjklmnopqklmnopqrlmnopqrsmnopqrstnopqrstu,
[info]       hash = cf5b16a778af8380036ce59e7b0492370b249b11e8f07a51afac45037afee9d1
[info]     )

When I reran, the test passed.

@aslesarenko aslesarenko changed the base branch from v5.0.13-RC to v5.0.14-RC December 2, 2023 14:57
@megatron00999
Copy link
Contributor Author

The tests should be fixed now with 0a2390d.

@aslesarenko aslesarenko merged commit ef1fd14 into ergoplatform:v5.0.14-RC Dec 12, 2023
3 of 4 checks passed
@megatron00999 megatron00999 deleted the isUsingBlockchainContext branch December 15, 2023 09:23
@kushti
Copy link
Member

kushti commented Dec 22, 2023

@megatron00999 please reach me over Discord or email [email protected] and provide ERG address (ordinary or stealth) to get the bounty paid

@megatron00999
Copy link
Contributor Author

@kushti Thanks, sent you a message on Discord with the address.

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.

Add a flag on whether blockchain context is used in ErgoTree
3 participants