-
Notifications
You must be signed in to change notification settings - Fork 723
Export events in readonly endpoints #6741
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
Open
rob-stacks
wants to merge
11
commits into
stacks-network:develop
Choose a base branch
from
rob-stacks:feat/readonlycall_events
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+420
−239
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
611fb20
export events in readonly endpoints
rob-stacks cb376c9
updated changelog and openapi
rob-stacks ddff527
fixed unit test
rob-stacks 3b85f12
refactored events gathering
rob-stacks cbf0ebb
added keep_event_batches
rob-stacks 126afd8
refacotred events management
rob-stacks 2938895
refactored contract calls events management
rob-stacks 9b45a03
reverted change for event batch
rob-stacks dfdb2fa
added test for inner events
rob-stacks f068733
nit fixes
rob-stacks 418d09d
merged with develop
rob-stacks File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having some difficulty reasoning about what this returns. This makes it so that the
execute_function_as_transaction_and_eventsreturns the events at the time of the outermost call -- ignoring the result of the transaction or the read-only-ness but only for the outermost call.Imagine like the following:
Then I think what we'd see is:
Which would be weird, right?
In normal clarity execution, read-only calls won't emit events: they get rolled back. So block events won't include them. With this change, the RPC endpoitn would return block events for those read-only calls, but only the outermost calls. The same logic applies to failing txs (seems bad, even though I'm not sure that failed txs are frequently invoked via the read-only endpoint.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and tested this on @rob-stacks's branch:
I also expanded your test case:
Looking at these events, the only event which does not get captured is the innermost event generated by
(print "foo"). Meaning:Environment<_', _'>will result in theGlobalContextcapturing all events, except for the inner-most one it seems.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checking it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have extended the test contract for having nested calls (and related events generation). As reported by @jcnelson all of the events are reported (in the order they are generated). I was not able to reproduce the issue with the innermost (but probably it was caused by some earlier commit that i reverted in the process)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe that this is correct. If you can't replicate this in testing, I think that's because testing is insufficient. The code definitely drops events for innermost read-only calls and error results for transactions (but does not do either of those things for outermost read-only calls and error results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In more detail, the returned events Vec is not used anywhere but in the API invocations. This means that when transactions are nested at all, the returned events Vec won't be added before the events get rolled back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your example is actually different from the one I posted:
Without this PR, I don't think any of these produce events at all -- the read-only function's events are rolled-back when the VM rolls back the read-only execution.
bar's invocation is rolled-back because it returnserr u1, so thebarevent is not emitted. The same is true forbaz.With the changes in this PR, the gathering of events before the rollback for API invocations means that the outermost invocations save those events before they get rolled back. In the case of
errresults, that seems bad. In the case of read-only results, that seems good because its the point of this PR, but the fact that its only working on the outermost invocations comes down to the way that this PR is gathering events (by pulling them out of the batch a the outermost call, after rollbacks have been applied in internal calls).Your patch in this thread I think confuses matters further -- none of the examples above produce any events in the CLI with your patch. That's because all of them are rolled back before your patch does any event collection.
The higher-level point here is that the current event system specifically goes through the effort of dropping events whenever mutations are being rolled back (relevant times when this happens: read-only calls, transaction failures, at-block invocations). The changes in this PR (and your patch) only collect events from an outermost call, meaning that any such dropped events won't be picked up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually news to me. Is this API contract spelled out anywhere? Is it covered by unit tests?
Okay, yes, I can see that now, given where all the calls to
global_context.roll_back()happen.Okay, so a new strategy is needed here. @aaronb-stacks would it be okay to add a way to buffer up event batches in
GlobalContextthat would not be rolled back viaroll_back()? This would only be consumed by the RPC endpoint, and these events would not be gathered by default. The caller would need to request it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think my preference here would be add some kind of callback fn/hook to the event batch struct. That way, the RPC endpoint can specify exactly how it wants to deal with events and it won't impact any other callers.