Mining: Enforce that segwit option must be set in GBT #14811

jnewbery · 2018-11-26T17:38:53Z

Calling getblocktemplate without the segwit rule specified is most
likely a client error, since it results in lower fees for the miner.
Prevent this client error by failing getblocktemplate if called without
the segwit rule specified.

Of the previous 1000 blocks (measured at block 551591 (hash 0x...173c811)), 991 included segwit transactions.

gmaxwell · 2018-11-26T17:50:01Z

Concept ACK

maflcko · 2018-11-26T19:22:37Z

Looks like a nice cleanup, and it might encourage miners running mining hardware or mining software that can't yet handle segwit transactions to upgrade. Though, I can't evaluate how much of a hassle it would be for them, so I won't review this conceptually.

DrahtBot · 2018-11-26T19:24:56Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14918 (RPCHelpMan: Check default values are given at compile-time by MarcoFalke)
#14459 (More RPC help description fixes by ch4ot1c)
#10595 (Bugfix: RPC/Mining: Use pre-segwit sigops and limits, when working with non-segwit GBT clients by luke-jr)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2018-11-26T23:25:50Z

Concept ACK

promag

Is there a reason to not make it implicit and optionally return a warning?

Otherwise could update getblocktemplate description to mention that template_request.rules is not optional, at least be {"rules":["segwit"]}?

luke-jr · 2018-11-27T16:21:56Z

The only time it wouldn't be present is in the case of non-segwit miners. In that case, implying it would likely result in them producing invalid blocks.

promag · 2018-11-27T16:25:37Z

The only time it wouldn't be present is in the case of non-segwit miners. In that case, implying it would likely result in them producing invalid blocks.

@luke-jr required or implicit, they still/eventually have to update.

luke-jr · 2018-11-27T16:28:03Z

Yes, but it's much nicer to have the miner give them an error immediately upon updating bitcoind, than find out weeks later that they've been wasting electricity mining invalid blocks.

gmaxwell · 2018-11-27T17:40:03Z

@promag Silently losing money (and mining invalid blocks!) is not an acceptable failure mode.

Sjors · 2018-11-27T18:20:14Z

Concept ACK after IRC discussion. Suggest adding a "Mining" section to the release notes to make this more visible to the small percentage of miners this may impact.

jnewbery · 2018-11-28T20:17:07Z

Suggest adding a "Mining" section to the release notes

Done

doc/release-notes.md

maflcko · 2018-11-29T21:06:53Z

utACK 66c0f5bde1ba2c188933115337cf26a007d979be

promag

utACK 66c0f5b, just 2 nits for your consideration.

promag · 2018-12-03T23:15:24Z

doc/release-notes.md

@@ -182,6 +189,8 @@ in the Low-level Changes section below.
  P2SH-P2WPKH, and P2SH-P2WSH. Requests for P2WSH and P2SH-P2WSH accept
  an additional `witnessscript` parameter.

+- See the *Mining* section for changes to `getblocktemplate`.


