--- Day changed Wed Sep 11 2019
01:47 -!- jonatack [~jon@atoulouse-656-1-803-118.w86-221.abo.wanadoo.fr] has joined #bitcoin-core-pr-reviews     
02:24 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has quit [Remote host closed the connection]     
02:59 -!- jonatack [~jon@atoulouse-656-1-803-118.w86-221.abo.wanadoo.fr] has quit [Quit: jonatack]     
03:57 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews     
04:04 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ufvrpouhhpfhmzeb] has joined #bitcoin-core-pr-reviews     
06:16 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has joined #bitcoin-core-pr-reviews     
06:21 -!- emilengler_ is now known as emilengler     
07:15 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews     
08:37 -!- davterra [~none@84.39.112.22] has joined #bitcoin-core-pr-reviews     
09:26 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has quit [Remote host closed the connection]     
09:30 < jkczyz> hi all! We'll be starting the PR review session in 30 minutes
09:36 -!- lightlike [~lightlike@2001:16b8:576d:7600:4039:b3e7:8eb9:b331] has joined #bitcoin-core-pr-reviews     
09:50 -!- b10c [~b10c@2001:16b8:2e61:7e00:4c0a:3bd1:2970:83ce] has joined #bitcoin-core-pr-reviews     
09:53 -!- alon-e [~androirc@2.53.141.195] has joined #bitcoin-core-pr-reviews     
09:55 < sosthene> hi jkczyz
09:56  * jkczyz waves hi
09:56 < jnewbery> jkczyz is hosting today. Notes and questions in the normal place: https://bitcoin-core-review-club.github.io/16688.html
09:58 < sosthene> thanks, I'll read it now, this has been a busy day and when I tried to build the branch I encountered some error
09:58 -!- hanhua [6885085e@104.133.8.94] has joined #bitcoin-core-pr-reviews     
09:58 < sosthene> I rage quitted :)
09:58  * jkczyz laughs
09:59 < jkczyz> I'll have to make sure it's not something I did wrong
10:00 < jkczyz> hi     
10:00 < sosthene> nah it certainly comes from my side, happens all the time
10:00 < lightlike> hi     
10:00 < jnewbery> hi     
10:00 < b10c> hi     
10:00 < udiWertheimer> jnewbery: that’s dedication!
10:00 < emzy> Hi     
10:01 < jkczyz> As jnewbery noted, I'm guest hosting today. He may be around to help with the finer points :)
10:01 < jnewbery> udiWertheimer: haha. I'm going to duck out in a few minutes. Just wanted to make sure jkczyz has everything he needs to get started
10:01 < jkczyz> Today we’re reviewing PR #16688, which adds logging to validation layer’s event notification
10:01 < jkczyz> The validation layer processes blocks and transactions from the net processing layer and updates the validated chain state and mempool
10:02 < jkczyz> Any initial thoughts on this PR?
10:02 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews     
10:03 < jnewbery> I think it's great, and definitely going to be helpful for debugging issues as we move towards a more modular architecture
10:03 -!- afigs [2f29e955@47.41.233.85] has joined #bitcoin-core-pr-reviews     
10:04 < jkczyz> jnewbery: Glad I can help in even a small way! While this is a fairly simple change, there are some lessons we can learn about the code through it
10:05 < jkczyz> Alright, let's jump into the questions
10:05 < jkczyz> What’s the reason for adding logging to event notification?
10:06 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has joined #bitcoin-core-pr-reviews     
10:06 < sosthene> I guess it helps debugging?
10:07 < jkczyz> sosthene: correct, what sort of issues can it help debug?
10:07 < jnewbery> specifically, since the CValidationInterface is asynchronous, it can help with debugging threading issues and race conditions
10:07 < lightlike> it helps with error analysis, especially in case of race conditions, which appear in functional tests sometimes.
10:07 < emzy> There was a race condition where it would had helpd to debug.
10:07 < sosthene> In fact I wonder why there wasn't any log to begin with, as it seems a pretty critical part of Bitcoin to me.
10:08 < jkczyz> jnewbery, emzy: correct! Specifically it could have helped with issue #12978
10:08 < jkczyz> https://github.com/bitcoin/bitcoin/issues/12978
10:08 < lightlike> ironically, there is a is a race condition in the TSAN Travis build of this PR (which seems unrelated though)
10:08 < jkczyz> Here there was a race condition resulting in UpdatedBlockTip() signals delivered out of order
10:08 < jnewbery> sosthene: I agree this is critical, but for historic context, the CValidationInterface was updated about a year ago by Matt. Before then, I think it wasn't asynchronous (I may be wrong)
10:09 < jnewbery> bitcoind is (mostly) single threaded on the Message Handler thread. As we try to make it more multi-threaded, better logging at the interfaces is going to be more important
10:10 < jkczyz> lightlike: Yes, appears to be so. I thought about force pushing to see if there was some flakiness to the test
10:10 < jnewbery> I've got to head out now. Bye!
10:10 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has quit [Ping timeout: 240 seconds]     
10:11 < jkczyz> jnewbery: Bye! Thanks for stopping in
10:11 < jkczyz> ok, next question: What mechanism is used to notify listeners of validation events?
10:11 < lightlike> jkczyz: i looked at it a little. seems related to https://github.com/bitcoin/bitcoin/issues/16133
10:12 < jkczyz> lightlike: cool, thanks for investigating! I'll have to look at that issue
10:12 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has joined #bitcoin-core-pr-reviews     
10:13 -!- nehan [~nehan@41.213.196.104.bc.googleusercontent.com] has joined #bitcoin-core-pr-reviews     
10:13 < jkczyz> Hint: it uses a third-party library
10:14 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ufvrpouhhpfhmzeb] has quit [Quit: Connection closed for inactivity]     
10:15 < lightlike> don't know about it in depth, but via callback functions
10:16 < sosthene> I'm not even sure to quite get the question
10:16 < jkczyz> lightlike: yes, it uses callbacks, specifically with the boost:signals2 library
10:16 < jkczyz> https://www.boost.org/doc/libs/1_71_0/doc/html/signals2.html
10:16 -!- av7 [dfe15342@223.225.83.66] has joined #bitcoin-core-pr-reviews     
10:17 -!- av7 [dfe15342@223.225.83.66] has quit [Remote host closed the connection]     
10:17 < jkczyz> sosthene: The code in question is a bit tricky to reason through. I had to draw out a diagram to be honest!
10:17 < jkczyz> https://github.com/bitcoin/bitcoin/blob/d0f81a96d9c158a9226dc946bdd61d48c4d42959/src/validationinterface.cpp
10:17 -!- av7 [dfe15342@223.225.83.66] has joined #bitcoin-core-pr-reviews     
10:17 -!- Bitcoin1776 [604578a1@96.69.120.161] has joined #bitcoin-core-pr-reviews     
10:17 < jkczyz> Ok, let's move on to some more interesting questions
10:18 < jkczyz> What code implements CValidationInterface? What does it use event notifications for?
10:18 -!- av7 [dfe15342@223.225.83.66] has quit [Remote host closed the connection]     
10:18 < jkczyz> There are many correct answers to this one
10:19 -!- peterthiel [dfe15342@223.225.83.66] has joined #bitcoin-core-pr-reviews     
10:19 < lightlike> the wallet does, for tracking the status of wallet transactions
10:21 < jkczyz> lightlike: yes, though this interface is adapted by NotificationsHandlerImpl for use by the wallet
10:21 < jkczyz> https://github.com/bitcoin/bitcoin/blob/9ab9d6356938ffe069ffc005a371d5405976823e/src/interfaces/chain.cpp#L158
10:22 < sosthene> what about the networking part? I guess you need to know if a peer is feeding you invalid blocks or tx?
10:24 < jkczyz> sosthene: correct, PeerLogicValidation (net processing layer) makes use of notificaitons to send peers new data and to validate peer messages (I believe :)
10:25 < jkczyz> On the wallet-side, using event notification helps with separation betweeen node and wallet functionality
10:25 < jkczyz> anything else?
10:25 < sosthene> I see it implemented in the BaseIndex class too, but I'm not too sure about what it does
10:25 < sosthene> https://github.com/bitcoin/bitcoin/blob/e4beef611a423e9f836fa210a51634e94f14d830/src/index/base.h
10:26 -!- afigs [2f29e955@47.41.233.85] has quit [Remote host closed the connection]     
10:26 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 246 seconds]     
10:26 < jkczyz> sosthene: Good find! It uses BlockConnected and ChainStateFlushed to write to on-disk indexes
10:27 < sosthene> I see
10:28 < jkczyz> The remaining uses are not as important as these, but they are still interesting to look at
10:28 < sosthene> I see it also mentioned in the transaction.h file
10:28 -!- shesek [~shesek@141.226.218.61] has joined #bitcoin-core-pr-reviews     
10:28 -!- shesek [~shesek@141.226.218.61] has quit [Changing host]     
10:28 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
10:28 < sosthene> https://github.com/bitcoin/bitcoin/blob/be0e8b4bff88b421128239e7140fc6bfdb654806/src/node/transaction.h
10:28 < jkczyz> submitblock RPC, ZMQ notificaitons, withing tests.
10:29 < lightlike> sosthene: my understanding is that the validation routines are called from the networking part (net_processing.cpp), and if we are sent something invalid, validationinterface will never be involved.
10:30 < sosthene> lightlike: I don't quite understand sorry, how do we know we have been sent something invalid without involving validationinterface?
10:32 -!- wahwhowah [d130077e@209.48.7.126] has joined #bitcoin-core-pr-reviews     
10:32 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 240 seconds]     
10:33 < jkczyz> I may have been mistaken earlier about validating peer messages. Net processing will call the validation layer. Then only if the validation succeeds do subscrivers get notified
10:33 < lightlike> sosthene: we have a network thread that receives the messages. net_processing deals with the logic around these, and if we get a new block or tx, calls procedures in validation. Only if these procedures pass, validation will create signals for validationinterface that are passed on to the wallet etc.
10:34 < jkczyz> lightlike: yes, put more eloquently :)
10:34 < sosthene> lightlike: ok, it makes more sense, thanks!
10:34 < jkczyz> Alright, let's move on
10:34 < jkczyz> What is a CBlockLocator? How is it used in this PR? How else is it used?
10:34 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
10:37 < lightlike> jkczyz: i have encountered it in networking/getblocks messages, where it is used to determine the point up to which the chains of two peers are identical (for example in IBD when one peer has only part of the chain).
10:38 < sosthene> I found this, it makes sense with what lightlike says https://github.com/bitcoin/bitcoin/blob/78dae8caccd82cfbfd76557f1fb7d7557c7b5edb/src/primitives/block.h
10:38 -!- peterthiel [dfe15342@223.225.83.66] has quit [Remote host closed the connection]     
10:39 < jkczyz> lightlike: yes, also in getheaders
10:39 < lightlike> https://github.com/bitcoin/bitcoin/blob/78dae8caccd82cfbfd76557f1fb7d7557c7b5edb/src/chain.cpp#L23 shows the logic behind it, how the internal vector of block hashes is filled
10:39 < jkczyz> See details: https://en.bitcoin.it/wiki/Protocol_documentation#getblocks
10:39 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 268 seconds]     
10:40 < jkczyz> lightlike: great! I was just about to ask how it was constructed
10:40 < jkczyz> Any insight into why it is constructed in that manner?
10:41 -!- shesek [~shesek@141.226.218.61] has joined #bitcoin-core-pr-reviews     
10:41 -!- shesek [~shesek@141.226.218.61] has quit [Changing host]     
10:41 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
10:42 < lightlike> i think it's quite elegant, with the exponential thinning out: most of the time, two peers are just a few blocks away, so all the recent blocks are there. in rarer cases, where the chains differ strongly, multiple locators need to be exchanged to get the highest common block.
10:43 < jkczyz> lightlike: good observation! This is a nice way to limit the amount of data exchanged between peers.
10:43 < lightlike> jkczyz: do you know if satoshi came up with it?
10:44 < jkczyz> The locators are also used when writing to the on-disk block index
10:45 < jkczyz> lightlike: I'm afraid that predates my involvement. :) Checking out Github's blame layer might help determine the origin, though you may have to do some code archeology as the code has probably been moved from other files
10:46 < jkczyz> Back to one of the original questions: How is CBlockLocator used in this PR?
10:47 < lightlike> just to get the first element (which is the tip)
10:48 < sosthene> I see it in ChainStateFlushed, but I don't understand what it does
10:48 < jkczyz> lightlike: yup, this is logged as part of ChainStateFlushed.
10:49 -!- wahwhowah [d130077e@209.48.7.126] has quit [Remote host closed the connection]     
10:49 -!- shesek [~shesek@unaffiliated/shesek] has quit [Ping timeout: 276 seconds]     
10:50 < sosthene> jkczyz: I got to go, I'll read the transcript to get the end of the story, thank you and see you!
10:50 < jkczyz> sosthene: My understanding is that this is written to the on-disk index. I may be mistaken so others may correct me, but I believe this will give a compact path of block back to the genessis block
10:50 < lightlike> looks like everyone is at Scaling Bitcoin today :-)
10:51 < jkczyz> sosthene: thanks for joining!
10:51 < jkczyz> lightlike: indeed :)
10:51 < jkczyz> Hopefully there are some lurkers following along, too
10:52 < jkczyz> ok, let's finish up the last couple questions
10:52 < jkczyz> What refactorings are included in this PR?
10:53 < jkczyz> Hint: see the commits from the PR
10:53 -!- shesek [~shesek@141.226.218.61] has joined #bitcoin-core-pr-reviews     
10:53 -!- shesek [~shesek@141.226.218.61] has quit [Changing host]     
10:53 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
10:54 < lightlike> MempoolEntryRemoved is renamed to TransactionRemovedFromMempool, some String functions are changed to return a const ref
10:54 < jkczyz> yep, fairly trivial changes but good for code hygiene
10:55 < jkczyz> Lastly, as we are running out of time: How can this PR be tested? Could any other tests have been added?
10:56 < lightlike> not sure about that one. I think there is some mechanism to check for log messages in functional tests.
10:57 < jkczyz> lightlike: yeah, I remember seeing that in the PR that logged bitcoind's args
10:57 -!- hanhua [6885085e@104.133.8.94] has quit [Remote host closed the connection]     
10:58 < jkczyz> Here, I manually verified the logging through the p2p_compactblocks.py functional test
10:59 < lightlike> though i am not sure if testing log messages is a bit overkill.
10:59 < jkczyz> This was the flaky test from issue
10:59 < jkczyz> issue #12978
10:59 < jkczyz> https://github.com/bitcoin/bitcoin/issues/12978
10:59 < jkczyz> lightlike: agreed
11:00 < jkczyz> Other testing may be unit tests for newly added code
11:00 < jkczyz> And that's the bell!
11:00 < jkczyz> Thanks lightlike and sosthene for participating today!
11:00 < lightlike> by the way you don't have to force push for a new travis run. you could ask in #bitcoin-core-dev, several people can restart it.
11:00 < lightlike> thanks jkczyz!
11:01 < jkczyz> As jnewbery would want, please added any interesting PR's to https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/issues/14
11:01 < jkczyz> So they can be covered in future weeks
11:01 < jkczyz> lightlike: Thanks! This was fun :)
11:02 < lightlike> jkczyz: gonna review/ack on github in the next days.
11:05 -!- Bitcoin1776 [604578a1@96.69.120.161] has quit [Remote host closed the connection]     
11:09 -!- shesek [~shesek@unaffiliated/shesek] has quit [Read error: Connection timed out]     
11:09 -!- shesek [~shesek@141.226.218.61] has joined #bitcoin-core-pr-reviews     
11:09 -!- shesek [~shesek@141.226.218.61] has quit [Changing host]     
11:09 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
11:41 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving]     
11:46 -!- b10c [~b10c@2001:16b8:2e61:7e00:4c0a:3bd1:2970:83ce] has quit [Quit: Leaving]     
12:00 -!- shesek [~shesek@unaffiliated/shesek] has quit [Read error: Connection timed out]     
12:00 -!- shesek [~shesek@141.226.218.61] has joined #bitcoin-core-pr-reviews     
12:00 -!- shesek [~shesek@141.226.218.61] has quit [Changing host]     
12:00 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews     
12:12 < jonatack> jkczyz: Great job! I'm sorry to have missed it. Good prep by lightlike and sosthene and discussion. I'll try to review the PR.
13:10 -!- fox2p [~fox2p@185.183.104.83] has joined #bitcoin-core-pr-reviews     
13:11 -!- fox2p_ [~fox2p@185.212.170.51] has quit [Ping timeout: 244 seconds]     
13:19 -!- alon-e [~androirc@2.53.141.195] has quit [Quit: AndroIRC - Android IRC Client ( http://www.androirc.com )]     
13:27 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ickxtndpjukebfle] has joined #bitcoin-core-pr-reviews     
13:39 -!- davterra [~none@84.39.112.22] has quit [Ping timeout: 240 seconds]     
13:40 -!- davterra [~none@84.39.112.22] has joined #bitcoin-core-pr-reviews     
14:45 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has quit [Remote host closed the connection]     
15:55 -!- meshcollider [meshcollid@gateway/shell/elitebnc/x-eljktkufsrujivha] has quit [Ping timeout: 268 seconds]     
16:00 -!- meshcollider [meshcollid@gateway/shell/elitebnc/x-yyqatxpvvetoplhv] has joined #bitcoin-core-pr-reviews     
16:18 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-ickxtndpjukebfle] has quit [Quit: Connection closed for inactivity]     
16:35 -!- lightlike [~lightlike@2001:16b8:576d:7600:4039:b3e7:8eb9:b331] has quit [Quit: Leaving]     
19:05 -!- emilengler_ [~emilengle@unaffiliated/emilengler] has joined #bitcoin-core-pr-reviews     
19:07 -!- emilengler [~emilengle@unaffiliated/emilengler] has quit [Ping timeout: 246 seconds]     
20:40 -!- hebasto [~hebasto@95.164.65.194] has joined #bitcoin-core-pr-reviews     
21:11 -!- hebasto [~hebasto@95.164.65.194] has quit [Remote host closed the connection]     
21:15 -!- pinheadmz [~matthewzi@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Quit: pinheadmz]     
21:53 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has joined #bitcoin-core-pr-reviews     
22:24 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has quit [Remote host closed the connection]     
23:33 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has joined #bitcoin-core-pr-reviews     
23:37 -!- ccdle12 [~ccdle12@89-139-132-205.bb.netvision.net.il] has quit [Ping timeout: 246 seconds]