consensus/ethash: move remote agent logic to ethash internal #15853

rjl493456442 · 2018-01-11T05:31:22Z

In this pull request, i move the remote miner relative logics to ethash engine internally. Since this part of logics are PoW characteristic(including remote miner, hashrate submission or statistic). It can make miner package more clear.

rjl493456442 · 2018-01-26T06:30:00Z

@karalabe PTAL

karalabe · 2018-03-28T07:34:08Z

consensus/ethash/api.go

+//
+// You should have received a copy of the GNU Lesser General Public License
+// along with the go-ethereum library. If not, see <http://www.gnu.org/licenses/>.
+package ethash


Please add a newline here, otherwise the copyright text will be interpreted as the package documentation.

karalabe · 2018-03-28T07:35:31Z

consensus/ethash/api.go

+// GetWork returns a work package for external miner. The work package consists of 3 strings
+// result[0], 32 bytes hex encoded current block header pow-hash
+// result[1], 32 bytes hex encoded seed hash used for DAG
+// result[2], 32 bytes hex encoded boundary condition ("target"), 2^256/difficulty


Please format the doc a bit cleaner:

// GetWork returns a work package for external miner. // // The work package consists of 3 strings: // result[0] - 32 bytes hex encoded current block header pow-hash // result[1] - 32 bytes hex encoded seed hash used for DAG // result[2] - 32 bytes hex encoded boundary condition ("target"), 2^256/difficulty

karalabe · 2018-03-28T07:35:47Z

consensus/ethash/api.go

+	"github.com/ethereum/go-ethereum/core/types"
+)
+
+// API exposes ethash related methods for the RPC interface


Please add a . at the end.

karalabe · 2018-03-28T07:36:59Z

consensus/ethash/api.go

+// result[0], 32 bytes hex encoded current block header pow-hash
+// result[1], 32 bytes hex encoded seed hash used for DAG
+// result[2], 32 bytes hex encoded boundary condition ("target"), 2^256/difficulty
+func (s *API) GetWork() ([3]string, error) {


Please convert s to api throughout.

karalabe · 2018-03-28T07:39:17Z

consensus/ethash/api.go

+		return <-workCh, nil
+	} else {
+		return [3]string{}, err
+	}


if err := <-errCh; err == nil { return <-workCh, nil } return [3]string{}, err

karalabe · 2018-03-28T08:01:37Z

consensus/ethash/ethash.go

+		Version:   "1.0",
+		Service:   &API{ethash},
+		Public:    true,
+	}}


Just to make it nicer for future APIs, lets also advertize these in the ethash namespace, similar to how clique is done.

karalabe · 2018-03-28T08:02:00Z

consensus/ethash/ethash_test.go

