getblocktemplate improvements for segwit and sigops

Sjors commented at 6:18 pm on April 6, 2023: member

This adds the following commits:

remove unnecessary check for whether SegWit is active
drop no longer relevant warning about missing sigops field
warn that CheckBlock() underestimates sigops
~mark default_witness_commitment result field as not optional~ (Update 2024-01-09: incorrect, dropped)

Based on #26201, because it touches and adds a relevant test to getblocktemplate. I can unrebase and pick the test if that PR doesn’t make it.

Earlier description, just historical context now.

Sigops

Two recent F2Pool blocks violated the sigops limit. Both by 3.

I suspect they were not using getblocktemplate. If you look at the template right before the valid block at the same height, which was produced two minutes later, you’ll see that it matches the block with 3 small transactions difference. In other words, the valid block producer likely did use getblocktemplate around the same time and did not experience an issue.

It’s also clear by looking at Mempool.Space that the valid block excluded many large bare multisig transactions. Those transactions paid more than enough in fees to be included, but we would have excluded them due to running out of sigops.

As can be seen in BlockAssembler::addPackageTxs and TestPackage we simply don’t add a package to the block if it doesn’t fit the remaining weight or sigops. Custom block construction software might not do that. I initially though they might have forgotten to check SegWit related sigops, but that seems unlikely given that they were only off by 3 in both blocks.

I suspect their mistake was to not count the number of sigops in the coinbase: it pays to legacy P2PKH which uses 4 sigops. Our code reserves 400 sigops for the coinbase, so would not have caused this.

Update 2024-01-09: I dropped the extra debug fields mentioned here

Anyway, while checking our code I added a few extra fields to the block statistics of~ getblocktemplate:

sigops: total sigops for the block excluding the coinbase
coinbase_reserved_sigops: how many sigops we reserve for the coinbase (400)
weightlimit
coinbase_reserved_weight

~I added a belt and suspenders check for the sigops limit in the RPC code.~ (update 2024-01-09: dropped) Note that we already check it when adding packages to a block and in TestBlockValidity.

Segwit

~I also removed the need to set {"rules": "!segwit"}, the generation of pre-segwit blocks, and the test code that checks activation.~ update 2023-08-18: commit removed

DrahtBot commented at 6:18 pm on April 6, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27427 (validation: Replace MinBIP9WarningHeight with MinBIP9WarningStartTime by dimitaracev)
#26201 (Remove Taproot activation height by Sjors)

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.

Sjors force-pushed on Apr 6, 2023

Sjors commented at 7:18 pm on April 6, 2023: member

The “rules” field was introduced in #7935. It’s not specified in BIP 22 and BIP 23 which describe the expected behaviour of getblocktemplate. But then in BIP 145 for segwit it’s assumed to be there. I’m guessing something in between is documented in Stratum software?

Here’s an example of how it’s used by pool software. There’s also some documentation through bug reports, e.g. #17946. My understanding is that ! means that it’s unsafe to mine without understanding the rule.

It’s unclear to me how it is intented to be used, both as an argument and in the results. I’m also not sure how to interpret this wording in BIP 145:

The ‘!’ rule prefix MUST be enabled on the “segwit” rule for templates including transactions with witness data.

For 6ab119d4c1c8cf3d9c0994c22d75d016a30cdd6a I have assumed that the consumer of this RPC may care about it, so we keep !segwit in the result. I’ve the read the above sentence as referring only to the result of the RPC. With this commit it doesn’t matter if you set it or not.

cc @luke-jr

glozow added the label Mining on Apr 11, 2023

luke-jr changes_requested

luke-jr commented at 10:45 pm on June 22, 2023: member

Concept NACK

Two recent F2Pool blocks violated the sigops limit. Both by 3. I suspect they were not using getblocktemplate.

This PR is unlikely to have avoided it, then.

Anyway, while checking our code I added a few extra fields to the block statistics of getblocktemplate:

These are non-standard and don’t provide anything beyond what we already do implicitly(?). Having them risks clients depending on non-standard behaviour.

(weightlimit was already there)

I also removed the need to set {“rules”: “!segwit”}, the generation of pre-segwit blocks, and the test code that checks activation. These two commits can easily be dropped if people don’t like them.

This violates the BIP 9 spec, and changes the BIP 145 semantics of “sigops”.

Sjors commented at 3:20 pm on June 26, 2023: member

changes the BIP 145 semantics of “sigops”.

How so?

From BIP 145:

Sigops

For templates with “segwit” enabled as a rule, the “sigoplimit” and “sigops” keys must use the new values as calculated in BIP 141.

This PR leaves the “segwit” rule enabled. You just don’t have to specify it: https://github.com/bitcoin/bitcoin/pull/27433/commits/edd00677dbddd5b40fa3d2954e8cb04fcd8359f4#diff-ccc24453c13307f815879738d3bf00eec351417537fbf10dde1468180cacd2f1R821-R822

We do the same thing with csv.

Two recent F2Pool blocks violated the sigops limit. Both by 3. I suspect they were not using getblocktemplate.

This PR is unlikely to have avoided it, then.

I’ve since learned that they do use it, but with patches.

These are non-standard

Do you mean BIP 22/23? It says:

getblocktemplate MUST return a JSON Object containing the following keys

