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

Major fixes & improvements for getwork #583

Merged
merged 4 commits into from
Apr 2, 2021
Merged

Major fixes & improvements for getwork #583

merged 4 commits into from
Apr 2, 2021

Conversation

chjj
Copy link
Contributor

@chjj chjj commented Mar 31, 2021

It appears some (if not most) miners are using getwork/submitwork instead of getblocktemplate. This is presumably because getwork 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 optional mask 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 and witnessroot are also added to the template if coinbasetxn is present (this assumes the client will not be modifying the coinbase).

@coveralls
Copy link

coveralls commented Mar 31, 2021

Pull Request Test Coverage Report for Build 706325466

  • 32 of 43 (74.42%) changed or added relevant lines in 2 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.2%) to 59.948%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/mining/template.js 0 1 0.0%
lib/node/rpc.js 32 42 76.19%
Files with Coverage Reduction New Missed Lines %
lib/covenants/rules.js 1 68.63%
lib/node/rpc.js 2 28.31%
Totals Coverage Status
Change from base Build 680815712: 0.2%
Covered Lines: 19738
Relevant Lines: 30672

💛 - 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)) {
Copy link
Member

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?

5e20a5a

@@ -468,7 +445,7 @@ class BlockTemplate {
*/

toCoinbase() {
return this.coinbase;
return this.coinbase.clone();
Copy link
Member

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" )');
Copy link
Member

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.

@pinheadmz
Copy link
Member

Should would we be adding this attempt from getTemplate() to the merkleMap as well?

hsd/lib/node/rpc.js

Lines 2613 to 2619 in 2d1cbe9

if (attempt) {
this.miner.updateTime(attempt);
} else {
attempt = await this.miner.createBlock();
this.attempt = attempt;
this.lastActivity = util.now();
}

Comment on lines +1540 to +1541
json.merkleroot = attempt.merkleRoot.toString('hex');
json.witnessroot = attempt.witnessRoot.toString('hex');
Copy link
Member

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
Comment on lines 2613 to 2616
// 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) {
Copy link
Member

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();

Copy link
Member

@pinheadmz pinheadmz left a 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

@chjj chjj merged commit 66d8f73 into master Apr 2, 2021
Copy link

@0racl3z 0racl3z left a comment

Choose a reason for hiding this comment

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

awsome!

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

Successfully merging this pull request may close these issues.

None yet

4 participants