Child chain

[bun919tw]

issue #34

Use web3 in child chain to communicate with ganache-cli for deposit and block submitting.

[smartcontracts]

Concept is good :+1:, but requesting changes -

1. Tests don't pass because we're currently initializing child chain in tests as:

child_chain = ChildChain(None, None, None)

However, now that we're using web3 in child_chain.py, we'll need to add a provider. The simplest fix is to initialize child_chain as:

child_chain = ChildChain(None)

assuming we have ganache-cli running on localhost:8545.

However, it's likely that users will want to test this project on their local machines without shutting down their testnet/mainnet local nodes. A few potential solutions:

• Allow users to specify a port when testing
• Add ganache-cli as a dev dependency and host on an open port for the duration of testing

2. The codebase contains several references to submit_deposit, such as child_chain/server.py

https://github.com/omisego/plasma-mvp/blob/51eadc38b0d16e1ff2dea5d5f71d8ac2e19e586c/plasma/child_chain/server.py#L15

and client/child_chain_service.py

https://github.com/omisego/plasma-mvp/blob/51eadc38b0d16e1ff2dea5d5f71d8ac2e19e586c/plasma/client/child_chain_service.py#L24-L25

All of these need to be updated or removed accordingly if we're changing submit_deposit to an event listening apply_deposit.

3. I'd also request that tests be added to cover apply_deposit and submit_block.

[bun919tw]

@kfichter Thanks for the feedback. I've made some modifications.

Sorry, I'm not skilled in unittest. I ended up launching a ganache-cli in a given port when testing. All the tests pass now locally but travis is not happy because it doesn't recognize ganache-cli. Please give me some feedback so I can approve it! Thanks.

[smartcontracts]

I think having users launch their own Ethereum node and then adding some sort of optional flag to specify the port is the best solution here. This allows users to test using whatever Ethereum implementation they desire. In that respect, I think we shouldn't run ganache-cli while testing (going against my initial recommendation).

I'm going to add an issue to change the travis build so that we're running an instance of ganache-cli and exposing the RPC API on the default port. This should solve the issue temporarily, but I request that

@pytest.fixture
  def child_chain(root_chain):
      child_chain = ChildChain(AUTHORITY, HTTPProvider('http://localhost:{}'.format(GANACHE_TEST_PORT)), None)

be modified so that GANACHE_TEST_PORT is a CLI flag that defaults to 8545 and that

 @pytest.fixture
 def root_chain():
     subprocess.Popen(['ganache-cli', '-m=plasma_mvp', '-p={}'.format(GANACHE_TEST_PORT)])

be reverted entirely.

[bun919tw]

@kfichter I let the ganache-cli port to be a CLI argument when testing. Is that something you said? Thanks!

[bun919tw]

Hey, @kfichter Finally~ all checks have passed. Here are my thoughts. I mock all the root chain reference in test_child_chain.py because I only want to test the function correctness in child chain. Compare to provide a real testrpc, I think this is more in line with the unittest logic. However, I do think launching a testrpc when testing is necessary, because we might write integration tests in the future. Does this make sense to you?

[smartcontracts]

@bun919tw Yes, mocking the root chain in test_child_chain.py makes 100% sense to me. I think it's fine to keep ganache-cli in the travis build (for now) because we'll need it for test_root_chain.py eventually.

Agreed on separating unit and integration tests.

[smartcontracts]

This is an artifact of the original submit_block, but we should probably consider validating that the block being submitted here (block):

https://github.com/bun919tw/plasma-mvp/blob/a49651a0e5dc513a01da387c11cbda5e6c894241/plasma/child_chain/child_chain.py#L83-L92

is equivalent to current_block. Currently, we aren't checking that this is the case. I don't think we want an operator to be able to submit a block that isn't the current_block. Adding this check should also remove the necessity of checking that all of the transactions in the block are valid, because we'll already have checked that in apply_transaction (which should be the only way to append to the current block's transaction set).

Edit: here's one possible implementation of this

    def submit_block(self, block):
        block = rlp.decode(utils.decode_hex(block), Block)
        assert block.merkilize_transaction_set == self.current_block.merkilize_transaction_set

        valid_signature = block.sig != b'\x00' * 65 and block.sender == self.authority
        assert valid_signature

        self.root_chain.transact({'from': '0x' + self.authority.hex()}).submitBlock(block.merkilize_transaction_set, self.current_block_number)

        self.blocks[self.current_block_number] = self.current_block
        self.current_block_number += 1
        self.current_block = Block()

the corresponding unit test is:

def test_submit_block_invalid_tx_set(child_chain):
    block = Block()
    block.transaction_set = child_chain.current_block.transaction_set[:] # Copy the correct transaction set

    unsubmitted_tx = Transaction(0, 0, 0, 0, 0, 0, newowner1, 1234, b'\x00' * 20, 0, 0)
    block.transaction_set.append(unsubmitted_tx) # Add some arbitrary transaction that hasn't been correctly submitted

    block.make_mutable()
    block.sign(block_key)
    block = rlp.encode(block).hex()

    with pytest.raises(AssertionError):
        child_chain.submit_block(block)

[bun919tw]

Add two more commits.

• Discussed with @boolafish, I modified the ChildChain's constructor. Pass in a root chain instance as a parameter instead of a root chain provider.
• Advised from @kfichter, I added the validation of the submitting block content and its corresponding test.

I think we should customize the exception in another PR.

[smartcontracts]

I agree with customizing exceptions in another PR, I'll go ahead and open an issue for that right now.

Something I didn't think of earlier:

I think ChildChain needs to be modified in case it receives an event late. To illustrate, let's assume deposit_filter.watch(self.apply_deposit) fails and self.apply_deposit is being called late or not at all.

This has some potential downsides:

1. The operator never sees the deposit, so the user can't spend their deposit and must exit. Or the operator sees the deposit late and the user can't spend until the operator has seen the deposit. I'm not really worried about this, it probably won't be a huge issue.
2. The operator sees the deposit late and applies the deposit via:

   ...
       deposit_tx = Transaction(0, 0, 0, 0, 0, 0,
                                newowner1, amount1, b'\x00' * 20, 0, 0)
       deposit_block = Block([deposit_tx])
       # Add block validation
       self.blocks[self.current_block_number] = deposit_block
       self.current_block_number += 1

However, current_block_number might not be currentChildBlock as defined in RootChain.deposit():

        childChain[currentChildBlock] = childBlock({
            root: root,
            created_at: block.timestamp
        });
        currentChildBlock = currentChildBlock.add(1);
        Deposit(txList[6].toAddress(), txList[7].toUint());

This means that block numbers between ChildChain and the root chain contract will differ. This is a serious problem because ChildChain.get_block(blknum) will return the wrong block.

I need to think about potential mitigations for this, just a second. The issue gets considerably more complex in the case that the operator is on a chain that experiences a reorg.

[DavidKnott]

LGTM, @bun919tw Thanks a lot for the contribution!