Add evm_coinbase

[DeviateFish]

Adds a non-standard RPC method to set the coinbase for mined blocks.

Setting the coinbase allows for testing contracts that depend on block.coinbase, especially in the case where multiple miners are being tested.

NOTE: Unless this is called, the value returned by eth_coinbase
actually differs from the coinbase included in blocks. The default coinbase for
mined blocks is actually the null address, and I opted to not change it, so as to not introduce a breaking change. BlockchainDouble never sets block.header.coinbase in createBlock, so the block's miner (coinbase) field is always the null address.

If the breaking change is fine, an alternative approach would be to remove coinbase from StateManager entirely, since it's only used for eth_coinbase, and isn't even accurate. Instead, introduce a getCoinbase to BlockchainDouble, and delegate all interactions with coinbase to BlockchainDouble's api.

Second note: I haven't actually tested this against the current master HEAD, only against a branch forked off from the v3.0.3 release. This is the same change ported to be compatible with the existing master branch.

[DeviateFish]

@tcoulter Ping on this?

[tcoulter]

Quick feedback from you: Would setting the coinbase to the first available account be a good default? Happy to make breaking changes as the next release will be a big one with plenty of breaking changes.

[tcoulter]

And thanks for the PR!

[DeviateFish]

I think setting the coinbase to the first available account would be the right choice. In general, I think it's assumed that the default account is the coinbase account (I think that assumption is made in a number of places in testrpc, and in the things that use it), it's just never actually set as the coinbase on the blocks that are mined.

This leads to block.coinbase always being the null address, which was the issue I was originally trying to solve. Adding a method to set it was just a compromise so that I wouldn't break backwards compatibility. Turned out to be more useful in the long run, though.

[axic]

Wouldn't a --coinbase setting on the CLI accomplish the same?

[DeviateFish]

For most cases, yes. However, the case I'm trying to support requires being able to change the block coinbase dynamically (e.g. at runtime, during tests).

[benjamincburns]

For most cases, yes. However, the case I'm trying to support requires being able to change the block coinbase dynamically (e.g. at runtime, during tests).

@DeviateFish can you please elaborate on this case?

I ask because I'm not aware of any Ethereum clients supporting changing coinbase at runtime. From what I can tell, there's nothing which would prohibit this from happening (so long as the change is synchronized so that it doesn't take effect in the middle of mining a block), but since this isn't implemented in other clients we'd be disinclined to add it here without first understanding how it helps with testing. I'm hoping that's what you can provide us here.

[DeviateFish]

To simulate multiple miners on the network?

[benjamincburns]

@DeviateFish for the moment, at least, TestRPC is really designed to simulate a single node. Simulating multiple miners expands this scope substantially, though I can see the usefulness of this request.

I think this expansion of scope needs careful consideration. In the mean time, if this feature is important for you, can I please request that you implement it by adding an optional coinbase argument to evm_mine rather than adding a new RPC method altogether?

Closing this one in lieu of the requested design.

[benjamincburns]

To be clear, if you submit a PR w/ the recommended design and include a test for the new functionality, I'll be sure to accept it quickly.

[DeviateFish]

👍