Default to NODE_WITNESS in nLocalServices (p2p)

Host: jnewbery  -  PR author: dhruv

The PR branch HEAD was c44bac14b4 at the time of this review club meeting.


  • 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.


  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK?

  2. This PR removed two subtests in What are those two subtests? What were they testing? Why can they now be removed?

  3. What does the CNode.GetLocalServices() method do?

  4. Why is this PR able to remove several calls to GetLocalServices()?

  5. What does the GetBlockScriptFlags() function do?

  6. 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!)

  7. What does GenerateCoinbaseCommitment() do? Why is ok to remove the consensusParams.SegwitHeight check in that function?

Meeting Log

  117:00 <jnewbery> #startmeeting
  217:00 <jnewbery> hello! Welcome to PR Review Club. Feel free to say hi to let people know you're here.
  317:00 <glozow> hi
  417:00 <schmidty> hi
  517:00 <sriramdvt> hi
  617:00 <dergoegge> hallo
  717:01 <lightlike> hi
  817:01 <emzy> hi
  917:01 <gite> hi
 1017:01 <svav> hi
 1117:01 <jnewbery> Is anyone here for the first time?
 1217:01 <gite> yes sir
 1317:01 <glozow> welcome gite!
 1417:01 <jnewbery> gite: welcome!
 1517:01 <jnewbery> as usual, the notes and questions are here:
 1617:01 <gite> Excited to be here!! thanks
 1717: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!
 1817:02 <jnewbery> who had a chance to review the PR / read the notes & questions? (y/n)
 1917:02 <emzy> n
 2017:03 <dergoegge> ~y
 2117:03 <gite> n
 2217:03 <svav> y
 2317:03 <lightlike> y, some weeks ago though
 2417:03 <sriramdvt> read the notes - y
 2517:03 <jnewbery> lightlike: I saw you left some review comments and found some additional dead code. Good stuff!
 2617: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?
 2717:06 <jnewbery> Does it change external behaviour/functionality?
 2817:07 <dergoegge> no NODE_WITNESS is always set already
 2917: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.
 3017: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)
 3117:08 <jnewbery> dergoegge lightlike: exactly right
 3217:08 <emzy> disabeling segwit with -segwitheight=-1 is not posible anymore. But it has no practical use outsite of tests.
 3317:09 <jnewbery> any questions about the motivation/high level summary of this PR?
 3417:09 <jnewbery> emzy: yes, exactly right
 3517:09 <jnewbery> Next question: This PR removed two subtests in What are those two subtests? What were they testing? Why can they now be removed?
 3617:12 <glozow> test_getblocktemplate_before_lockin and test_upgrade_after_activation
 3717:12 <jnewbery> glozow: right, and what did those tests do?
 3817:12 <jnewbery> and why is it ok to remove them?
 3917:12 <lightlike> looks like test_upgrade_after_activation is moved to some degree, not just deleted
 4017: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 (and it's not testing p2p functionality)
 4117:14 <glozow> fum
 4217:14 <glozow> oops sorree
 4317:15 <glozow> functionality* being upgrading a presegwit node after activation
 4417:15 <glozow> and redownloading the blocks it wasn't able to fully balidate before
 4517: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
 4617:16 <jnewbery> glozow: yes, that's right. That functionality was modified in
 4717:16 <lightlike> why were segwit-aware nodes checking the witness commitment before lock in?
 4817:17 <glozow> lightlike: oo they were?
 4917:17 <lightlike> glozow: looking at the removed code of test_getblocktemplate_before_lockin, it seems that way
 5017:17 <jnewbery> glozow:
 5117:18 <jnewbery> This is checking the witness commitment in the getblocktemplate output
 5217:19 <jnewbery> lightlike: I'm not sure why they were doing that. I'd need to dig into the history a bit
 5317:20 <jnewbery> here we go:
 5417: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)."
 5517:21 <lightlike> interesting, thanks!
 5617:21 <jnewbery> In any case, segwit is now active on mainnet, so this functionality doesn't need to be tested any more
 5717:21 <jnewbery> Next question: What does the CNode.GetLocalServices() method do?
 5817:22 <dergoegge> Returns the service bits offered to the node (NODE_NETWORK, NODE_BLOOM, etc).
 5917:22 <dergoegge> how would i get the service bits that the node offers to us?
 6017:23 <glozow> who is "us?"
 6117: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
 6217:23 <jnewbery> us = this node
 6317:24 <glozow> oh oh
 6417:24 <jnewbery> nLocalServices is the service bits the we have offered to the peer
 6517:25 <dergoegge> yes
 6617:25 <jnewbery> nServices is the service bits that the peer has offered to us
 6717:25 <dergoegge> ah thanks
 6817:25 <jnewbery> the service bits are included in the p2p `version` message that's sent by both parties at the start of the connection
 6917:26 <ben85> I think it happens in VERSION message processing
 7017:26 <ben85> nServices = ServiceFlags(nServiceInt);
 7117:26 <ben85> if (!pfrom.IsInboundConn())
 7217:26 <ben85> {
 7317:26 <ben85> m_addrman.SetServices(pfrom.addr, nServices);
 7417:26 <ben85> }
 7517:27 <dergoegge> do we offer different services to different nodes? or why does each CNode instance have its own nLocalServices?
 7617:27 <jnewbery> ben85: yes - that's where we receive the service bits that the peer is sending to us. You'll see further down in that function, we store those in pfrom.nServices:
 7717:27 <jnewbery> dergoegge: very good question! What do you think?
 7817:28 <dergoegge> i dont know but i think nLocalServices is the same for every peer
 7917:28 <jnewbery> dergoegge: if that were the case, then we wouldn't need an nLocalServices member variable in each CNode object
 8017:28 <dergoegge> true
 8117:29 <dergoegge> in what case do we serve peers differently?
 8217:29 <svav> if they are using segwit or not?
 8317:29 <jnewbery> there are some base service bits that we set for all peers. They're defined globally here:
 8417:29 <jnewbery> (and after this PR, NODE_WITNESS is added to that)
 8517: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)
 8617:30 <jnewbery> dergoegge: there are additional service bits that we may offer to peers. Can you think of what some of those are?
 8717:31 <dergoegge> NODE_BLOOM?
 8817:31 <jnewbery> dergoegge: exactly right
 8917:32 <ben85> NODE_COMPACT_FILTERS ?
 9017: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.
 9117:32 <dergoegge> they are all listed here:
 9217:32 <jnewbery> dergoegge: you can see here where we'll add NODE_BLOOM if we've set certain permissions for the peer
 9317:32 <jnewbery> ben85: I think NODE_COMPACT_FILTERS is enabled/disabled globally for all peers
 9417:33 <jnewbery> lightlike: ah yes, you're right. That does seem a bit messy.
 9517:33 <dergoegge> so we limit who we advertise the NODE_BLOOM service bit to? this might be a bit off-topic
 9617:34 <jnewbery> dergoegge: yes, we can advertise NODE_BLOOM only to peers that we've configured to support sending bloom filters to
 9717:35 <ben85> Very interesting these NetPermissionFlags. Are they attributed to an other peer manually?
 9817:36 <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
 9917:36 <jnewbery> next question: Why is this PR able to remove several calls to GetLocalServices()?
