omgnetwork / omg-childchain-v2

pronounced /Ch-ch/
Apache License 2.0
5 stars 2 forks source link

/transaction.submit should return the blknum and txindex #121

Closed achiurizo closed 3 years ago

achiurizo commented 4 years ago
Feature: 
A user may want to get information back about the transaction the submit right away. 
This is particularly useful when it comes to transaction chaining ability. 
/transaction.submit returns the blknum, txindex, oindex (utxo position) 
so that the end user can determine how to craft the next transaction in said chain with this information.

Scenario: A user submits a transaction to Childchain

As a User
When I submit transaction [insert txbytes hex]
Then I should get a success response
And it should return the <field> with the value of <value>

Examples:

| field | value |
| blknum | 1 |
| txindex | 0 |

To have full parity with /transaction.submit, we need to ensure we send the blknum, and txindex back. We will need to make changes to the flow. Currently, we accept transactions first and defer the block formation until later(via a timer). This will prevent us from having the blknum ahead of time to return in these responses.

We'll need to re-work how we form blocks. An approach may be to pre-populate the incoming transaction with the next blknum, and then have the latter Block.form/0 hook pick that up and form the block with that number. Need to figure out these details more.

mederic-p commented 4 years ago

Here are my thoughts on this for now:

When submitting a transaction, we get or insert a "forming" block. At most 1 "forming" block is allowed at the DB level to avoid race conditions.

Block state:

Feel free to comment/add anything missing

achiurizo commented 4 years ago

@mederic-p this is great! Love the state machine approach here. Wondering if it would be helpful to use transaction/row locks to ensure atomic operations here.

Additionally, I'd like us to clean up and organize the other state machines on transaction/output as well. Not necessarily tied to this particular story, but it also follows suit with another issue we are missing(backfilling output/utxo position).

mederic-p commented 4 years ago

Totally agree with you that we need to cleanup entities that have a state. It's hard for now to follow/track where and how the state changes. I'm pretty sure there are libraries around to handle state machine but I'd vote to not use them, simple state machines are easy to code and we don't need external tools.

