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

Store tx final application state on block #712

Closed
igormcoelho opened this issue Apr 26, 2019 · 11 comments
Closed

Store tx final application state on block #712

igormcoelho opened this issue Apr 26, 2019 · 11 comments
Labels
discussion Initial issue state - proposed but not yet accepted enhancement Type - Changes that may affect performance, usability or add new features to existing modules. ledger Module - The ledger is our 'database', this is used to tag changes about how we store information

Comments

@igormcoelho
Copy link
Contributor

I think it's important to persist on blocks the final state of the application transactions, for example, FAULT or HALT.
It's a single extra byte per tx, but the gains with this are huge. The capability of evolving the chain will also be larger, as past faulted/halted events will be guaranteed to stay in the same state.

As discussed on other threads, this complements audit capabilities, including MPT for storages, MPT for past transactions (including return value) and MPT for past blocks. Notifications can perhaps be kept on another MPT.

@shargon
Copy link
Member

shargon commented Apr 26, 2019

Agree!

@vncoelho vncoelho added discussion Initial issue state - proposed but not yet accepted potential enhancement and removed potential feature labels Apr 26, 2019
@vncoelho
Copy link
Member

vncoelho commented Apr 26, 2019

We are doing a draft of this implementation, but only during export and import.
In this sense, we are not really including this state into the blocks.

@shargon
Copy link
Member

shargon commented May 4, 2019

Any advance @vncoelho ? can I help you with this ?

We have two alternatives, change public Transaction[] Transactions; to TransactionState -> With transaction and state, or create a new property VMState[] TransactionStates with the state of every TX, i think that is better the first one, because we could want to store the result, or something more in the future.

@vncoelho
Copy link
Member

vncoelho commented May 6, 2019

I had thought about just during export and import.
But if the idea is to store direct in the block let's proceed with it.
In the future, If the MPT provides new tools enabling it to be removed we revert it.
However, we should be aware that it would be a breaking change.

@vncoelho
Copy link
Member

@igormcoelho, following our recent discussion, is this still being considered?

@shargon
Copy link
Member

shargon commented Jun 11, 2019

For me this is very important

@igormcoelho
Copy link
Contributor Author

This is a thing we need to decide... solid states and less bug fixing possibilities, or else? 🤔
another problem is that tx results require actually executing all applications at the time block is made, or before... so, less tps too..

@Tommo-L
Copy link
Contributor

Tommo-L commented Aug 8, 2019

Could we store the stateRoot and receiptRoot in the header, is it better than this?

@igormcoelho
Copy link
Contributor Author

igormcoelho commented Aug 10, 2019

Could we store the stateRoot and receiptRoot in the header, is it better than this?

@Tommo-L we are going in a slightly different direction... it's better than stateRoot on header. I'll explain.

We have discussed in other posts, alternatives to definitive solid states on blocks, that may cause network forks when bugs are fixed. From a "code-is-law" perspective, codes and bugs are "absorbed" into protocol, but we (at least myself) are defending a "protocol-is-law" perspective, where code bugs are treated as bugs, not protocol decisions, so they need fixing without causing network forks.

If we put a stateRoot field, two bad things will happen:

  1. Bugs will never be fixed, as long as they affect blockchain state (and they usually do)
  2. The CN needs to execute all transaction applications (including storage updates, etc) during block proposal time, and other CN also need to execute the whole thing (to verify stateRoot), before even responding if they agree or not to the block on dBFT. This takes time, and will strongly affect high TPS capabilities of Neo.

So, this path we are proposing is very novel on blockchain world, in my opinion, and may require three things in my opinion:

  1. Guarantee that applications finish execution after block is persisted, before next block proposal: Dettached Verification + Application: ensure delay is at most one block #965 (this is easy)
  2. Distribute stateRoot via P2P (not on blocks), after every node finishes its execution (so that other nodes may audit their own implementations): [Neo3] Distribute state hash via p2p messages #901
  3. To create a solid verification mechanism, I'm defending in this PR that we store the FAULT state on the current or next block, so that a failed tx never becomes "unfailed" in the future. I'll have to adjust the text up there, because it's slightly different now... I think that non-FAULT tx may be faulted on the future, if some bug is discovered, although this is not a desirable thing.
  4. Users can defend themselves naturally, by simply adopting a Solid State Transfer NEP (even if blockchain doesn't include storage audits by state tries): Solid State Transfers proposals#97

I think we need to better discuss this, to find a "perfect" solution, because depending on next block to be sure on a FAULT state is like looking to the future, to decide the present 😂 and the future may not be available...
We can also decide to execute everything on block proposal/dBFT time, as long as we know that it's feasible and very fast on practice, for hundreds/thousands of tx. It would be nice to reduce dBFT time too, from 15 seconds to much less... 10... or 5 secs.. so this tightens even more this decision here.

What do you think @Tommo-L , is it going in a reasonable direction?

@lock9 lock9 added enhancement Type - Changes that may affect performance, usability or add new features to existing modules. ledger Module - The ledger is our 'database', this is used to tag changes about how we store information and removed potential enhancement labels Aug 12, 2019
@shargon
Copy link
Member

shargon commented Sep 12, 2019

I think that this was done

@lock9
Copy link
Contributor

lock9 commented Nov 25, 2019

We think this is done. I'm closing it but if you think this is a mistake, please reopen this issue.

@lock9 lock9 closed this as completed Nov 25, 2019
Thacryba pushed a commit to simplitech/neo that referenced this issue Feb 17, 2020
Thacryba pushed a commit to simplitech/neo that referenced this issue Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Initial issue state - proposed but not yet accepted enhancement Type - Changes that may affect performance, usability or add new features to existing modules. ledger Module - The ledger is our 'database', this is used to tag changes about how we store information
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants