OnPersist and PostPersist have the same application log hash

[fyrchik]

Describe the bug
After addition of postPersist method, we have 2 scripts with their own application logs (onPersist and postPersist), which are both saved and queried by the block hash.
https://github.com/neo-project/neo-modules/blob/master/src/ApplicationLogs/LogReader.cs#L78

Expected behavior
Both onPersist and postPersist scripts should be accessible for query by getapplicationlog RPC.

[roman-khimov]

IMO the situation is a little worse than just an RPC problem, because it's impossible to differentiate between these two ApplicationExecuted events generated by the system, so all users of this event stream might be affected.

[Tommo-L]

I think it's reasonable, onPersist and postPersist, which are both saved and queried by the block hash.

[roman-khimov]

So, how do we store both with the same key and which one do we return when there is a getapplicationlog request for block hash?

[Tommo-L]

I got your point, they both use the same storage key.

[erikzhang]

I prefer to use different triggers.

[shargon]

Does we need to publish the result Context.System.EventStream.Publish(application_executed); ?

[roman-khimov]

Absolutely. Events are important and events can be generated in any execution context, be it block or transaction. And they're generated already for blocks (GAS burn/mint events).

[Tommo-L]

I prefer to use different triggers.

Any more details about "different triggers"?

[superboyiii]

@erikzhang Any solution? Or shall we move it to the version after preview4?

[erikzhang]

https://github.com/neo-project/neo/blob/6fc4161258408cae3d754cab784dcee2235b63ef/src/neo/SmartContract/TriggerType.cs#L5-L27

We can use different triggers to call these two methods.

[roman-khimov]

I don't think we can close this one. neo-project/neo#2022 makes it possible to solve this, but we still need to decide how to store and request these.

[Tommo-L]

#380 fixed?

[roman-khimov]

Not yet, it solves storage problem, but getapplicationlog needs to be updated to take this change into account. The way #380 is now actually breaks the plugin completely, GetApplicationLog still does db.Get(ReadOptions.Default, hash.ToArray()) and now the key has changed.

We'll probably need another parameter for getapplicationlog (trigger) and then decide on behavior for requests lacking this parameter (for transactions that's easy, but for blocks I'd probably answer with an array of execution results for different triggers).

[Tommo-L]

Close? I think it was fixed by #380

[roman-khimov]

Yep, I think we're fine now. We've implemented the same scheme in neo-go and it works.