Also it's just a small detail but I'd prefer to use atoms in the code to represent the state (it's still saved as a string in the DB). I've done something similar when saving the kind of transactions.

pgebal commented 4 years ago

@mederic-p Good idea. I would just make it a bit simpler and merge finalizing and pending submission states. Everything you mentioned in finalizing can be done on first submission try.

I can start with coding forming.

@achiurizo Do you want devs to come up with full specs after it is agreed how to handle an issue? In this case, for example, specifying scenarios like:

Scenario: New transaction is submitted when pending block is full

Given there is a maximum number of transactions in pending block
When new transaction is submitted
Then new block is created
And the new transactions is included in the new block
achiurizo commented 4 years ago

@achiurizo Do you want devs to come up with full specs after it is agreed how to handle an issue? In this case, for example, specifying scenarios like:

Yes please! For most of the case, the PMs will help craft the basis of this spec. Eng will need to help fix the specs, like refactoring the steps.

mederic-p commented 4 years ago

Cool, I have a couple questions as well:

pgebal commented 4 years ago

@mederic-p

  1. I am not aware of any reason we should reject transactions. We just have to make sure blocks are submitted in the right order.
  2. I like the auto incrementing field approach but I don't know how to do it yet. It's been a while since I played with relational databases and I have limited experience with ecto (I will improve on that in this cycle :+)

I'll code a draft tomorrow and show you. Then I'll come up with specs, and if everything is good we'll start coding the final version for CR.

mederic-p commented 4 years ago

Sounds good @pgebal, can you split your work in small issues/PR when possible, like if you're going to do the "forming" step, create a dedicated issue for that, then you can link it in your PR :)

mederic-p commented 4 years ago

Here is an update of what I have in mind for the state of our different entities across the new childchain:

Transactions:

- pending: just received and included in a forming block or included in a “finalized” block that is pending submission

- confirmed: included in a block that was successfully submitted

Blocks:

- forming:

        Block is accepting incoming transactions, every time a transaction is submitted, we check that the current “tx_index” in this block is less than what it can contain.
        “tx_index” is an auto incrementing field on transactions that is bound to the block number. It resets for every new block.

Condition to go to the next state:

    - tx count in block reaches maximum allowed
    or
    - an external trigger forces the state changes. This is what will happen in most cases, we want to submit to the root chain as often as possible (once per rootchain block), so we will need to finalize blocks earlier even if they are not full.
    This can be for example a listener on the rootchain that checks if we have non-empty “forming” blocks at each new ethereum height.

- finalizing:

    Block is NOT accepting new payment transaction.
    Block is calculating and appending fee transactions at the end of its transaction list (do we need to store them in the DB?).
    Block is calculating the Merkle root hash of all transactions.
    Block is waiting to be submitted to the rootchain, an external trigger will scan for blocks that need to be submitted and will submit them.
    Gas price is estimated following our algorithm

Condition to go to the next state:

    Block submitted to the rootchain (but not confirmed)

- submitted:

    Block is submitted to the rootchain and the transaction is waiting to be mined.
    We need to check the status of the transaction. If it takes too long time to be mined, we need to re-submit it with a higher gas price.

Condition to go to the next state:

    Transaction is included in a block

- confirmed:

transaction is confirmed on the rootchain, state of involved utxos and involved transactions can be updated to “confirmed”

UTXOs:

- pending same as pending transaction, these are utxos of "pending" transactions
- confirmed: same as confirmed transaction, these are utxos of "confirmed" transactions
- exiting: Received an exit event from contract
- piggybacked: Received a piggyback event from contract
InoMurko commented 4 years ago

You don't have to stop accepting incoming transactions if you're not going faster then 4000TPS. At this speed, you'd almost have two full blocks per ethereum block - which means you wouldn't be able to submit them.

Here's a another thing you need to think about. A block can only be forming if the childchain has seen a larger ethereum block number then what the previous block was formed. The question is if Infura gives every single childchain the same view of Ethereum? Or could one be stale over the other?

boolafish commented 4 years ago

which means you wouldn't be able to submit them.

I think technically you can. And contract does take that into consideration if 2 childchain blocks are with same timestamp (since they are mined together), the tx_pos would take into consideration for the exit priority.

InoMurko commented 4 years ago

as long as nonces are higher you can submit N amount of blocks per Ethereum block and there are not security implications?

boolafish commented 4 years ago

I believe so. I guess the high TPS is probably more of a problem on watcher to sync instead of on the protocol itself. In fact, in a reorg and txpool world you probably have no way to really ensure there will be no two childchain blocks in same eth block during submission (unless you wait for confirmation which becomes bad UX if we really have load).

mederic-p commented 4 years ago

Here's a another thing you need to think about. A block can only be forming if the childchain has seen a larger ethereum block number then what the previous block was formed. The question is if Infura gives every single childchain the same view of Ethereum? Or could one be stale over the other?

What I have in mind is that we don't need to wait for a larger ethereum block to open a new forming block. If a forming block transitions to finalized (see above condition on how to make this state change) then we can open a new forming block right away. We can have many finalized block pending submission.

We may still want to add a limit to the number of blocks that are pending submission so we don't fall too much behind.

pgebal commented 3 years ago

I posted some thoughts on potential performance problems if we want to be consistent with old API:https://github.com/omgnetwork/childchain/issues/131#issuecomment-697503030

Is this Gherkin good enough or do we want better?:

Scenario: New transaction is submitted when forming block is full

Given there is a maximum number of transactions in forming block
When new transaction is submitted
Then it is not accepted in the current block
And new block is created
And the transactions is included in the new block

Scenario: New transaction is submitted and forming block is not full

Given there is a forming block
When new transaction is submitted
It is inlcuded in forming block
And its index is one greater than the index of previous transaction included in that block