Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create eth_coinbase.md #48

Closed
wants to merge 2 commits into from
Closed

Create eth_coinbase.md #48

wants to merge 2 commits into from

Conversation

DockBoss
Copy link
Contributor

  • Geth
  • OpenEthereum
  • Nethermind
  • Besu
  • Erigon

- [ ] Geth
- [ ] OpenEthereum
- [ ] Nethermind
- [ ] Besu
- [ ] Erigon
@alita-moore
Copy link

cc @MicahZoltu @timbeiko @marcindsobczak @gnidan @rekmarks @rakitadragan @vorot93

This is a PR for adding edgecases for the proposed method. In particular, an edgecase is defined as any behavior outside of the OpenRPC specification defined in this repository. This needs approval from all 5 clients before it can be merged. This is one method in a slew soon to come.

@@ -0,0 +1 @@
* If the client has a benificary address it **MUST** return it, otherwise it **MUST** throw an exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe at the moment, eth_coinbase returns one of the accounts that a client has private keys for. I'm not sure how it is decided which to return in the case of multiple, but this method returns even for non-mining clients (which are the only ones with a beneficiary).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did update it. I think geth returns the first account in the keystore folder. I am out of the office until Tuesday evening so I will not be able to test it till them. I do not know about the other clients though

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First alphabetically?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would actually reword this to be less prescriptive:

* If the client is a mining client, this **MUST** return the beneficiary address for the next block it will produce.  If the client is a non-mining client, this **MAY** return any account that it has the private key for.

The idea here is to make it so this is less prescriptive than it is currently, but still useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what do you think about this?

  • If the client is a mining client it MUST return the the beneficiary address that is specified during start up, otherwise it MUST return the first account added to the client.
  • If the client is a non-mining client it MAY return any account the client has the private key for

So if I remember correctly it returns the first account that the client has the private key for when geth has more than one account and non is specified as the beneficiary.
I will be able to test it soon, so I will give you a follow up then

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"specified during startup" and "first account added" are unnecessarily prescriptive. not all clients will receive coinbase accounts during startup, and it may not be obvious in all cases which account was "first". Also, how/when a client gets an account is not relevant to the caller at all. What the caller presumably wants to know is what account is being used when creating blocks, which may not be the first account or an account provided at startup.

The goal with a good API spec should not be to precisely define what the server is doing, but rather to define the contract between the client and server. This often means leaving some decisions up to the individual server, especially implementation details like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • If the client is a mining client it MUST return the the beneficiary address for the block rewards, otherwise it MUST error if the client does not have a beneficiary address.
  • If the client is a non-mining client it MAY return any account the client has the private key for

I did take a look you were right. It returns the first account alphabetically.

@alita-moore alita-moore mentioned this pull request Aug 20, 2021
50 tasks
@lightclient lightclient added the A-edge-case Area: edge cases label Aug 23, 2021
@DockBoss DockBoss closed this Oct 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edge-case Area: edge cases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants