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

ApplicationLog: New storage implementation for expandability #807

Closed
wants to merge 0 commits into from

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Aug 2, 2023

With this new implementation of "ApplicationLog" you will be easier to expand the "ApplicationLog" for use in neo-node-cli and RPC. Tested with my xunit tests locally.

Note that you need to resync the whole blockchain for this new version of "ApplicationLog"

Change Log

  • New Storage implementation
  • Added new console commands to neo-cli
  • Added lookups by block, transaction and contract in neo-cli
  • Added Sort methods for by event name

Storage Size comparison to blockchain

Sizes BlockChain Application
2.30GB Testnet T5 Blockchain
2.41GB Testnet T5 ApplicationLog

Now you can use ApplicationLog in neo-cli here are the commands

image

Output of Command Console for ApplicationLog

image

image

image

@cschuchardt88
Copy link
Member Author

@dotnet-policy-service agree

@cschuchardt88 cschuchardt88 marked this pull request as draft August 10, 2023 00:55
@cschuchardt88 cschuchardt88 marked this pull request as ready for review August 10, 2023 03:21
@cschuchardt88 cschuchardt88 marked this pull request as draft August 13, 2023 15:13
@Jim8y
Copy link
Contributor

Jim8y commented Aug 13, 2023

what vm in your command means?

@cschuchardt88
Copy link
Member Author

@Liaojinghui means execution of the vm and event and/or notifylog

@cschuchardt88 cschuchardt88 changed the title GetApplicationLog: fixed storage for optimization ApplicationLog: New Storage implementation for expandability Aug 14, 2023
@cschuchardt88 cschuchardt88 changed the title ApplicationLog: New Storage implementation for expandability ApplicationLog: New storage implementation for expandability Aug 14, 2023
@Jim8y
Copy link
Contributor

Jim8y commented Aug 14, 2023

vm is not an intuitive name, how about log. And please update the format with dotnet format

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Aug 14, 2023

@Liaojinghui ok made the changes you requested. I'm not good with names, so thank you.

Edit: if i were to add "GetContractLog" or update "GetApplicationLog" to allow scripthashes for RPC Methods. Is that a bad thing to do? reason being @roman-khimov told me not to change schema. also if its ok to do that, would i have to update "RpcClient" and create the tests? Or is that someone else's job?

@cschuchardt88 cschuchardt88 marked this pull request as ready for review August 14, 2023 11:56
@Jim8y
Copy link
Contributor

Jim8y commented Aug 14, 2023

Change existing interface should be something to avoid as much as we can, it might cause inconsistancy and break applications that rely on it. and you can update rpcclient directly, no need to wait for others.

@shargon
Copy link
Member

shargon commented Aug 16, 2023

@superboyiii could you test it?

@cschuchardt88
Copy link
Member Author

Should issue #665 be added to this PR?

@roman-khimov
Copy link
Contributor

It's a very different problem and problems better be solved one by one.

