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

Auxiliary data structures for tx re-verification #812

Closed
vncoelho opened this issue Jun 11, 2019 · 14 comments
Closed

Auxiliary data structures for tx re-verification #812

vncoelho opened this issue Jun 11, 2019 · 14 comments
Labels
Consensus Module - Changes that affect the consensus protocol or internal verification logic Design Issue state - Feature accepted but the solution requires a design before being implemented 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

@vncoelho
Copy link
Member

vncoelho commented Jun 11, 2019

We probably must define a special balance structure for efficiently accounting remaining GAS of senders in order to quickly re-verify.

Scenario 1:

  • Imagine that an address has 10 GAS ans sends 10 TXs with 1 GAS each as network_fees.
  • All these TXs enters in the same block
  • We need to be sure that tx_1 will not spend any GAS (that is why balance need to be updated before)
@vncoelho vncoelho added this to the NEO 3.0 milestone Jun 11, 2019
@vncoelho vncoelho added the Discussion Initial issue state - proposed but not yet accepted label Jun 11, 2019
@vncoelho
Copy link
Member Author

After persisting, first of all:

  • Process all TXs with Payable_Flag. Tx's should allow Payable and not the contract (or both).

@jsolman, we would change the unverified and verified of the mempool.
The idea would be to update the balances and mark transactions with negative balances as not valid (removing them from mempool).

@vncoelho
Copy link
Member Author

We are planning to include this in the specification before implementation.

@erikzhang
Copy link
Member

We need to be sure that tx_1 will not spend any GAS

Why?

@igormcoelho
Copy link
Contributor

igormcoelho commented Jun 13, 2019

I think Vitor mentioned this case Erik (imagine 5 transactions in a block, all from the same address A):
Balance(A) = 7 GAS

Tx A1:  1 GAS network fee
Tx A2:  1 GAS network fee
Tx A3:  1 GAS network fee
Tx A4:  1 GAS network fee
Tx A5:  1 GAS network fee
ok: final balance of A is 7-5 = 2 GAS

Now imagine that A2 and A3 will perform some payable operation, with 2 GAS each:

Tx A1:  1 GAS network fee
Tx A2:  1 GAS network fee
    -> payable operation 2 GAS (ok)
Tx A3:  1 GAS network fee
    -> payable operation 2 GAS (FAULT)
Tx A4:  1 GAS network fee
Tx A5:  1 GAS network fee
ok: final balance of A is 7-5-2 = 0 GAS

Note that Tx A3 should FAULT, otherwise it will consume resources from the network fee of Tx A4 and Tx A5. This is what he meant I guess...
We need to process all network fees before entering any Application trigger on block.

@igormcoelho
Copy link
Contributor

The logic we are thinking for Neo 3.0 is the following Erik, can you confirm?

Tx A1:  
Verification: 1 GAS network fee  ("transfer" -> "ConsensusAddress")
Application:  Invoke token TKN transfer... example!
-----------------------
Tx A2:  
Verification: 1 GAS network fee  ("transfer" -> "ConsensusAddress")
                       transfer 2 GAS -> "ContractX address" (pre transfer to contract)
Application:  Invoke ContractX, and it requires to pay someone else 2 GAS...
                                    -> payable operation 2 GAS (ok)
-----------------------
Tx A3:  1 GAS network fee
Verification: 1 GAS network fee  ("transfer" -> "ConsensusAddress")
Application:  Invoke ContractX, and it requires to pay someone else 2 GAS...
                                    -> Invoke Application GAS-Native-NEP-5 Transfer operation 2 GAS (FAULT)
-----------------------
Tx A4:  
Verification: 1 GAS network fee  ("transfer" -> "ConsensusAddress")
Application:  Invoke token TKN transfer... example!
----------------------------------------------
Tx A5:  
Verification: 1 GAS network fee  ("transfer" -> "ConsensusAddress")
Application:  Invoke token TKN transfer... example!
----------------------------------------------
ok: final balance of A is 7-5-2 = 0 GAS

@erikzhang
Copy link
Member

Are we not like this now?

@igormcoelho
Copy link
Contributor

igormcoelho commented Jun 19, 2019

It's just like this, specifically for GAS tokens. But I understood that balance will be managed manually for GAS, not directly on storage operations, so the final effect is the same. As long as verifications are consumed before applications, and managed Application.Send() operations lock the funds before the actual send, things will be fine.

@lock9
Copy link
Contributor

lock9 commented Jul 30, 2019

@neo-project/core closeable?

@vncoelho
Copy link
Member Author

It is still open discussion, not yet defined or implemented on Neo3.

@shargon
Copy link
Member

shargon commented Jul 31, 2019

@vncoelho what is your proposal, when should be reverified the mempool?

@vncoelho vncoelho changed the title Policy for network fee after verification + Tokens acceptance Auxiliary data structures for tx re-verification Jul 31, 2019
@vncoelho vncoelho changed the title Auxiliary data structures for tx re-verification Auxiliary data structures for tx re-verification + Tokens acceptance Jul 31, 2019
@vncoelho
Copy link
Member Author

@shargon re-verified normally, it is just an auxiliary data structure for monitoring the balance during Application phase, or, more efficiently than it, an extra field in all payable transactions, in which will limit the maximum amount that GAS will be spent.

We need to think about the most practical design. Re-verification should be straightforward, we should easily remove invalid tx from the mempool after block is defined as persisted (even before its final state)

@lock9
Copy link
Contributor

lock9 commented Jul 31, 2019

@vncoelho what is the difference between this issue and #840 ? Are they the same? Only #840 is planned for the next release.

@vncoelho
Copy link
Member Author

vncoelho commented Jul 31, 2019

This one can be planned for the second release, preview-2.

@vncoelho vncoelho changed the title Auxiliary data structures for tx re-verification + Tokens acceptance Auxiliary data structures for tx re-verification + Template for NEP5 tokens acceptance Jul 31, 2019
@vncoelho vncoelho changed the title Auxiliary data structures for tx re-verification + Template for NEP5 tokens acceptance Auxiliary data structures for tx re-verification + Discussions for NEP5 tokens acceptance Jul 31, 2019
@vncoelho
Copy link
Member Author

I will move NEP5 tokens to another issue.

@vncoelho vncoelho changed the title Auxiliary data structures for tx re-verification + Discussions for NEP5 tokens acceptance Auxiliary data structures for tx re-verification Jul 31, 2019
@lock9 lock9 added Consensus Module - Changes that affect the consensus protocol or internal verification logic Design Issue state - Feature accepted but the solution requires a design before being implemented 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 Discussion Initial issue state - proposed but not yet accepted high-priority labels Aug 11, 2019
@erikzhang erikzhang removed this from the NEO 3.0 milestone Dec 6, 2019
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
Consensus Module - Changes that affect the consensus protocol or internal verification logic Design Issue state - Feature accepted but the solution requires a design before being implemented 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

No branches or pull requests

5 participants