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

A few thoughts on things... #9

Open
lookfirst opened this issue Dec 27, 2018 · 20 comments
Open

A few thoughts on things... #9

lookfirst opened this issue Dec 27, 2018 · 20 comments
Labels
question Further information is requested

Comments

@lookfirst
Copy link
Contributor

lookfirst commented Dec 27, 2018

First off, thanks so much for your work on this. Apologies, I am not sure the best way to discuss these thoughts (individual issues or one big issue, so lets start here so that I don't annoy you with a bunch of small issues):

  • What is the point of hex encoding? Doesn't gzip handle the size of the messages? I feel like the concept of hex everything but extranonce is awkward. I also feel like the convolutions around formatting the hex numbers is overkill as well.

  • Why is port hex encoded? For simplicity and readability, let's not.

  • What should happen when decoding a gzipped message fails on either client or server?

  • "Why should a pool advertise the node's version? It's a matter of transparency. Miners should know whether or not the pool has upgraded to latest patches/releases for node's software." -- I think this is not valuable for this specification. The pool can respond with whatever value it wants to because this cannot be enforced. It may also lead to security issues if the pool exposes its client information.

  • "Session Handling - Session Subscription" -- Because it is an ID and that is of type integer, shouldn't the example show a "params": integer instead of a string?

  • "Session Handling - Response to Subscription" -- Same issue, but with "result" member.

  • "A server receiving a subscription request with params being a string holding the session id." -- Same integer issue

  • Why would the client send the hashrate to the server? How could that be trusted and what real benefit would it have for the server?

@AndreaLanfranchi
Copy link
Owner

Gzip encoding has been dropped as an idea as it turns out on such small messages gzip increases the overall number of bytes transferred among endpoints (gzip adds it's own headers and CRC).

The pool can respond with whatever value it wants to because this cannot be enforced. It may also lead to security issues if the pool exposes its client information.

As I told it's a matter of transparency. There are some risks but miners have the right to know if the backend node is up to date or not. When pool mining it's all a matter of trust.

"Session Handling - Session Subscription" -- Because it is an ID and that is of type integer, shouldn't the example show a "params": integer instead of a string?

No. While message ids are bound to the scope of a session session ids are kept by pool software using a key they prefer and there might be well more than 65535 sessions at a time.

Why would the client send the hashrate to the server? How could that be trusted and what real benefit would it have for the server?

Almost 99.9% of clients do that as users do want to see it on the graphs provided by pools. I agree it's a totally unuseful information (as what really matters is the effective computed hashrate) but ... you know ... users are users ... they always want un-useful bells and whistles (like colored output for instance).

@AndreaLanfranchi AndreaLanfranchi added the question Further information is requested label Dec 30, 2018
@AndreaLanfranchi
Copy link
Owner

Why is port hex encoded? For simplicity and readability, let's not.

Json format provides some sort of human readability ... nevertheless humans are not the data consumer. Hex encoding allows to save some byte.

@lookfirst
Copy link
Contributor Author

lookfirst commented Dec 30, 2018

Json format provides some sort of human readability ... nevertheless humans are not the data consumer. Hex encoding allows to save some byte.

1-2 bytes isn't much on a message that is only sent once (hello) or rarely (reconnect). HTTP doesn't encode it as hex. I'm sorry, but it feels like a premature optimization. Especially since we are sticking with json and not using gzip.

Gzip encoding has been dropped as an idea as it turns out on such small messages gzip increases the overall number of bytes transferred among endpoints (gzip adds it's own headers and CRC).

Hmm... that is a bummer. I wonder if there is another way to compress things more efficiently. Should the spec get updated to remove this? Makes me really wonder why we didn't end up with HTTP2 for this.

As I told it's a matter of transparency. There are some risks but miners have the right to know if the backend node is up to date or not. When pool mining it's all a matter of trust.

Agreed 💯with the idea of transparency, I just do not trust the pools to actually provide it. As a spec, do you really want to encourage something that cannot be guaranteed?

Almost 99.9% of clients do that as users do want to see it on the graphs provided by pools.

Again, I agree with you 💯, seeing this information is very useful to ensure that the pool isn't cheating. I actually wrote prometheus agents to pull the data both from the pools and the miners so that it could be all loaded into a single place. I disagree that this sort of functionality belongs in this spec though. Saying no to users is often a good thing.

Sessions

Sorry, I confused the session with the message id! Too much reading of the long spec. ;-)

@AndreaLanfranchi
Copy link
Owner

Hmm... that is a bummer. I wonder if there is another way to compress things more efficiently. Should the spec get updated to remove this?

There actually is but it's work-in-progress and can't disclose anything.

I disagree that this sort of functionality belongs in this spec though. Saying no to users is often a good thing.

Actually all three existing stratum flavours make use of eth_submitHashrate method which is defined only on getWork http specs. Writing a strong spec for this make no harm and helps devs to stay in a single context.

About "saying no to users" ... how I wish it would be possible. I would have dropped colorization a long time ago !!!

@AndreaLanfranchi
Copy link
Owner

I actually wrote prometheus agents

Interesting ... a prometheus endpoint from ethminer is on my task-list. Willing to write some specs about the outputs ?

@AndreaLanfranchi
Copy link
Owner

1-2 bytes isn't much on a message that is only sent once (hello) or rarely (reconnect). HTTP doesn't encode it as hex. I'm sorry, but it feels like a premature optimization. Especially since we are sticking with json and not using gzip.

Think about this ... we've been reported the avg socket lifetime is roughly 17 hours. (disconnections, miner crashes ... whatever). This means a large pool like ethermine.org refreshes (at the moment of writing this) roughly 300k connections with a new hello more than once a day. Every byte counts.

@lookfirst
Copy link
Contributor Author

There actually is but it's work-in-progress and can't disclose anything.

I am confused... can't disclose anything for the spec modifications?

Every byte counts.

Then let's stop using raw json as the format and switch to something more efficient. This is 2.0 after all. gRPC or Twirp (my preference) are two candidates.

Interesting ... a prometheus endpoint from ethminer is on my task-list. Willing to write some specs about the outputs ?

Yes, this will be on my TODO list at some point. Not sure I can OSS it though. We will see.

@AndreaLanfranchi
Copy link
Owner

I am confused... can't disclose anything for the spec modifications?

We wish to extract some bucks from the compression work.

Then let's stop using raw json as the format and switch to something more efficient. This is 2.0 after all. gRPC or Twirp (my preference) are two candidates.

Json messages are plain strings both ends can parse with the language they prefer. We cant force pools, users, miner developers to adopt a framework.

Not sure I can OSS it though

I mean if you could spec the definition metrics which would be necessary for a prometheus ep.
We can easily output the metrics using the http interface.

@AndreaLanfranchi
Copy link
Owner

The freedom of pure Json strings is that you can add new members keeping complete backwards compatibility with consumers which do not make use of them.

Recently pools have integrated the blocknumber into their notification messages but all old miners releases didn't need to change anything.

@lookfirst
Copy link
Contributor Author

We wish to extract some bucks from the compression work.

From the pools? Meh.

Json messages are plain strings both ends can parse with the language they prefer. We cant force pools, users, miner developers to adopt a framework.

Twirp is json.

I mean if you could spec the definition metrics which would be necessary for a prometheus ep.
We can easily output the metrics using the http interface.

It is basically the same as what cgminer API has. Namely the devs, pools, summary results.

@AndreaLanfranchi
Copy link
Owner

Ethminer has already it's own API set.
I mean prometheus direct endpoint like requested here

ethereum-mining/ethminer#124

@lookfirst
Copy link
Contributor Author

Also, I'd argue that you should remove the required "encoding" : "plain", if you're trying to save bytes. ;-)

@AndreaLanfranchi
Copy link
Owner

Encoding is kept as there might be different encoding types in the (not far) future.

@lookfirst
Copy link
Contributor Author

Then add it in the future version of the spec or at least make it non-mandatory.

@AndreaLanfranchi
Copy link
Owner

Twirp generates Json messages but is a framework which must be on both ends of the transmission.

A simple RPC framework with protobuf service definitions

@AndreaLanfranchi
Copy link
Owner

Then add it in the future version of the spec or at least make it non-mandatory.

Lol ... you're arguing with everything ... 😉

@lookfirst
Copy link
Contributor Author

Ethminer has already it's own API set.
I mean prometheus direct endpoint like requested here

ethereum-mining/ethminer#124

Ah, I wouldn't bother with that. I'd just keep it as a separate binary that polls the ethminer api. That's what I did with cgminer and it worked just fine.

Lol ... you're arguing with everything ... 😉

Nah. Sorry you have that impression. I'm just a stickler for details and want to see this spec be as good as it can be (I'm also the guy who submitted the PR to fix your boolean logic in ethminer).

@AndreaLanfranchi
Copy link
Owner

Nice.

I'd like to invite you to review also
https://github.com/AndreaLanfranchi/ethminer

which is the new mixed ethash/ProgPoW ethminer.

Any hint appreciated.

@lookfirst
Copy link
Contributor Author

Woah. Is ProgPow actually happening? Thought this was all theory and discussion still.

@AndreaLanfranchi
Copy link
Owner

AndreaLanfranchi commented Dec 30, 2018

Yes it is ... and is working.
Testnet already up and running.

http://boot.gangnam.ethdevops.io/
https://gangnam.etherchain.org/

ethermine.org has also a test ep
progpow.ethermine.org:3333

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants