miner: --miner.notify.full flag: enable full pending block notifications

[AlexSSD7]

This PR implements the --miner.notify.full flag that enables full pending block notifications. Extends --miner.notify flag.

Notification without this flag:

POST / HTTP/1.1
Host: localhost
User-Agent: Go-http-client/1.1
Content-Length: 219
Content-Type: application/json
Accept-Encoding: gzip

["0x4ca621099304576a5d482bd5530d4fabfac22e6a5cc523b6b5b8209cf293bb89","0x1438f845a55515d47e39b80d72f530939c61d6b1e9cd28c2396c5bf25e4ac3a2","0x00000002fc4aad70f11023a1a899d5a5ae39540cf37b101aa84828e94e325419","0x93440c"]

Notification with this flag:

POST / HTTP/1.1
Host: localhost
User-Agent: Go-http-client/1.1
Content-Length: 1341
Content-Type: application/json
Accept-Encoding: gzip

{"difficulty":"0x4f8359f2","extraData":"0x","gasLimit":"0x7a1200","gasUsed":"0x6f328c","hash":"0xb218996ce8127b4dbacbc6058b298602338f88e5e7a61973ee2493803297706a","logsBloom":"0x0000000000000400000000a0001000000000040000000004000040000004000000000000000000000000410000000000000000000000002000000000002420000000101044008000000000080000040000002000001400044001000040000000000040000200000002000800004000000000000000020000000010180400000002a4004000000000000000000000000000001404021000000000000000000000020004000000000420000000000000000000804000000828001000000000000010000502000000000000000000000000000440005000008008400200010040600010000000400000004000000000000000000000010000014000011000800000","miner":"0000000000000000000000000000000000000000","mixHash":"0x0000000000000000000000000000000000000000000000000000000000000000","nonce":"0x0000000000000000","number":"0x934054","parentHash":"0x5c38bf02ae71e2b27fddd96a36b4896c2f0716ee8f8a76839c7fd5b6fd1f129a","receiptsRoot":"0xe6413d8544987151d2c52000de5b950b818156447110360ab8963f7577817f45","sha3Uncles":"0x1dcc4de8dec75d7aab85b567b6ccd41ad312451b948a7413f0a142fd40d49347","size":"0x6df5","stateRoot":"0xa65f8d3d3833bbbed03162287db97ce382bd82073c1ccbec8b8dc9f2c6a906cf","timestamp":"0x6026bc67","transactionsRoot":"0x1e552804d6b796c6675c54b1a4b74faa820f1cf8cb5c52c8fd585e920b1111e5","uncles":[]}

[AlexSSD7]

Added test coverage to the implemented functionality. Ready for the review.

[karalabe]

What's the goal of this PR? You can mine without the block details, but the details might be quite large. What's the advantage of knowing all the transactions included?

[AlexSSD7]

Good question. Advanced mining pools require having the block template for things like computing block hash when one was mined.

Right now, on our pool, when new work was prepared, it gets notified in this way:

Node ======================================================= Pool
Work notification ->->->->->->->->-LATENCY->->->->->->->->->->->
<-<-<-<-<-LATENCY-<-<- Full block request via eth_getBlock("pending")
Full block ->->->->->->->->->->->->-LATENCY->->->->->->->->->->->

With this PR, it can be simplified to this:

Node ======================================================= Pool
Pending block ->->->->->->->->-LATENCY->->->->->->->->->->->->->->

I know that the miner part of the go-ethereum is barely maintained right now, but I hope that you will merge this because it is a fairly small fix compared to the performance gains that can be achieved with this.

[AlexSSD7]

but the details might be quite large.

Size increased from 219 bytes to 1341 bytes. 6x more in total.

What's the advantage of knowing all the transactions included?

Also, only the transaction root is included in the response.

[AlexSSD7]

Sorry for asking, but do you plan to merge this? A lot of stuff is waiting for this PR.

[AlexSSD7]

Resolved conflicts. Having:

• The functionality itself
• Test coverage
• Sync with the upstream (no conflicts)

What's the reason for ignoring this PR? It will take only a couple of minutes to review and test.

[holiman]

Advanced mining pools require having the block template for things like computing block hash when one was mined.

I still don't understand. In the code/docs, it seems like you're sending the full block. But in the actual example from the PR description, it looks like a block header only. And if you want to compute the block hash, the header is the only thing needed.

So is it full block or header? If the latter, the descriptions in PR and code needs to be updated.

What's the reason for ignoring this PR? It will take only a couple of minutes to review and test.

It's not being ignored, it was added for triage three days ago. We just can't spend all our time just on PRs, then we'd never do anything else.

It will take only a couple of minutes to review and test.

That is seldom if ever true. Most PRs require roughly the same time for review as it takes coding it up. Aside from reviewing for potential side-effects and mistakes, we also review for maintainability long-term, coding style. We have to maintain the code once it's merged, so almost every PR has several rounds of review/update/review/update.

[AlexSSD7]

Thank you for the reply, @holiman. Sorry if I was pushing this too hard. I was just worried that this PR would be abandoned just like thousands of others in this repo.

And yes, this flag will notify the block header because the true full block doesn't have any use cases in mining. I will update the flag description.

