--- Log opened Wed Aug 24 00:00:59 2022
00:12 -!- o3 [uid554669@id-554669.uxbridge.irccloud.com] has joined #bitcoin-core-pr-reviews
00:21 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
00:24 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Read error: Connection reset by peer]
00:39 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
00:43 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 255 seconds]
00:54 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
00:58 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
01:00 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Read error: Connection reset by peer]
01:16 -!- ghost43_ [~ghost43@gateway/tor-sasl/ghost43] has quit [Ping timeout: 268 seconds]
01:17 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
01:34 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
01:39 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 268 seconds]
02:09 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
02:11 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Read error: Connection reset by peer]
02:38 -!- raj [~raj@167.182.81.172.lunanode-rdns.com] has joined #bitcoin-core-pr-reviews
03:01 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
03:03 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Read error: Connection reset by peer]
04:46 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
04:51 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 244 seconds]
05:04 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
05:09 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 268 seconds]
05:37 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
05:41 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 252 seconds]
05:54 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
05:59 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 255 seconds]
06:14 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
06:18 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 260 seconds]
06:29 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:35a2:9e12:9985:3788] has joined #bitcoin-core-pr-reviews
08:20 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
08:50 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Remote host closed the connection]
08:50 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
08:58 -!- ubbabeck [uid566434@id-566434.tinside.irccloud.com] has joined #bitcoin-core-pr-reviews
09:07 -!- ubbabeck [uid566434@id-566434.tinside.irccloud.com] has left #bitcoin-core-pr-reviews []
09:11 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Remote host closed the connection]
09:13 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
09:17 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Read error: Connection reset by peer]
09:17 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
09:22 -!- OxBEEFCAF3 [~OxBEEFCAF@c-73-149-241-246.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews
09:32 -!- kevkevin [~kevkevin@66.219.242.102] has joined #bitcoin-core-pr-reviews
09:40 -!- ubbabeck [uid566434@id-566434.tinside.irccloud.com] has joined #bitcoin-core-pr-reviews
09:43 < kevkevin> Hey guys here for the reivew club today :) will mostly be lurking still getting used to the repo
09:50 < Zaidan> kev: Same here, welcome
09:53 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:53 -!- juancama [~juancama@pool-74-96-218-208.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
09:55 -!- hernanmarino [~hernanmar@190.16.216.111] has joined #bitcoin-core-pr-reviews
09:58 -!- BlueMoon [~BlueMoon@dgb3.dgbiblio.unam.mx] has joined #bitcoin-core-pr-reviews
09:59 -!- pablomartin [~pablomart@45.149.175.162] has joined #bitcoin-core-pr-reviews
10:00 < glozow> #startmeeting
10:00 < stickies-v> hi
10:00 < glozow> hi everyone! if you're looking for Bitcoin Core PR Review Club, you're in the right place. feel free to say hi :)
10:00 < pablomartin> hello!
10:00 < raj> hi.. 
10:00 < brunoerg> hi 
10:00 < BlueMoon> Hello
10:00 < hernanmarino> Hi 
10:01 < larryruane_> hi
10:01 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
10:01 < glozow> we're looking at a wallet PR today, "Properly rebroadcast unconfirmed transaction chains." Notes in the usual place https://bitcoincore.reviews/25768
10:01 < kevkevin> hi
10:01 < effexzi> Hi every1 
10:01 < ishaanam[m]> hi
10:01 < lightlike> hi
10:01 < glozow> Did everybody get a chance to review the PR and/or look at the notes?
10:01 < juancama> hi
10:02 < hernanmarino> yes
10:02 < raj> y
10:02 < brunoerg> y
10:02 < juancama> y
10:02 -!- mando [~mando@p5490620d.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews
10:02 < stickies-v> y
10:02 < kevkevin> y
10:02 < ishaanam[m]> y
10:02 < glozow> wonderful! for those of you who reviewed the PR, what was your review approach?
10:03 < pablomartin> I have reviewed the code, compiled it but still need to do some testing
10:03 < stickies-v> so far just code review and understanding what the functional test was trying to catch
10:03 < raj> Read through the code.. Makes sense of the logic.. Compile it.. Run the test.. Try to understand if testing covering the situation intended..
10:04 < kevkevin> Reviewd the code ran the functional tests but still need to do some manualy testing, still understanding the repo aswell as im still new here
10:04 < raj> Trying to recreate the bug would be cool.. But I am guessing theres no easy way to ensure child before parents in `mapWallet`?
10:04 < ishaanam[m]> I mainly looked at the first commit for any changes in behavior and made sure the added test fails on master sometimes
10:04 < hernanmarino> tested succesfully and read the code. I still have to dive deeply in the test code to think about alternatives and perhaps improvements
10:05 < larryruane_> I'm running the functional test (wallet_resendwallettransactions.py) in the debugger, and watching the node's `debug.log` file ... but I haven't really got to the new test code that this PR adds, still trying to understand the first (existing) part of the test
10:06 < glozow> great! glad to hear people are using the test to reproduce the issue. so what exactly is the incorrect behavior this PR addresses?
10:06 < larryruane_> It's amazing what the functional test framework can do, lots to learn just about that!
10:06 < larryruane_> (things like how the functional test sets itself up as a peer, for example)
10:08 < raj> I am guessing its about the situation where a child appears in `mapWallet` before its parent, and thus at the time of rebroadcasting the child would get rejected in mempoolcheck if mempool doesn't have the parent..  
10:08 < stickies-v> `ResendWalletTransactions()` rebroadcasts transactions based on txid instead of their sequential order, which leads to child transactions not being broadcast if their parents txid is higher
10:08 < larryruane_> I think the problem is that if we (our node) broadcasts 2 transactions, A and B, and let's say B is a child of A (B spends an output of A), then if we happen to broadcast tx B first, then A, then our peers won't accept B into their mempool because it's not valid
10:08 -!- Amirreza [~Amirreza@5.200.118.126] has joined #bitcoin-core-pr-reviews
10:08 < Zaidan> ResendWalletTranactions will fail to rebroadcast child transactions if their txids happen to be lexicographically less than their parent's txid (copy and paste)
10:09 -!- Amirreza [~Amirreza@5.200.118.126] has quit [Remote host closed the connection]
10:09 < Zaidan> and man is this ever a subtle bug wow
10:09 < achow101> The lexicographically part is no longer true as mapWallet is now an std::unsorted_map instead of a normal std::map
10:09 < juancama> 50% of the time the child is placed before the parent in mapWallet
10:09 < achow101> now it's basically just random of which will appear first
10:09 < glozow> raj: stickies-v: larryruane_: Zaidan: good answers! indeed, the problem is rebroadcasting children before parents. the order is not "by txid" or "lexicographical" though, as it's a `std::unordered_map`
10:10 < glozow> thanks achow101
10:10 < larryruane_> in general, is there a long-term effort to replace `std::map`s with unordered maps where possible? aren't the latter considered more efficient?
10:11 < larryruane_> (i'm not sure if that's space or time, or both)
10:11 < sipa> they're a tiny bit more memory efficient, but not much
10:11 < achow101> larryruane_: for me, yes. I've been trying to use unsorted_map and unsorted_set where possible
10:12 < glozow> larryruane_: i think it depends on the use case. and same here, I tend to use unsorted when possible
10:12 < sipa> for sufficiently large size, unordered_map is faster, due to O(1) lookups, but they're also more complicated (you need a hash function, possibly a salted one depending on whether it can be under attacker influence) rather than just a comparator
10:14 < glozow> good thing we have a `SaltedTxidHasher`
10:14 < glozow> I think we've just answered the next question, "Why is it difficult to write a test that reliably demonstrates that transactions may be rebroadcast in the wrong order?"
10:14 < glozow> i.e. because it's implemented as a hashmap. can't really guarantee which bucket keys are going to be in
10:15 < Zaidan> Don't answer this if it is out of scope I'm new: but why are these UTXO's transactions all seperate objects, and not wrapped up in a tree structure where their depth order can be easily maintained and referenced? It seems maintaining tx order is a pain.
10:15 < raj> Because its not possible to enforce a child will always appear before the parent in the mapWallet..  
10:15 < larryruane_> because the test can't control all the randomness that it would need to in order to make the unordered_map iteration order predictable?
10:15 < sipa> Zaidan: the transaction graph is generally a DAG, not just a tree
10:15 < pablomartin> glozow: yeah perhaps we could force the rebroadcast in wrong order
10:16 < sipa> plus, we do need a way to access individual transactions by txid
10:16 < sipa> so just keeping them in a tree structure wouldn't work
10:16 < larryruane_> I'm not even sure, is `std::unordered_map` iteration deterministic for exactly the same inputs?
10:16 < glozow> i did wonder if we're going to end up with a boost multi index container rewrite for map wallet, heh
10:16 < Zaidan> sipa: ah ty
10:16 < sipa> larryruane_: no, depends on insertion order, even if the hash function is the same
10:17 < larryruane_> ok yes but I meant if insertion order is the same
10:17 < sipa> (and you don't want identical hash functions)
10:17 < glozow> next question is about the functional test: Does the test added in the PR adequately test this behavior, i.e., does it succeed with this fix but fail without it?
10:17 < raj> Isn't there any way to manually enforce the wallet to take to store the the child tx before the parent tx? 
10:18 < raj> *manually enforce the wallet to stor
10:18 < sipa> raj: iteration order is determined by the map structure (std::map, or std::unordered_map), you can't control it.
10:19 < raj> Ah.. Okay.. 
10:19 < glozow> raj: no, and I don't think it's necessary
10:19 < hernanmarino> glozow: didn't test on master, but I'm sure it fails randomnly
10:19 < sipa> with a boost::multiindex you could have multiple indexes (one insertion ordered, another txid-based) simultaneously... but indeed I don't think that's useful.
10:20 < stickies-v> after `git revert 521169f2428be8e78599aa4fcb96f7ada7bb7e04` (and recompiling) the functional test does indeed fail again
10:20 < sipa> raj: Point is that the purpose of this data structure is lookup things based on txid - the iteration order follows that structure 
10:20 < ishaanam[m]> On master the test fails around 50% of the time for me, though it would be nice if we had a higher chance of failing without the fix. I think glozow left a comment on the PR that would help with this?
10:20 < larryruane_> glozow: i didn't actually try it, but there's a great comment added to the functional test that explains that the new test will fail only about half of the time
10:21 < glozow> stickies-v: ishannam: wonderful, ⭐ for you
10:21 < raj> I am seeing test failures too in the new test.. And I am not sure why that should happen.. 
10:21 < glozow> raj: hernanmarino: you're both seeing the test fail *with* the PR's changes?
10:21 < stickies-v> I don't think the new test is supposed to fail at all? without the fix it's meant to fail 50% of the time, I think
10:21 < raj> Yes.. Though I need to double check.. 
10:21 < larryruane_> I have a question, would it be possible to run this entire new part of the functional test a few times (within a single run of the functional test)? To prevent CI from passing when it shouldn't?
10:21 < glozow> yes the new test should pass every time with the changes
10:22 < pablomartin> stickies-v +1
10:22 < glozow> larryruane_: yes, that should be pretty simple. just do 10 parent-child pairs.
10:22 < larryruane_> I'm just imagining that a future change breaks this, and the dev runs the test suite (or CI does), and it passes, so the dev thinks it's okay
10:22 < larryruane_> glozow: +1
10:23 < lightlike> I wonder if that's actually 50%. I guess it's not a good idea to build a RNG based on the order of an unordered map...
10:23 < glozow> ok if you're seeing the test fail with the PR changes, that shouldn't happen, so let us know where it's failing...
10:23 < glozow> i'm going to continue with the questions
10:23 < glozow> Can you think of any other methods of getting the wallet transactions sorted in this order?
10:24 < glozow> Would it make sense to keep the wallet transactions sorted in this order?
10:24 < Zaidan> Wouldn't we want to depth sort?
10:24 < hernanmarino> glozow: No , within the PR it always succeeds
10:24 < glozow> hernanmarino: ah okay! that's good
10:25 < stickies-v> with std::sort you just need to allocate an additional vector instead of an additional vector and a map, but there's probably some downside to it I don't understand?
10:26 < achow101> Zaidan: depth stops being relevant pretty quickly
10:27 < Zaidan> achow101: okay ty
10:27 -!- Paul_C [~Paul_C@pool-74-96-218-208.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
10:28 < glozow> stickies-v: indeed, I think this could perhaps just be a std::sort where the comparison function uses `nOrderPos`
10:28 < stickies-v> yeah it makes for quite a bit less code: https://pastebin.com/1Hxp4YRM
10:30 -!- Paul_C [~Paul_C@pool-74-96-218-208.washdc.fios.verizon.net] has quit [Client Quit]
10:30 < larryruane_> stickies-v: +1 seems like a good idea!
10:31 < larryruane_> Just to confirm, `nOrderPos` is the order transactions entered the wallet, and that's guaranteed to be "parent before child"
10:31 -!- Paul_C [~Paul_C@pool-74-96-218-208.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
10:32 < larryruane_> it's a 64-bit integer so we probably assume it will never wrap around
10:32 < glozow> larryruane_: AFAIK yes, parents always inserted before children. I suppose more precise would be if they cached their ancestor counts or something, but can't imagine why we wouldn't insert a parent before child.
10:32 < glozow> well, 2^64 transactions would be a lot of transactions...
10:32 -!- OxBEEFCAF3 [~OxBEEFCAF@c-73-149-241-246.hsd1.ma.comcast.net] has quit [Ping timeout: 260 seconds]
10:33 < achow101> larryruane_: I think it is a reasonable assumption. I believe it is possible to mess with it using importprunedfunds though, however that only works on confirmed txs anyways
10:33 < sipa> we're not even at 2^30 transactions on-chain
10:34 < larryruane_> yes I'm not saying there's anything wrong, just understanding why it's okay :)
10:34 < glozow> ok. I also wonder if filtering should happen before sorting
10:36 < glozow> i.e. `transactions.filter(should_rebroadcast).sort(insertion_order)`
10:36 < larryruane_> glozow: I was wondering about that, `mapWallet` contains confirmed transactions, right? so it could be pretty large (so we're adding many entries to the temporary map in `GetSortedTxs()`)?
10:37 < larryruane_> (and there would be a large `out` vector that's returned?)
10:38 < ishaanam[m]> glozow: yeah, it looks like the main reason that the filtering is done after is because both of the functions that call GetSortedTxs() do different kinds of filtering? why is that?
10:38 < glozow> indeed. I'll say the inefficiency is probably not noticeable to users since this is only happing once a day, and there *usually* aren't that many transactions in mapWallet, but yes.
10:39 < glozow> ishaanam: good question, now is a good time to ask: What's the difference between `ResendWalletTransactions` and `ReacceptWalletTransactions`?
10:41 < Zaidan> Reaccept has to have a lock on the wallet
10:42 < stickies-v> in terms of usage: resend is triggered ~once per day, reaccept is used every time a wallet is loaded
10:42 < raj> `Resend` sends all the unconfirmed tx 5 mins before the last known block.. `ReAccept` sends all the wallet transaction unconfirmed, not coinbase and not abandoned.  
10:42 < lightlike> The filters in ReacceptWalletTransactions() seem unnecessary, since SubmitTxMemoryPoolAndRelay() seens to do the same checks again.
10:42 < glozow> stickies-v: yes, thank you
10:42 < ishaanam[m]> Zaidan: I think that Resend also obtains a lock on the wallet at some point?
10:42 < glozow> lightlike: and so does `BroadcastTransaction()`, haha
10:42 < sipa> Reaccept is loading mempool transactions into the wallet. Rebroadcast is dumping wallet transactions into the mempool.
10:43 < sipa> I'm wrong, ignore me.
10:43 < achow101> they both do ~same thing. I'm still trying to figure whether we can de-duplicate here, but both of these functions have existed for very long time and git blaming to figure out why is difficult.
10:44 < Zaidan> Resend is when we think transactions should have been included in a block
10:45 < Zaidan> ishaanam: yes, i see that now
10:45 -!- Paul_C [~Paul_C@pool-74-96-218-208.washdc.fios.verizon.net] has quit [Quit: Connection closed]
10:45 < glozow> Zaidan: right. rebroadcasting is done automatically, for transactions we're waiting to get confirmed but they haven't yet. reaccept is triggered once, at the start.
10:45 -!- b_101 [~b_101@189.236.23.182] has joined #bitcoin-core-pr-reviews
10:45 < glozow> achow101: ideally we get rid of resend from the wallet.
10:45 < stickies-v> I'm curious why for Reaccept we don't also check if we're in IBD or reindex, because that would raise the same concerns as mentioned for Resend?
10:46 -!- Paul_C [~Paul_C@pool-74-96-218-208.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
10:46 < pablomartin> stickies: i was thinking the same
10:47 < pablomartin> isReadyToBroadcast()
10:47 -!- OxBEEFCAF3 [~OxBEEFCAF@mobile-107-77-226-170.mobile.att.net] has joined #bitcoin-core-pr-reviews
10:47 -!- Paul_C [~Paul_C@pool-74-96-218-208.washdc.fios.verizon.net] has quit [Client Quit]
10:48 < glozow> stickies-v: I don't know the answer tbh
10:48 < achow101> ah, right. so these functions date back to the first commit...
10:49 < Zaidan> stickies-v: Does the lock handle that?
10:49 < Zaidan> Can the wallet be locked before IBD or reindex is completed?
10:50 < larryruane_> Zaidan: a lock would never be held for that long (im pretty sure)
10:51 < glozow> achow101: woah yeah
10:51 < glozow> we'll need to ask satoshi why it's this way
10:51 < larryruane_> "date back to the first commit" -- let's just ask satoshi!
10:51 < hernanmarino> :)
10:51 < larryruane_> glozow: sorry .. you beat me to it!
10:52 < stickies-v> larryruane_: +1, afaik we always try to minimize lock usage to just the bare minimum needed to avoid race conditions?
10:52 < glozow> Zaidan: assuming you're talking about `cs_wallet`, the wallet lock doesn't have anything to do with the node's state. it's just a object used to make sure multiple threads don't try to access wallet members at the same time.
10:53 < pablomartin> glozow: achow101: is the resend being used? i see it checks fBroadcastTransactions which is set to false in wallet.h and I don't see it's changed anywhere else
10:53 < Zaidan> ah ty
10:54 < glozow> good questions everybody. I'll throw out a few more from the review club notes. The test calls both setmocktime and mockscheduler to trigger a rebroadcast. What is the difference between these two calls, and why or why not is it necessary to call both of them?
10:54 < achow101> pablomartin: fBroadcastTransactions depends on startup options
10:54 < pablomartin> achow101: I see, thanks!
10:55 < glozow> pablomartin: see `DEFAULT_WALLETBROADCAST` https://github.com/bitcoin/bitcoin/blob/1420547ec30a24fc82ba3ae5ac18374e8e5af5e5/src/wallet/wallet.h#L106
10:56 < glozow> https://github.com/bitcoin/bitcoin/blob/1420547ec30a24fc82ba3ae5ac18374e8e5af5e5/src/wallet/wallet.cpp#L3018
10:56 < Zaidan> it seems the scheduler runs the time
10:57 < pablomartin> glozow: thank you! I see then it's passed to SetBroadcastTransactions
10:57 < glozow> for reference, I'm talking about this part of the test: https://github.com/bitcoin-core-review-club/bitcoin/commit/e40ddf36bed81bdf28d386eb961c9ed22b69e207#diff-2dd85d481900d4ad19d113d2114861b0134bcd283435e95b18d10adf5ad381a0R111-R113
10:58 < stickies-v> `ResendWalletTransactions()` is exclusively called from the scheduler (every second). I suppose we need to mock the scheduler because otherwise it'd use system time instead of mocked time?
10:58 < Zaidan> mocktime, explicitly sets the time and then scheduler has the node run to that time?
10:58 < Zaidan> I like stickies answer, +1
10:58 < Zaidan> I'm getting on that bandwagon
10:59 < glozow> stickies-v: yes, we need to mock the scheduler to make it call `MaybeResendWalletTxs` again. and why do we need to call setmocktime?
10:59 < Zaidan> we have to pass the eviction time so the transactions are dropped?
11:00 < stickies-v> oh because ResendWalletTransactions() has up to 36h to actually run so we need to advance the mocked time by enough to ensure that
11:00 < Zaidan> dropped as in not included into a block
11:00 < glozow> right, if we *just* mock the scheduler forward, `ResendWalletTransactions()` will execute but we won't be at the `nNextResend` time yet.
11:01 < stickies-v> yeah so first we advance time by 336h (2w) to trigger mempool eviction, then we run node.syncwithvalidationinterfacequeue() to sync, and then we advance by another 36h to ensure ResendWalletTransactions() executes
11:01 < Zaidan> ahh I missed that last step, I understand now
11:01 < larryruane_> I'm not quite clear on why we need `mockscheduler` to back up the scheduler items to an earlier time (IIUC), why can't we instead just advance mock time?
11:02 < larryruane_> stickies-v: +1 good explanation
11:02 < glozow> stickies-v: exactly
11:03 < glozow> oh sorry i just realized we're out of time o.O
11:03 < Zaidan> Larry: I think thats what's happening, we go to the block time, + evict time + evict time + 36*60*60
11:03 < glozow> #endmeeting
11:03 < pablomartin> thanks glozow!
11:03 < larryruane_> thank you @glozow this was super informative!
11:03 < Zaidan> Thank you for hosting, I learned  a lot.
11:03 < pablomartin> thanks all! see you soon...
11:03 < glozow> wow that was fast. thought someone had setmocktimed me
11:03 -!- pablomartin [~pablomart@45.149.175.162] has quit [Quit: Leaving]
11:03 < lightlike> thanks!
11:03 < Zaidan> lmao
11:03 < stickies-v> :-D
11:03 < ishaanam[m]> thanks for hosting glozow!
11:04 < hernanmarino> thanks glozow and everybody for your insights
11:04 < glozow> sorry we didn't get through all the questions from the notes. hope it was helpful in reviewing the PR
11:04 < stickies-v> thank you glozow , really interesting questions - made me think much more about the PR. and thank you achow101 for authoring
11:04 < raj> larryruane_, one reason could be `mockscheduler` advances by a delta, where `setmocktime` sets it at a specific timepoint.. So I think they are both essentially same?
11:04 < glozow> lightlike is hosting next week :)
11:04 < raj> Thanks glozow , this was a nice one..  
11:05 < larryruane_> glozow: "thought someone had setmocktimed me" -- HAHAHA
11:07 < glozow> to answer the question of "why do we need to mock the scheduler too," there are a few ways of answering. One is to remove that line from the test and see what happens. Another is to look at how the timepoints in `CScheduler::taskQueue` are handled. What exactly triggers a task to be executed?
11:15 -!- BlueMoon [~BlueMoon@dgb3.dgbiblio.unam.mx] has quit [Quit: Connection closed]
11:16 -!- juancama [~juancama@pool-74-96-218-208.washdc.fios.verizon.net] has left #bitcoin-core-pr-reviews []
11:16 < achow101> on the question of resend vs reaccept: these functions were introduced in the first commit (or thereabouts) where everything was very tightly coupled together. At that point in time, the mempool was not in charge of deciding what got relayed and when. So Reaccept meant that the transactions would be re-added to the mempool after startup, but not necessarily rebroadcast. Resend meant that the transactions were actually being rebroadcast; 
11:16 < achow101> it would send out invs. Over time, as the mempool began to be in charge of tx relay, adding to the mempool also meant potentially relaying, and resending turned into adding to the mempool. And so now they are not really different and could be deduplicated.
11:30 < sipa> interesting
11:36 < glozow> mm, cool
12:05 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Remote host closed the connection]
12:05 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
12:08 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Read error: Connection reset by peer]
12:08 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has joined #bitcoin-core-pr-reviews
12:13 -!- mando [~mando@p5490620d.dip0.t-ipconnect.de] has quit [Quit: Connection closed]
12:20 -!- Zaidan [zaidan@gateway/vpn/protonvpn/zaidan] has quit [Ping timeout: 268 seconds]
12:23 -!- kevkevin [~kevkevin@66.219.242.102] has quit [Remote host closed the connection]
12:24 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
12:30 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
12:49 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 268 seconds]
12:49 -!- __gotcha1 [~Thunderbi@94.105.116.67.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
12:51 -!- __gotcha1 is now known as __gotcha
13:13 -!- OxBEEFCAF3 [~OxBEEFCAF@mobile-107-77-226-170.mobile.att.net] has quit [Quit: Connection closed]
13:59 -!- __gotcha [~Thunderbi@94.105.116.67.dyn.edpnet.net] has quit [Ping timeout: 260 seconds]
14:02 -!- o3 [uid554669@id-554669.uxbridge.irccloud.com] has quit [Quit: Connection closed for inactivity]
14:14 -!- ghost43_ [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
14:15 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Ping timeout: 268 seconds]
15:09 -!- brunoerg [~brunoerg@2804:14d:5281:8ae2:35a2:9e12:9985:3788] has quit []
15:23 -!- ghost43_ [~ghost43@gateway/tor-sasl/ghost43] has quit [Remote host closed the connection]
15:24 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
15:38 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
15:43 -!- b_1o1 [~b_1o1@189.236.23.182] has joined #bitcoin-core-pr-reviews
15:59 -!- ghost43_ [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
15:59 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Quit: Leaving]
17:03 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Remote host closed the connection]
18:03 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
18:07 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Ping timeout: 260 seconds]
18:37 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
18:42 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Ping timeout: 268 seconds]
18:56 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
18:58 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
18:59 -!- ghost43_ [~ghost43@gateway/tor-sasl/ghost43] has quit [Remote host closed the connection]
19:28 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has quit [Remote host closed the connection]
19:28 -!- ghost43 [~ghost43@gateway/tor-sasl/ghost43] has joined #bitcoin-core-pr-reviews
19:58 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Ping timeout: 252 seconds]
20:17 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
20:23 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Ping timeout: 268 seconds]
20:53 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
21:06 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Read error: Connection reset by peer]
21:07 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
21:13 -!- o3 [uid554669@id-554669.uxbridge.irccloud.com] has joined #bitcoin-core-pr-reviews
21:57 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Ping timeout: 252 seconds]
22:55 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has joined #bitcoin-core-pr-reviews
23:42 -!- o3 [uid554669@id-554669.uxbridge.irccloud.com] has quit [Quit: Connection closed for inactivity]
23:48 -!- Zaidan [~zaidan@d75-156-179-9.abhsia.telus.net] has quit [Ping timeout: 268 seconds]
--- Log closed Thu Aug 25 00:01:00 2022