--- Day changed Wed Nov 04 2020 00:16 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 00:17 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 00:27 -!- glozow [uid453516@gateway/web/irccloud.com/x-fnmzdyejosltdszp] has quit [Quit: Connection closed for inactivity] 01:55 -!- b10c [~b10c@2a01:4f8:192:612a:216:3eff:fef3:dc6a] has quit [Ping timeout: 272 seconds] 01:55 -!- murch [murch@2a01:4f8:141:1272::2] has quit [Ping timeout: 260 seconds] 01:56 -!- fronti [~fronti@irc.fh-biergarten.de] has quit [Ping timeout: 272 seconds] 01:56 -!- emzy [~quassel@unaffiliated/emzy] has quit [Ping timeout: 272 seconds] 01:56 -!- takinbo [~takinbo@unaffiliated/takinbo] has quit [Ping timeout: 272 seconds] 01:56 -!- murch [murch@2a01:4f8:141:1272::2] has joined #bitcoin-core-pr-reviews 01:56 -!- b10c [~b10c@2a01:4f8:192:612a:216:3eff:fef3:dc6a] has joined #bitcoin-core-pr-reviews 01:57 -!- emzy [~quassel@2a01:4f8:192:628a::83] has joined #bitcoin-core-pr-reviews 01:58 -!- fronti [~fronti@irc.fh-biergarten.de] has joined #bitcoin-core-pr-reviews 01:58 -!- takinbo [~takinbo@unaffiliated/takinbo] has joined #bitcoin-core-pr-reviews 01:59 -!- emzy [~quassel@2a01:4f8:192:628a::83] has quit [Changing host] 01:59 -!- emzy [~quassel@unaffiliated/emzy] has joined #bitcoin-core-pr-reviews 02:08 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 02:11 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 02:11 -!- vasild_ is now known as vasild 02:43 -!- molz_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 02:47 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 260 seconds] 03:45 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 03:49 -!- molz_ [~mol@unaffiliated/molly] has quit [Ping timeout: 258 seconds] 03:51 -!- jonatack [~jon@88.124.242.136] has quit [Ping timeout: 260 seconds] 03:52 -!- jonatack [~jon@213.152.161.101] has joined #bitcoin-core-pr-reviews 03:52 -!- belcher_ [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 03:55 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 272 seconds] 03:57 -!- belcher_ is now known as belcher 04:56 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 04:57 -!- molz_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 04:59 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 265 seconds] 05:00 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 05:03 -!- molz_ [~mol@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 05:11 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 05:18 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 272 seconds] 05:36 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 05:43 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 05:51 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 06:02 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 06:06 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 272 seconds] 06:19 -!- tralfaz [~davterra@gateway/tor-sasl/tralfaz] has joined #bitcoin-core-pr-reviews 06:19 -!- davterra [~davterra@gateway/tor-sasl/tralfaz] has quit [Remote host closed the connection] 06:22 -!- S3RK [~S3RK@116.118.73.76] has quit [Ping timeout: 240 seconds] 06:24 -!- S3RK [~S3RK@116.118.74.232] has joined #bitcoin-core-pr-reviews 06:29 -!- _andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 06:29 -!- molz_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 06:32 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 06:32 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 06:33 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 06:37 -!- molz_ [~mol@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 07:13 -!- jarolrod [~xyz@198.12.64.41] has joined #bitcoin-core-pr-reviews 07:19 -!- tralfaz [~davterra@gateway/tor-sasl/tralfaz] has quit [Remote host closed the connection] 07:21 -!- glozow [uid453516@gateway/web/irccloud.com/x-tvlshikuvzciizmp] has joined #bitcoin-core-pr-reviews 07:36 -!- sergei-t [~sergei@2001:a18:0:b23::6] has joined #bitcoin-core-pr-reviews 07:36 -!- sergei-t [~sergei@2001:a18:0:b23::6] has left #bitcoin-core-pr-reviews [] 07:37 -!- andozw [49bd3c74@c-73-189-60-116.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 07:48 * troygiorshev realizes that daylight savings time ending means PR Review Club starts in just over an hour 07:50 < pinheadmz> troygiorshev whoa just noticed too! I have a calendar event scheduled in UTC ;-) 07:57 < jnewbery> troygiorshev: thanks! My calendar event was in one of those stupid human timezones that keeps floating around 07:57 < jnewbery> see everyone in an hour! 08:20 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has joined #bitcoin-core-pr-reviews 08:39 -!- stacie [~stacie@c-24-60-139-217.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews 08:51 -!- sergei-t [~sergei@94.103.211.82] has joined #bitcoin-core-pr-reviews 08:52 -!- fi3 [5ffa4b61@host-95-250-75-97.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 08:59 -!- sift [~sift@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 09:00 < jnewbery> #startmeeting 09:00 -!- lightlike [~lightlike@p200300c7ef134400c136c474592c42f6.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews 09:00 < jnewbery> hi folks. Welcome to review club! 09:00 < emzy> hi 09:00 < stacie> hi 09:00 < sift> hi 09:00 < murch> hi 09:00 < andozw> hi 09:00 < jnewbery> feel free to say hi to let everyone know you're here 09:00 < fjahr> hi 09:00 < lightlike> hi 09:00 < glozow> hi 09:00 < carlakc> hi 09:00 < dongcarl> hi 09:00 < michaelfolkson> hi 09:00 < jnewbery> anyone here for the first time? 09:01 < andozw> :waves: :) 09:01 < troygiorshev> hi 09:01 < fi3> hi 09:01 < jnewbery> welcome andozw! Thanks for joining us :) 09:01 < MarcoFalke> hi 09:01 < andozw> thank you! excited to learn 09:01 < nehan> hi 09:01 < jnewbery> Notes and questions are in the normal place: https://bitcoincore.reviews/19905 09:02 < MarcoFalke> Reminder don't ask to ask, just ask :) 09:02 < jnewbery> MarcoFalke wrote the notes and questions and is hosting today, so I'll pass over to him. Thanks Marco! 09:02 < sergei-t> hi 09:02 < MarcoFalke> ok, let's get started. Who reviewed the pull? 09:02 < jonatack> hi 09:02 < jnewbery> y 09:02 < stacie> y 09:02 < sergei-t> jnewbery: I'm for the first time. Totally unprepared, will probably just lurk. 09:02 < nehan> y 09:03 < troygiorshev> n (only a little) 09:03 < MarcoFalke> sergei-t: Welcome 09:03 < dongcarl> y 09:03 < fi3> n 09:03 < lightlike> 0.5 09:03 < jnewbery> sergei-t: that's fine. Lurkers are welcome too! 09:03 < carlakc> I went through it, but still getting a handle on cpp so very high level 09:03 < carlakc> will likely also lurk :) 09:04 < fjahr> wip 09:04 < felixweis> hi 09:04 < emzy> n 09:04 < MarcoFalke> Today we are talking about chain splits. What are some examples of when a chain split can happen? Which ones should users be warned about? 09:04 < felixweis> 2013 types of chain splits 09:05 < michaelfolkson> Name all 2013 please 09:05 < nehan> if the network is partitioned 09:05 < emzy> There was the problem with 32 and 64 bit systems. Remember not more about it. 09:06 < sipa> emzy: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2015-July/009697.html 09:06 < MarcoFalke> Which ones should the user *not* be warned about? 09:06 < stacie> BIP-50 describes a chain split as an unintended result of switching from Berkely DB to Level DB. Level DB was able to handle blocks with more tx inputs 09:06 < nehan> or if there's a soft fork 09:06 < michaelfolkson> I guess there are bugs causing network chain splits. Natural re-orgs would be considered temporary chain splits? 09:06 < lightlike> one harmless example would just two blocks found at almost the same time by chance. 09:06 < nehan> a user probably shouldn't be notified if it's very small, or if they're still doing IBD 09:06 < fjahr> Competing blocks mined by different miners competing for the longest chain. 09:06 < carlakc> cccccclvlvjdgbhiitljktetkrlrggffbdinjlgknlcb 09:06 < felixweis> few blog reorgs. 1 stale block happens regularly 09:07 < pinheadmz> chainsplits are worrisome after a soft fork like strict DER - miners were not fully validating! 09:07 < MarcoFalke> Good points so far 09:07 < sift> pinheadmz +1 09:07 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 09:08 < MarcoFalke> About expected stale blocks, users shouldn't be warned, but if there is a disagreement about consensus rules, they should be warned. 09:08 < MarcoFalke> Let's got to the next question. 09:08 < MarcoFalke> ActivateBestChain() consists of two nested loops. What is the condition for each loop to terminate? 09:08 < sift> consensus-equivalent, block-production equivalent chain split would be worrying 09:09 < nehan> i think outer loop: caught up to tip of most-work chain we've seen 09:10 < MarcoFalke> btw, this is the function for those lurking: https://github.com/bitcoin/bitcoin/blob/ed9f5477502e5856eba742656e8fc0fcf7bb053b/src/validation.cpp#L2870 09:10 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 09:10 < stacie> I noticed CBlockIndexWorkComparator returns true if the block passed in as the second parameter is the “better” block (based on the following criteria, in order: if it has more work, was received earlier (nSequenceId), or has a lower pointer address). My question is, is that how comparators typically work in C++? Return true if the second parameter is “better”? 09:10 < MarcoFalke> nehan: right 09:11 < lightlike> inner loop: our current tip (after connecting some blocks) has more work than the one we started this loop with 09:11 < MarcoFalke> lightlike: correct 09:12 < sipa> stacie: comparators' contract is to return true for "less than" 09:12 < sipa> e.g. std::set is the same as std::set> 09:13 < MarcoFalke> The inner loop calls ActivateBestChainStep(), which disconnects blocks (in case of a reorg) and connects a batch of up to 32 blocks toward the new tip. When an invalid block is found, CheckForkWarningConditionsOnNewFork() is called. Which block from the batch is passed to CheckForkWarningConditionsOnNewFork()? 09:13 < sipa> where std::less is an object that exposes an operator()(int a, int b) { return a < b; } 09:13 < jnewbery> Compare function: https://en.cppreference.com/w/cpp/named_req/Compare 09:14 < stacie> ahh! thanks sipa & jnewbery! 09:14 < MarcoFalke> again for lurkers: https://github.com/bitcoin/bitcoin/blob/ed9f5477502e5856eba742656e8fc0fcf7bb053b/src/validation.cpp#L2744 09:16 < stacie> the first invalid block from the batch (the one with the lowest height) is the one passed to CheckForkWarningConditionsOnNewFork() 09:16 -!- elle [~ellemouto@155.93.252.70] has joined #bitcoin-core-pr-reviews 09:17 < MarcoFalke> stacie: correct 09:17 < MarcoFalke> ActivateBestChainStep() will also call m_chain.SetTip(), which updates the global reference to the active chain tip (::ChainActive().Tip()). What is the maximum block height difference between pindexNewForkTip and pfork? 09:18 < MarcoFalke> (This question is a bit more tricky, so I won't accept an answer that is just a single number ;) 09:19 < MarcoFalke> for lurkers, we just jumped into a new function: https://github.com/bitcoin/bitcoin/blob/ed9f5477502e5856eba742656e8fc0fcf7bb053b/src/validation.cpp#L1378 09:20 < MarcoFalke> reminder that pindexNewForkTip is "the first invalid block from the batch (the one with the lowest height)" (correct answer from stacie) 09:21 < michaelfolkson> A tip within 72 blocks but a fork that is at least 7 blocks. What does that mean? 09:22 < jnewbery> MarcoFalke: are you asking for the difference in height between pindexNewForkTip and plonger? 09:22 < jnewbery> (pfork is set to pindexNewForkTip) 09:22 < MarcoFalke> jnewbery: pfork, when it has been assigned it's final value 09:23 < MarcoFalke> pfork is initialized to pindexNewForkTip, and then "walked back" to determine the base of the fork 09:23 -!- stacie_ [~stacie@c-24-60-139-217.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews 09:24 < fjahr> Should be just 1 because if its only the first block in the fork that is passed to the function 09:24 < murch> That comment is odd. Was that second sentence inserted into the middle of the big sentence? 09:24 < jnewbery> I think pindexNewForkTip is either on the active chain, or it's the child of a block on the active chain 09:25 < MarcoFalke> michaelfolkson: The code is more clear on that. It means the fork tip is within 72 blocks of our current tip, but the fork is at least 7 blocks "heavy" (with pow) 09:26 < MarcoFalke> fjahr: Correct 09:26 < murch> Given that different difficulties can only appear across diff adjustments, and the adjustment is limited to a factor of four, why a factor of 10? 09:26 < MarcoFalke> jnewbery: Also correct. If it is on the active chain, the difference is 0. If not, it is 1 09:27 < MarcoFalke> murch: Are you asking about the "10%"? 09:27 < jnewbery> just to clarify a point above. The block that's passed to CheckForkWarningConditionsOnNewFork() isn't necessarily an invalid block 09:27 -!- stacie [~stacie@c-24-60-139-217.hsd1.ma.comcast.net] has quit [Ping timeout: 260 seconds] 09:27 < sift> MarcoFalke "but the fork is at least 7 blocks "heavy" (with pow)" 7 blocks worth of PoW weight...relative to what? 09:28 < MarcoFalke> sift: Measured relative to the pow of the fork base block 09:28 < stacie_> For the earlier question about the inner loop in ActivateBestChain(). terminating, I see the line where the conditions are defined, https://github.com/bitcoin/bitcoin/blob/ed9f5477502e5856eba742656e8fc0fcf7bb053b/src/validation.cpp#L2932 but I can't find where starting_tip is updated. I'm guessing it might have something to do with the fact that it's a pointer? (my cpp is rusty :) ) 09:28 < sift> MarcoFalke e.g. 7 blocks worth of PoW weight at the divergence point? 09:28 < jnewbery> imagine we've tried to connect 32 blocks in a loop in ActivateBestChainStep() and the 10th block is invalid. The pindex that we pass to CheckForkWarningConditionsOnNewFork() will be the first block in that batch of 32, which is a valid block 09:28 < MarcoFalke> sift: yes, that's how I see it 09:29 < sift> ty 09:29 < sipa> stacie_: https://github.com/bitcoin/bitcoin/blob/ed9f5477502e5856eba742656e8fc0fcf7bb053b/src/validation.cpp#L2898 09:29 * sift thinks about a miner, under producing blocks to drop that 7 block pow weight and then turning on hash rate 09:30 < MarcoFalke> jnewbery: thanks for the extended explanation :) 09:30 < MarcoFalke> jnewbery: To round up, can you also explain why the diff wouln't be negative in case the passed block is valid? 09:31 < stacie_> sipa - thanks. so every time the loop progresses, the m_chain.Tip() call will result in a new value, right? 09:31 < murch> 7 blocks worth of weight, but 72 blocks long seems impossible, because diff could only change minusculy between two competing tips 09:31 < MarcoFalke> murch: not "72 blocks long", but "withing 72 blocks of our tip" 09:31 < sipa> stacie_: starting_tip doesn't change 09:31 < murch> But I think I haven't really grokked what the 72 and 7 blocks are 09:32 < jnewbery> MarcoFalke: because plonger will walk back until it's the same block as pindexNewForkTip 09:32 < sipa> but m_chain.Tip will 09:32 < murch> oh, you mean it could include a stale tip forking off 30 blocks ago? 09:32 < MarcoFalke> jnewbery: Correct, thx. 09:32 < troygiorshev> stacie_: if I'm seeing it right, it's line 2547 that makes the change to m_chain.Tip 09:32 < MarcoFalke> murch: Yes, that was the design goal (I think) 09:33 < lightlike> stacie_: I think that starting_tip is not updated once defined (it is the reference we compare to), but m_chain.Tip() ist updated inside of ConnectTip(), which is called from ActivateBestChainStep 09:33 < troygiorshev> stacie_: i take it back, it's what lightlike said :) 09:34 < michaelfolkson> murch: I am unsure why you need a warning about a stale tip forking off 30 blocks ago. Who cares? 09:34 < murch> Well, would still indicate a consensus failure 09:35 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews 09:35 < murch> someone got forked off and recognized an ancestor of your chaintip as invalid 09:35 < MarcoFalke> michaelfolkson: For example if 40% of miners are building on a different chain, that is 30 blocks back 09:35 < michaelfolkson> Oh ok, that makes sense 09:37 < MarcoFalke> Is it possible to hit the condition that updates pindexBestForkTip? 09:37 < stacie_> catching up now - thank you sipa, trygiorshev and lightlike! That explains it. I didn't dive into the methods that ActivateBestChainStep calls but it makes sense now! 09:37 < stacie_> *troygiorshev 09:38 < elle> MarcoFalke: pIndexNewForkTip never gets set because it is only set if pfork is set...which is only set if pIndexNewForkTip is set 09:40 < elle> oh sorry , looking at wrong variable 09:40 < MarcoFalke> To recap: pfork is set, but the height difference between pindexNewForkTip and pfork is at most 1 09:41 < jnewbery> We need to look at this condition: pindexNewForkTip->nChainWork - pfork->nChainWork > (GetBlockProof(*pfork) * 7) 09:42 < MarcoFalke> Jup, we are in CheckForkWarningConditionsOnNewFork, which is supposed to update the pindexBest* variables 09:43 < glozow> is pfork the base of the fork there? sorry very basic question 09:43 < MarcoFalke> glozow: Correct 09:43 < MarcoFalke> pfork is the assumed base of the fork relative to our current chain tip 09:43 < glozow> so the condition jnewbery is talking about = there's 7 blocks worth of work on the fork? 09:44 < glozow> 7 blocks of work on 1 block? 🤔 09:44 < MarcoFalke> glozow: Yes. This is one of the conditions that need to be fulfilled to update the pindexBest* variables 09:45 < michaelfolkson> 7 blocks of work on 7 blocks 09:45 < fjahr> michaelfolkson: but the condition would only be fulfilled if you would have 7 blocks of work in 1 block 09:46 < jnewbery> so we've established that pindexNewForkTip and pfork can be at most one block apart, and the condition is that there's at least 7 block's worth of difference in their total work. Is that possible? 09:46 < fjahr> that's the thing why it can't be hit or only in theory 09:46 < glozow> seems like the answer to Marco's question is ~no 09:47 < MarcoFalke> Correct, this can not be hit 09:47 < MarcoFalke> Extra question: if the we passed the highest block of the to-be-connected chain instead, what would the height difference between pfork and pindexNewForkTip be? 09:48 < jnewbery> the to-be-connected chain is 32 blocks 09:48 < fjahr> 32 09:48 < jnewbery> *up to 32 blocks, sorry 09:48 < MarcoFalke> up to 32 or always 32? 09:48 < MarcoFalke> jnewbery: Heh, nice 09:49 < MarcoFalke> That the lowest hight block is passed is an oversight/bug, but can the fork detection logic be fixed by passing the highest block? 09:49 < murch> jnewbery: no, because two competing blocks can only differ in difficulty in forks across a difficulty adjustment and a factor 7 won't be possible there 09:50 < MarcoFalke> murch: Even with a difficulty adjustment this should not be possible 09:50 < sift> 10 minutes left 09:50 < murch> yes 09:50 < MarcoFalke> I think those are limited to a factor of 4 09:50 < murch> Well, one could go down, the other up 09:51 < murch> but the time difference wouldn't be that big in practice 09:51 < MarcoFalke> What are your thoughts on the approach of the pull request? Should the bug in the warning system be fixed or should the code be removed? What alternative places/ways to implement the warning logic can you think of? 09:52 < michaelfolkson> I have a couple of unrelated questions. What tools are available to the user to react to the warning? Presumably if they became aware of a UASF like scenario they could join it 09:52 < michaelfolkson> And then where is the warning test in test/functional/feature_block.py? I couldn't find it with a quick search 09:53 < MarcoFalke> michaelfolkson: Good q. If I was an exchange and I'd see a warning, I'd shut down all nodes for now 09:53 -!- shesek [~shesek@164.90.217.137] has joined #bitcoin-core-pr-reviews 09:53 -!- shesek [~shesek@164.90.217.137] has quit [Changing host] 09:53 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews 09:53 < MarcoFalke> michaelfolkson: It is the "re-org" test (last test which creates more than 1000 blocks) 09:53 < jnewbery> MarcoFalke: I think that removing dead code and adding working code can be seen as two separate questions. I don't think "You should fix it" is a reasonable response to someone removing broken code. 09:54 < MarcoFalke> jnewbery: I tend to agree 09:54 < jnewbery> if it's broken and has been for many releases, then removing the dead code so the code reflects reality is a quality improvement in itself 09:54 < jnewbery> if someone then wants to implement a working warning system, that should be judged on its own merits 09:54 < troygiorshev> Might this be a similar situation to the old "alert notify" system? Where (even if it was fixed) it's simply outdated? 09:54 < sift> seems like priority should be soundness first, performance second 09:55 < michaelfolkson> In response to your questions MarcoFalke you say in the PR comment "add a fork detection somewhere outside of the ActivateBestChainStep logic (maybe net_processing)." 09:55 < murch> Anything that is an obvious improvement has merit, and removing dead code is an obvious improvement 09:55 < MarcoFalke> michaelfolkson: Yes, I think that the internal validation logic is the wrong place to put the fork detection algorithm 09:56 < michaelfolkson> I don't think anything has changed re whether the warning is needed or not. If it was needed back then it is needed today too 09:56 < murch> Upping the bar just leads to abandoned PRs and frustrated would be contributors 09:57 < fjahr> It doesn't seem like the ideal way to solve this so removing and (possibly) reimplementing seems better than fixing which would make the PR more complicated. But I don't have great ideas how it should be done. 09:57 < michaelfolkson> But yeah if it doesn't work and it is in the wrong place it should be removed 09:57 < michaelfolkson> Ideally there would be a follow up PR reimplementing it :) 09:57 < MarcoFalke> A warning system is very much needed 09:58 < jnewbery> michaelfolkson: you're very welcome to open that PR! 09:58 < emzy> Could be first an external one. 09:58 < michaelfolkson> Haha 09:58 < emzy> To figure out what's the best parameters. 09:59 < fjahr> michaelfolkson: just change the 7 to 1 and reuse the rest of the code :P 09:59 < sift> The_scream.jpg 09:59 < MarcoFalke> Any last minute questions/suggestions? 10:00 < MarcoFalke> #endmeeting 10:00 < MarcoFalke> Thanks everyone! 10:00 < troygiorshev> thanks MarcoFalke! 10:00 < sift> thanks MarcoFalke ! 10:00 < murch> Thanks 10:00 < emzy> Thank you MarcoFalke! 10:00 < stacie_> Thank you MarcoFalke! 10:00 < jnewbery> I tend to think that if it wasn't working, then ripping it out and starting from scratch is safer. It shouldn't be the case, but tweaking code that wasn't working would probably receive less scrutiny in review than implementing something new 10:00 < felixweis> Thanks MarcoFalke! 10:00 < sergei-t> thanks! 10:00 < lightlike> thanks! 10:00 < carlakc> thanks for the lurker links marco! 10:00 < fjahr> thanks MarcoFalke 10:00 < jnewbery> Thanks MarcoFalke! 10:00 -!- Klopp [59d42b3d@89-212-43-61.dynamic.t-2.net] has quit [Ping timeout: 245 seconds] 10:00 -!- elle [~ellemouto@155.93.252.70] has quit [Quit: leaving] 10:01 < dongcarl> thanks MarcoFalke! 10:01 < michaelfolkson> Thanks MarcoFalke, very interesting 10:01 < MarcoFalke> jnewbery: If the fix was trivial, I might have fixed it. But I don't even see a way to fix this without rewriting the validation core 10:01 < MarcoFalke> logic 10:02 < michaelfolkson> I guess in terms of review the most important thing is to check that the code being ripped out isn't used elsewhere. Just for the warning 10:02 < jnewbery> Well done everyone. Today was a difficult PR to review. We went really deep into the consensus code. If you managed to follow along for the whole meeting you can give yourself a pat on the back :) 10:02 < jnewbery> we'll do something a bit easier next week 10:04 < fjahr> BTW, that the code discussed is not hit by any of the tests can also be seen in the coverage analysis: https://marcofalke.github.io/btc_cov/total.coverage/src/validation.cpp.gcov.html obviously also provided by MarcoFalke 10:04 < MarcoFalke> fjahr: lol, I initially spent a day trying to write a test for it 10:05 < MarcoFalke> writing tests for uncovered code is a good way to find dead and buggy code 10:05 < michaelfolkson> What are you using to identify code that can be safely be ripped out? Just eyes and something like ctags? 10:05 < fjahr> :) 10:05 < MarcoFalke> michaelfolkson: writing tests. I haven't used ctags yet 10:06 < MarcoFalke> I only use vanilla git and vim (and firefox for GitHub) 10:06 < michaelfolkson> OK cool, thanks 10:13 -!- sift [~sift@195.181.160.175.adsl.inet-telecom.org] has left #bitcoin-core-pr-reviews [] 10:19 -!- fi3 [5ffa4b61@host-95-250-75-97.retail.telecomitalia.it] has quit [Remote host closed the connection] 10:30 < jnewbery> logs for today's meeting are posted: https://bitcoincore.reviews/19905. I'll get notes and questions for next week's PR up in a day or two 10:35 -!- andozw [49bd3c74@c-73-189-60-116.hsd1.ca.comcast.net] has quit [Remote host closed the connection] 10:38 -!- stacie_ [~stacie@c-24-60-139-217.hsd1.ma.comcast.net] has quit [Quit: stacie_] 11:13 -!- sergei-t [~sergei@94.103.211.82] has left #bitcoin-core-pr-reviews [] 11:17 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 11:22 -!- csknk [~csknk@unaffiliated/csknk] has quit [Client Quit] 11:24 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Quit: Leaving] 11:34 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 12:13 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!] 12:18 -!- lightlike [~lightlike@p200300c7ef134400c136c474592c42f6.dip0.t-ipconnect.de] has quit [Quit: Leaving] 12:41 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 12:52 -!- mol_ [~mol@unaffiliated/molly] has quit [Read error: Connection reset by peer] 12:52 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 12:59 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 14:08 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 14:08 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Remote host closed the connection] 14:11 -!- wullon [~wullon@241.243.86.88.rdns.comcable.net] has joined #bitcoin-core-pr-reviews 14:12 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 14:12 -!- vasild_ is now known as vasild 15:39 -!- dappdever [~dappdever@pool-100-8-184-107.nwrknj.fios.verizon.net] has quit [Remote host closed the connection] 15:52 -!- dappdever [~dappdever@pool-100-8-184-107.nwrknj.fios.verizon.net] has joined #bitcoin-core-pr-reviews 15:55 -!- dappdever [~dappdever@pool-100-8-184-107.nwrknj.fios.verizon.net] has quit [Client Quit] 16:01 -!- molz_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 16:03 -!- jonatack [~jon@213.152.161.101] has quit [Quit: jonatack] 16:04 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 264 seconds] 16:10 -!- jonatack [~jon@88.124.242.136] has joined #bitcoin-core-pr-reviews 16:15 -!- jonatack [~jon@88.124.242.136] has quit [Ping timeout: 258 seconds] 16:15 -!- jonatack [~jon@213.152.161.30] has joined #bitcoin-core-pr-reviews 16:18 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 16:21 -!- molz_ [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 16:22 -!- molz_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 16:25 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 260 seconds] 16:28 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 16:28 -!- molz_ [~mol@unaffiliated/molly] has quit [Remote host closed the connection] 16:36 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 16:37 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 16:48 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 16:49 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 17:42 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 17:45 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 17:46 -!- davterra [~davterra@gateway/tor-sasl/tralfaz] has joined #bitcoin-core-pr-reviews 18:18 -!- seven_ [~seven@cpe-90-157-197-248.static.amis.net] has quit [Ping timeout: 260 seconds] 22:51 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 258 seconds] 22:56 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 23:14 -!- shesek [~shesek@unaffiliated/shesek] has quit [Remote host closed the connection] 23:20 -!- kristapsk_ [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 23:22 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Ping timeout: 240 seconds]