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

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

Merged
merged 2 commits into from Dec 21, 2018

Conversation

Projects
None yet
9 participants
@jnewbery
Copy link
Member

commented Nov 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Concept ACK

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 26, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 26, 2018

Concept ACK

@fanquake fanquake added the Mining label Nov 26, 2018

@promag
Copy link
Member

left a comment

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

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Nov 27, 2018

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 jnewbery force-pushed the jnewbery:remove_non_segwit_mining branch Nov 28, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Nov 28, 2018

Suggest adding a "Mining" section to the release notes

Done

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

@jnewbery jnewbery force-pushed the jnewbery:remove_non_segwit_mining branch Nov 28, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Nov 29, 2018

utACK 66c0f5bde1ba2c188933115337cf26a007d979be

@promag
Copy link
Member

left a comment

utACK 66c0f5b, just 2 nits for your consideration.

doc/release-notes.md Outdated
@@ -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`.

This comment has been minimized.

Copy link
@promag

promag Dec 3, 2018

Member

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 4, 2018

Author Member

Updated in the latest commit.

src/rpc/mining.cpp Outdated
// 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()) {

This comment has been minimized.

Copy link
@promag

promag Dec 3, 2018

Member

nit, count() instead of find()?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Dec 4, 2018

Author Member

thanks. Updated

@jnewbery jnewbery force-pushed the jnewbery:remove_non_segwit_mining branch Dec 4, 2018

@jnewbery

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2018

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

@promag

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

utACK 6c57746, no problem

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 4, 2018

re-utACK 6c577469c7

jnewbery added some commits Nov 26, 2018

[mining] segwit option must be set in GBT
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
GBT must now be called with the segwit rule.

@jnewbery jnewbery force-pushed the jnewbery:remove_non_segwit_mining branch to d2ce315 Dec 10, 2018

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

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

@Sjors

This comment has been minimized.

Copy link
Member

commented Dec 11, 2018

tACK d2ce315

@MarcoFalke MarcoFalke merged commit d2ce315 into bitcoin:master Dec 21, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

MarcoFalke added a commit that referenced this pull request Dec 21, 2018

Merge #14811: Mining: Enforce that segwit option must be set in GBT
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

domob1812 added a commit to domob1812/namecore that referenced this pull request Dec 24, 2018

Merge branch 'bitcoin' into auxpow
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.

@jnewbery jnewbery deleted the jnewbery:remove_non_segwit_mining branch Jan 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.