--- Log opened Wed Jun 02 00:00:26 2021
00:44 -!- Kiminuo [~Kiminuo@141.98.103.180] has joined #bitcoin-core-pr-reviews
03:18 -!- belcher_ [~belcher@user/belcher] has quit [Ping timeout: 272 seconds]
03:28 -!- belcher [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
04:02 -!- belcher [~belcher@user/belcher] has quit [Quit: Leaving]
04:43 -!- belcher [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
04:57 -!- belcher [~belcher@user/belcher] has quit [Quit: Leaving]
04:57 -!- belcher [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
05:34 -!- Kiminuo [~Kiminuo@141.98.103.180] has quit [Ping timeout: 272 seconds]
07:00 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has joined #bitcoin-core-pr-reviews
08:10 -!- prusnak [sid403625@tooting.irccloud.com] has quit [Changing host]
08:10 -!- prusnak [sid403625@user/prusnak] has joined #bitcoin-core-pr-reviews
08:24 -!- Guest64 [~Guest64@184.75.221.35] has joined #bitcoin-core-pr-reviews
08:25 -!- Guest64 [~Guest64@184.75.221.35] has quit [Quit: Client closed]
08:27 -!- jonatack [jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
08:39 -!- marqusat [~marqusat@81.92.200.51] has joined #bitcoin-core-pr-reviews
08:55  * michaelfolkson needs reminding of how orphans are defined to make it into the orphanage
08:55 < michaelfolkson> Turns out orphan txs aren't as confusing as orphan blocks
08:56 < michaelfolkson> https://bitcoin.stackexchange.com/questions/5859/what-are-orphaned-and-stale-blocks
08:58 -!- jonatack [jonatack@user/jonatack] has left #bitcoin-core-pr-reviews []
09:07 < michaelfolkson> I guess you could have a temporary orphan (parent is not known but on its way and is otherwise unspent) versus a permanent orphan (parent has been spent by alternative transaction). The latter would be a double spend attempt though (and hence rare)
09:10 < michaelfolkson> And the latter wouldn't be stored in memory
09:13 < sipa> nodes cannot distinguish between the two
09:14 < sipa> both look like a transaction spending unknown utxos
09:15 < sipa> distinguishing between a spent output and an output that never existed requires more than just a UTXO set
09:17 < michaelfolkson> If the node was unpruned it would still have access to the parent and could check that the parent had been spent by an alternative (earlier) child?
09:17 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:18 < michaelfolkson> If the node was pruned it wouldn't have access to that parent?
09:22 < sipa> validation doesn't use anything but the UTXO set
09:23 < sipa> and the UTXO set cannot distinguish between these situations
09:24 < michaelfolkson> So this tx orphanage is quite the easy DoS vector? Easy to fill it up to the limit anyway
09:25 < michaelfolkson> Just send a bunch of permanent orphan txs to a node and fill up its tx orphanage
09:28 < michaelfolkson> And no way to identify you are being DoSed? Other than a strangely high number of orphan txs
09:39 < michaelfolkson> Nice previous Core PR review club on orphan transaction processing https://bitcoincore.reviews/15644
09:40 < michaelfolkson> They used to get booted after 20 minutes
09:41 < michaelfolkson> Sorry sipa I can't remember all the previous Core PR review clubs :)
09:45 < jnewbery> Hi folks. We'll get started in around 15 minutes. Notes and questions are at https://bitcoincore.reviews/21527
09:46 < michaelfolkson> (And they still are booted after 20 minutes)
10:00 < jnewbery> #startmeeting
10:00 < glozow> hi
10:00 < jnewbery> Hi folks! Welcome to review club. Feel free to say hi to let everyon know you're here.
10:00 < jnewbery> Also feel free to not say hi and just hang out :)
10:00 < michaelfolkson> hi
10:00 < glozow> crap. can i still hang out?
10:00 < hernanmarino> hi !
10:00 < marqusat> hi
10:00 < emzy> hi
10:01 < jnewbery> you can {say|not say} hi and {hang out|not hang out}
10:01 -!- lightlike [~lightlike@user/lightlike] has joined #bitcoin-core-pr-reviews
10:01 < jnewbery> Is anyone here for the first time?
10:01 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
10:01 < lightlike> hi
10:01 < michaelfolkson> Is anyone in Miami?
10:01 < jnewbery> Today we're going to be looking at PR 21527: "Net_processing: lock clean up". Notes and questions are in the normal place: https://bitcoincore.reviews/21527
10:02 < svav> Hi
10:02 < jnewbery> I'll be asking questions from https://bitcoincore.reviews/21527 to guide the conversation, but feel free to jump in at any point and ask questions if anything's unclear
10:03 < jnewbery> ok, who had a chance to review the PR this week (y/n)?
10:03 < hernanmarino> I didn't have much time to read de PR notes today, but i would like to hang out here and read the discussion anyway :)
10:03 < emzy> n
10:03 < michaelfolkson> y
10:03 < svav> y
10:03 < jnewbery> hernanmarino: you're very welcome to!
10:03 < lightlike> n
10:04 < marqusat> .5y
10:04 < jnewbery> ok, let's get started. For those who reviewed the PR, can you give a short summary of the changes. How did you go about reviewing the PR?
10:06 < svav> continues the work of better encapsulating net_processing
10:06 < jnewbery> svav: yes indeed!
10:07 < svav> It is like a code cleanup operation
10:07 < svav> Which will help with clarity and performance
10:07 < svav> It is moving a few thinks relating to mutex locking
10:07 -!- ben3223 [~ben3223@189.122.121.102] has joined #bitcoin-core-pr-reviews
10:07 < svav> *things*
10:08 < jnewbery> svav: I agree with the clarity part. This PR shouldn't change performance, or maybe you're thinking of work that could be built on top of this PR?
10:08 < glozow> what could be built on top? 👀
10:08 < svav> It was a slight newbie guess because no-one else was answering
10:08 < michaelfolkson> Reducing globals, attempting to improve modularity or at least loosening ties
10:09 < jnewbery> svav: It was a good guess!
10:09 < michaelfolkson> There is kinda the clear separation of net and net_processing and also cleaning up within net_processing
10:09 < svav> but i see things like #include <txorphanage.h> being moved out of init.cpp where they are not needed
10:10 < jnewbery> Currently, the critical paths in Bitcoin Core are mostly single-threaded, and most of the important state is guarded by a single mutex called cs_main
10:10 < glozow> oo, so compilation performance
10:10 < jnewbery> if we can break that mutex up, it opens up the possibility of having a bit more concurrency, for example, having a validation thread independent from net_processing
10:11 < hernanmarino> that would be great
10:11 < jnewbery> michaelfolkson: yes! Better modularity and decoupling is a goal
10:11 < glozow> o so right now, if we receive a TX and validate it, the message processing thread sits there waiting for ATMP to finish?
10:12 < svav> For the newbies, can you define the difference between net and net_processing?
10:12 < michaelfolkson> And so the (interesting) discussion with vasild was a disagreement on stepping stones towards that goal? There are trade-offs depending on which stepping stones you pick?
10:12 < jnewbery> glozow: correct! If there was a separate thread for validation, the net_processing thread could start serving other peers while validation is validating the tx
10:12 < michaelfolkson> svav: https://bitcoin.stackexchange.com/questions/106751/what-does-net-processing-cover-conceptually-in-the-bitcoin-core-codebase
10:13 < ben3223> net  is lower level that net_processing, which handles the protocol messages
10:13 -!- b10c [uid500648@id-500648.charlton.irccloud.com] has joined #bitcoin-core-pr-reviews
10:13 < glozow> jnewbery has an issue for net vs net_processing: https://github.com/bitcoin/bitcoin/issues/19398
10:13 < ben3223> net -> network level / net_processing -> message protocol handling 
10:13 < michaelfolkson> svav: Net handles P2P messages. Net processing works whether it needs to go to validation or not
10:13 < michaelfolkson> *works out
10:14 < jnewbery> svav: great question. The "net" layer is responsible for opening and maintaining connections to peers, and reading/writing messages to the wire. The main class in that layer is CConnman. The "net processing" layer sits above that, deserializes the p2p messages and then maybe passes them up to validation to be validated
10:14 < jnewbery> ben3223: michaelfolkson: correct!
10:15 < jnewbery> michaelfolkson: yes, the discussion with vasild was about approach rather than concept I think
10:15 < jnewbery> ok, next question: Does this PR change observable behaviour of the node in any way?
10:16 < svav> I'm guessing it shouldn't
10:16 < michaelfolkson> I'm also guessing that
10:16 < hernanmarino> i didn't read the code, but my guess is it doesn't 
10:16 < marqusat> Does not seem so
10:17 < ben3223> I think it doesn't 
10:17 < emzy> I hope not.
10:17 < michaelfolkson> I did look at the tx orphanage and that appears to have the same orphan processing logic as before (from a cursory glance)
10:17 < svav> It is just moving mutexes to better places, so should not affect behaviour
10:18 < jnewbery> that's right. This PR should be a refactor only. If you think it changes observable behaviour, you should speak up!
10:18 < jnewbery> The first version of the PR had some behavioural changes. We can talk about those changes at the end if we have time
10:18 < jnewbery> ok, next question. What was the cs_sendProcessing mutex responsible for before this PR? When was cs_sendProcessing introduced? (Hint use git blame and git log -S "string" to search back through previous commits).
10:19 < jnewbery> This is a good test of your git skills
10:20 < michaelfolkson> Ha there were a few movements before you got to the creator
10:20 < michaelfolkson> glozow's many layers of git blame
10:20 < jnewbery> Hint: run `git log -S cs_sendProcessing` in your git repo and scroll down to the bottom to see where the mutex was added
10:21 < michaelfolkson> I got a Matt PR in Jan 2017 I think
10:21  * michaelfolkson checks notes
10:21 < glozow> don't think i've touched `cs_sendProcessing` 🤔
10:21 < jnewbery> anyone get anything different?
10:23 < jnewbery> ok, if I run it, I get commit d7c58ad514ee00db00589216166808258bc16b60 from Dec 2016
10:23 < jnewbery> "Split CNode::cs_vSend: message processing and message sending"
10:23 < sipa> i don't think i've ever used `git log -S`... i should
10:24 < michaelfolkson> What's the PR number?
10:24 < hernanmarino>  jnewbery: i got that too
10:24 < michaelfolkson> I had #9535
10:24 < glozow> #9535 is what i got as well
10:24 < jnewbery> being able to move back and forward through the git history is really helpful for understanding the historic context for changes.
10:25 < jnewbery> Yes, PR #9535 is where it was introduced
10:26 < jnewbery> I have a few tools that help me navigate the history of the repo. `git log -S` is one of them. The fugitive plugin for vim is another great tool
10:26 < jnewbery> it lets you walk back through the commits and see how things change over time
10:26 < ben3223> Did it replace cs_vSend  ?
10:26 < michaelfolkson> Hmm nice. Always interested in a Vim plugin recommendation
10:27 < glozow> ben3223: just split it
10:27 < jnewbery> and then using a custom search engine in my browser https://github.com/bitcoin/bitcoin/search?q=%s to lookup which PR a commit was merged in
10:28 < jnewbery> ben3223: exactly - the PR split cs_vSend into two mutexes (mutices?)
10:28 < jnewbery> ok, next question
10:28 < jnewbery> This PR moves one mutex from net to net_processing (CNode.cs_sendProcessing is replaced by PeerManager.m_mutex_message_handler) and one mutex from net_processing to txorphanage (g_cs_orphans is replaced by TxOrphanage.m_mutex). What are the benefits of moving these global/externally visible mutexes to being defined internally in their respective classes?
10:29 < michaelfolkson> Makes it easier to test, safer for future changes?
10:30 < marqusat> Information hiding, harder to make a mistake and acquire a mutex when it shouldn’t be acquired and cause a deadlock.
10:30 < michaelfolkson> "Easier to reason about" is always the right answer :)
10:31 < jnewbery> michaelfolkson marqusat: Yes!
10:31 < svav> You will enable more concurrency, although what exactly I'm not sure!!
10:31 < jnewbery> by making the locking internal, you hide those requirements from the caller
10:31 < glozow> it'd be nice for modules to make themselves thread-safe instead of requiring others to grab locks all the time
10:32 < jnewbery> glozow: +1
10:32 < jnewbery> Next question. What are vExtraTxnForCompact and vExtraTxnForCompactIt? Why is it ok to stop guarding them with g_cs_orphans and guard them with m_mutex_message_handling instead?
10:32 < svav> The code will be easier to maintain, because you will have thread opening and closing in the same place, so less chance of messing up locking.
10:35 < jnewbery> svav: I think almost all of our locking uses RAII, so it's quite hard to mess up locking. The lock lasts as long as the std::unique_lock stays in scope (and that's wrapped by the LOCK() macro)
10:35 < jnewbery> We also have lock annotations now, which give us a bit more confidence, since the compiler enforces that the right locks are held when entering functions/accessing data
10:35 < michaelfolkson> Not quite sure on vExtraTxnForCompact. An extra transaction for compact block reconstruction whatever that means
10:36 < glozow> `vExtraTxnForCompact` = transactions you conflicted out, but might see in a block
10:36 < jnewbery> but yes, I agree that if the mutex is not exposed at all, there's no way for a caller to get locking wrong (since it's not responsible for locking)
10:36 < marqusat> Orphan/conflicted/etc transactions that are kept for compact block reconstruction, private in PeerManagerImpl which uses its internal mutex
10:36 < glozow> if you have compact blocks enabled, your peers won't send you those txns in the block?
10:36 < glozow> so you'll want to have them somewhere in case you need them?
10:37 < sipa> glozow: that's not how compact blocks work; the sender sends you a compact block with compact tx hashes; you then request the transactions which you miss
10:37 < jnewbery> glozow: very good! Yes, they're extra transactions that can be used for compact block reconstruction.
10:37 < sipa> glozow: by keeping conflicted/orphan transactions around for a bit, they are available for reconstruction, so you won't have to ask for them if they show up in a compact block
10:38 < jnewbery> sipa: is it different between high bandwidth/low bandwidth mode?
10:38 < sipa> jnewbery: no, high bandwidth just drops a roundtrip (you don't need to ask for the compact block after announcement; they announce through a compact block directly)
10:39 < jnewbery> sipa: thanks!
10:39 < jnewbery> https://github.com/bitcoin/bips/blob/master/bip-0152.mediawiki#Intended_Protocol_Flow for reference
10:39 < glozow> sipa: ah okay, got it
10:39 < glozow> for some reason i thought they might go by inventory
10:40 < jnewbery> ok, so part two of that question: Why is it ok to stop guarding them with g_cs_orphans and guard them with m_mutex_message_handling instead?
10:40 < sipa> (i have not reviewed the PR, i am guessing) there is no strict requirement that they are in sync with the rest of the orphan data structure
10:40 < jnewbery> sipa: yes!
10:41 < svav> Because they now have their own mutex lock and unlock in TXOrphanage which can be dealt with by m_mutex_message_handling
10:41 < jnewbery> svav: what are you referring to as they?
10:42 < sipa> right, locking achieves two things: multithreaded r/w access (you're instantly UB if you don't protect variabled read/written by multiple threads with some form of locking/synchronization)
10:42 < sipa> but it also achieves consistency; by putting multiple variables under the same lock, they will always be in sync from the perspective of other threads
10:42 < svav> jnewbury: vExtraTxnForCompact and vExtraTxnForCompactIt
10:42 < sipa> if consistency across certain variables isn't required, they can by protected by distinct locks
10:43 < svav> jnewbery: vExtraTxnForCompact and vExtraTxnForCompactIt
10:44 < jnewbery> sipa: exactly right. The class has certain invariants that must always be true for callers. We can lock a mutex, and then temporarily violate those invariants during processing, before restoring the invariants and releasing the lock.
10:44 < jnewbery> if there are no logical invariants between separate data, they don't need to be guarded by the same lock
10:44 < michaelfolkson> So there is always this trade-off re locks between few locks (guarantees re being in sync) versus many locks (freedom of concerns)
10:45 < sipa> right, and the consistency requirements for vExtraTxnForCompact etc are basically "whatever"
10:45 < jnewbery> sipa: yes
10:46 < jnewbery> svav: ah, I was a little confused because you said they have mutex lock and unlock in TXOrphange. vExtraTxnForCompact and vExtraTxnForCompactIt are not in TXOrphange
10:47 < jnewbery> you're right that they're not protected under a different lock (m_mutex_message_handling), which prevents multiple threads accessing them concurrently
10:47 < jnewbery> ok, next question. This PR removes the Peer.m_orphan_work_set member and replaces it with a m_peer_work_set map in TxOrphanage. What is the peer orphan work set used for?
10:49 < michaelfolkson> Didn't get this far, don't know
10:50 < glozow> peer -> orphans to be considered, for which the parents were provided by this peer?
10:50 < glozow> or is it that they provided the orphan?
10:50 < jnewbery> glozow: right first time. The parents were provided by this peer
10:50 < glozow> so you could have multiple entries if the parents were provided by multiple peers?
10:51 < glozow> er, multiple parents by different peers
10:51 < jnewbery> so if we receive a transaction where we don't have its inputs in our UTXO set or mempool from peer A, and then peer B provides the missing parent and it gets accepted to our mempool, whose orphan work set will the orphan go into?
10:51 < glozow> B?
10:52 < jnewbery> glozow: right. We'll validate the orphan transaction next time we go through the ProcessMessages loop for peer B.
10:53 < jnewbery> and if the orphan transaction was consensus-invalid, which peer would we punish?
10:53 < glozow> and punish B if it's a bad orphan?
10:53 < glozow> ha oops. B?
10:54 < jnewbery> glozow: that seems unfair. Peer B might not even know about the orphan
10:54 < glozow> yeah peer B's just parenting hey
10:54 < michaelfolkson> Punishment seems challenging/impossible with orphans
10:55 < jnewbery> no, we won't punish the peer that provided the parent. That'd be a good way to allow a malicious actor to get peers to disconnect other peers.
10:56 < jnewbery> we remember who provided the orphan (https://github.com/bitcoin/bitcoin/blob/1186910b6b7ba7c7e5193c76f33f25825e6cc0b7/src/txorphanage.h#L53), so if it's bad we'll punish the peer that sent us the orphan
10:56 < jnewbery> ok, final question. (Bonus question) This PR originally included some behavioural changes in the way that orphans are handled. Those have now been moved from this PR to a separate branch and may be proposed as a follow-up PR. What are those behaviour changes?
10:58 < jnewbery> alright, maybe one for people to go and investigate for themselves.
10:58 < michaelfolkson> A lot of commits on that branch
10:58 < jnewbery> the branch is here: https://github.com/ajtowns/bitcoin/commits/202104-whohandlesorphans
10:58 < michaelfolkson> Something about who handles orphans :)
10:58 < jnewbery> any final questions before we wrap up?
10:58 < svav> Dont process tx after processing orphans?
10:59 < hernanmarino> :)
10:59 < jnewbery> ok, let's call it. Thanks everyone
11:00 < jnewbery> quiet week - I guess everyone else is hanging out on Saylor's yacht
11:00 < jnewbery> #endmeeting
11:00 < hernanmarino> Thanks ! 
11:00 < marqusat> thanks
11:00 < emzy> Thank you jnewbery!
11:00 < sipa> jnewbery: hey his name is saylor
11:00 < michaelfolkson> Transaction orphanage or Saylor's yacht. Hmm tough choice
11:00  * emzy is stuck in the EU.
11:01 < svav> Thanks jnewbery and everyone
11:01  * michaelfolkson is in the UK, formerly of the EU
11:01 < michaelfolkson> Thanks jnewbery
11:01 -!- marqusat [~marqusat@81.92.200.51] has quit [Quit: Client closed]
11:02 < glozow> thanks jnewbery thanks sipa!
11:02 < sipa> don't thank me, i hardly know what PR we're talking about :)
11:03 < svav> I see Tony Hawk the skateboarder will be at the Miami Conference, apparently he is an early adopter!
11:03 < michaelfolkson> glozow thanks to sipa have been RBFed back to glozow
11:08 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
11:12 < jnewbery> logs are posted: https://bitcoincore.reviews/21527
11:37 -!- ben3223 [~ben3223@189.122.121.102] has quit [Ping timeout: 264 seconds]
11:41 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has quit [Quit: ZNC - https://znc.in]
12:02 -!- powerjungle [~powerjung@h081217087223.dyn.cm.kabsi.at] has joined #bitcoin-core-pr-reviews
12:29 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
13:23 -!- b10c [uid500648@id-500648.charlton.irccloud.com] has quit [Quit: Connection closed for inactivity]
17:11 -!- lightlike [~lightlike@user/lightlike] has quit [Quit: Leaving]
21:31 -!- hernanmarino [~hernanmar@OL121-235.fibertel.com.ar] has quit [Ping timeout: 272 seconds]
21:32 -!- belcher_ [~belcher@user/belcher] has joined #bitcoin-core-pr-reviews
21:36 -!- belcher [~belcher@user/belcher] has quit [Ping timeout: 245 seconds]
21:43 -!- dongcarl [~dongcarl@96.224.58.144] has quit [Ping timeout: 244 seconds]
22:01 -!- dongcarl [~dongcarl@96.224.58.144] has joined #bitcoin-core-pr-reviews
23:20 -!- SenX [~textual@2600:1700:42f0:f60:c0f4:898b:4a73:14b1] has joined #bitcoin-core-pr-reviews
--- Log closed Thu Jun 03 00:00:28 2021