Move everything into LedgerContract

[erikzhang]

No description provided.

[erikzhang]

Any one can help me to fix the UTs? 😅

[chenzhitong]

I'll try.

[shargon]

Missing VMState?

[erikzhang]

Yes. I removed it.

[shargon]

I think that it's useful for explorers know if the transaction was fault.

[erikzhang]

Explorers can use ApplicationLogs.

[shargon]

I think that only cost one byte and it's a useful information

[erikzhang]

If you need VMState, inLedgerContract.OnPersist you have to run the script of all the transactions.

[chenzhitong]

How to get transaction.BlockIndex, Is this correct?

TransactionState txState = Blockchain.Singleton.View.Transactions.TryGet(hash);
var blockIndex = txState.BlockIndex;

↓

using var snapshot = Blockchain.Singleton.GetSnapshot();
Transaction tx = NativeContract.Ledger.GetTransaction(snapshot, hash);
TransactionState txState = new TransactionState()
{
    Transaction = tx,
    BlockIndex = NativeContract.Ledger.CurrentIndex(snapshot) - tx.ValidUntilBlock
};
var blockIndex = txState.BlockIndex;

[vncoelho]

I like this clean and organization.
In particular, empowering Native Contracts is a promising direction in our opinion.
It is better for users, maintenance and general view of the system.

[roman-khimov]

This move adds unnecessary pressure on MPT and breaks tail cutting (MaxTraceableBlock) because the node that deletes blocks/transactions after MaxTraceableBlock will have a different state from the one that doesn't do that.

Does it solve any real problem?

[erikzhang]

What do you think? @neo-project/core

[shargon]

I am worry about the TPS, I think that it could be slower

[erikzhang]

I am worry about the TPS, I think that it could be slower

Why do you think that it could be slower?

[shargon]

Why do you think that it could be slower?

There are more method calls, the key is more complex, and also its converted to StackItem before serialization, so serialization is less optimized.

[vncoelho]

In terms of design I believe it is the correct design, appropriated in terms of security, transparency and practice.
It is an innovative use of certificates aligned with Native contracts.

I still see other possible important Native contracts in a near future, such as MultiSign composing that we are discussing and other mechanisms.

In terms of computation costs, I believe it is almost the same.
The MPT could be outside as we have been suggesting, but since it is now embedded we should not block innovative features because of this choice.

[roman-khimov]

we should not block innovative features

Sure, but what this feature really buys us? What is the problem we're trying to solve?

Apart from problems mentioned above we're currently defining state as a set of contract storage key-value pairs. Which are side-effects of block/transaction executions. But they're side-effects, not blocks and transactions themselves. The difference is that blocks (with transactions included) are easy to obtain via P2P while state is not, it requires some processing overhead to get it (even if we're to imagine P2P state exchange mechanism that would imply synchronization to some height H and while we're getting it the chain would advance to H+X). So to me adding blocks to contract storage space just adds unnecessary overhead, it's not a state, it's what state is derived from.

It also makes it harder to optimize some things, like nspcc-dev/neo-go#1586 or trying to move block/transaction serialization in a separate thread (it's somewhat visible in the profiler and yes, I have to confess, I have already tried that in neo-go).

[vncoelho]

defining state as a set of contract storage key-value pairs

That is the state.

The MPT and other mechanisms are certificates.

[erikzhang]

This move adds unnecessary pressure on MPT

We can exclude the storage of some contracts from the calculation of MPT.

Sure, but what this feature really buys us? What is the problem we're trying to solve?

For me, it provides a unified aesthetic design that makes everything in the contracts.

[igormcoelho]

Hi all, I think this is an innovative approach, couldn't look into implementation details though... but one very important indicator is this basic code diff summary: +1,302 −2,204
I don't know if proposal is far from complete, but judging it's not, it may be really interesting to see the ledger as an active contract inside the network. As long as each contract hold its logic, and they manage to interoperate to make the network fulfill its purpose (providing a global state for turing complete computation in decentralized p2p environment), it looks nice to me.
Regarding performance, unless we are at the microbenchmarking level, it's quite hard to assess the impact of every operation, so again I'll quote "+1,302 −2,204"... small codes tend to be much easier to review and maintain, therefore, much easier to find the real bottlenecks, in some near future.

[erikzhang]

Maybe we can merge first and optimize it later.

[roman-khimov]

The difference is quite significant and I'm concerned with block time metric which usually is quite stable for C# node, but this change almost doubles it compared to master branch at the end of neo-bench run (with 1M transactions). So I'd rather wait for performance fixes (at least proving that they're possible in this case).

[vncoelho]

I prefer to merge and optimize later in order to keep compatibility and maintenance.

[Tommo-L]

Maybe we can merge first and optimize it later.

Agree, as the change is relatively large, we need to merge it first to update other modules. And we still will continue to investigate the performance.

[superboyiii]

If we make sure it will be added to Preview5, then we should ensure function complete in every repo. Performance can be improved on the second step I think. So maybe we could merge it first and make other repos ready, then we could improve it later together.

[Qiao-Jin]

After retesting it shows TPS between master and this PR is somehow similiar. Difference in previous result is due to test data incompatibility.