It doesn’t say there can’t be extra fields, though I can see how people can screw up by not implementing from the spec, but rather from the JSON RPC. I can add a comment to the help pointing out they’re not BIP 22/23 fields. Or hide them behind a debug option. It’s useful to know these totals with resorting to jq magic.

DrahtBot added the label Needs rebase on Aug 17, 2023

Sjors marked this as a draft on Aug 18, 2023

Sjors force-pushed on Aug 18, 2023

Sjors commented at 1:30 pm on August 18, 2023: member

Marked as draft because I’m now building on top of #26201, which touches and adds a relevant test to getblocktemplate. I’ll unrebase and pick the test if that PR doesn’t make it.

I dropped 6ab119d4c1c8cf3d9c0994c22d75d016a30cdd6a: it’s still necessary for the (mining software) client to tell us it supports SegWit. This is merely a checkbox now.

In the context of dropping segwit as a required rule I wrote:

We do the same thing with csv.

I guess the argument here is that, unlike segwit, the csv (and taproot) soft fork is safe to use by a miner if they simply use the response from getblocktemplate without modifying it.

The extra sigops, coinbase_reserved_sigops and coinbase_reserved_weight field in the result is now tucked behind a debug argument.

Hope that addresses @luke-jr’s concerns.

Although the debug-only coinbase_reserved_ arguments are just repeating a constant*, and the sigops field is straight-forward to calculate by iterating over all transactions, we can use the debug argument in the future to add additional stuff. Things that aid in anticipating what the next block should look like, e.g. for the Mempool.Space audit feature and @0xB10C’s miningpool.observer. For example we could expand the transactions result with a timestamp of when our mempool first saw it.

I could however still drop the fields from 09fbc4cbfa7b7d86e6da00a737317151088b4b6c and ac3fcb240af8ef96d4e348548995ec1a536ff531, but keep the Assume.

* = given that F2pool shot itself in the foot with custom patches, trying to squeeze out a few more sigops in their blocks, maybe we should make this configurable.

DrahtBot removed the label Needs rebase on Aug 18, 2023

DrahtBot added the label CI failed on Aug 18, 2023

Sjors force-pushed on Aug 21, 2023

DrahtBot added the label Needs rebase on Sep 12, 2023

Sjors force-pushed on Oct 11, 2023

DrahtBot removed the label Needs rebase on Oct 11, 2023

DrahtBot removed the label CI failed on Oct 11, 2023

DrahtBot added the label Needs rebase on Jan 5, 2024

Sjors force-pushed on Jan 9, 2024

Sjors commented at 9:48 am on January 9, 2024: member

Rebased due to (trivial) merge conflict in test.

I simplified this PR by dropping the debug argument total sigop and other debug info from the RPC result (52e24f662563dbd0adbcfda963052b3999ea689c, f1afd72a94ae198e8612f0fb54c3d68479680a2b).

I’ll use the Stratum v2 integration #28983 instead to improve debugging tools where needed.

I also dropped the additional check for the sigops limit in 52e24f662563dbd0adbcfda963052b3999ea689c. We’ve seen in practice that miners sometimes patch template generation code, so additional checks can be helpful. But this particular check would not have worked: the patch set nBlockSigOpsCost to 1, which after SegWit should have been adjusted to 4. Since we have no way to check how many sigops are added to the coinbase, afaik there’s nothing we can do catch a bug like that.

Sjors commented at 10:01 am on January 9, 2024: member

Also dropped b175798c67fbe50bc08007fe184d5407a8c4acf4 because the default_witness_commitment is absent for empty-block templates.

Sjors force-pushed on Jan 9, 2024

DrahtBot removed the label Needs rebase on Jan 9, 2024

DrahtBot added the label CI failed on Jan 13, 2024

Sjors force-pushed on Feb 13, 2024

DrahtBot removed the label CI failed on Feb 13, 2024

DrahtBot added the label Needs rebase on Mar 5, 2024

Remove Taproot activation height

Drop DEPLOYMENT_TAPROOT from consensus.vDeployments.

Bump MinBIP9WarningHeight.

Clarify what is considered a BuriedDeployment and
drop taproot from getdeploymentinfo RPC.

Add a test to getblocktemplate to ensure the taproot
rule is still set.

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

9ca9dcd930

miner: always treat SegWit as active

The getblocktemplate RPC would check if SegWit has activated yet at the tip. This has been the case for more than five years.

c0b01fdb89

rpc: getblocktemplate drop warning for missing sigops field

This field is always present.

The warning was added in a6099ef319a73e2255dca77065600abb22c4f5f8, but even then the field was always present.

eba5ce676f

doc: warn that CheckBlock() underestimates sigops

Counting sigops in the witness requires context that CheckBlock()  does not have, so it only counts sigops for non-segwit transactions.

This should not be a problem. There are 2 call sites:

1. ConnectBlock(): this checks all sigops
2. TestBlockValidity() used by getblocktemplate: this also calls ConnectBlock()
3. AcceptBlock(): this performs limited checks before storing a block on disk
4. ProcessNewBlock(): calls AcceptBlock()
4. VerifyDB(): disconnects the block at level 3, which results in ConnectBlock()

f56480e5fe

Sjors force-pushed on Mar 7, 2024

Sjors commented at 10:40 am on March 7, 2024: member

Rebased after #26201 was rebased.

DrahtBot removed the label Needs rebase on Mar 7, 2024

getblocktemplate improvements for segwit and sigops #27433

Code Coverage

Reviews

Conflicts