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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

ApplicationLog - New Storage implementation #865

Merged
merged 13 commits into from
Mar 20, 2024

Conversation

cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Jan 9, 2024

See PR #807

I close it by accident. Because I had it on my master branch 馃尶


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

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

image

It looks all good.

superboyiii
superboyiii previously approved these changes Jan 10, 2024
@superboyiii
Copy link
Member

@shargon here

@shargon
Copy link
Member

shargon commented Jan 10, 2024

I need more time to review it

@roman-khimov
Copy link
Contributor

It's hard to review, because there are a lot of things mixed here. Storage scheme refactoring and extensions better be separated. I'm mostly worried about API (JSON-RPC) backwards compatibility here.

Do I understand correctly that in the current form there are no new APIs added, only getapplicationlog implementation has changed?

@cschuchardt88
Copy link
Member Author

@roman-khimov That is correct. Only the way things are stored, with some in additions to the command-line CLI commands. The RPC functions you see are the same output just different logic.

Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

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

Why it's reverted?

@shargon shargon self-requested a review January 10, 2024 20:37
@cschuchardt88
Copy link
Member Author

What do you mean @shargon ? I updated file headers and formating ffa303c

@cschuchardt88
Copy link
Member Author

nvm i see let me change that

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 10, 2024

@roman-khimov @shargon @superboyiii @vncoelho
I forgot to mention #841 is added. But only if DebugMode is enabled in the config.json. However we did talk about it in the PR before it closed. Issue will have links.

@shargon
Copy link
Member

shargon commented Jan 11, 2024

@cschuchardt88 I see that we create some Guid for storage enteies, that means that two nodes with the same data won't be able to compare the stored data (as raw), and that's a problem

@cschuchardt88
Copy link
Member Author

Ok, it can be changed. But can you explain more in details on what you are or need to do or the reason and what you are trying to compare.

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 11, 2024

@shargon
After thinking about it. I have indexing for fast lookups. So how do you handle that with other plugins? I know we want to use BlockHash, TxHash or ContractHash but a search would be slow.... and DDOS-able

@shargon
Copy link
Member

shargon commented Jan 11, 2024

Ok, it can be changed. But can you explain more in details on what you are or need to do or the reason and what you are trying to compare.

@superboyiii usually did to ensure that the storage didn't change with the changes.

@cschuchardt88
Copy link
Member Author

@shargon but this plugin data doesn't really matter? I thought blockchain data like Transactions, Blocks and Contract are only important. Plus with the indexing you can't compare and that limits us on what we can do. This is one reason why lookups are so slow and ddos-able.

@shargon
Copy link
Member

shargon commented Jan 11, 2024

@shargon but this plugin data doesn't really matter? I thought blockchain data like Transactions, Blocks and Contract are only important. Plus with the indexing you can't compare and that limits us on what we can do. This is one reason why lookups are so slow and ddos-able.

@superboyiii you only check certains prefixes?

@cschuchardt88
Copy link
Member Author

@shargon

I think @superboyiii only checks RPC-JSON data. He already tested and approved, i thought in other PR before closed.

You know how big the database would be if i copy the data to each of these "tables"? Each one is an lookup index for the most part, each bring a different way to look up something. Because if you remember I had a RPC-JSON method for GetContractLog and its still there but only functions for looking up but no JSON-RPC, hints the CLI commands.

private static readonly int Prefix_Id = 0x414c4f47;                 // Magic Code: (ALOG);
private static readonly byte Prefix_Engine = 0x18;                  // Engine_GUID -> ScriptHash, Message
private static readonly byte Prefix_Engine_Transaction = 0x19;      // TxHash -> Engine_GUID_List
private static readonly byte Prefix_Block = 0x20;                   // BlockHash, Trigger -> NotifyLog_GUID_List
private static readonly byte Prefix_Notify = 0x21;                  // NotifyLog_GUID -> ScriptHash, EventName, StackItem_GUID_List
private static readonly byte Prefix_Contract = 0x22;                // ScriptHash, TimeStamp, EventIterIndex -> txHash, Trigger, NotifyLog_GUID
private static readonly byte Prefix_Execution = 0x23;               // Execution_GUID -> Data, StackItem_GUID_List
private static readonly byte Prefix_Execution_Block = 0x24;         // BlockHash, Trigger -> Execution_GUID
private static readonly byte Prefix_Execution_Transaction = 0x25;   // TxHash -> Execution_GUID
private static readonly byte Prefix_Transaction = 0x26;             // TxHash -> NotifyLog_GUID_List
private static readonly byte Prefix_StackItem = 0xed;               // StackItem_GUID -> Data

@Jim8y Jim8y added waiting for review Need Active Pr will be closed after one week if no new activity. labels Feb 5, 2024
@cschuchardt88
Copy link
Member Author

@shargon @Jim8y This has been tested already by @superboyiii. I deleted branch by mistake cause it was on my master. So i reopened it here.

@shargon
Copy link
Member

shargon commented Feb 13, 2024

I will review it likely the next week (working of bug fixes now).

@superboyiii
Copy link
Member

It's good. Let's move on. @shargon

vncoelho
vncoelho previously approved these changes Mar 1, 2024
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I also tested and have been running on neocompiler for some time now.

@superboyiii
Copy link
Member

@shargon Could you have a review on this?

src/ApplicationLogs/ApplicationLogs.csproj Outdated Show resolved Hide resolved
public virtual void Deserialize(ref MemoryReader reader)
{
// It should be safe because it filled from a transaction's notifications.
uint aLen = reader.ReadUInt32();
Copy link
Member

Choose a reason for hiding this comment

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

ReadVarInt and WriteVarInt in all of them?

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 do it this way so; there is no extra byte for type of Integer.

@vncoelho vncoelho removed the Need Active Pr will be closed after one week if no new activity. label Mar 19, 2024
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

I have been running this branch for sometime without problems.
However, we need @superboyiii to re-ensure that there is not state problems and I think it is ok.

@vncoelho vncoelho merged commit 4274076 into neo-project:master Mar 20, 2024
4 checks passed
@cschuchardt88
Copy link
Member Author

馃槃 My 1st PR merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants