Make sure contract calls apply to the current open block.

[afck]

We are currently adding the randomness contract calls (#51) to the transaction queue in AuthorityRound::on_new_block and assume they will be applied to the current open block. We make the constant calls via the EngineClient using BlockId::Latest and assume that will always give us up-to-date information.

We should either verify that our assumptions are correct, or make those calls in a more direct way to the currently open block.

It would be easy to pass an OpenBlock to on_new_block instead of an ExecutedBlock: It is available as r at the only call site, OpenBlock::new. OpenBlock has a push_transaction method that would allow us to add transactions directly, without the queue.

Client::call_contract, which is currently used for the constant calls, first turns the call into a transaction in Client::contract_call_tx and then passes it to Client::call, which in turn uses Client::do_virtual_call (a static method) to retrieve the result of the transaction. Note that this updates the state, but that state object is just a local copy that has been created in call_contract.
Client::do_virtual_call uses Executive::transact_virtual, which automatically adds sufficient gas to the state and executes the transaction. (The method name seems to be misleading: it is meant to be used with cloned state objects that won't be persisted anywhere, but what the method does is no more virtual than Executive::transact.)
We should probably do something like Client::contract_call_tx in BoundContract, and then just use Client::call with a clone of the current block's state.

[afck]

Alternatively, instead of changing what's passed into on_new_block, we could also just copy the relevant part of the push_transaction implementation, so we keep our changes local.

[afck]

Another complication is that we need the client to turn the call data into a transaction first. It looks like we'll need to change the Client trait for that: We need something like Client::transact, but instead of putting the transaction on the queue, it should return it to us.

[DemiMarie]

Is this actually possible for any node except the currently mining validator?

[afck]

No, these methods would all be just for use-cases like ours, where the current block author has to make contract calls.

[DemiMarie]

@afck what would happen if any other node tried to do this?

[afck]

You can only call the methods with a mutable ExecutedBlock (that contains a State and everything), and they are applied to that block. If it doesn't end up getting sealed and accepted into the blockchain, then the transaction is effectively not executed, and the constant calls are made to a "fork".

[afck]

I have an implementation that seems to work now: I realized that on_new_block is also called in other nodes, not only in the block author. I had missed that because its only call site is in OpenBlock::new. But that, of course, gets called in lots of places, in particular in block::enact and therefore in block::enact_verified, which seem to replay a block's transactions. This is used in Importer::check_and_lock_block, which in the end compares the resulting state to the one in the block header (that's the place where it used to fail for me when on_new_block would always insert more transactions), which in turn is called by Importer::import_verified_blocks, which has several call sites.
A similar story applies to on_close_block, I think.

So we can't assume that we're creating a block in either of those calls. I'm not entirely sure what we can assume: E.g. if someone sent us any bogus block, could they trick us into creating a transaction that reveals our secret?

Maybe it's cleanest to add yet another method to Engine, that is only called from the Miner, but not from OpenBlock itself. It would allow the author to insert transactions, but nobody else.

Edit: Possibly something like

fn on_new_mining_block(&self, _block: &mut M::LiveBlock) -> Result<(), M::Error>;

or

fn block_author_transactions(&self) -> Result<Vec<SignedTransaction>, M::Error>;

(where the latter would just use BlockId::Latest again, and return the new transactions)? That would be called only in Miner::prepare_block, and in the latter case, the result would be prepended to the transactions.

[DemiMarie]

@afck I will handle this.

[DemiMarie]

Miner::prepare_block calls PrepareOpenBlock::prepare_open_block whenever it needs to autthor a new block. Other than code that is only compiled for testing, the only implementation of that trait is Client::prepare_open_block. I plan to have that method call block_author_transactions.

[afck]

Caveat: prepare_block is called even if we're not the proposer (2-year-old TODO).
That's wasteful but at least if we don't put our transactions in the queue, it won't do any harm (other than wasting CPU).
(Edit: According to my logs, it's also sometimes called twice for the same block number.)

[DemiMarie]

Marking as low-priority, per discussion on Zoom. The current approach is more robust, and fixing this should be left until after the network is operational.

[varasev]

@afck Could we close this?