--- Log opened Wed Jun 30 00:00:53 2021
00:22 -!- johnzweng [~johnzweng@zweng.at] has joined #bitcoin-core-pr-reviews
01:20 -!- hedron [~hedron@gateway/tor-sasl/hedron] has joined #bitcoin-core-pr-reviews
01:50 -!- fossa [~arne@5.10.5.22] has joined #bitcoin-core-pr-reviews
02:02 -!- kexkey [~kexkey@198.54.132.166] has joined #bitcoin-core-pr-reviews
02:04 -!- kexkey_ [~kexkey@static-198-54-132-118.cust.tzulo.com] has quit [Ping timeout: 246 seconds]
02:30 -!- fossa [~arne@5.10.5.22] has quit [Quit: fossa]
04:36 -!- hedron [~hedron@gateway/tor-sasl/hedron] has quit [Ping timeout: 244 seconds]
06:29 -!- murch [~murch@user/murch] has quit [Quit: WeeChat 1.9.1]
06:31 -!- murch [~murch@pool-162-83-132-16.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews
06:31 -!- murch [~murch@pool-162-83-132-16.nycmny.fios.verizon.net] has quit [Changing host]
06:31 -!- murch [~murch@user/murch] has joined #bitcoin-core-pr-reviews
06:33 -!- murch [~murch@user/murch] has quit [Client Quit]
06:33 -!- murch [~murch@user/murch] has joined #bitcoin-core-pr-reviews
06:34 -!- murch [~murch@user/murch] has quit [Client Quit]
06:34 -!- murch [~murch@user/murch] has joined #bitcoin-core-pr-reviews
07:04 -!- hostadig [~hostadig@45.92.228.38] has joined #bitcoin-core-pr-reviews
07:44 -!- kexkey_ [~kexkey@198.54.132.118] has joined #bitcoin-core-pr-reviews
07:47 -!- kexkey [~kexkey@198.54.132.166] has quit [Ping timeout: 258 seconds]
08:27 < jnewbery> Hi folks. We'll get started in just over 90 minutes. The notes and questions are in the normal place: https://bitcoincore.reviews/21090
08:55 -!- sriramdvt [~sriramdvt@183.83.132.111] has joined #bitcoin-core-pr-reviews
09:02 -!- jamesob [sid180710@id-180710.brockwell.irccloud.com] has joined #bitcoin-core-pr-reviews
09:16 -!- kexkey_ [~kexkey@198.54.132.118] has quit [Read error: Connection reset by peer]
09:16 -!- kexkey [~kexkey@198.54.132.118] has joined #bitcoin-core-pr-reviews
09:31 -!- lightlike [~lightlike@user/lightlike] has joined #bitcoin-core-pr-reviews
09:32 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:38 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has quit [Ping timeout: 252 seconds]
09:39 -!- lightlike [~lightlike@user/lightlike] has quit [Ping timeout: 240 seconds]
09:44 -!- lightlike [~lightlike@user/lightlike] has joined #bitcoin-core-pr-reviews
09:51 -!- dyke [~arne@2a02:810d:a8bf:d4ac:9186:a57:44d:8813] has joined #bitcoin-core-pr-reviews
09:56 -!- observer58 [~observer@cpe-23-242-148-67.socal.res.rr.com] has joined #bitcoin-core-pr-reviews
09:59 -!- gite [~gite@103.81.243.2] has joined #bitcoin-core-pr-reviews
09:59 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
09:59 -!- ben85 [~ben@189.122.121.102] has joined #bitcoin-core-pr-reviews
10:00 < jnewbery> #startmeeting
10:00 < jnewbery> hello! Welcome to PR Review Club. Feel free to say hi to let people know you're here.
10:00 < glozow> hi
10:00 < schmidty> hi
10:00 < sriramdvt> hi
10:00 < dergoegge> hallo
10:01 < lightlike> hi
10:01 < emzy> hi
10:01 < gite> hi
10:01 < svav> hi
10:01 < jnewbery> Is anyone here for the first time?
10:01 < gite> yes sir
10:01 < glozow> welcome gite!
10:01 < jnewbery> gite: welcome!
10:01 < jnewbery> as usual, the notes and questions are here: https://bitcoincore.reviews/21090
10:01 < gite> Excited to be here!! thanks
10: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!
10:02 < jnewbery> who had a chance to review the PR / read the notes & questions? (y/n)
10:02 < emzy> n
10:03 < dergoegge> ~y
10:03 < gite> n
10:03 -!- zakinator [~zakinator@c-98-32-80-238.hsd1.ut.comcast.net] has joined #bitcoin-core-pr-reviews
10:03 < svav> y
10:03 < lightlike> y, some weeks ago though
10:03 < sriramdvt> read the notes - y
10:03 < jnewbery> lightlike: I saw you left some review comments and found some additional dead code. Good stuff!
10:04 -!- zakinator [~zakinator@c-98-32-80-238.hsd1.ut.comcast.net] has quit [Client Quit]
10: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?
10:06 < jnewbery> Does it change external behaviour/functionality?
10:06 -!- zakinator [~zakinator@c-98-32-80-238.hsd1.ut.comcast.net] has joined #bitcoin-core-pr-reviews
10:06 -!- cviss [~cviss@c-75-75-7-8.hsd1.va.comcast.net] has joined #bitcoin-core-pr-reviews
10:07 < dergoegge> no NODE_WITNESS is always set already
10: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.
10: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)
10:08 < jnewbery> dergoegge lightlike: exactly right
10:08 < emzy> disabeling segwit with -segwitheight=-1 is not posible anymore. But it has no practical use outsite of tests.
10:09 < jnewbery> any questions about the motivation/high level summary of this PR?
10:09 < jnewbery> emzy: yes, exactly right
10: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?
10:12 < glozow> test_getblocktemplate_before_lockin and test_upgrade_after_activation
10:12 < jnewbery> glozow: right, and what did those tests do?
10:12 < jnewbery> and why is it ok to remove them?
10:12 < lightlike> looks like test_upgrade_after_activation is moved to some degree, not just deleted
10: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)
10:14 < glozow> fum 
10:14 < glozow> oops sorree
10:15 < glozow> functionality* being upgrading a presegwit node after activation
10:15 < glozow> and redownloading the blocks it wasn't able to fully balidate before
10: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
10:16 < jnewbery> glozow: yes, that's right. That functionality was modified in https://github.com/bitcoin/bitcoin/pull/21009
10:16 < lightlike> why were segwit-aware nodes checking the witness commitment before lock in?
10:17 < glozow> lightlike: oo they were?
10:17 < lightlike> glozow: looking at the removed code of test_getblocktemplate_before_lockin, it seems that way
10:17 < jnewbery> glozow: https://github.com/bitcoin/bitcoin/pull/21090/files#diff-63f1c1e404966953bce51497a03d99e26f333237e26b9c69b20fb43b05520533L569-L578
10:18 < jnewbery> This is checking the witness commitment in the getblocktemplate output
10:19 < jnewbery> lightlike: I'm not sure why they were doing that. I'd need to dig into the history a bit
10:20 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Read error: Connection reset by peer]
10:20 < jnewbery> here we go: https://github.com/bitcoin/bitcoin/pull/9189
10: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)."
10:21 < lightlike> interesting, thanks!
10:21 < jnewbery> In any case, segwit is now active on mainnet, so this functionality doesn't need to be tested any more
10:21 < jnewbery> Next question: What does the CNode.GetLocalServices() method do?
10:22 < dergoegge> Returns the service bits offered to the node (NODE_NETWORK, NODE_BLOOM, etc).
10:22 -!- luke-jr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
10:22 < dergoegge> how would i get the service bits that the node offers to us?
10:23 < glozow> who is "us?"
10: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
10:23 < jnewbery> us = this node
10:24 < glozow> oh oh
10:24 < jnewbery> nLocalServices is the service bits the we have offered to the peer
10:25 < dergoegge> yes
10:25 < jnewbery> nServices is the service bits that the peer has offered to us
10:25 < dergoegge> ah thanks
10:25 < jnewbery> the service bits are included in the p2p `version` message that's sent by both parties at the start of the connection
10:26 < ben85> I think it happens in VERSION message processing
10:26 < ben85> nServices = ServiceFlags(nServiceInt);
10:26 < ben85>         if (!pfrom.IsInboundConn())
10:26 < ben85>         {
10:26 < ben85>             m_addrman.SetServices(pfrom.addr, nServices);
10:26 < ben85>         }
10:27 < dergoegge> do we offer different services to different nodes? or why does each CNode instance have its own nLocalServices?
10: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: https://github.com/bitcoin/bitcoin/blob/3fc20abab03d71a982d6fe9c47155834b256ab17/src/net_processing.cpp#L2525
10:27 < jnewbery> dergoegge: very good question! What do you think?
10:28 < dergoegge> i dont know but i think nLocalServices is the same for every peer
10:28 < jnewbery> dergoegge: if that were the case, then we wouldn't need an nLocalServices member variable in each CNode object
10:28 < dergoegge> true
10:29 < dergoegge> in what case do we serve peers differently?
10:29 < svav> if they are using segwit or not?
10:29 < jnewbery> there are some base service bits that we set for all peers. They're defined globally here: https://github.com/bitcoin/bitcoin/blob/3fc20abab03d71a982d6fe9c47155834b256ab17/src/init.cpp#L718
10:29 < jnewbery> (and after this PR, NODE_WITNESS is added to that)
10: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)
10:30 < jnewbery> dergoegge: there are additional service bits that we may offer to peers. Can you think of what some of those are?
10:31 < dergoegge> NODE_BLOOM? 
10:31 < jnewbery> dergoegge: exactly right
10:32 < ben85> NODE_COMPACT_FILTERS ?
10: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.
10:32 < dergoegge> they are all listed here: https://github.com/bitcoin/bitcoin/blob/3fc20abab03d71a982d6fe9c47155834b256ab17/src/protocol.h#L271
10:32 < jnewbery> dergoegge: you can see here https://github.com/bitcoin/bitcoin/blob/3fc20abab03d71a982d6fe9c47155834b256ab17/src/net.cpp#L1184-L1187 where we'll add NODE_BLOOM if we've set certain permissions for the peer
10:32 < jnewbery> ben85: I think NODE_COMPACT_FILTERS is enabled/disabled globally for all peers
10:33 < jnewbery> lightlike: ah yes, you're right. That does seem a bit messy.
10:33 < dergoegge> so we limit who we advertise the NODE_BLOOM service bit to? this might be a bit off-topic 
10:34 < jnewbery> dergoegge: yes, we can advertise NODE_BLOOM only to peers that we've configured to support sending bloom filters to
10:35 < ben85> Very interesting these NetPermissionFlags. Are they attributed to an other peer manually?
10: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 https://github.com/bitcoin/bitcoin/issues/19398
10:36 < jnewbery> next question: Why is this PR able to remove several calls to GetLocalServices()?
10:37 < dergoegge> if (GetLocalServices() & NODE_WITNESS) checks are no longer needed since NODE_WITNESS is always set.
10:37 < ben85> Because this validation (pfrom->GetLocalServices() & NODE_WITNESS) is no longer necessary 
10: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.
10:40 -!- cviss [~cviss@c-75-75-7-8.hsd1.va.comcast.net] has quit [Quit: Connection closed]
10:41 < jnewbery> q5: What does the GetBlockScriptFlags() function do?
10:42 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has joined #bitcoin-core-pr-reviews
10:42 -!- Azorcode [~Azorcode@201.211.47.94] has joined #bitcoin-core-pr-reviews
10:43 < lightlike> it determines how to verify scripts; segwit/taproot/other softwork rules for scripts must only should be enforced after their activation height.
10:43 < ben85> According to the comment, it returns the script flags which should be checked for a given block
10:44 < jnewbery> lightlike ben85: yes, exactly right
10:44 < jnewbery> next question is a little harder
10:44 < jnewbery> q6: Why is it ok to always set SCRIPT_VERIFY_WITNESS when SCRIPT_VERIFY_P2SH is set?
10:44 < ben85> GetBlockScriptFlags() is called only in CChainState::ConnectBlock() and MemPoolAccept::ConsensusScriptChecks(). Interesting.
10:45 -!- zakinator [~zakinator@c-98-32-80-238.hsd1.ut.comcast.net] has quit [Quit: Connection closed]
10:45 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has quit [Client Quit]
10: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)
10:47 < jnewbery> any thoughts about the SCRIPT_VERIFY_WITNESS/SCRIPT_VERIFY_P2SH question?
10:47 < ben85> Because Segwit was activated after the BIP 16
10:47 < ben85> After the P2SH ?
10:48 < jnewbery> the logic was added in this PR, which includes a good summary: https://github.com/bitcoin/bitcoin/pull/11739
10:50 < dergoegge> why do we backdate these flags? just to save some logic?
10:50 < jnewbery> The idea was to simplify the logic around when to enforce the rules
10:50 < dergoegge> will we be able to do the same for taproot?
10:51 < jnewbery> The link to the chat is dead because botbotme no longer hosts chat logs. You can get it here instead: https://www.erisian.com.au/bitcoin-core-dev/log-2017-10-12.html#l-264
10:51 < dergoegge> https://gnusha.org/bitcoin-core-dev/2017-10-12.log
10:52 < jnewbery> dergoegge: good question. I'm not sure
10:52 < dergoegge> ah oops should have read your message to the end :D
10:52 < dergoegge> i guess we could ask that question for all the soft forks?
10: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
10: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?
10:53 < jnewbery> (or any witness version 1 outputs that are invalid under taproot rules and get spent between now and activation)
10:54 < jnewbery> I'd recommend looking at PR 11739 and reading the chat history around it. I think it's very interesting history.
10:54 < jnewbery> ok, final question. What does GenerateCoinbaseCommitment() do? Why is ok to remove the consensusParams.SegwitHeight check in that function?
10: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)
10:55 < sipa> it's not just that it was added later, it actively depends on it
10:55 < jnewbery> sipa: 👍
10: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
10:56 < sipa> for production this doesn't matter of course, as we know P2SH was long buried when WITNESS came into play
10:58 < jnewbery> GenerateCoinbaseCommitment calculates the block commitment defined in BIP 141 here: https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#commitment-structure
10:58 < lightlike> GenerateCoinbaseCommitment() is called in mining.cpp and adds the commitment to the merkle root of the witness data to the coinbase tx
10:58 < lightlike> since consensusParams.SegwitHeight can no longer be std::numeric_limits<int>::max , the check can be removed
10:59 < jnewbery> lightlike: exactly right!
10:59 -!- zakinator [~zakinator@c-98-32-80-238.hsd1.ut.comcast.net] has joined #bitcoin-core-pr-reviews
10:59 < jnewbery> any final questions before we wrap up?
11: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!
11:00 < jnewbery> thanks all
11:00 < jnewbery> #endmeeting
11:00 < dergoegge> thanks jnewbery!
11:00 < lightlike> thanks jnewbery!
11:00 < svav> Thanks
11:01 < gite> Thanks!
11:01 < sriramdvt> Thank you!
11:01 < ben85> thanks jnewbery !
11:01 < emzy> Thanks!
11:01 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
11:01 < Azorcode> thanks 
11:01 -!- gite [~gite@103.81.243.2] has quit [Quit: Connection closed]
11:01 -!- sriramdvt [~sriramdvt@183.83.132.111] has quit [Quit: Connection closed]
11:01 < glozow> thamks jnewbery!
11:02 < jnewbery> welcone glozow!
11:03 < glozow> new keyboard. completely handicapped 😂
11:09 -!- zakinator [~zakinator@c-98-32-80-238.hsd1.ut.comcast.net] has quit [Quit: Connection closed]
11:11 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 246 seconds]
11:12 -!- belcher_ is now known as belcher
11:16 -!- Azorcode [~Azorcode@201.211.47.94] has quit [Ping timeout: 265 seconds]
11:22 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has joined #bitcoin-core-pr-reviews
11:25 -!- observer58 [~observer@cpe-23-242-148-67.socal.res.rr.com] has quit [Ping timeout: 272 seconds]
11:25 -!- ben85 [~ben@189.122.121.102] has quit [Quit: Connection closed]
12:19 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:24 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
12:39 -!- dyke [~arne@2a02:810d:a8bf:d4ac:9186:a57:44d:8813] has quit [Quit: WeeChat 3.1]
13:25 -!- hedron [~hedron@gateway/tor-sasl/hedron] has joined #bitcoin-core-pr-reviews
14:39 -!- hostadig [~hostadig@45.92.228.38] has quit [Remote host closed the connection]
14:47 -!- jonatack [~jonatack@user/jonatack] has quit [Quit: Client closed]
14:56 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
15:00 -!- meshcollider [meshcollid@meshcollider.jujube.ircnow.org] has quit [Changing host]
15:00 -!- meshcollider [meshcollid@user/meshcollider] has joined #bitcoin-core-pr-reviews
15:08 -!- vasanth2[m] [~vasanth2m@2001:470:69fc:105::3548] has quit [Remote host closed the connection]
15:08 -!- robertspigler [~robertspi@2001:470:69fc:105::2d53] has quit [Write error: Connection reset by peer]
15:08 -!- prusnak[m] [~stickmatr@2001:470:69fc:105::98c] has quit [Remote host closed the connection]
15:09 -!- prusnak[m] [~stickmatr@2001:470:69fc:105::98c] has joined #bitcoin-core-pr-reviews
15:15 -!- robertspigler [~robertspi@2001:470:69fc:105::2d53] has joined #bitcoin-core-pr-reviews
15:15 -!- vasanth2[m] [~vasanth2m@2001:470:69fc:105::3548] has joined #bitcoin-core-pr-reviews
15:56 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Ping timeout: 265 seconds]
16:18 -!- lightlike [~lightlike@user/lightlike] has quit [Quit: Leaving]
16:47 -!- luke-jr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
17:21 -!- belcher_ [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
17:24 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 240 seconds]
17:45 -!- hedron [~hedron@gateway/tor-sasl/hedron] has left #bitcoin-core-pr-reviews [Leaving]
21:00 -!- achow101 [~achow101@user/achow101] has quit [Quit: Bye]
21:00 -!- achow101 [~achow101@user/achow101] has joined #bitcoin-core-pr-reviews
--- Log closed Thu Jul 01 00:00:54 2021