Always add default_witness_commitment with GBT client support #9189
Conversation
sipa
force-pushed
the
alwayscommit
branch
2 times, most recently
from
04a1ba9 to
26af85c
Compare
|
Concept ACK. Will test this on some live pools using #9000. |
src/test/miner_tests.cpp
Outdated
| txCoinbase.vin[0].scriptSig = CScript(); | ||
| txCoinbase.vin[0].scriptSig.push_back(blockinfo[i].extranonce); | ||
| txCoinbase.vin[0].scriptSig.push_back(chainActive.Height()); | ||
| txCoinbase.vout.resize(1); |
There was a problem hiding this comment.
I'd forgotten how this test worked and it took me a little while to track down why this is needed. In case other reviewers are wondering: the blocks generated here won't have valid proof-of-work if we change the contents, because of the hardcoded nonces up above.
|
ACK. Tested with some hacks to p2p-segwit.py to verify that the default_witness_commitment is correct (afaik there was no explicit test for this before(?)), and that it is only produced if the segwit softfork is defined and the client specifies the segwit rule in the gbt call. |
|
@sdaftuar Awesome, can you publish those tests?
|
|
Concept ACK, looking forward to hearing back poolserver test results. It would be good to try a mutation test with that harness Suhas mentioned. |
|
This should be tagged for backport. |
|
@sipa I tried to clean up my hacks but this is still on the quick&dirty side of things, but maybe useful for others to continue testing: sdaftuar@cdafaad |
|
@sipa needs rebase |
|
Rebased. |
|
Tested ACK, works as intended. Unfortunately though, several pool implementations only check for default_witness_commitment if they receive the segwit rule. I'll work on upstreaming patches for insertion any time default_witness_commitment is present. |
|
ACK. |
|
Backported in #9264, removing tag. |
Github-Pull: bitcoin#9189 Rebased-From: ad04d1c
Github-Pull: bitcoin#9189 Rebased-From: 95f4a03
6 participants
As a consistency check, it is useful to see whether GBT clients are ready for segwit deployment before segwit is actually used on the network (and even before it activates).
The earlier behaviour only inserted a
default_witness_commitmentfield in the GBT response whenever actual witness transactions were present. This PR changes it to do whenever 1) the GBT client claims to support segwit and 2) a segwit softfork is defined (which is effectively always in the current codebase, but may not be true for backports).