--- Day changed Wed Mar 18 2020 00:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 00:43 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 00:43 -!- vasild_ is now known as vasild 00:47 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 01:08 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 01:10 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 01:13 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 01:22 -!- kristapsk_ [~KK@gateway/tor-sasl/kristapsk] has quit [Ping timeout: 240 seconds] 01:26 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 01:27 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews --- Log closed Wed Mar 18 03:01:41 2020 --- Log opened Wed Mar 18 03:02:06 2020 03:02 -!- kanzure_ [~kanzure@unaffiliated/kanzure] has joined #bitcoin-core-pr-reviews 03:02 -!- Irssi: #bitcoin-core-pr-reviews: Total of 86 nicks [0 ops, 0 halfops, 0 voices, 86 normal] 03:02 -!- nehan_ [~nehan@41.213.196.104.bc.googleusercontent.com] has joined #bitcoin-core-pr-reviews 03:04 -!- Netsplit *.net <-> *.split quits: raj_149, nehan, kanzure 03:06 -!- seven_ [~seven@2a00:ee2:410c:1300:6428:b981:df56:c111] has joined #bitcoin-core-pr-reviews 03:09 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 03:11 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 03:12 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 03:14 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 03:15 -!- Irssi: Join to #bitcoin-core-pr-reviews was synced in 823 secs 03:23 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 03:43 -!- apaval_ [~apaval@103.140.137.190] has quit [Remote host closed the connection] 03:44 -!- apaval_ [~apaval@103.140.137.9] has joined #bitcoin-core-pr-reviews 04:03 -!- Rhett53Rowe [~Rhett53Ro@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 04:08 -!- Rhett53Rowe [~Rhett53Ro@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 250 seconds] 04:30 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 04:58 -!- slivera [~slivera@185.104.186.203] has quit [Remote host closed the connection] 05:10 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 05:15 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 05:15 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 05:51 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 06:04 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Remote host closed the connection] 06:04 -!- emilengler [~emilengle@stratum0/entity/emilengler] has joined #bitcoin-core-pr-reviews 06:08 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-wiarqpxplzhwfubz] has quit [Ping timeout: 256 seconds] 06:08 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 06:08 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 06:12 -!- You're now known as kanzure 06:26 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-eocmmwsmzrsbaslx] has joined #bitcoin-core-pr-reviews --- Log closed Wed Mar 18 06:46:21 2020 --- Log opened Wed Mar 18 06:46:42 2020 06:46 -!- kanzure [~kanzure@unaffiliated/kanzure] has joined #bitcoin-core-pr-reviews 06:46 -!- Irssi: #bitcoin-core-pr-reviews: Total of 86 nicks [0 ops, 0 halfops, 0 voices, 86 normal] 06:49 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 06:49 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 07:00 -!- Irssi: Join to #bitcoin-core-pr-reviews was synced in 818 secs 07:08 < jnewbery> we'll get started in just under three hours 07:09 < emilengler> πŸš€ 07:11 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 07:11 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 07:16 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 07:34 -!- kristapsk_ [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 07:34 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 07:35 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has joined #bitcoin-core-pr-reviews 08:03 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 08:13 -!- lukedashjr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews 08:15 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 256 seconds] 08:17 -!- lukedashjr is now known as luke-jr 08:54 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 08:59 -!- an4s [d10646ba@209.6.70.186] has joined #bitcoin-core-pr-reviews 09:12 < luke-jr> so what's the plan for today? jnewbery 09:12 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 09:17 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 250 seconds] 09:20 -!- shaunsun [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has joined #bitcoin-core-pr-reviews 09:20 -!- shaunsun_ [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has joined #bitcoin-core-pr-reviews 09:24 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 09:36 -!- jungly [~jungly@host73-184-dynamic.250-95-r.retail.telecomitalia.it] has quit [Remote host closed the connection] 09:42 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 09:44 < jnewbery> we start at 17:00 UTC. Notes are here: https://bitcoincore.reviews/18113.html 09:45 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 09:47 -!- Prayank [8ba73da2@139.167.61.162] has joined #bitcoin-core-pr-reviews 09:50 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:9cc8:c06d:d41d:695e] has joined #bitcoin-core-pr-reviews 09:59 -!- Prayank [8ba73da2@139.167.61.162] has quit [Remote host closed the connection] 10:00 < jnewbery> #startmeeting 10:00 -!- an4s [d10646ba@209.6.70.186] has quit [Ping timeout: 240 seconds] 10:00 < jkczyz> hi 10:00 < willcl_ark> hi 10:00 < fjahr> hi 10:00 < emzy> hi 10:00 < pinheadmz> hi 10:00 < michaelfolkson> hi 10:00 < amiti> hi 10:01 -!- Prayank [67101aa2@103.16.26.162] has joined #bitcoin-core-pr-reviews 10:01 < nehan_> hi 10:01 < jnewbery> Hi folks. Welcome to PR Review Club! Feel free to say hi to let everyone know you're here. 10:01 < jnewbery> As always, feel free to jump in at any point to ask questions. No need to ask to ask, just ask! 10:01 < Prayank> Hello Everyone πŸ˜€ 10:02 -!- earpiece [~earpiece@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 10:02 -!- lightlike [~lightlike@p200300C7EF13EC002815F0B7956AB5C8.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews 10:02 < jnewbery> wow. Emojis. That's novel :) 10:02 < jnewbery> This week's PR is rather short, but I think it's a good opportunity to dig into the coins structures a bit more. Notes are in the normal place: https://bitcoincore.reviews/18113.html 10:02 < andrewtoth> hi 10:02 < jonatack> hi 10:02 < lightlike> hi 10:02 < earpiece> hi 10:02 < jnewbery> who had a chance to review the changes? y/n 10:02 < fjahr> y 10:02 < pinheadmz> y 10:02 < andrewtoth> y 10:02 < amiti> y 10:02 < jkczyz> y 10:02 < emzy> y 10:03 < nehan_> n 10:03 < willcl_ark> y 10:03 < jnewbery> great. Solitary confinement seems to be doing wonders for people's time for code review 10:03 < jnewbery> question 1: Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? 10:04 -!- LarryRuane [495f892d@73.95.137.45] has joined #bitcoin-core-pr-reviews 10:04 < jnewbery> and we might as well do question 2 at the same time: This PR touches consensus-critical code. Does it meet the high bar for justifying a change to such critical code? Is improving code clarity justification enough? 10:04 < fjahr> Concept ACK, the code looks good but I don't feel comfortable enough with everything especially considering reorgs 10:05 < fjahr> If it’s a significant improvement I think it is justified because it reduces the risk of bugs being introduced in the future and makes β€œreal” improvements and new feature development easier 10:05 < willcl_ark> tACK from me. But I have to confess I was a bit less clear on the code-side this week. It was a new area for me to be looking at (i didn't attent the previous related meeting in Jan) 10:05 < emzy> Conxept ACK, seens the code is more clear for future changes. 10:05 < jnewbery> (it's my code but NACK is an acceptable answer) 10:05 < jkczyz> Concept ACK. IMHO, consensus-critical code *should* be clear 10:06 < andrewtoth> Concept ACK, but it seems like it's doing too much for how critical the code is. Could the renaming of PRUNED to SPENT and comment updates be a separate change to the consensus code change? 10:06 < jonatack> scripted diffs and improving commenting, even in consensus code, seem to still have a worthwhile benefit/risk ratio 10:06 < jnewbery> willcl_ark: the notes and meeting log from https://bitcoincore.reviews/17487 are really good, so I'd recommend reading through those if this has piqued your interest 10:07 < michaelfolkson> Yeah we can't leave consensus code untouched in fear of breaking something if clarity can be improved. Needs lots of review though 10:07 < jnewbery> andrewtoth: yes, this could definitely be split in two. One that changes the commends and variable naming, and then a second that makes the logical changes 10:08 < andrewtoth> It just seems like review will not be as focused with such a large diff and essentially two different things being changed 10:08 < jnewbery> that's fair feedback 10:08 < pinheadmz> jnewbery: the organization of the different cache layers is whats missing from my overall view of this PR 10:08 < nehan_> i think it would be better to split this PR into two: one for comments/no consensus changes and one with the changes 10:09 < jnewbery> pinheadmz: definitely check out https://bitcoincore.reviews/17487 and james's diagram here: https://jameso.be/dev++2018/#54 10:09 < willcl_ark> One thing I couldn't tell, is there any guarantee that this could not create a chain split? At a high level, if the change is modifying _both_ the code and the associated test, it seems like that guarantee is harder to prove... 10:10 < jnewbery> willcl_ark: if the logical change is bad, then yes, this could cause a consensus failure. Does anyone want to expand on how? 10:11 < jkczyz> regarding splitting into PRs, if the changes don't bleed across commits then I think one PR is fine. Unless the logical is contentious. But in that case the comments would need to be updated if the comment PR goes in first 10:11 < fjahr> consensus failure: a block would be rejected because different nodes have a different utxo set, one of them would miss a coin that is spent in the block 10:11 < Prayank> Interesting 10:12 < jnewbery> fjahr: yeah, that's what we should be scared about with any change like this 10:13 < jnewbery> I'm going to move onto the next question, but feel free to ask questions/give input on previous questions as we go 10:13 < jnewbery> What does it mean for a coin to be FRESH? What about DIRTY? 10:14 < amiti> DIRTY means it exists in the parent cache, but has been changed in the child cache 10:14 < amiti> FRESH means it doesn't exist in the parent cache 10:14 < jnewbery> amiti: exactly right 10:15 < fjahr> in the comment it says *potentially* different from the parent cache 10:15 < jnewbery> a Coin object can also be spent or unspent. How many different potential states are there? 10:15 < jkczyz> 8 10:15 < jnewbery> fjahr: ah yes, thanks for clarifying 10:15 < jnewbery> jkczyz: yes, and which of those are valid? 10:16 < jkczyz> good question. Haven't gone through all them yet :) 10:16 < amiti> if a coin is FRESH, is it *always* DIRTY, or is it *never* DIRTY? 10:16 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Quit: Leaving...] 10:16 < amiti> I think it has to be one of those two, right? 10:16 < michaelfolkson> fjahr: Why only potentially? It has been changed? 10:17 < lightlike> why can't a coin be "both FRESH and spent" then as one of the commit messages says? Couldn't it be created in the child cache, spent there and still be FRESH because it never existed in the parent? 10:17 < amiti> lightlike: my understanding is because in that case you could delete it from the child cache 10:18 < jonatack> jkczyz: i agree, istm the main reason to split would be if over time there was a lack of review that could be alleviated by splitting, or to get in worthwhile standalone changes if others were contentious. 10:18 < jnewbery> lightlike: important to distinguish between before this PR and after this PR 10:18 < fjahr> michaelfolkson: I am not 100% clear on this in which cases this occurs, I am referring to the comment in coins.h: "DIRTY: This cache entry is potentially different from the version in the parent cache" 10:18 < jnewbery> before this PR, a Coin could be spent and FRESH 10:18 -!- an4s [d10646ba@209.6.70.186] has joined #bitcoin-core-pr-reviews 10:19 < willcl_ark> Semantically that reads badly, but according to the two definitions above... it appears to be an acceptable state. 10:19 < lightlike> oh ok, got it. 10:20 < jnewbery> michaelfolkson: I'd need to look back at the code in validation.cpp, but I think there could be a situation where a coin gets deleted in a child cache and then re-created. It would then be the same as the parent cache but would be marked as DIRTY 10:20 < michaelfolkson> Ah ok 10:21 < michaelfolkson> Should it be marked as DIRTY in this case? 10:21 < jnewbery> I think that might happen in a re-org - the block that spent the coin is disconnected, which adds the coin to the cache, and then the competing block also spends the coin and it gets removed 10:22 < jnewbery> or something like that. The point is that if we change a coin in a cache, it MUST be marked as DIRTY 10:22 < michaelfolkson> Ok 10:22 < jnewbery> if not, changes might not get saved when the cache is flushed 10:23 < fjahr> michaelfolkson: To be sure you can not mark it as DIRTY you would probably have to check the parent cache all the time and that would be too much of a performance penalty I guess 10:23 < Prayank> I have a noob question. Sorry have not read much details but it looked interesting and this is my first meeting in core or review club. Does this FRESH and DIRTY thing in any way affect privacy when a full node interacts with other full nodes. 10:24 < fjahr> I don't really know, just speculating 10:24 < jnewbery> Prayank: noob questions are welcome here :) 10:24 < andrewtoth> FRESH must always be DIRTY. You cannot have a state with FRESH and not DIRTY. 10:24 < an4s> I am a noob here as well. Joined late so kind of outta sync 10:24 < amiti> andrewtoth: gotcha. thanks 10:25 -!- LarryRuane [495f892d@73.95.137.45] has quit [Ping timeout: 240 seconds] 10:25 < andrewtoth> because to be FRESH it must necessarily be different than what the parent has, since the parent does not have it 10:25 < jnewbery> andrewtoth: Yes, I think that's right. FRESH implies DIRTY 10:26 < michaelfolkson> Prayank: I would say not with these definitions of fresh and dirty. There is another context where coins from a coinbase transaction are referred to as fresh and that's where there's a potential privacy benefit 10:26 < fjahr> Let me try to answer Prayank: We are discussing internal management of data, this not information that is shared between nodes so it can't impact privacy 10:26 < Prayank> Thanks 10:26 < jnewbery> michaelfolkson: fjahr: exactly right 10:26 < amiti> so what's the point of additionally marking FRESH? Is there a performance benefit when flushing the cache? 10:27 < amiti> (before this PR) 10:27 < jnewbery> there's also another meaning of 'dirty' in the wallet for addresses that have already been used, but that is unrelated to what we're talking about here 10:28 < jnewbery> can anyone answer amiti? Why do we have FRESH at all? 10:28 < willcl_ark> So will nodes from before/after this patch be marking coins in their caches differently to each other, and therefore flushing them differently (risking chainsplit)? That's what I can't quite tell.. 10:29 < andrewtoth> If it's marked FRESH and is spent, it can be removed instead of flushed to parent 10:29 < pinheadmz> jnewbery: i think its bc if that coin is spent, the parent caceh never needs to know about it 10:29 < andrewtoth> If it's not spent, it needs to be flushed to parent because it's also DIRTY 10:30 < jnewbery> willcl_ark: the logical change in this PR is to do with the node's internal caching strategy. The underlying coins database shouldn't be affected 10:30 < amiti> oh right. ah got confused and thought thats what this PR is introducing, but actually the change here is that we don't need to fetch spent coins 10:30 < amiti> right? 10:30 < jnewbery> (you could potentially remove the cache entirely and just use the disk's coins database and arrive at the same view of consensus, but performance would be horrendous) 10:30 < earpiece> fjahr: the pr is not related to P2P layer but so not information intentionally shared but the privacy impact to be answered is, are the changes creating a side channel etc 10:31 < willcl_ark> jnewbery: Ok thanks. 10:31 < jnewbery> (but only kinda - an extra layer of cache is used during block connection) 10:31 < jkczyz> Are the assumptions then (1) FRESH implies DIRTY and (2) spent coins should not have entries? 10:32 < earpiece> could a probing peering observe additional information by using the fresh/dirty/spent/unspent to exfiltrate info about wallet etc, I don't think it could but not certain 10:32 < jnewbery> amiti: right. That' the change 10:32 < andrewtoth> Would spent and ~Dirty be an invalid state as well? 10:32 < jnewbery> earpiece: nice question. What do people think? 10:32 < fjahr> earpiece: good question. I don't think there is a chance because this is general chain state and it is not connected to the wallet 10:33 < jnewbery> fjahr: I agree. This is logic that all nodes do on all transactions/blocks, so leaks no information about the wallet 10:34 < jnewbery> earpiece: if you're interested in remote wallet side-channel attacks, check out the talk here: https://bitcoinops.org/en/newsletters/2020/02/26/#remote-side-channel-attacks-on-anonymous-transactions 10:35 < jnewbery> that paper deals with monero/zcash, but I think it's generally interesting 10:35 < earpiece> ah yeah, Tramer's work. it's a good recent reference :) 10:35 < amiti> jkczyz: re (2) spent coins should not have entries -> if a coin is retrieved from the parent cache then spent in the child cache, it should have a DIRTY entry there until its flushed, right? 10:36 < jkczyz> amiti: Yeah, that was my thinking 10:36 < jkczyz> In which case I could only see three possible states 10:37 < jnewbery> amiti: jkczyz: yes, spent and DIRTY is a valid state. It means the child cache has spent the coin and that spentness needs to be propogated to the parent when the cache is flushed 10:37 < nehan_> jkczyz amiti: re(2) I think only coins that are created and spent in the child view should not have entries (with this change. instead of returning an entry with FRESH) 10:39 < andrewtoth> spent and not DIRTY is not a valid state though right? 10:39 < jnewbery> andrewtoth: it was before this PR. After this PR, it's not 10:40 < andrewtoth> ahh gotcha 10:40 < jnewbery> before this PR we could fetch a spent coin from the parent and keep it in the child cache. That would be spent and not-DIRTY 10:40 < nehan_> jnewbery: does that mean you can remove the line return !coin.IsSpent() in GetCoin? 10:40 < nehan_> remove the check, that is. 10:41 < andrewtoth> I was looking at all the commenting changes and the actual logic change escaped me. Thanks! 10:41 < jkczyz> jnewbery: In that case I'll change my answer to 5 valid states :) 10:41 < amiti> jkczyz: before, or after the PR? 10:42 < jkczyz> after 10:42 < jnewbery> andrewtoth: that's another good reason to split this PR up! 10:42 < michaelfolkson> Not DIRTY is FRESH right? There are only two states. DIRTY and FRESH 10:43 < michaelfolkson> Oh no sorry, ignore me I'm with you 10:43 < nehan_> michaelfolson: no this tripped me up last time. DIRTY is not the opposite of FRESH. 10:43 < jnewbery> what do people think of nehan_'s question? Can we remove the check for spentness in https://github.com/bitcoin/bitcoin/blob/3a8d25064e700ff2e69600cc1ede597751283a85/src/coins.cpp#L63 after this PR? 10:44 < nehan_> oh, no, because it might be spent in the child view 10:44 < jnewbery> right 10:45 < jnewbery> yes, FRESH and DIRTY aren't the most intuitive names. Perhaps NEWCOIN and MUSTFLUSH would be better 10:45 < amiti> I'm seeing 4 valid states with the PR. 1) spent, not fresh, dirty 2) unspent, fresh, dirty 3) unspent, not fresh, dirty 4) unspent, not fresh, not dirty. 10:45 < amiti> jkczyz: hows that match up to your 5? 10:46 < amiti> jnewbery: NEWCOIN and MUSTFLUSH would be _so_ much better 10:46 < nehan_> amiti: +1 10:46 < michaelfolkson> Start with 8 (2^3) and then subtract from that 10:46 < jonatack> +1 on better naming 10:47 < jnewbery> well they say there are only two hard things in computer science: naming things and cache invalidation, and we've covered both of them today 10:47 < andrewtoth> lol 10:47 < michaelfolkson> Do "they" say that?! haha 10:47 < jkczyz> amiti: also had (5) spent, fresh, dirty since FRESH implies DIRTY. Thinking if that one makes sense now... 10:48 < andrewtoth> It does make sense 10:48 < jnewbery> jkczyz: after this PR, a spent coin can't be FRESH 10:48 < amiti> jkczyz: thats the one that this PR changes. if it's fresh and spent, drop it. 10:48 < andrewtoth> It is a candidate to remove 10:48 < amiti> wait wait 10:49 < nehan_> i got four states as well 10:49 < amiti> ok nevermind, don't wait 10:49 < andrewtoth> it can't be spent and NOT dirty 10:49 < nehan_> yeah 10:49 < jnewbery> amiti: this PR doesn't change that. The behavior is already 'if it's spent and FRESH, drop it'. The change in this PR is 'don't fetch a spent coin from a parent cache' 10:50 < amiti> yeah thats what I mixed up before πŸ€¦β€β™€οΈ 10:50 < jnewbery> which introduces a new invariant: a spent coin can now *never* be fresh 10:50 < amiti> maybe this time I'll get it :) 10:51 < jnewbery> ok 10 minutes left. I have one last question, but now is also your chance to say anything you've been holding back on 10:51 < jnewbery> Could this change have a performance impact during block validation or Initial Block Download? 10:51 < jkczyz> jnewbery: so 4 then? 10:51 < jnewbery> oh yeah, I agree that the answer is 4 10:51 < nehan_> ugh but which 4? i think i had the wrong ones too: 1) not dirty & not fresh & not spent (or does this mean it wouldn't be in the child cash?) 2) dirty & not fresh & spent 3) dirty & fresh & spent 4) dirty & not fresh & spent 10:51 < nehan_> s/cash/cache 10:52 < jnewbery> bitcoin cache? 10:52 < amiti> nehan_: #1 would be when a child retrieves a not spent coin from the parent, right? 10:52 < nehan_> because i think you're saying after this change #3 will no longer happen 10:53 < nehan_> amiti: right. just not sure when that happens? 10:53 < jnewbery> My 4 are: (spent & not-FRESH & not-DIRTY), (unspent & not-FRESH & not-DIRTY), (unspent & not-FRESH & DIRTY), (unspent & FRESH & DIRTY) 10:54 < jnewbery> nehan_: retrieving unspent coins from a parent happens all the time. It's how we retrieve coins from the disk coins DB 10:55 < amiti> jnewbery: why not (spent & not-FRESH & DIRTY)? 10:55 < jnewbery> any thoughts about performance impact? 10:56 < jonatack> This PR LGTM, but naming-wise I think "FRESH" and "DIRTY" generally imply mutually exclusive states to people. 10:56 < michaelfolkson> Agreed 10:56 < jnewbery> amiti: sorry you're right. Replace my first one with (spent and not-FRESH and DIRTY) 10:56 < nehan_> why wouldn't the first one happen? 10:57 < nehan_> that seems like the case where you read a spent UTXO from the parent 10:57 < jnewbery> nehan_: after this PR, we wouldn't keep it: https://github.com/bitcoin/bitcoin/pull/18113/commits/7053da36bf3879f1f160303f369605fd91ba957d#diff-cd7b305fd4b4280f22ae88960e60398eR48 10:57 < willcl_ark> jonatack: agree, FRESH is too close to (the opposite of DIRTY,) CLEAN 10:57 < nehan_> jnewbery: ah right, thanks 10:58 < willcl_ark> ...although CCoinsCacheEntry::Flags does explain pretty clearly what the two flags mean 10:58 < jnewbery> seems like we have almost unanimous agreement that the FRESH/DIRTY terminology is confusing! 10:58 < Prayank> Yes 10:59 < jnewbery> I think that naming should always be as intuitive as we can make it. Any confusion stemming from other associations with the name can eventually exhibit themselves as bugs 10:59 < jnewbery> ok, that's about time. I just realised that I didn't shill my own podcast episode in the notes, which is uncharacteristic of me 11:00 < jnewbery> if you like coins, you'll love https://podcast.chaincode.com/2020/02/26/utxos-5.html 11:00 < Prayank> lol 11:00 < jnewbery> #endmeeting 11:00 < jnewbery> thanks all! 11:00 < willcl_ark> so can we have an unspent && DIRTY coin? 11:00 < Prayank> Thanks πŸ‘ 11:00 < willcl_ark> thanks jnewbery 11:00 < michaelfolkson> I have a backlog to catch up on. The sipa one was immense 11:00 < michaelfolkson> Thanks 11:00 < amiti> thanks! 11:00 < nehan_> thanks 11:00 < emzy> Thanks jnewbery and all! 11:00 < jkczyz> thanks! 11:01 < pinheadmz> ty all confusing and interesting today 11:01 < jonatack> Thanks jnewbery and everyone! Good to see new people too :) 11:01 < jnewbery> willcl_ark: yes, a Coin can be uspent and DIRTY 11:01 < amiti> pinheadmz: well put πŸ˜‚ 11:01 < willcl_ark> jnewbery: is that the re-org case? 11:01 < fjahr> jnewbery: Since the spent coins from the parent cache are skipped there could be a small performance impact? 11:01 < andrewtoth> Thanks jnewbery! 11:01 < fjahr> Thanks! 11:02 < jnewbery> fjahr: too late! 11:02 < fjahr> jnewbery: i mean small performance improvement 11:02 < fjahr> :( 11:02 < jnewbery> I don't know the answer for sure. I was just throwing it out there as a conversation starter 11:03 < fjahr> ok 11:03 < jonatack> fjahr: I'm thinking it's possible but only benchmarking can tell. Have to measure. 11:03 < jnewbery> jonatack: agree with benchmarking 11:04 < jonatack> PSA: We're cooking up some better-looking meeting logs https://github.com/bitcoin-core-review-club/bitcoin-core-review-club.github.io/pull/147 11:07 < earpiece> jonatack nice! 11:07 < jonatack> earpiece: πŸ‘ 11:12 -!- Prayank [67101aa2@103.16.26.162] has quit [Ping timeout: 240 seconds] 11:19 -!- earpiece [~earpiece@195.181.160.175.adsl.inet-telecom.org] has left #bitcoin-core-pr-reviews [] 11:24 < jnewbery> if you had comments on the PR during the meeting, I'd encourage you to leave them on the PR: https://github.com/bitcoin/bitcoin/pull/18113. The point of review club is to make you feel comfortable with the github review process. 11:29 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:9cc8:c06d:d41d:695e] has quit [Quit: Sleep mode] 11:44 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:9cc8:c06d:d41d:695e] has joined #bitcoin-core-pr-reviews 11:51 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:9cc8:c06d:d41d:695e] has quit [Quit: Sleep mode] 11:59 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:9cc8:c06d:d41d:695e] has joined #bitcoin-core-pr-reviews 12:00 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:9cc8:c06d:d41d:695e] has quit [Client Quit] 12:16 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 12:16 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 12:26 < jnewbery> meeting log is posted: https://bitcoincore.reviews/18113.html 12:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 12:44 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 12:44 -!- vasild_ is now known as vasild 13:03 -!- an4s [d10646ba@209.6.70.186] has quit [Remote host closed the connection] 13:09 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 13:46 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 13:51 -!- slivera [~slivera@185.104.186.203] has joined #bitcoin-core-pr-reviews 14:02 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 14:23 -!- Jovan40Hamill [~Jovan40Ha@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 14:28 -!- Jovan40Hamill [~Jovan40Ha@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 250 seconds] 14:33 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 14:34 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 14:40 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 14:45 < jnewbery> amiti has added a table summarizing the possible coin states: https://bitcoincore.reviews/18113.html#appendix . Thanks amiti! 14:55 < amiti> thanks for making the PR & hosting! I only started understanding all these possibilities in todays session :) 15:06 < jonatack> amiti: good idea -- nice! 15:07 -!- seven_ [~seven@2a00:ee2:410c:1300:6428:b981:df56:c111] has quit [Read error: Connection reset by peer] 15:09 -!- seven_ [~seven@2a00:ee2:410c:1300:6428:b981:df56:c111] has joined #bitcoin-core-pr-reviews 15:29 < meshcollider> Very nice 15:30 < meshcollider> kallewoof, jnewbery: I won't be able to make it today to the asia-pacific review club because I have another meeting on, but I promise I'll help to host one in the coming weeks 15:31 < meshcollider> I have reviewed the PR already and the notes from this morning's group were very helpful 15:55 < amiti> jonatack: thanks! 16:00 -!- apaval_ [~apaval@103.140.137.9] has quit [Read error: Connection reset by peer] 16:00 -!- apaval [~apaval@103.140.137.9] has joined #bitcoin-core-pr-reviews 16:12 -!- emilengler [~emilengle@stratum0/entity/emilengler] has quit [Quit: Leaving] 16:31 < jnewbery> meshcollider: great. Thanks! 16:39 -!- gleb1 [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 16:42 -!- gleb1 [~gleb@cpe-67-244-100-77.nyc.res.rr.com] has quit [Client Quit] 16:43 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 16:43 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Remote host closed the connection] 17:34 -!- shaunsun [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has quit [Ping timeout: 256 seconds] 17:34 -!- shaunsun_ [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has quit [Ping timeout: 264 seconds] 17:40 -!- slivera_ [~slivera@217.138.204.72] has joined #bitcoin-core-pr-reviews 17:42 -!- slivera [~slivera@185.104.186.203] has quit [Ping timeout: 256 seconds] 17:46 -!- lightlike [~lightlike@p200300C7EF13EC002815F0B7956AB5C8.dip0.t-ipconnect.de] has quit [Ping timeout: 246 seconds] 19:52 -!- belcher [~belcher@unaffiliated/belcher] has quit [Quit: Leaving] 20:35 -!- felixfoertsch [~felixfoer@2001:16b8:5086:2300:f880:50b6:211b:3a40] has joined #bitcoin-core-pr-reviews 20:35 -!- felixfoertsch23 [~felixfoer@2001:16b8:50e1:8900:65d9:2970:69d3:ceb9] has quit [Ping timeout: 246 seconds] 21:02 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds] 21:12 < kallewoof> hi, sorry i'm late. i don't know if we will have a meeting today, as i was super busy. i think it would be fun to go through the notes from the review PR meeting, if other people are here and up for it (though i'm still working so a bit distracted) 21:13 < kallewoof> i know meshcollider is absent for this one, but not sure about others 21:13 < aj> *wave* 21:14 < aj> haven't looked at this one at all 21:19 < aj> jnewbery: s/invariable/invariant/ in the commit message though... 21:23 < meshcollider> Hi I actually made it back in time :) 21:23 < meshcollider> Looks like no meeting though 21:25 < kallewoof> Yeah, sorry I am right in the middle of a thing at work, so I can't lead this one :) I try to glance here though in case you do something 21:26 < aj> reading through the PR and the log from this morning, i'm agreeing with what they said... 21:27 < aj> breaking out the comment/scripteddiff into a separate pr from the logic changes maybe makes sense, DIRTY/FRESH is kinda confusing 21:28 < aj> and describing 3-or-4 valid states with 3 flags so that half of the possible combinations aren't even valid is kinda weird 21:31 < kallewoof> Concept ACK, for starters... still looking at code. 21:33 < fanquake> hi 21:33 < fanquake> sorry, I'm not really available at the moment 21:33 < fanquake> Also had 2 hours of build-system meeting late last night, so kinda meeting'd out hah 21:34 < fanquake> Was some good discussion if anyone is interested: http://gnusha.org/bitcoin-builds/2020-03-18.log 21:34 < kallewoof> nice 23:10 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-eocmmwsmzrsbaslx] has quit [Ping timeout: 256 seconds] 23:17 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-jtzbynqfgtrhnaly] has joined #bitcoin-core-pr-reviews 23:31 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-jtzbynqfgtrhnaly] has quit [Remote host closed the connection] 23:57 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-crzloldrvcwcmbym] has joined #bitcoin-core-pr-reviews