getblocktemplate improvements for segwit and sigops #27433
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
6e57bfe to
42e5f49
Compare
|
The "rules" field was introduced in #7935. It's not specified in BIP 22 and BIP 23 which describe the expected behaviour of 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 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:
For 6ab119d I have assumed that the consumer of this RPC may care about it, so we keep cc @luke-jr |
There was a problem hiding this comment.
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".
How so? From BIP 145:
This PR leaves the "segwit" rule enabled. You just don't have to specify it: edd0067#diff-ccc24453c13307f815879738d3bf00eec351417537fbf10dde1468180cacd2f1R821-R822 We do the same thing with
I've since learned that they do use it, but with patches.
Do you mean BIP 22/23? It says:
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 |
|
Marked as draft because I'm now building on top of #26201, which touches and adds a relevant test to I dropped 6ab119d: 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
I guess the argument here is that, unlike The extra Hope that addresses @luke-jr's concerns. Although the debug-only I could however still drop the fields from 09fbc4c and ac3fcb2, but keep the
|
|
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 (52e24f6, f1afd72). 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 52e24f6. 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 |
|
Rebased after #26201 was rebased. |
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>
The getblocktemplate RPC would check if SegWit has activated yet at the tip. This has been the case for more than five years.
This field is always present. The warning was added in a6099ef, but even then the field was always present.
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()
|
Simple rebase after #30440. |
This adds the following commits:
mark(Update 2024-01-09: incorrect, dropped)default_witness_commitmentresult field as not optionalBased 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 usegetblocktemplatearound 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::addPackageTxsandTestPackagewe 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 coinbasecoinbase_reserved_sigops: how many sigops we reserve for the coinbase (400)weightlimitcoinbase_reserved_weightI 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 inTestBlockValidity.Segwit
I also removed the need to setupdate 2023-08-18: commit removed{"rules": "!segwit"}, the generation of pre-segwit blocks, and the test code that checks activation.