10017:37 <dergoegge> if (GetLocalServices() & NODE_WITNESS) checks are no longer needed since NODE_WITNESS is always set.
10117:37 <ben85> Because this validation (pfrom->GetLocalServices() & NODE_WITNESS) is no longer necessary
10217: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.
10317:41 <jnewbery> q5: What does the GetBlockScriptFlags() function do?
10417:43 <lightlike> it determines how to verify scripts; segwit/taproot/other softwork rules for scripts must only should be enforced after their activation height.
10517:43 <ben85> According to the comment, it returns the script flags which should be checked for a given block
10617:44 <jnewbery> lightlike ben85: yes, exactly right
10717:44 <jnewbery> next question is a little harder
10817:44 <jnewbery> q6: Why is it ok to always set SCRIPT_VERIFY_WITNESS when SCRIPT_VERIFY_P2SH is set?
10917:44 <ben85> GetBlockScriptFlags() is called only in CChainState::ConnectBlock() and MemPoolAccept::ConsensusScriptChecks(). Interesting.
11017: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)
11117:47 <jnewbery> any thoughts about the SCRIPT_VERIFY_WITNESS/SCRIPT_VERIFY_P2SH question?
11217:47 <ben85> Because Segwit was activated after the BIP 16
11317:47 <ben85> After the P2SH ?
11417:48 <jnewbery> the logic was added in this PR, which includes a good summary:
11517:50 <dergoegge> why do we backdate these flags? just to save some logic?
11617:50 <jnewbery> The idea was to simplify the logic around when to enforce the rules
11717:50 <dergoegge> will we be able to do the same for taproot?
11817:51 <jnewbery> The link to the chat is dead because botbotme no longer hosts chat logs. You can get it here instead:
11917:51 <dergoegge>
12017:52 <jnewbery> dergoegge: good question. I'm not sure
12117:52 <dergoegge> ah oops should have read your message to the end :D
12217:52 <dergoegge> i guess we could ask that question for all the soft forks?
12317: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
12417: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?
12517:53 <jnewbery> (or any witness version 1 outputs that are invalid under taproot rules and get spent between now and activation)
12617:54 <jnewbery> I'd recommend looking at PR 11739 and reading the chat history around it. I think it's very interesting history.
12717:54 <jnewbery> ok, final question. What does GenerateCoinbaseCommitment() do? Why is ok to remove the consensusParams.SegwitHeight check in that function?
12817: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)
12917:55 <sipa> it's not just that it was added later, it actively depends on it
13017:55 <jnewbery> sipa: 👍
13117: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
13217:56 <sipa> for production this doesn't matter of course, as we know P2SH was long buried when WITNESS came into play
13317:58 <jnewbery> GenerateCoinbaseCommitment calculates the block commitment defined in BIP 141 here:
13417:58 <lightlike> GenerateCoinbaseCommitment() is called in mining.cpp and adds the commitment to the merkle root of the witness data to the coinbase tx
13517:58 <lightlike> since consensusParams.SegwitHeight can no longer be std::numeric_limits<int>::max , the check can be removed
13617:59 <jnewbery> lightlike: exactly right!
13717:59 <jnewbery> any final questions before we wrap up?
13818: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!
13918:00 <jnewbery> thanks all
14018:00 <jnewbery> #endmeeting