@@ -69,11 +71,55 @@ func verifyTest(wg *sync.WaitGroup, e *Ethash, workerIndex, epochs int) {
 	const wiggle = 4 * epochLength
 	r := rand.New(rand.NewSource(int64(workerIndex)))
 	for epoch := 0; epoch < epochs; epoch++ {
-		block := int64(epoch)*epochLength - wiggle/2 + r.Int63n(wiggle)
+		block := int64(epoch) * epochLength - wiggle / 2 + r.Int63n(wiggle)


karalabe · 2018-03-28T08:02:54Z

consensus/ethash/sealer.go

+var (
+	errNoMineWork        = errors.New("No mining work available yet, don't panic.")
+	errInvalidSealResult = errors.New("Invalid or stale proof-of-work solution.")
+	errUndefinedRemoteOp = errors.New("Undefined remote mining operation.")


Errors need to be composable. I.e. don't terminate them with . and don't capitalize them.

You won't need errUndefinedRemoteOp if you go down the multiple task channel approach.

karalabe · 2018-03-28T08:04:19Z

consensus/ethash/sealer.go


 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/consensus"
 	"github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/log"
 )

+var (
+	errNoMineWork        = errors.New("No mining work available yet, don't panic.")


errNoMineWork -> errNoMiningWork

karalabe · 2018-03-28T08:07:28Z

consensus/ethash/sealer.go

@@ -150,3 +164,122 @@ search:
 	// during sealing so it's not unmapped while being read.
 	runtime.KeepAlive(dataset)
 }
+
+// remote starts a standalone goroutine to handle remote mining related stuff.
+func (ethash *Ethash) remote(resultCh chan *types.Block) {


Why does this take a channel as a parameter? I saw that sometimes you pass in nil, but wouldn't that be the same as ethash.resultCh being nil?

Do we even want to start this goroutine in the first place if there's no result channel?

holiman

It generally looks good to me, but I am not fluent in all the ins and outs regarding channels.

holiman · 2018-04-07T18:30:18Z

consensus/ethash/sealer.go

@@ -43,9 +50,9 @@ func (ethash *Ethash) Seal(chain consensus.ChainReader, block *types.Block, stop
 	if ethash.shared != nil {
 		return ethash.shared.Seal(chain, block, stop)
 	}
+


please remove empty line

holiman · 2018-04-07T18:30:39Z

consensus/ethash/sealer.go

+	if ethash.workCh != nil {
+		ethash.workCh <- block
+	}
+


pls remove empty line

holiman · 2018-04-07T18:30:53Z

consensus/ethash/sealer.go

@@ -64,12 +71,18 @@ func (ethash *Ethash) Seal(chain consensus.ChainReader, block *types.Block, stop
 	if threads < 0 {
 		threads = 0 // Allows disabling local mining without extra logic around local/remote
 	}
+


karalabe · 2018-08-03T06:55:27Z

consensus/clique/clique.go

@@ -672,6 +672,11 @@ func CalcDifficulty(snap *Snapshot, signer common.Address) *big.Int {
 	return new(big.Int).Set(diffNoTurn)
 }

+// Close implements consensus.Engine, returning internal error and close the clique.


// Close implements consensus.Engine. It's a noop for clique as there is are no background threads.

karalabe · 2018-08-03T06:55:51Z

consensus/consensus.go

@@ -96,6 +96,9 @@ type Engine interface {

 	// APIs returns the RPC APIs this consensus engine provides.
 	APIs(chain ChainReader) []rpc.API
+
+	// Close closes the consensus engine.


// Close terminates any background threads maintained by the consensus engine.

karalabe · 2018-08-03T06:58:20Z

consensus/ethash/api.go

+
+var (
+	errEthashStopped   = errors.New("ethash stopped")
+	errAPINotSupported = errors.New("the current ethash running mode does not support this API")


I don't really think we need a pre-declared error for this. It's a very special case that won't ever happen. Let's just inline it.

karalabe · 2018-08-03T06:58:44Z

consensus/ethash/api.go

+func (api *API) GetWork() ([3]string, error) {
+	if api.ethash.config.PowMode != ModeNormal && api.ethash.config.PowMode != ModeTest {
+		return [3]string{}, errAPINotSupported
+	}


MHO we can just do errAPINotSupported -> errors.New("not supported").

karalabe · 2018-08-03T07:00:48Z

consensus/ethash/api.go

+		return false
+	}
+
+	var errCh = make(chan error, 1)


Error channels are canonically named errc throughout all Go code. Please rename.

karalabe · 2018-08-03T07:17:39Z

consensus/ethash/api.go

+	if err = <-errCh; err == nil {
+		return <-workCh, nil
+	}
+	return [3]string{}, err


Wouldn't this construct be simpler with:

select { case work := <-workCh: return work, nil case err := <-errCh: return nil, err }

Then you wouldn't need to signal first by closing errCh and then retrieve the result from a second channel.

karalabe · 2018-08-03T07:18:22Z

consensus/ethash/sealer.go

+			} else {
+				close(work.errCh)
+				work.resCh <- miningWork
+			}


I think it would be simpler with:

if err != nil { work.errCh <- err } else { work.resCh <- miningWork }

karalabe · 2018-08-03T07:19:07Z

consensus/ethash/sealer.go

+		case result := <-ethash.submitWorkCh:
+			// Verify submitted PoW solution based on maintained mining blocks.
+			if submitWork(result.nonce, result.mixDigest, result.hash) {
+				close(result.errCh)


This is a bit unorthodox. Lets use result.errCh <- nil instead. It feels a bit wrong to close an error channel.

karalabe · 2018-08-03T07:20:56Z

consensus/ethash/sealer.go

+	ticker := time.NewTicker(5 * time.Second)
+	defer ticker.Stop()
+
+running:


I don't think a label is warranted here, you only use it once. Just do a return :)

karalabe · 2018-08-03T07:21:30Z

consensus/ethash/sealer.go

+		case errCh := <-ethash.exitCh:
+			// Exit remote loop if ethash is closed and return relevant error.
+			errCh <- nil
+			break running


I think it's simpler to just return here. Gets rid of the label, simplifies the code flow. Just move the log.Trace up here too

karalabe

LGTM! Lets see how it behaves live.

ghost · 2018-08-03T10:26:15Z

@karalabe @rjl493456442 @holiman @zsfelfoldi Shouldn't we update the documentation for this new API? https://github.com/ethereum/wiki/wiki/JSON-RPC https://github.com/ethereum/go-ethereum/wiki/Management-APIs

hadv · 2018-08-06T10:30:54Z

it seems make the duplicate output log for clique

INFO [08-06|17:28:26.004] Signed recently, must wait for others
INFO [08-06|17:28:26.004] Signed recently, must wait for others

* consensus/ethash: start remote ggoroutine to handle remote mining * consensus/ethash: expose remote miner api * consensus/ethash: expose submitHashrate api * miner, ethash: push empty block to sealer without waiting execution * consensus, internal: add getHashrate API for ethash * consensus: add three method for consensus interface * miner: expose consensus engine running status to miner * eth, miner: specify etherbase when miner created * miner: commit new work when consensus engine is started * consensus, miner: fix some logics * all: delete useless interfaces * consensus: polish a bit

…m#15853) * consensus/ethash: start remote ggoroutine to handle remote mining * consensus/ethash: expose remote miner api * consensus/ethash: expose submitHashrate api * miner, ethash: push empty block to sealer without waiting execution * consensus, internal: add getHashrate API for ethash * consensus: add three method for consensus interface * miner: expose consensus engine running status to miner * eth, miner: specify etherbase when miner created * miner: commit new work when consensus engine is started * consensus, miner: fix some logics * all: delete useless interfaces * consensus: polish a bit

Krigpl · 2019-05-07T11:18:25Z

Hi @rjl493456442,
I suspect this PR might be a root cause of an error I started seeing in my unit tests. I did git bisect between v1.8.13 and v1.8.14 and this PR is the outcome.
The error:
I keep receiving request timeouts in my test, similar to this one:

Traceback (most recent call last):
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/providers/ipc.py", line 232, in make_request
    raw_response += sock.recv(4096)
socket.timeout: timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "p.py", line 63, in <module>
    g._fund_account(ZERO_ADDR)
  File "p.py", line 49, in _fund_account
    'gas': 21000,
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/eth.py", line 269, in sendTransaction
    [transaction],
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/manager.py", line 109, in request_blocking
    response = self._make_request(method, params)
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/manager.py", line 92, in _make_request
    return request_func(method, params)
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/formatting.py", line 50, in apply_formatters
    response = make_request(method, params)
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/gas_price_strategy.py", line 18, in middleware
    return make_request(method, params)
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/formatting.py", line 48, in apply_formatters
    response = make_request(method, formatted_params)
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/attrdict.py", line 18, in middleware
    response = make_request(method, params)
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/formatting.py", line 48, in apply_formatters
    response = make_request(method, formatted_params)
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/normalize_errors.py", line 9, in middleware
    result = make_request(method, params)
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/formatting.py", line 48, in apply_formatters
    response = make_request(method, formatted_params)
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/formatting.py", line 48, in apply_formatters
    response = make_request(method, formatted_params)
  File "cytoolz/functoolz.pyx", line 232, in cytoolz.functoolz.curry.__call__
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/middleware/formatting.py", line 50, in apply_formatters
    response = make_request(method, params)
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/providers/ipc.py", line 234, in make_request
    timeout.sleep(0)
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/utils/threads.py", line 68, in sleep
    self.check()
  File "/home/admin_imapp/geth-test/venv/lib/python3.6/site-packages/web3/utils/threads.py", line 61, in check
    raise self
web3.utils.threads.Timeout: 10 seconds

Here's a snippet that can be used to reproduce the above error:

import atexit
import subprocess
import tempfile
import time
import sys

from web3 import Web3
from web3.middleware import geth_poa_middleware
from web3.providers import IPCProvider


ZERO_ADDR = '0x' + 40 * '0'


class TestGeth:
    def wait_for_pending(self) -> None:
        while int(self.web3.txpool.status['pending'], 16) > 0:
            time.sleep(0.01)

    def spawn_geth_process(self):
        ipcpath = tempfile.mkstemp(suffix='.ipc')[1]
        proc = subprocess.Popen(
            ['geth', '--dev', '-ipcpath={}'.format(ipcpath)],
            stdout=subprocess.PIPE,
            stdin=subprocess.PIPE,
            stderr=subprocess.PIPE,
        )
        atexit.register(lambda: proc.kill())
        self.proc = proc
        self.web3 = Web3(IPCProvider(ipcpath))
        self.web3.middleware_stack.inject(geth_poa_middleware, layer=0)
        while not self.web3.isConnected():
            time.sleep(0.1)

    def fund_account(
            self,
            address: str,
            amount: int = 10 ** 18 ) -> None:
        from_addr = self.web3.personal.listAccounts[0]
        self.web3.eth.sendTransaction({
            'from': from_addr,
            'to': address,
            'value': amount,
            'gas': 21000,
        })

    def close(self):
        self.proc.kill()

    def __init__(self):
        self.spawn_geth_process()


if __name__ == '__main__':
    g = TestGeth()
    for _ in range(int(sys.argv[1])):
        g.fund_account(ZERO_ADDR)
        g.wait_for_pending()
    g.close()

It fails (or sometimes even hangs without the exception) on my machine when run like python3 snippet.py 75 (75 is a parameter of how many requests to make, it works fine with lower numbers). And it works consistently fine on the commit preceding yours.

I'd appreciate any help.

rjl493456442 requested a review from karalabe as a code owner January 11, 2018 05:31

rjl493456442 changed the title ~~WIP: consensus/ethash: move remote agent logic to ethash internal~~ [WIP] consensus/ethash: move remote agent logic to ethash internal Jan 11, 2018

rjl493456442 force-pushed the improve_miner branch from 00fcdd4 to 1dca875 Compare January 12, 2018 07:55

rjl493456442 changed the title ~~[WIP] consensus/ethash: move remote agent logic to ethash internal~~ [WIP Don't Review] consensus/ethash: move remote agent logic to ethash internal Jan 12, 2018

rjl493456442 force-pushed the improve_miner branch 5 times, most recently from 9f62c5f to 137db3f Compare January 15, 2018 09:27

rjl493456442 changed the title ~~[WIP Don't Review] consensus/ethash: move remote agent logic to ethash internal~~ consensus/ethash: move remote agent logic to ethash internal Jan 15, 2018

rjl493456442 changed the title ~~consensus/ethash: move remote agent logic to ethash internal~~ WIP: consensus/ethash: move remote agent logic to ethash internal Jan 15, 2018

rjl493456442 force-pushed the improve_miner branch from 137db3f to 8fbc8f7 Compare January 15, 2018 10:47

rjl493456442 changed the title ~~WIP: consensus/ethash: move remote agent logic to ethash internal~~ consensus/ethash: move remote agent logic to ethash internal Jan 15, 2018

rjl493456442 force-pushed the improve_miner branch 3 times, most recently from dd58b1d to da3d009 Compare January 17, 2018 03:31

rjl493456442 force-pushed the improve_miner branch from da3d009 to cd478b8 Compare January 31, 2018 06:35

karalabe requested changes Mar 28, 2018

View reviewed changes

rjl493456442 force-pushed the improve_miner branch 2 times, most recently from e290051 to e6c4652 Compare March 30, 2018 06:13

rjl493456442 requested a review from zsfelfoldi as a code owner March 30, 2018 06:13

rjl493456442 force-pushed the improve_miner branch 5 times, most recently from e869923 to 9ac699e Compare April 2, 2018 05:42

holiman reviewed Apr 7, 2018

View reviewed changes

rjl493456442 force-pushed the improve_miner branch from 9ac699e to 8a9e11c Compare April 12, 2018 12:28

rjl493456442 force-pushed the improve_miner branch from 8a9e11c to e0f84a4 Compare April 24, 2018 06:30

rjl493456442 force-pushed the improve_miner branch 4 times, most recently from 133895a to 21a9435 Compare June 20, 2018 10:42

rjl493456442 added 10 commits August 2, 2018 16:20

consensus/ethash: start remote ggoroutine to handle remote mining

48d15eb

consensus/ethash: expose remote miner api

9833ccd

consensus/ethash: expose submitHashrate api

d2d3b84

miner, ethash: push empty block to sealer without waiting execution

dd83e3d

consensus, internal: add getHashrate API for ethash

31bcd52

consensus: add three method for consensus interface

3255569

miner: expose consensus engine running status to miner

ecafffb

eth, miner: specify etherbase when miner created

41a32a9

miner: commit new work when consensus engine is started

a218f37

consensus, miner: fix some logics

64abec0

rjl493456442 force-pushed the improve_miner branch from 21a9435 to 64abec0 Compare August 2, 2018 08:21

all: delete useless interfaces

1e844fe

karalabe requested changes Aug 3, 2018

View reviewed changes

consensus: polish a bit

2a08b49

karalabe approved these changes Aug 3, 2018

View reviewed changes

karalabe added this to the 1.8.14 milestone Aug 3, 2018

karalabe merged commit 51db597 into ethereum:master Aug 3, 2018

Krigpl mentioned this pull request Sep 9, 2019

Certain tests do not work with Geth 1.8.14 and above golemfactory/golem-smart-contracts-interface#67

Open

JukLee0ira mentioned this pull request Aug 15, 2024

fix a bug in PR #583: snapshot is not updated XinFinOrg/XDPoSChain#604

Merged

20 tasks

tungng98 mentioned this pull request Nov 18, 2024

feat(miner/worker): remove contention on worker.currentMu for pending API requests BuildOnViction/victionchain#477

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consensus/ethash: move remote agent logic to ethash internal #15853

consensus/ethash: move remote agent logic to ethash internal #15853

rjl493456442 commented Jan 11, 2018 •

edited

Loading

rjl493456442 commented Jan 26, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

karalabe Mar 28, 2018

holiman left a comment

holiman Apr 7, 2018

holiman Apr 7, 2018

holiman Apr 7, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe Aug 3, 2018

karalabe left a comment

ghost commented Aug 3, 2018

hadv commented Aug 6, 2018

Krigpl commented May 7, 2019

consensus/ethash: move remote agent logic to ethash internal #15853

consensus/ethash: move remote agent logic to ethash internal #15853

Conversation

rjl493456442 commented Jan 11, 2018 • edited Loading

rjl493456442 commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karalabe left a comment

Choose a reason for hiding this comment

ghost commented Aug 3, 2018

hadv commented Aug 6, 2018

Krigpl commented May 7, 2019

rjl493456442 commented Jan 11, 2018 •

edited

Loading