BTW, getnotificationlog worries me here. I'm OK with just about any internal storage changes for existing APIs, but a new API needs to be discussed additionally. Not that it's bad, I'm just not sure it's very useful (we've worked for a long long time without it) and it requires some additional storage.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Aug 29, 2023

I figured it would be nice addition to add since we have the information. The reason for me adding it is, some people including me just want to get events on the blockchain for a certain contract and we don't know the block hash or index or even a transaction hash. I have jobs running that look for certain events on my contracts to fire than an action to do something i.e. an external application. I can remove it if need be. But instead of adding other plugins with similar storage data requirements. I added it in here. Most plugins don't have external API besides RPC which is limited. No sense in storing the same data again.

edit: plus i want to make the cli command line not be limited in anyway and require users to use a blockchain explorer or have to use Rcp (bad for non-technical users).

@superboyiii
Copy link
Member

Very great feature. I will test it on testnet first!

Jim8y
Jim8y previously approved these changes Sep 29, 2023
@Jim8y
Copy link
Contributor

Jim8y commented Nov 8, 2023

Very great feature. I will test it on testnet first!

@superboyiii Any update?

@superboyiii
Copy link
Member

superboyiii commented Nov 10, 2023

@cschuchardt88 How about supporting log block <blockhashOrIndex> [eventName=null], this could be more convenient?

@cschuchardt88
Copy link
Member Author

Currently the store i created doesn't not store Indexes, But i guess i could query hash than use the hash for my store. I'll look into it.

@superboyiii
Copy link
Member

superboyiii commented Nov 10, 2023

Currently the store i created doesn't not store Indexes, But i guess i could query hash than use the hash for my store. I'll look into it.

Maybe just like

return NativeContract.Ledger.GetBlockHash(snapshot, height).ToString();
to get BlockHash?

@Jim8y
Copy link
Contributor

Jim8y commented Nov 10, 2023

@shargon @roman-khimov

@roman-khimov
Copy link
Contributor

@Liaojinghui, #807 (comment) And we've only got worse wrt this. Each logical change should be tracked and evaluated separately. Right now we have too many of them here to me. I'd start with storage schema change (that is strictly something not affecting any of external interfaces, pure optimization), then any addons (and CLI better be split from RPC ones). It'd be much easier to review, track and communicate to the outside world then (docs/changelogs).

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Nov 10, 2023

@superboyiii for #807 (comment) is commenting the code out ok? or you want it removed completely?

Edit:

Nevermind i removed the code. Ill create new PR for RPC Methods after this PR is merged.

@cschuchardt88
Copy link
Member Author

LogEventArgs are now working

image

From Testnet T5

{
    "jsonrpc": "2.0",
    "id": 1,
    "result": {
        "txid": "0x49e493cee3798bd0e6f2e745ac75291534b290fa4033ac36bed9204d429ead4c",
        "executions": [
            {
                "trigger": "Application",
                "vmstate": "HALT",
                "exception": null,
                "gasconsumed": "264528",
                "stack": [
                    {
                        "type": "ByteString",
                        "value": "SGVsbG8sIGJvYg=="
                    }
                ],
                "notifications": [],
                "logs": [
                    {
                        "contract": "0xa6cd0a163167fc37b24cc3b226342474e853a30a",
                        "message": "Hello, bob"
                    }
                ]
            }
        ]
    }
}

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Nov 13, 2023

@superboyiii @Liaojinghui maybe you can help me with a problem. Trying to get all LogEventArgs for a transaction. But I get duplicates because the node is sending that transaction and than processing the transaction again once processed by the blockchain. Inturn emits LogEventArgs twice. Maybe an Issue to add LogEventArgs to Blockchain.ApplicationExecuted?

@Jim8y
Copy link
Contributor

Jim8y commented Nov 13, 2023

I have considered this issue, but solution would change the core, so i suggest to keep it this way for a while. We can fix it after the monorrpo merge.

@cschuchardt88
Copy link
Member Author

Let me know when the new release of neo-core is here in this repo so i can update

Now changed to public static byte[] Serialize(StackItem item, ExecutionEngineLimits limits) in neo-project/neo#2948

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

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

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

Tested. It's good for me now.

@Jim8y
Copy link
Contributor

Jim8y commented Nov 22, 2023

@shargon

@superboyiii
Copy link
Member

Could you have a look? @shargon

@shargon
Copy link
Member

shargon commented Nov 24, 2023

Could you have a look? @shargon

Yes, but I need more time

@cschuchardt88
Copy link
Member Author

Any update @shargon on this?

sizeof(byte) +
base.Size;

public override void Deserialize(ref MemoryReader reader)
Copy link
Member

Choose a reason for hiding this comment

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

Deserialize content always come from a Serialize output?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm overriding the base class of NotifyLogState in turn inherits ISerialize

Copy link
Member Author

@cschuchardt88 cschuchardt88 Dec 26, 2023

Choose a reason for hiding this comment

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

We initially create it 1st than serialize it, Hints the Create static method, above. But to answer question yes.

Copy link
Member Author

Choose a reason for hiding this comment

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

@shargon any news?

Copy link
Member

Choose a reason for hiding this comment

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

I need more time sorry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants