--- Day changed Wed Jan 22 2020 00:01 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 268 seconds] 00:11 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 01:41 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 01:43 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 03:03 -!- Laney38Tillman [~Laney38Ti@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 03:08 -!- Laney38Tillman [~Laney38Ti@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 265 seconds] 03:33 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 240 seconds] 03:37 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 04:09 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 246 seconds] 04:38 -!- jonatack [~jon@134.19.179.203] has joined #bitcoin-core-pr-reviews 04:47 -!- emilengler [~emilengle@unaffiliated/emilengler] has joined #bitcoin-core-pr-reviews 05:09 -!- slivera [slivera@gateway/vpn/privateinternetaccess/slivera] has joined #bitcoin-core-pr-reviews 05:29 -!- SirRichard [~MaxSikors@cpe-98-28-69-149.columbus.res.rr.com] has joined #bitcoin-core-pr-reviews 05:30 -!- slivera [slivera@gateway/vpn/privateinternetaccess/slivera] has quit [Remote host closed the connection] 05:34 -!- SirRichard [~MaxSikors@cpe-98-28-69-149.columbus.res.rr.com] has quit [Client Quit] 06:46 -!- michaelfolkson [~textual@host-92-6-97-247.as43234.net] has joined #bitcoin-core-pr-reviews 06:55 -!- michaelfolkson [~textual@host-92-6-97-247.as43234.net] has quit [Quit: Sleep mode] 06:56 -!- michaelfolkson [~textual@host-92-6-97-247.as43234.net] has joined #bitcoin-core-pr-reviews 07:43 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 07:59 -!- fox2p [~fox2p@cpe-66-108-32-173.nyc.res.rr.com] has quit [Ping timeout: 260 seconds] 08:02 -!- fox2p [~fox2p@cpe-66-108-32-173.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews 08:03 < jonatack> We'll get started in a little under 2 hours for this week's Review Club episode 08:12 -!- jonatack [~jon@134.19.179.203] has quit [Ping timeout: 268 seconds] 08:19 -!- rottensox [~rottensox@unaffiliated/rottensox] has quit [Quit: Bye] 08:34 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 258 seconds] 08:48 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews 08:58 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 09:05 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 09:05 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 09:09 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Client Quit] 09:09 < pinheadmz> Last week we were talking about code coverage -- I notice bitcoin isn't using Coveralls from Travis to report coverage on PRs - is there a reason why not? 09:10 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 09:18 < jonatack> curious what difftools everyone is using for reviewing... i've been using gitk (with dracula for dark mode) but wish i could switch from red/green to nicer syntax highlighting sometimes 09:20 < pinheadmz> jonatack: honestly on macos theres a piece of junk called filemerge that is the default for git difftool 09:21 < pinheadmz> i like it because i can scroll through the entire file (not just snippets like github) and i prefer side-by-side comaprison 09:22 < jonatack> we shouldn't be reviewing on GitHub anyway, only using for commenting 09:24 < jonatack> pinheadmz: interesting, i no longer use macOS but back then i was using things like opendiff and meld for macOS 09:25 < jonatack> i have a note-to-self blog post coming on about how to choose PRs to review 09:26 -!- gr0kchain [c5d7a71b@197.215.167.27] has joined #bitcoin-core-pr-reviews 09:27 < jonatack> which PRs we review with our limited time, of the 300+ open ones on the stack, is an important choice 09:28 < jonatack> and the trivial/newest/easy ones were receiving more attention (at least from me) than they would warrant 09:30 < jonatack> so i'm actively trying to pay less attention to new/easy/trivial ones in favor of high-prio, higher-value, harder ones 09:30 < pinheadmz> Looking forward to review club for Taproot :-) 09:30 < jonatack> we see really important PRs sometimes sitting for months or even years without enough reviewers 09:32 < jonatack> so the daily choice of what to review and what to ignore is maybe underappreciated... it was by me for the first months 09:33 < jonatack> i'm also actively trying to review PRs by people who also review PRs 09:33 < jonatack> and less by people who add PRs to the stack but don't review PRs or test issuens 09:34 < jonatack> heh, enough ranting, will make a blog post methinks 09:35 < jonatack> i mean, sure, coding is more fun than reviewing -- but reviewing is too vital here, and when i see trivial PRs i keep thinking, maybe too much, about the review resources they might be taking away from catching a regression or CVE 09:36 < jonatack> hopefully this review club can help encourage more reviewing but i'm thinking about how else we can encourage it, add incentives or social norms maybe, etc. 09:45 -!- lightlike [~lightlike@46.114.7.207] has joined #bitcoin-core-pr-reviews 09:53 < jnewbery> jonatack: those are good things to think about. I'm looking forward to reading that blog post! 09:54 < jnewbery> pinheadmz: I expect we'll do a series of review clubs on the schnorr/taproot PR. There's too much to cover in one meeting 09:56 < jonatack> jnewbery: +1 09:57 -!- gr0kchain [c5d7a71b@197.215.167.27] has quit [Remote host closed the connection] 09:59 < jonatack> FWIW I have begun building a personal website (as a sort of online resume for funding or employment) and migrating my articles here: https://jonatack.github.io/articles/ 09:59 < kanzure> 404 10:00 < jonatack> it's WIP and thanks to lisa neigut for helping with the styling 10:00 < jonatack> #startmeeting 10:00 < jnewbery> hi 10:00 < ajonas> hi 10:00 < pinheadmz> hi 10:00 < kanzure> hi 10:00 < lightlike> hi 10:00 < amiti> hi 10:00 -!- gr0kchain [c5d7a71b@197.215.167.27] has joined #bitcoin-core-pr-reviews 10:00 < jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club 10:00 < raj_> hi 10:00 < andrewtoth> hi 10:00 < michaelfolkson> hi 10:01 -!- van15 [18bf0fb0@ool-18bf0fb0.dyn.optonline.net] has joined #bitcoin-core-pr-reviews 10:01 < jonatack> topic Today we are looking at PR 17477, "Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals" (validation) 10:01 < fjahr> hi 10:01 < jonatack> We usually start Bitcoin Core IRC meetings with a 'hi' so it's clear who's at keyboard. Feel free to say hi, even if you arrive in the middle of the meeting! 10:01 < jonatack> Do jump in at any point with thoughts and questions. 10:01 < jonatack> I am personally here above all to learn -- and look forward to everyone sharing their thoughts. 10:01 < jonatack> Don't be shy! This discussion is about your thoughts and input. 10:02 < gr0kchain> hi 10:02 -!- van15 [18bf0fb0@ool-18bf0fb0.dyn.optonline.net] has left #bitcoin-core-pr-reviews [] 10:02 < jonatack> To start, everyone please give a quick y or n: did you have the chance to read the notes and questions for this meeting? 10:02 -!- phuvp [18bf0fb0@ool-18bf0fb0.dyn.optonline.net] has joined #bitcoin-core-pr-reviews 10:02 < raj_> y 10:02 < gr0kchain> n 10:02 < fjahr> y 10:02 < amiti> y 10:02 < lightlike> y 10:02 < andrewtoth> y 10:02 < jnewbery> y 10:03 < pinheadmz> y 10:03 < jonatack> Nice. Now y or n: did you have the chance to review the PR? 10:03 < gr0kchain> n 10:03 < raj_> y 10:03 < fjahr> y 10:04 < lightlike> y 10:04 < amiti> I started reviewing but am not done 10:04 < andrewtoth> n 10:04 < pinheadmz> yeah 10:04 -!- phuvp [18bf0fb0@ool-18bf0fb0.dyn.optonline.net] has quit [Remote host closed the connection] 10:04 < jnewbery> y 10:05 < jonatack> Great. raj_ fjahr lightlike pinheadmz: Concept ACK, approach ACK, ACK, or NACK? 10:05 < pinheadmz> "limited knowledge ACK" haha 10:05 < jonatack> (I think jnewbery is an ACK) 10:05 < pinheadmz> I dont know if this breaks anythign 10:06 < jonatack> amiti: (initial thoughts yay or nay?) 10:06 < raj_> Concept ACK. Ran ubit and functional test, standard tests passing. Havent tested manually. 10:06 < fjahr> still need to think about it a little more, 90% there to ACK 10:06 < jonatack> Would anyone like to describe what these mempool notifications do? 10:07 -!- SirRichard [~MaxSikors@cpe-98-28-69-149.columbus.res.rr.com] has joined #bitcoin-core-pr-reviews 10:07 < jonatack> As an aside, note that the PR author tags contributors for review in a separate comment and not in the PR description itself. 10:07 < lightlike> concept ack for me, not 100% sure about the subtle differences to the new handling of conflicted txs 10:07 < fjahr> The wallet needs information about its txs so it needs to be informed when the mempool changes 10:08 < jonatack> (anyone want to describe why?) 10:08 < raj_> They notify tx entry/removal events in mempool to subscribing instances. 10:08 < amiti> seems reasonable. tried to think through concrete examples of any issues from separating the notifications, but I can't come up with anything. 10:09 < andrewtoth> I believe the description becomes part of the commit description, so any upstream projects will notify the tagged users. Is that right? 10:09 < jonatack> amiti: saame, if anything it seems better with these changes, but i'd like to try to write a test to check that (not sure how feasible that would be) 10:09 < andrewtoth> *downstream 10:10 < jonatack> or at least go through the existing tests 10:10 < fjahr> andrewtoth: +1 10:10 < jonatack> existing test files: git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*' 10:10 < jonatack> andrewtoth: right, anyone tagged with get a bunch of notifs 10:10 < jonatack> will* 10:11 < jonatack> recently iiuc the merge script was updated to remove the @-tags 10:11 < pinheadmz> this PR removes code that is essentially dead since mempool notifiactions were intorduced 10:11 < jonatack> but it's a good practice to not tag in the PR description and in the commits 10:11 < pinheadmz> is there a rason why it wasnt pulled out by that PR? 10:12 < jonatack> pinheadmz: i'm guessing here but can imagine at least two good reasons: 10:12 < jnewbery> pinheadmz: which PR? 10:13 < jonatack> - smaller, more focused PRs are generally easier to review and have merged 10:13 < michaelfolkson> A recent example (non-related to this PR) of code that was assumed to be dead but wasn't.... https://github.com/bitcoin/bitcoin/pull/17965 10:13 < pinheadmz> jnewbery: 9371 for example 10:13 < jonatack> - anything involving validation needs a high degree of review and careful scrutiny 10:13 < pinheadmz> jonatack: makes sense 10:14 < jonatack> as maintaining consensus is the overarching mission of bitcoin core 10:14 < pinheadmz> it just means this unused vector was still getting written to memory for about 3 years of release :-) (IIUC) 10:14 < jnewbery> You're talking about the NotifyEntryAdded and NotifyEntryRemoved signals? 10:15 < pinheadmz> jnewbery: right, since those signals replace the vector you removed, they couldve been part of that PR? or at least a follow up after merge 10:15 < pinheadmz> but i may be missing other uses for it that kept it around until now 10:15 < jnewbery> NotifyEntryRemoved is used to add conflicted txs to the conflictedTxs vector in PerBlockConnectTrace 10:16 < jonatack> kanzure: thank you, the link had an extra / ... it's https://jonatack.github.io/articles (and WIP) 10:16 < jnewbery> that's changed by this PR17477 10:17 < pinheadmz> jnewbery: oh i see! so that is the last reference to the conflicted Txs vector 10:17 < jonatack> right, this code isn't dead per se 10:17 < pinheadmz> ty. 10:18 < jnewbery> pinheadmz: exactly. The first commit changes the way the wallet is notified about conflicted txs, and the rest of the commits are just removing the now-dead code 10:18 < jonatack> I liked how jnewbery structured the commits with step-by-step sequentially logical changes that made it much easier to review 10:19 < jonatack> The first commit is the most impactful one 10:19 < raj_> is there any process through which we can determine which parts of the code base are dead? or its just by experienced telling? 10:19 < jonatack> followed by almost mechanical changes that lead to the last commit removing the callbacks 10:19 < raj_> jonatack: +1, really loved the organization of this commit. made things much easier. 10:20 < jonatack> yes! 10:20 < pinheadmz> yes +1 for long commit messages and including the module name in [brackets] 10:20 < jonatack> imagine if it had been a single commit, and pinheadmz yes +1 on the messages 10:21 < jonatack> the commit messages are so clear that i didn't see what else to add to describing them in the notes 10:21 < jonatack> Anyone: What are conflicted transactions? 10:22 < pinheadmz> you can tell when an author spends a lot of time reviewing :-) "what would make this easier for everyone else to review?" 10:22 < michaelfolkson> I think the answer to raj_ question is it depends (on the supposed dead code). Sometimes just checking whether it is defined, initialized is enough? 10:22 < pinheadmz> conflicted TX can happen in the mempool for lots of reasons, easiest to consider is a TX in a block that double spends something in the mempool 10:22 < jonatack> pinheadmz: I agree, this is an underrated quality 10:22 < pinheadmz> but more interesting cases inolve premature coinbase spends when a block is UN condifmred 10:23 < pinheadmz> and relative timelocks 10:23 < jonatack> raj_: one dumb trick, other than git grepping for things, is to rename or remove a call site and see if building fails 10:24 < michaelfolkson> Call site? 10:24 < fjahr> pinheadmz: "when a block is UN condifmred" => I don't understand 10:24 < jonatack> raj_: this is what i did, for instance, with the boost signals include in my review... i removed it to see what would happen 10:24 < raj_> jonatack: nice trick, thnaks. 10:25 < jonatack> Is a double spend and a conflicted transaction the same thing? 10:25 < jnewbery> michaelfolkson: where the function is called from somewhere else 10:25 < michaelfolkson> fjahr Unconfirmed 10:26 < jonatack> For example, to try to see what code covers conflicted transactions and double spending: 10:26 < jonatack> git grep "double spend\|double-spend\|conflicted" -- 'src/' -- :^'*/qt/*' 10:27 < pinheadmz> fjahr: imagine spending a coinbase output after exactly 100 blocks. then there is a chain reorganiztion. the chain tip block becomes unconfirmed and in that moment, the TX is invalid 10:27 < jonatack> and to try to see the Mempool policy on double-spending, try: git grep -B4 -A3 "transaction that double-spends" 10:28 < fjahr> Ok, I have never thought of that as an unconfirmed block, thanks! 10:28 < lightlike> yesterday i got confused by the splitup to #17562. After the split, there is still a comment in GetBlocksConnected() mentioning the (now removed) conflicted txes vector, which will be addressed only in the follow-up. So maybe it would make sense to mention the follow-up in the description (or change the comment). 10:28 < jnewbery> pinheadmz: I dno't think I'd refer to that as 'unconfirmed'. By unconfirmed, we really mean something that can't be included in the blockchain because its ancestors have been spent somehow 10:28 < pinheadmz> jnewbery: "disconnected" ? 10:28 < jonatack> lightlike: thanks, where exactly? 10:29 < jnewbery> in your example, the transaction can be included later because it's still valid and its inputs haven't been spent 10:29 < fjahr> I would have said "re-orged out block" or something I guess 10:29 < pinheadmz> jnewbery: yeah true, but woudlnt that fire the removeed signel anyway? in my example, i imageine the coinbase spend is till in the mempool, witing for confirmation in the next block 10:30 < jnewbery> pinheadmz: I don't know if we have a name. When there's a reorg we try to put the transactions from that block back in the mempool, but as you've pointed out with the coinbase maturity example, that's not always possible 10:30 < lightlike> jonatack: line 2499, https://github.com/bitcoin/bitcoin/pull/17477/files#diff-24efdb00bfbe56b140fb006b562cc70bL2513 if that link works. 10:30 < jnewbery> pinheadmz: no, because the tx isn't in the mempool. The signal is fired when a tx is removed from the mempool 10:30 < jnewbery> pinheadmz: oh right. Sorry, I misunderstood. Yes, the signal gets fired 10:31 < jnewbery> I think the reason given is REORG rather than CONFLICTED in this case 10:31 < pinheadmz> jnewbery: ah yea 10:32 < pinheadmz> jnewbery: and in the coinbase spend case, if there's a reorg - the tx will likely be valid after the reorg is done. (the chain height will be equal or greater than it was when the tx was first broadcast) but still in the midst of the reorg process, it will be evicted from the mempool (IIUC) 10:32 < jnewbery> pinheadmz: here: https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/validation.cpp#L371 10:33 < jnewbery> pinheadmz: I think we make some attempt to put those transactions back into the mempool after the re-org, but I can't remember the details 10:34 < jnewbery> (described here: https://github.com/bitcoin/bitcoin/blob/631df3ee87ec93e1fc748715671cdb5cff7308e6/src/txmempool.h#L766) 10:35 < pinheadmz> jnewbery: very cool ty 10:35 < jonatack> txmempool.h is well-commented, kudos to the contributors who did that 10:36 < michaelfolkson> git blame 10:38 < jnewbery> I think it's quite important to distinguish between what we mean by MemPoolRemovalReason::CONFLICT and what the wallet means by conflicted transactions. Can anyone try to explain that? 10:38 < jonatack> lightlike: the comment beginning with "// We always keep one extra block at the end of our list because"? 10:41 < jnewbery> ok, maybe simpler question: what does the wallet mean by a conflicted tx? Why does a wallet care about tracking these? 10:41 < lightlike> jonatack: yes, blocks are no longer "added after all the conflicted transactions have been filled in." after this PR. 10:41 < pinheadmz> the wallet should notify the user if they are being double spent against 10:41 < michaelfolkson> A conflict with a transaction in newly connected block versus another in the mempool? 10:41 < raj_> conflicted tx: two tx trying to spend same inputs. 10:42 < jonatack> jnewbery: in AddToWalletIfInvolvingMe? 10:42 -!- rottensox [~rottensox@unaffiliated/rottensox] has joined #bitcoin-core-pr-reviews 10:42 < jnewbery> raj_: it's usually that, but imagine if we have a chain of transactions A->B1->C and then B2 double spends B1. Is C a conflicted transaction? 10:43 < raj_> i would guess B1 would be the conflicted tx here. 10:43 < amiti> I think.. in the wallet you could mark a txn as `abandoned` because its no longer in your mempool and you think its not going to be mined, but some miner or mempool kept it around and it later gets mined, so then you'd update to `conflicted` 10:44 < amiti> but in the mempool, you're not really storing a history of txns, so conflicts are amongst the current set 10:44 < jnewbery> C is also conflicted. Usually the case is that a transaction's inputs are double spent, but it could be the transaction's ancestor's inputs that are double spent 10:44 < jnewbery> amiti: 'abandoned' is a slightly different thing. It just means that the wallet isn't going to continue trying to resubmit the tx 10:45 < raj_> oh okay. so conflicted tx can be a full chain of such txs. 10:45 < jonatack> lightlike: i see your point that it should be removed in 17477 10:46 < jnewbery> raj_: exactly. Imagine Alice pays Bob pays me, and then the payment from Alice to Bob is double-spent. I was notified of the payment from Bob to me when it entered the mempool, but it can never confirm. I need to mark that tx as conflicted 10:47 < jnewbery> So the wallet wants to know if a transaction that it is tracking can never be confirmed. That's why it has a concept of conflicted 10:47 < jnewbery> what about MemPoolRemovalReason::CONFLICT? 10:48 < jonatack> jnewbery: ah, in AvailableCoins 10:48 < jonatack> // It's possible for these to be conflicted via ancestors which we may never be able to detect 10:48 < jnewbery> jonatack: yes! 10:49 < jnewbery> you can imagine an arbitrarily long chain of unconfirmed txs that ends in a payment to you, and then something in that chain is double-spent 10:49 -!- gr0kchain [c5d7a71b@197.215.167.27] has quit [Remote host closed the connection] 10:49 < raj_> `CONFLICT, //!< Removed for conflict with in-block transaction`, so that means if only a tx conflicts with another that is already in a block? 10:49 < jnewbery> (in practice the mempool descendant limit is 25, but that's still a long chain) 10:50 < jnewbery> raj_: yes. MemPoolRemovalReason::CONFLICT is the reason given to a transaction being removed from the mempool when a block connects because the tx's inputs or one of the tx's ancestor's inputs is spent by a different transaction included in the block 10:51 < jonatack> raj_: yes: git grep -A7 "enum class MemPoolRemovalReason" 10:51 < jnewbery> so this notification from the mempool to the wallet o fMemPoolRemovalReason::CONFLICT is one way for the wallet to learn about conflicted transactions, but it doesn't capture everything the wallet needs to know to mark its transactions as conflicted 10:52 < jnewbery> it's a little bit subtle 10:53 < raj_> just to see if i got it. The wallet would also like to know about in-mempool conflicts, but this signal is only for in-block conflicts. is that correct? 10:53 < jonatack> jnewbery: sounds like more code docs on this that would show up in trivial git grepping might be sweet 10:54 < jnewbery> raj_: almost. The wallet wants to know about arbitrarily deep conflicts, even when the transaction is no longer in the mempool. The mempool can only tell the wallet about txs that are in the mempool at the point the block is connected 10:55 < jonatack> all: note that this is touched on in the notes https://bitcoincore.reviews/17477 :) 10:56 < raj_> any idea how big such tx chains usally be in practise? 10:56 < jonatack> 3 minutes! 10:57 < jonatack> jnewbery: great distinction to make, ty 10:57 < jonatack> any last questions or comments? 10:57 < lightlike> I’m sure that this is unrelated to this PR, but did anyone else experience non-reproducible strange output of program code to the console when running the unit tests via test_bitcoin, like “pindexNew->GetBlockHash()” or “m_expected_tip” even though all tests succeed? 10:58 < jonatack> lightlike: not in my case 10:58 < pinheadmz> im a bit curious about boost signals in general, might be out of scope or we can talk after the meeting. Im familiar with libuv "events" in nodejs and just wonder how comprable it is 10:58 < jonatack> lightlike: running the whole suite or single files? 10:58 < fjahr> So, for a short window the tx is marked not in mempool but also not in a block yet. What if there are high fees, a full mempool, and we do some automatical RBFing, so we replace a tx with a higher fee one if our original is evicted from the Mempool because of low fees. We might try to RBF again although it is already in the block. That's the only scenario I could come up with and I did not have time to 10:58 < fjahr> think it through yet. 10:58 < lightlike> jonatack: the whole suite, but via test_bitcoin (not make check) 10:59 < jonatack> lightlike: right, running just test_bitcoin or src/test/test_bitcoin? sometimes i've thought i'm seeing strange behavior there but i'm not sure yet 10:59 < fjahr> * that I could come up with concerning that window question 11:00 < jonatack> fjahr: I think it would be very interesting to go through the current test coverage on these scenarii and add any missing 11:01 < lightlike> jonatack: src/test/test_bitcoin - ran it like 10 times and got varying strange output in 50% of the runs 11:01 < jonatack> tests that cover conflicted transactions and double spending: git grep "double spend\|double-spend\|conflicted" -- 'test' -- 'src/test/*' 11:01 < jonatack> let's wrap up 11:01 < jonatack> feel free to continue after! 11:01 < jonatack> #action review PR 17477 on GitHub 11:01 < fjahr> jonatack: yeah, but race conditions like this will always be somewhat flakey :/ 11:01 < jonatack> and the follow-up PR 11:02 < raj_> thanks. Very nice pr. 11:02 < andrewtoth> thanks! 11:02 < lightlike> thanks jonatack and jnewbery! 11:02 < jonatack> fjahr: sure. maybe worth trying to make it work and reliable though. 11:02 < jonatack> thanks all for coming, thank you jnewbery for your insights 11:02 < jonatack> #endmeeting 11:02 < fjahr> thanks! 11:03 < amiti> fjahr: do you know, what would happen if we tried to RBF but txn was already in a block 11:03 < jonatack> if anyone would like to host a meeting, feel free to come forth 11:04 < jonatack> for instance, michaelfolkson +1 11:04 < michaelfolkson> One day ;) 11:04 < jonatack> TBH, I learn waay more when hosting 11:05 < michaelfolkson> amiti: What do you mean what would happen? It wouldn't get into a block unless there was a re-org. And it would leave our mempool too unless there was a re-org? 11:06 < fjahr> amiti: no, I did look into it further because I had the idea 20min before the meeting. I think there would be no real consequences, I was just thinking if anythink could go wrong after the change, even if consequences are insignificant 11:10 < amiti> fjahr: yeah, totally. I'm still trying to think through how the eviction and conflict and RBF could play together to create the scenario 11:11 < amiti> michaelfolkson: I don't follow your question 11:14 < jonatack> fjahr: amiti: agree, i think going through the existing tests on this topic and noodling with adding any that seem missing could be time well-spent 11:14 < jonatack> to think through the cases 11:20 < michaelfolkson> amiti: It's ok. I don't think I understood your question :) 11:38 -!- michaelfolkson [~textual@host-92-6-97-247.as43234.net] has quit [Quit: Sleep mode] 11:56 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 11:56 -!- emilengler [~emilengle@unaffiliated/emilengler] has quit [Quit: Leaving] 12:00 -!- diogosergio [~diogoserg@0545f855.skybroadband.com] has joined #bitcoin-core-pr-reviews 12:05 -!- diogosergio [~diogoserg@0545f855.skybroadband.com] has quit [Ping timeout: 240 seconds] 12:38 < jonatack> Log from today's meeting is now up: https://bitcoincore.reviews/17477#meeting-log 12:52 -!- lightlike [~lightlike@46.114.7.207] has quit [Quit: Leaving] 13:41 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 13:42 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 14:02 -!- diogosergio [~diogoserg@0545f855.skybroadband.com] has joined #bitcoin-core-pr-reviews 14:05 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 14:05 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 14:07 -!- diogosergio [~diogoserg@0545f855.skybroadband.com] has quit [Ping timeout: 272 seconds] 14:09 -!- SirRichard [~MaxSikors@cpe-98-28-69-149.columbus.res.rr.com] has quit [Quit: SirRichard] 14:49 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Ping timeout: 260 seconds] 15:19 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews 15:27 -!- michaelfolkson [~textual@92.6.97.247] has joined #bitcoin-core-pr-reviews 15:27 -!- michaelfolkson [~textual@92.6.97.247] has quit [Client Quit] 15:50 < fjahr> If people still plan to give ACKs, don't get confused like me: The commits are displayed in the wrong order on Github, probably caused by use of rebase. I knew this existed but haven't stumbled over it in long time. 15:50 -!- dr-orlovsky [~dr-orlovs@194.230.155.171] has quit [Ping timeout: 268 seconds] 15:51 -!- orlovsky [~dr-orlovs@194.230.155.171] has joined #bitcoin-core-pr-reviews 15:53 < fanquake> fjahr: It's not due to rebasing, it's due to how GitHub displays commits. 15:53 < aj> fjahr: github displays commits ordered by their timestamp, it's crazy 15:54 < fanquake> Yah. Been around for ages: https://github.com/isaacs/github/issues/386 15:56 < jonatack> fjahr: Best to never copy the commit hash from GH anyway, just use the HEAD commit hash on the local PR branch 15:56 < jonatack> safer and that way you don't use the wrong one :) 15:57 < fjahr> ok, yeah, it can happen through other ways than rebasing, that's just what I mostly use to reorder commits 16:00 < jonatack> Yeah, for reviewing the only thing to use GitHub for is the metadata, e.g. reading and writing comments... All the rest locally. 16:02 -!- michaelfolkson [~textual@host-92-6-97-247.as43234.net] has joined #bitcoin-core-pr-reviews 16:02 -!- michaelfolkson [~textual@host-92-6-97-247.as43234.net] has quit [Client Quit] 16:09 < fjahr> @jonatack: what tools do you currently use for viewing diffs locally? 16:22 < fjahr> or anyone else too, I have tried a lot of tools but none of them made me really happy and I am jumping back into github often to look at the diffs because I am so used to them. 16:23 < aj> i like gitk 16:23 < fjahr> yeah, I guess i need to work on a .gitk file that works for me 16:23 < aj> or else just 'git diff' and '--word-diff' and the like 16:25 < fjahr> I use git diff quite a lot but still jump back into github during reviews somehow 16:26 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection] 16:28 < fanquake> fjahr: I mostly use git diff. Sometimes Sublime Merge. 16:43 < jonatack> like aj, gitk with dracula for me (dark mode) 16:44 < jonatack> saame, git diff too 16:50 -!- slivera [slivera@gateway/vpn/privateinternetaccess/slivera] has joined #bitcoin-core-pr-reviews 16:54 < jonatack> git show HEAD~X etc 16:55 < jonatack> git show --color-moved=dimmed-zebra and so on 17:00 < aj> i like "git rebase -i $(git merge-base HEAD origin/master)" change all the "pick"s to "edit" then "make -j12 && make -j5 check && (cd ../test/functional; ./test_runner.py) && git rebase --continue # to check each commit when i'm feeling patient too 17:01 < jonatack> oooooh 19:09 -!- davterra [~dulyNoded@195.206.105.106] has quit [Ping timeout: 260 seconds] 19:54 -!- felixfoertsch [~felixfoer@92.117.41.97] has joined #bitcoin-core-pr-reviews 19:56 -!- felixfoertsch23 [~felixfoer@92.117.40.164] has quit [Ping timeout: 268 seconds] 23:15 -!- rottensox [~rottensox@unaffiliated/rottensox] has quit [Ping timeout: 260 seconds] 23:20 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 23:23 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 23:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 23:43 -!- rottensox [~rottensox@unaffiliated/rottensox] has joined #bitcoin-core-pr-reviews 23:43 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 23:58 -!- Netsplit *.net <-> *.split quits: kcud_dab, icota[m], ryanofsky, wumpus, provoostenator 23:58 -!- Netsplit *.net <-> *.split quits: nehan_, emzy, nothingmuch, raj_