Default to NODE_WITNESS in nLocalServices (
p2p) Jun 30, 2021
The PR branch HEAD was c44bac14b4 at the time of this review club meeting.
Segwit was a softfork defined in
P2P changes defined in BIP
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
How are transactions and blocks relayed between unupgraded and upgraded
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,
8418 made it possible to
configure the segwit activation parameters using a
configuration option. That configuration option was later renamed to
PR 10463, and
Those options allowed starting a node which would never activate segwit by
-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
Did you review the PR?
Concept ACK, approach ACK, tested ACK, or
This PR removed two subtests in
p2p_segwit.py. What are those two
subtests? What were they testing? Why can they now be removed?
What does the
CNode.GetLocalServices() method do?
Why is this PR able to remove several calls to
What does the
Why is it ok to always set
is set? (This isn’t immediately obvious and will require some code/github
do? Why is ok to remove the
consensusParams.SegwitHeight check in that function?
1 17:00 <jnewbery> #startmeeting
2 17:00 <jnewbery> hello! Welcome to PR Review Club. Feel free to say hi to let people know you're here.
11 17:01 <jnewbery> Is anyone here for the first time?
13 17:01 <glozow> welcome gite!
14 17:01 <jnewbery> gite: welcome!
16 17:01 <gite> Excited to be here!! thanks
17 17:02 <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!
18 17:02 <jnewbery> who had a chance to review the PR / read the notes & questions? (y/n)
23 17:03 <lightlike> y, some weeks ago though
24 17:03 <sriramdvt> read the notes - y
25 17:03 <jnewbery> lightlike: I saw you left some review comments and found some additional dead code. Good stuff!
26 17:04 <jnewbery> ok, to start off, can you all briefly explain what this PR is attempting to do. Are you concept ACK, approach ACK, tested ACK, or NACK?
27 17:06 <jnewbery> Does it change external behaviour/functionality?
28 17:07 <dergoegge> no NODE_WITNESS is always set already
29 17:07 <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.
30 17:07 <jnewbery> ok, so this PR is a refactor. It shouldn't change any externally observable behaviour (except in regtest mode in the the functional tests)
31 17:08 <jnewbery> dergoegge lightlike: exactly right
32 17:08 <emzy> disabeling segwit with -segwitheight=-1 is not posible anymore. But it has no practical use outsite of tests.
33 17:09 <jnewbery> any questions about the motivation/high level summary of this PR?
34 17:09 <jnewbery> emzy: yes, exactly right
35 17:09 <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?
36 17:12 <glozow> test_getblocktemplate_before_lockin and test_upgrade_after_activation
37 17:12 <jnewbery> glozow: right, and what did those tests do?
38 17:12 <jnewbery> and why is it ok to remove them?
39 17:12 <lightlike> looks like test_upgrade_after_activation is moved to some degree, not just deleted
40 17:14 <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)
42 17:14 <glozow> oops sorree
43 17:15 <glozow> functionality* being upgrading a presegwit node after activation
44 17:15 <glozow> and redownloading the blocks it wasn't able to fully balidate before
45 17:16 <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
47 17:16 <lightlike> why were segwit-aware nodes checking the witness commitment before lock in?
48 17:17 <glozow> lightlike: oo they were?
49 17:17 <lightlike> glozow: looking at the removed code of test_getblocktemplate_before_lockin, it seems that way
51 17:18 <jnewbery> This is checking the witness commitment in the getblocktemplate output
52 17:19 <jnewbery> lightlike: I'm not sure why they were doing that. I'd need to dig into the history a bit
54 17:20 <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)."
55 17:21 <lightlike> interesting, thanks!
56 17:21 <jnewbery> In any case, segwit is now active on mainnet, so this functionality doesn't need to be tested any more
57 17:21 <jnewbery> Next question: What does the CNode.GetLocalServices() method do?
58 17:22 <dergoegge> Returns the service bits offered to the node (NODE_NETWORK, NODE_BLOOM, etc).
59 17:22 <dergoegge> how would i get the service bits that the node offers to us?
60 17:23 <glozow> who is "us?"
61 17:23 <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
62 17:23 <jnewbery> us = this node
64 17:24 <jnewbery> nLocalServices is the service bits the we have offered to the peer
66 17:25 <jnewbery> nServices is the service bits that the peer has offered to us
67 17:25 <dergoegge> ah thanks
68 17:25 <jnewbery> the service bits are included in the p2p `version` message that's sent by both parties at the start of the connection
69 17:26 <ben85> I think it happens in VERSION message processing
70 17:26 <ben85> nServices = ServiceFlags(nServiceInt);
71 17:26 <ben85> if (!pfrom.IsInboundConn())
73 17:26 <ben85> m_addrman.SetServices(pfrom.addr, nServices);
75 17:27 <dergoegge> do we offer different services to different nodes? or why does each CNode instance have its own nLocalServices?
77 17:27 <jnewbery> dergoegge: very good question! What do you think?
78 17:28 <dergoegge> i dont know but i think nLocalServices is the same for every peer
79 17:28 <jnewbery> dergoegge: if that were the case, then we wouldn't need an nLocalServices member variable in each CNode object
81 17:29 <dergoegge> in what case do we serve peers differently?
82 17:29 <svav> if they are using segwit or not?
84 17:29 <jnewbery> (and after this PR, NODE_WITNESS is added to that)
85 17:30 <jnewbery> svav: no, we'll always advertise NODE_WITNESS to all peers (except, before this PR, if we've disabled segwit for use in a functional test)
86 17:30 <jnewbery> dergoegge: there are additional service bits that we may offer to peers. Can you think of what some of those are?
87 17:31 <dergoegge> NODE_BLOOM?
88 17:31 <jnewbery> dergoegge: exactly right
89 17:32 <ben85> NODE_COMPACT_FILTERS ?
90 17:32 <lightlike> hmm, if we are in pruned mode, we first set NODE_NETWORK and then unset it later in init.cpp. that seems a bit strange.
93 17:32 <jnewbery> ben85: I think NODE_COMPACT_FILTERS is enabled/disabled globally for all peers
94 17:33 <jnewbery> lightlike: ah yes, you're right. That does seem a bit messy.
95 17:33 <dergoegge> so we limit who we advertise the NODE_BLOOM service bit to? this might be a bit off-topic
96 17:34 <jnewbery> dergoegge: yes, we can advertise NODE_BLOOM only to peers that we've configured to support sending bloom filters to
97 17:35 <ben85> Very interesting these NetPermissionFlags. Are they attributed to an other peer manually?
99 17:36 <jnewbery> next question: Why is this PR able to remove several calls to GetLocalServices()?
100 17:37 <dergoegge> if (GetLocalServices() & NODE_WITNESS) checks are no longer needed since NODE_WITNESS is always set.
101 17:37 <ben85> Because this validation (pfrom->GetLocalServices() & NODE_WITNESS) is no longer necessary
102 17:39 <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.
103 17:41 <jnewbery> q5: What does the GetBlockScriptFlags() function do?
104 17:43 <lightlike> it determines how to verify scripts; segwit/taproot/other softwork rules for scripts must only should be enforced after their activation height.
105 17:43 <ben85> According to the comment, it returns the script flags which should be checked for a given block
106 17:44 <jnewbery> lightlike ben85: yes, exactly right
107 17:44 <jnewbery> next question is a little harder
108 17:44 <jnewbery> q6: Why is it ok to always set SCRIPT_VERIFY_WITNESS when SCRIPT_VERIFY_P2SH is set?
109 17:44 <ben85> GetBlockScriptFlags() is called only in CChainState::ConnectBlock() and MemPoolAccept::ConsensusScriptChecks(). Interesting.
110 17:46 <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)
111 17:47 <jnewbery> any thoughts about the SCRIPT_VERIFY_WITNESS/SCRIPT_VERIFY_P2SH question?
112 17:47 <ben85> Because Segwit was activated after the BIP 16
113 17:47 <ben85> After the P2SH ?
115 17:50 <dergoegge> why do we backdate these flags? just to save some logic?
116 17:50 <jnewbery> The idea was to simplify the logic around when to enforce the rules
117 17:50 <dergoegge> will we be able to do the same for taproot?
120 17:52 <jnewbery> dergoegge: good question. I'm not sure
121 17:52 <dergoegge> ah oops should have read your message to the end :D
122 17:52 <dergoegge> i guess we could ask that question for all the soft forks?
123 17:52 <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
124 17:52 <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?
125 17:53 <jnewbery> (or any witness version 1 outputs that are invalid under taproot rules and get spent between now and activation)
126 17:54 <jnewbery> I'd recommend looking at PR 11739 and reading the chat history around it. I think it's very interesting history.
127 17:54 <jnewbery> ok, final question. What does GenerateCoinbaseCommitment() do? Why is ok to remove the consensusParams.SegwitHeight check in that function?
128 17:55 <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)
129 17:55 <sipa> it's not just that it was added later, it actively depends on it
130 17:55 <jnewbery> sipa: 👍
131 17:56 <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
132 17:56 <sipa> for production this doesn't matter of course, as we know P2SH was long buried when WITNESS came into play
134 17:58 <lightlike> GenerateCoinbaseCommitment() is called in mining.cpp and adds the commitment to the merkle root of the witness data to the coinbase tx
135 17:58 <lightlike> since consensusParams.SegwitHeight can no longer be std::numeric_limits<int>::max , the check can be removed
136 17:59 <jnewbery> lightlike: exactly right!
137 17:59 <jnewbery> any final questions before we wrap up?
138 18:00 <jnewbery> Reminder that we're always looking for volunteer hosts. Let me know if you'd like to host a future review club meeting!
139 18:00 <jnewbery> thanks all
140 18:00 <jnewbery> #endmeeting