The PR branch HEAD was c44bac14b4 at the time of this review club meeting.
Notes
Segwit was a softfork defined in BIP
141, with
P2P changes defined in BIP
144.
Segwit was activated at block 481,824 in August 2017. Prior to activation,
some very careful testing was carried out to verify different scenarios, for
example:
How are transactions and blocks relayed between unupgraded and upgraded
nodes?
How do upgraded nodes find other upgraded nodes to connect to?
If a node is unupgraded at activation time and subsequently upgrades, how
does it ensure that the blocks that it previously validated (without segwit
rules) are valid according to segwit rules?
To enable this kind of testing, PR
8418 made it possible to
configure the segwit activation parameters using a -bip9params
configuration option. That configuration option was later renamed to
-vbparams in PR 10463, and
replaced with -segwitheight in PR
16060.
Those options allowed starting a node which would never activate segwit by
passing -vbparams=segwit:0:0 (or later, -segwitheight=-1). This was used
in the functional tests to test the node’s behavior across activation.
The segwit mainnet activation was a one-time event. Now that segwit has been
activated, those tests are no longer required.
This PR removes the final tests that made use of -segwitheight=0. With those
tests removed, the special casing for -segwitheight=-1 behavior can also be
removed. That special casing impacted logic in net_processing, validation and
mining.
Why is it ok to always set SCRIPT_VERIFY_WITNESS when SCRIPT_VERIFY_P2SH
is set? (This isn’t immediately obvious and will require some code/github
archaeology!)
What does
GenerateCoinbaseCommitment()
do? Why is ok to remove the consensusParams.SegwitHeight check in that function?
<jnewbery> I'll use the questions to prompt conversation, but if you have an observation or question of your own, feel free to jump in at any time. Don't be shy!
<lightlike> cleaning up some special functionality which was needed for the segwit transition period, which is no longer needed after segwit has locked in/is only used in tests.
<jnewbery> Next question: This PR removed two subtests in p2p_segwit.py. What are those two subtests? What were they testing? Why can they now be removed?
<jnewbery> lightlike: right. We're still testing that functionality, but it's moved to its own test, since it doesn't have any interaction with the rest of p2p_segwit.py (and it's not testing p2p functionality)
<jnewbery> the test_getblocktemplate_before_lockin was testing the mining functionality before segwit lock-in. Segwit has locked in (and activated) for many years so we don't need to test that any more
<jnewbery> "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)."
<jnewbery> dergoegge: it's important to distinguish between what service bits were advertised by us to the peer, and what service bits were advertised by the peer to us
<jnewbery> it's a shame that this logic is spread out between init, net and net_processing. I'd like to consolidate that logic in net_processing as part of https://github.com/bitcoin/bitcoin/issues/19398
<jnewbery> dergoegge ben85: right. Those conditionals in net_processing only remained because of the possibility that segwit was disabled in the tests. Now that we no longer need that functionality in the tests, we can remove the conditionals.
<lightlike> it determines how to verify scripts; segwit/taproot/other softwork rules for scripts must only should be enforced after their activation height.
<jnewbery> ben85: right, that's where we're validating a block, or validating a transaction to put into the mempool (which must be valid for the next block)
<jnewbery> I guess it depends on whether there have been any witness version 1 outputs that have been spent and would be invalid after the taproot softfork
<sipa> i would assume that (if there are no pre-activation taproot violations when it activates), when taproot gets buried, it can be unconditionally turned on?
<sipa> VERIFY_WITNESS depends on VERIFY_P2SH being set; the reason is that if WITNESS was allowed without P2SH, adding P2SH would not be a softfork (due to the order of validation operations in the current code)
<sipa> this is really just a nice property for testing, so that the invariant applies that adding more verification flags can never make validation go from invalid to valid