I appreciate the response :)

[holiman]

So, one thing is that you made NewRPCTransaction a public struct, with a new package serialization, in order to use it from a different package. Since you don't actually need the full block, I don't see why all of that refactoring is needed, since you could just use types.Header. It would 'natively' json-export the following fields:

	type Header struct {
		ParentHash  common.Hash    `json:"parentHash"       gencodec:"required"`
		UncleHash   common.Hash    `json:"sha3Uncles"       gencodec:"required"`
		Coinbase    common.Address `json:"miner"            gencodec:"required"`
		Root        common.Hash    `json:"stateRoot"        gencodec:"required"`
		TxHash      common.Hash    `json:"transactionsRoot" gencodec:"required"`
		ReceiptHash common.Hash    `json:"receiptsRoot"     gencodec:"required"`
		Bloom       Bloom          `json:"logsBloom"        gencodec:"required"`
		Difficulty  *hexutil.Big   `json:"difficulty"       gencodec:"required"`
		Number      *hexutil.Big   `json:"number"           gencodec:"required"`
		GasLimit    hexutil.Uint64 `json:"gasLimit"         gencodec:"required"`
		GasUsed     hexutil.Uint64 `json:"gasUsed"          gencodec:"required"`
		Time        hexutil.Uint64 `json:"timestamp"        gencodec:"required"`
		Extra       hexutil.Bytes  `json:"extraData"        gencodec:"required"`
		MixDigest   common.Hash    `json:"mixHash"`
		Nonce       BlockNonce     `json:"nonce"`
		Hash        common.Hash    `json:"hash"`
	}

That would reduce the size of this PR to be basically just a few additions, maybe around 40 LOC. Am I missing something?

[AlexSSD7]

Okay, things I've done:

1. Removed CreateConsensusEngine inside eth/backend.go
2. Removed the serialization package; types.Header is now used.

@holiman I think that everything is ready for the final review.

[holiman]

You can ignore the error here, just like we do below. It's not a random interface struct, it's a known type so it should be safe

[AlexSSD7]

Makes sense, will commit the change in a moment

[karalabe]

Could you describe your use case a bit? Explicitly?

Why is it beneficial to return the full header? You are aware that you'd need to compute the work package yourself? Also, Geth won't accept solutions to work packets it doesn't track, so you can't really modify it.

[AlexSSD7]

You are aware that you'd need to compute the work package yourself? Also, Geth won't accept solutions to work packets it doesn't track, so you can't really modify it.

I understand this. Primarily, it will be used for block hash computation.

[holiman]

Could you describe your use case a bit? Explicitly?

it will be used for block hash computation.

We're a bit loathe to add a feature which we don't understand. Maintaining code means that we need to know why something is the way it is, otherwise the thing might be overly complicated, or happened to get refactored away at some point in the future, with the reasoning "there's no good point in having this here".

So while the PR generally looks good at this point, we still would like to know more about exactly how this feature fits into the mining stack, and what the usecase is that prompts it.

[AlexSSD7]

Sorry for not explaining it verbosely.

When the block was mined on the mining pool, the mining pool must understand which exact block was mined. It is impossible to derive the block hash with just a default work package, so mining pools are forced to retrieve the entire block header. By receiving the work package via --miner.notify, the mining pool is retrieving the pending block right after this by sending eth_getBlock("pending"). This causes latency, especially if the mining node is remote and is not located on the same host. It would be rather better for the mining node to send the entire pending block immediately.

[karalabe]

the mining pool must understand which exact block was mined

Why? The work packages are controlled by Geth. Geth will track all open work packages and you can submit a solution to any of them. We still don't understand why a mining pool wants to care about the details of a block. In our mental model, the pool just schedules and aggregates remote GPUs. In your model you're obviously doing something more, we'd like to understand what that is.

[holiman]

Geth will track all open work packages and you can submit a solution to any of them

Which means, once we get the solution, geth already knows exactly what the block was.

[AlexSSD7]

The work packages are controlled by Geth. Geth will track all open work packages, and you can submit a solution to any of them. We still don't understand why a mining pool wants to care about the details of a block.

I see that you forgot about the reward distribution step. How will the pool distribute the rewards if the block reward is unknown? And to determine the block reward, the pool must understand what block it has mined (aka have the block hash).

This PR is not about sealing the block but the further processing of it. This PR will provide needed information to the mining pool directly without doing the extra eth_getBlock("pending").

[karalabe]

How does the header help wrt the block reward?

[karalabe]

Hmm, yes, the header can be used to get the hash and then go from there. Still, there's no immediate guarantee that the block will be included and whether it's a full block or an uncle. But yeah without the hash it's poblematic.

[AlexSSD7]

Still, there's no immediate guarantee that the block will be included and whether it's a full block or an uncle.

Before the reward is distributed, it requires a certain amount of confirmations, and the reward is being verified on the confirmation stage as well.

[karalabe]

"block header"

[karalabe]

dead code

[karalabe]

Generally ok, we'll patch it up a bit to reduce the API changes (and thus the diff).

[fjl]

I have recreated this PR in #22558 in order to make some fixes.

[AlexSSD7]

Thank you!