omgnetwork / plasma-cash

122 stars 34 forks source link

Automatically add new block on sidechain when getting the deposit event #78

Open ldmtam opened 6 years ago

ldmtam commented 6 years ago

Love your implementation, but I think we should automatically add new block on sidechain when getting the Deposit event from RootChain smart contract. We don't need sidechain admin to call submitBlock function by hand.

I've changed your code a little bit. Please review it

def apply_deposit(self, event):
        new_owner = utils.normalize_address(event['args']['depositor'])
        amount = event['args']['amount']
        uid = event['args']['uid']
        deposit_tx = Transaction(0, uid, amount, new_owner)
        self.current_block.add_tx(deposit_tx)
        self.db.save_block(self.current_block, self.current_block_number)
        self.current_block_number = self.db.increment_current_block_num()
        self.current_block = Block()
boolafish commented 6 years ago

hi @ldmtam,

Me and @bun919tw had a discussion about this. We agree that it is not practical to call submitBlock by hand. But submitting block only when deposit is not enough IMHO. Operator still need to manually submit block for other cases. We've always have the idea of having a cron job that would submit the block every certain time. And it might be easier for the operator to do so instead of event-trigger base.

How do you think? If everyone is good with this idea we will create a story for this and go for the implementation.

BTW, the code above lacks the part of submitting block to root chain.

ldmtam commented 6 years ago

Yeah thanks, I forgot about submiting block hash to root chain part. Actually, in the recent proposals, we have 2 kind of blocks: deposit block with having only 1 tx and plasma block; so I think we can still use event-trigger to create deposit block whenever an user deposits tokens to RootChain smart contract. And about plasma block, we may create a tx pool to store tx and if number of tx exceeds a threshold, the operator will automatically validate all tx, create new block and then send block hash to RootChain. I don't think we need to create and submit block every certain time if there is no validated tx in the block.

boolafish commented 6 years ago

in the recent proposals, we have 2 kind of blocks: deposit block with having only 1 tx and plasma block

Yes, that's what we see in most plasma-cash spec as well. However, we are not convinced that there the need of separating those into different blocks. On the other hand, we think it is more efficient to just include all in the same block. That's why we actually ignored that in our implementation. Correct me if I'm wrong, but do you see any benefits from this? If there actually do is value to separate then we might need to alter for this.

we may create a tx pool to store tx and if number of tx exceeds a threshold, the operator will automatically validate all tx, create new block and then send block hash to RootChain

Our current implementation actually declined invalid tx immediately. For the validation part, as a centralized operator service I think there's no need to go for tx pool. I think tx pool has more value on p2p system. eg. newly synced block data might make some tx invalid after accepting them to tx pool. However, I don't think we have the same issue. Also, in plasma-cash, I don't see the downside of having too many txs in a block yet. I am still kind of on the fence that whether there's a need of having threshold (max capacity) for a block.

I don't think we need to create and submit block every certain time if there is no validated tx in the block.

I do agree with this one. But we can simply check before submit block which is fairly easy to do as well.

Do you mind to share your opinion with us on the above statements? @ldmtam

ldmtam commented 6 years ago

Sorry, I was too busy these days so I'm now able to answer you.

Yes, that's what we see in most plasma-cash spec as well. However, we are not convinced that there the need of separating those into different blocks. On the other hand, we think it is more efficient to just include all in the same block. That's why we actually ignored that in our implementation. Correct me if I'm wrong, but do you see any benefits from this? If there actually do is value to separate then we might need to alter for this.

Yes, I think you're right. But imagine a case that each block is added every 10 seconds, and users deposit a coin in second number 2, so they have to wait for a period of time(in this case is 8 seconds) for their deposits to be confirmed and then they're able to use coin(users must wait for a 10s period). But if we create blocks for every deposit event, we're able to avoid this problem.

Our current implementation actually declined invalid tx immediately. For the validation part, as a centralized operator service I think there's no need to go for tx pool. I think tx pool has more value on p2p system. eg. newly synced block data might make some tx invalid after accepting them to tx pool. However, I don't think we have the same issue. Also, in plasma-cash, I don't see the downside of having too many txs in a block yet. I am still kind of on the fence that whether there's a need of having threshold (max capacity) for a block.

Yes, you're absolutely right. I misunderstood this is a decentralized side chain using PoS as consensus engine. Truth is, we just need a constraint centralized authority.

boolafish commented 6 years ago

Cool I got your point now. So the benefit would be shorter deposit confirmation time. Note the user still need to wait Rootchain to confirm, which would take an expected 10~20 sec anyway, it would not be possible to make this really real-time. I think this is a good-to-have feature, but we will still prioritize to implement the cron job that submit block every certain time first. I believe that is more important (or more like a requirement actually) to have. However, feel free to send PR : ) But how to handle the sig might be the issue for this, since you would need that to send tx to Root chain.