-
Notifications
You must be signed in to change notification settings - Fork 284
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
Major fixes & improvements for getwork #583
Conversation
Pull Request Test Coverage Report for Build 706325466
💛 - Coveralls |
@@ -3223,7 +3223,7 @@ class Chain extends AsyncEmitter { | |||
const state = await this.getState(prev, deployment); | |||
|
|||
if (state === thresholdStates.LOCKED_IN | |||
|| state === thresholdStates.STARTED) { | |||
|| (state === thresholdStates.STARTED && deployment.force)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this effectively the same fix we did here?
@@ -468,7 +445,7 @@ class BlockTemplate { | |||
*/ | |||
|
|||
toCoinbase() { | |||
return this.coinbase; | |||
return this.coinbase.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to return a clone here, but I can't find reference to this function - what's it used for?
if (help || args.length !== 1) | ||
throw new RPCError(errs.MISC_ERROR, 'submitwork ( "data" )'); | ||
if (help || args.length < 1 || args.length > 2) | ||
throw new RPCError(errs.MISC_ERROR, 'submitwork ( "data" "mask" )'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: "data"
isn't optional, shouldn't be inside parentheses.
Should would we be adding this Lines 2613 to 2619 in 2d1cbe9
|
json.merkleroot = attempt.merkleRoot.toString('hex'); | ||
json.witnessroot = attempt.witnessRoot.toString('hex'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a convenient feature - client can still modify TXs in the block before submitting?
lib/node/rpc.js
Outdated
// This check is here to make sure a miner does not | ||
// accidentally OOM the node. This means the merkle | ||
// map is good for 2 hours without a block. | ||
if (this.merkleMap.size >= 720) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to be clear, this is because we won't create a new template / add to merkleMap
until at least 10 seconds pass. So if there are 720 items in the map, that means 7,200 seconds have passed (2 hours).
if (util.now() - this.lastActivity > 10)
this.refreshBlock();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I just left a few nits and questions but I think this makes sense. I like using a null mask as default, all the pools do already and have had to hack hsd to do so. The merkleMap
makes sense, only weird edge cases may be a miner switching from getwork
to getblocktemplate
or using both for some weird reason. The 2 hour timeout might leave a miner with a stale job but hopefully that is a far out edge case. Maybe in the future we can prune that map without wiping it out completely.
ACK 6d6dec5
Show Signature
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK 6d6dec50062618b33194facdf598c6c3d314681f
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmBks8UACgkQ5+KYS2KJ
yTpJrg//TAFQMiICydLN9ocVmvIf8MsTrg5BDJCEZMzX9ZhBOuz8H2XSs6ZxB2AO
peJ+mHY2gdJJ3W5xy/ZCbFcmZ+dTPwvFK9FeSHB1gpxENpd7EpMFIw3ean0yvP0p
qeel9iywyXpQ9WgTXOcYV8pYOZIXVZOtS007kFlA6HeskrSrVg5yM5u2ABmuK8Uh
0NQcn+gkXlIXNT75UZd129MPg1ivg70B1hcVeigKNrhXpxgQpxUvRJW5DYZR6XIb
LIkEamsTanROlYN2jzCamd/iZ7bYEBoblWMb9LmtfaEJ7voJnhQWQE8KOmjLJ+Fu
piKYCrn/jidN2b4zGEiBQTn2mwA6T5ry6urjGrYL711D1Eh/YQlrLfeuxEJ5VAnf
qrSx12LmhsMzOO2AGgfh1wSkqMB70rUQWmYGGBgMX14Db81Wy/4QMVU1UTQMJlWM
Dyir//fitqVPAc/ROalDDjzsVLg14uo/uT15FdrFrfbQKaOds3mYALJTG7uBEy32
SWKyYHrGBhw6FN19W/hL6MoCpUyduve0z6f9yVUFxsaI/t6+43AbTL7ztjgrVsvE
DkUL258Fjf3sHMVzRVMdzixV56OBSwiddh73DbsPyyEOdzLJpN/jI5c/4j+Ms4S3
uTRkxrv6kt8aZUBIMG6tN4WSqN3S5ZignN+JZxvNTZHnfZDPOpo=
=zK1H
-----END PGP SIGNATURE-----
pinheadmz's public key is on keybase
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awsome!
It appears some (if not most) miners are using
getwork
/submitwork
instead ofgetblocktemplate
. This is presumably becausegetwork
is just easier: it requires minimal implementation and serialization on the part of the mining software.getwork
was originally only included for testing immature mining clients, however, the community's reliance on it requires us to give it a second look.There is a nasty bug present in
getwork
which causes blocks to be invalidated when transactions enter the mempool. It seems that miners have been patching this bug themselves to avoid mining invalid blocks. I figure we should make an official fix asap.This PR fixes
getwork
to allow operation on stale blocks with older merkle roots.Some things are also simplified: the RPC no longer auto-generates a mask for the client. This really serves no purpose as mining pools should be doing this in their pool software, generating a mask in accordance with their share target.
submitblock
now takes an optionalmask
argument so the pool can notify hsd of the mask it generated.To encourage the use of
getblocktemplate
, we make it a bit easier to use and migrate to:{"capabilities":["coinbasetxn"]}
has been re-enabled in a sane way which should not conflict with popular bitcoin mining software (if it has been modified to support handshake).merkleroot
andwitnessroot
are also added to the template ifcoinbasetxn
is present (this assumes the client will not be modifying the coinbase).