nit, use anchor: See the [Mining](#mining) ....

Updated in the latest commit.

promag · 2018-12-03T23:17:33Z

src/rpc/mining.cpp

-    // don't).
-    bool fSupportsSegwit = setClientRules.find(segwit_info.name) != setClientRules.end();
+    // GBT must be called with 'segwit' set in the rules
+    if (setClientRules.find(segwit_info.name) == setClientRules.end()) {


nit, count() instead of find()?

thanks. Updated

jnewbery · 2018-12-04T15:30:38Z

Thanks @promag . I've taken both of those nits.

promag · 2018-12-04T15:49:26Z

utACK 6c57746, no problem

maflcko · 2018-12-04T16:25:40Z

re-utACK 6c577469c7

src/rpc/mining.cpp

Calling getblocktemplate without the segwit rule specified is most likely a client error, since it results in lower fees for the miner. Prevent this client error by failing getblocktemplate if called without the segwit rule specified.

GBT must now be called with the segwit rule.

jnewbery · 2018-12-10T21:42:54Z

Fixed @MarcoFalke's review comment: https://github.com/bitcoin/bitcoin/compare/6c577469c720ca68e1afb28f0a566f0033af365c..d2ce315fbfd8d8ed52bde9f714ded8c76521cce2

maflcko · 2018-12-10T21:49:35Z

re-utACK d2ce315 (Only change was rebase and making "rules" as well as "template_request" required arguments)

Sjors · 2018-12-11T13:06:58Z

tACK d2ce315

d2ce315 [docs] add release note for change to GBT (John Newbery) 0025c9e [mining] segwit option must be set in GBT (John Newbery) Pull request description: Calling getblocktemplate without the segwit rule specified is most likely a client error, since it results in lower fees for the miner. Prevent this client error by failing getblocktemplate if called without the segwit rule specified. Of the previous 1000 blocks (measured at block [551591 (hash 0x...173c811)](https://blockstream.info/block/000000000000000000173c811e79858808abc3216af607035973f002bef60a7a)), 991 included segwit transactions. Tree-SHA512: 7933b073d72683c9ab9318db46a085ec19a56a14937945c73f783ac7656887619a86b74db0bdfcb8121df44f63a1d6a6fb19e98505b2a26a6a8a6e768e442fee

Upstream Bitcoin contains a change (bitcoin/bitcoin#14811) that forces miners calling getblocktemplate to indicate segwit support. This is not an issue for Namecoin, though, since this is just about giving miners the option to opt-out of segwit *once activated*. Until activation, segwit transactions will still be ignored, independently of the choice made for getblocktemplate.

fanquake added the Mining label Nov 26, 2018

promag reviewed Nov 27, 2018

View reviewed changes

jnewbery force-pushed the remove_non_segwit_mining branch from 510952b to 62a4992 Compare November 28, 2018 20:16

maflcko reviewed Nov 28, 2018

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

jnewbery force-pushed the remove_non_segwit_mining branch from 62a4992 to 66c0f5b Compare November 28, 2018 20:46

promag reviewed Dec 3, 2018

View reviewed changes

jnewbery force-pushed the remove_non_segwit_mining branch from 66c0f5b to 6c57746 Compare December 4, 2018 15:29

maflcko reviewed Dec 5, 2018

View reviewed changes

src/rpc/mining.cpp Show resolved Hide resolved

jnewbery added 2 commits December 10, 2018 16:42

[mining] segwit option must be set in GBT

0025c9e

Calling getblocktemplate without the segwit rule specified is most likely a client error, since it results in lower fees for the miner. Prevent this client error by failing getblocktemplate if called without the segwit rule specified.

[docs] add release note for change to GBT

d2ce315

GBT must now be called with the segwit rule.

jnewbery force-pushed the remove_non_segwit_mining branch from 6c57746 to d2ce315 Compare December 10, 2018 21:42

maflcko merged commit d2ce315 into bitcoin:master Dec 21, 2018

maflcko mentioned this pull request Dec 21, 2018

Bugfix: RPC/Mining: Use pre-segwit sigops and limits, when working with non-segwit GBT clients #10595

Closed

jnewbery deleted the remove_non_segwit_mining branch January 7, 2019 18:17

ajtowns mentioned this pull request Jul 31, 2019

Bury bip9 deployments #16060

Merged

jnewbery mentioned this pull request Jan 17, 2020

Fix GBT: Restore "!segwit" and "csv" to "rules" key #17946

Merged

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Mining: Enforce that segwit option must be set in GBT #14811

Mining: Enforce that segwit option must be set in GBT #14811

Uh oh!

Conversation

jnewbery commented Nov 26, 2018

Uh oh!

gmaxwell commented Nov 26, 2018

Uh oh!

maflcko commented Nov 26, 2018

Uh oh!

DrahtBot commented Nov 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Nov 26, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr commented Nov 27, 2018

Uh oh!

promag commented Nov 27, 2018

Uh oh!

luke-jr commented Nov 27, 2018

Uh oh!

gmaxwell commented Nov 27, 2018

Uh oh!

Sjors commented Nov 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Nov 28, 2018

Uh oh!

Uh oh!

maflcko commented Nov 29, 2018

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

promag Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

promag Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery Dec 4, 2018

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Dec 4, 2018

Uh oh!

promag commented Dec 4, 2018

Uh oh!

maflcko commented Dec 4, 2018

Uh oh!

Uh oh!

jnewbery commented Dec 10, 2018

Uh oh!

maflcko commented Dec 10, 2018

Uh oh!

Sjors commented Dec 11, 2018

Uh oh!

Uh oh!

DrahtBot commented Nov 26, 2018 •

edited

Loading

Sjors commented Nov 27, 2018 •

edited

Loading