--- Day changed Wed Aug 21 2019 01:34 -!- setpill [~setpill@unaffiliated/setpill] has joined #bitcoin-core-pr-reviews 01:42 -!- michaelfolkson [~textual@109.226.49.47] has joined #bitcoin-core-pr-reviews 01:43 -!- michaelfolkson [~textual@109.226.49.47] has quit [Client Quit] 01:44 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 01:45 -!- michaelfolkson [~textual@109.226.49.47] has joined #bitcoin-core-pr-reviews 01:46 -!- michaelfolkson [~textual@109.226.49.47] has quit [Client Quit] 02:30 -!- michaelfolkson [~textual@109.226.49.47] has joined #bitcoin-core-pr-reviews 05:08 -!- michaelfolkson [~textual@109.226.49.47] has quit [Quit: Sleep mode] 05:25 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has joined #bitcoin-core-pr-reviews 05:41 -!- dergigi [~dergigi@95.211.188.11] has joined #bitcoin-core-pr-reviews 06:08 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-jlacsifbunngnxdy] has joined #bitcoin-core-pr-reviews 06:22 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 06:23 -!- csknk [~csknk@unaffiliated/csknk] has quit [Client Quit] 06:26 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has quit [Quit: Sleep mode] 06:26 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has joined #bitcoin-core-pr-reviews 06:36 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has quit [Quit: Sleep mode] 06:36 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has joined #bitcoin-core-pr-reviews 06:47 -!- fox2p_ [~fox2p@185.183.104.83] has quit [Quit: hash] 07:04 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has quit [Quit: Sleep mode] 07:09 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has joined #bitcoin-core-pr-reviews 07:14 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-jlacsifbunngnxdy] has quit [Ping timeout: 250 seconds] 07:14 -!- moneyball [sid299869@gateway/web/irccloud.com/x-hqofhoufwldkfgdq] has quit [Ping timeout: 250 seconds] 07:15 -!- petezz4 [uid2429@gateway/web/irccloud.com/x-khtvbfwezanbrpzm] has quit [Ping timeout: 250 seconds] 07:18 -!- petezz4 [uid2429@gateway/web/irccloud.com/x-agflkkgsvtpfilhb] has joined #bitcoin-core-pr-reviews 07:19 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-virceihzfqfvnmqe] has joined #bitcoin-core-pr-reviews 07:25 -!- provoostenator [~quassel@62.140.137.124] has joined #bitcoin-core-pr-reviews 07:25 -!- pinheadmz [~matthewzi@209.209.238.185] has joined #bitcoin-core-pr-reviews 07:26 -!- moneyball [sid299869@gateway/web/irccloud.com/x-wwgbwsasxjqnrqlh] has joined #bitcoin-core-pr-reviews 07:27 -!- davterra [~none@209.58.185.33] has joined #bitcoin-core-pr-reviews 07:54 -!- michaelfolkson [~textual@bzq-154-220.red.bezeqint.net] has quit [Quit: Sleep mode] 08:25 < jnewbery> Reminder to read notes/questions: https://bitcoin-core-review-club.github.io/15931.html . We'll get started in about 95 minutes. 08:29 -!- TonySanak [68253fa0@104-37-63-160.dyn.novuscom.net] has joined #bitcoin-core-pr-reviews 08:41 -!- setpill [~setpill@unaffiliated/setpill] has quit [Quit: o/] 08:58 -!- hebasto [~hebasto@95.164.65.194] has joined #bitcoin-core-pr-reviews 09:00 -!- pinheadmz [~matthewzi@209.209.238.185] has quit [Quit: pinheadmz] 09:03 < emilengler> 57 Minutes to go, set your timers! 09:15 -!- davterra [~none@209.58.185.33] has quit [Ping timeout: 244 seconds] 09:17 -!- davterra [~none@209.58.185.33] has joined #bitcoin-core-pr-reviews 09:24 -!- peevsie [peevsie@gateway/vpn/privateinternetaccess/peevsie] has joined #bitcoin-core-pr-reviews 09:37 -!- peevsie [peevsie@gateway/vpn/privateinternetaccess/peevsie] has quit [Ping timeout: 245 seconds] 09:37 -!- lightlike [~lightlike@2001:16b8:57fc:bf00:70c1:d04:c7c8:bb6b] has joined #bitcoin-core-pr-reviews 09:39 -!- peevsie [peevsie@gateway/vpn/privateinternetaccess/peevsie] has joined #bitcoin-core-pr-reviews 09:46 -!- michaelfolkson [~textual@109.226.49.47] has joined #bitcoin-core-pr-reviews 09:52 -!- csknk [~csknk@unaffiliated/csknk] has joined #bitcoin-core-pr-reviews 10:00 < jnewbery> hi! 10:00 < digi_james> Hello! 10:00 < kanzure> hi 10:00 < emilengler> hi 10:00 < dergigi> hi 10:00 < ariard> hi 10:00 < lightlike> hello 10:00 -!- thoragh [5db14b9a@93.177.75.154] has joined #bitcoin-core-pr-reviews 10:00 < peevsie> hi 10:00 < jonatack> hi 10:01 < michaelfolkson> Hey 10:01 < jnewbery> jkcqyq dergigg: thanks for leaving review comments on the PR! 10:01 < jnewbery> sorry, jkczyz 10:02 < jnewbery> what did people think of the PR this week? 10:02 < jkczyz> hi 10:02 < jnewbery> perhaps jkczyz or dergigi (or anyone else) could give some initial thoughts about the PR 10:02 < nehan> hi 10:04 < jnewbery> I like this PR. It's a good next step to decoupling the wallet from the node 10:04 < dergigi> I don't have any specific questions unfortunately - just ran into the compilation error (it's the first PR I'm looking at, still trying to wrap my head around the code) 10:04 < jkczyz> Initial thoughts were having separate members for hash and height could be prone to bugs, but noticed there was a suggestion to combine 10:04 < jnewbery> dergigi: that's ok. Thanks for trying to test and leaving a comment - that's useful in itself 10:04 < jkczyz> did not have a chance to look at the PR that was made for that thought 10:04 < jkczyz> s/thought/though 10:05 < jnewbery> jkczyz: you mean #16624? 10:05 < jkczyz> yes 10:05 < jnewbery> https://github.com/bitcoin/bitcoin/pull/16624 wallet : encapsulate transactions state 10:05 < dergigi> I like the concept - seems like a good idea to store the tx block height internally so we don't have to look into the chain for every tx 10:06 < jnewbery> right - the PR author pulled out a small part of this PR and made it into its own PR. I'd already chosen 15931 for today's discussion and didn't want to change it in case you'd all started reviewing it 10:06 < michaelfolkson> It is certainly one that is interlocking with a bunch of other PRs and requires some organizational roadmap so they are merged in the right order. Seems like a high quality PR by itself. 10:07 < jnewbery> dergigi: yeah, it's good to reduce the wallet's reliance on locking the node's chain state 10:07 < nehan> i found it pretty difficult to chase the chain of PRs and understand everything together 10:07 < lightlike> I found this PR not so easy to review, but obiviously the concept of not having to query the block height makes a lot of sense. 10:07 < jnewbery> nehan: do you think you figured it out in the end? Any questions? 10:07 < jnewbery> lightlike: what made it difficult? Was the PR just too large? 10:08 < jnewbery> I think https://github.com/bitcoin/bitcoin/pull/16426 gives a good summary of what the end goal is 10:08 < nehan> jnewbery: no, but looking forward to discussing. the goal is great, but given i'm pretty unfamiliar with the wallet code i can't convince myself that these PRs don't change behavior (or that the behavior they change is 'safe') 10:08 < lightlike> jnewbery: It was large, but also some of the elements (the callback mechanism) were not so trivial to understand for me. 10:09 < jnewbery> "I can't convince myself that these PRs don't change behavior (or that the behavior they change is 'safe')" - I agree this is very difficult! 10:10 < jnewbery> What would you want to do to convince yourself of that? 10:12 < jnewbery> anyone? 10:12 -!- hanhua [68840054@104.132.0.84] has joined #bitcoin-core-pr-reviews 10:12 < ariard> yes sorry for the big PR and splitting it between 15931 and 16624, it happened that wallet state transitions weren't clear for anyone 10:12 < jkczyz> Have test coverage for the behaviors 10:12 < jnewbery> jkczyz: yes please! 10:12 < lightlike> familiarize myself more with the wallet in general 10:13 < jnewbery> One thing that we're really missing is testing of old wallet.dat files 10:13 < jnewbery> There's an issue here: https://github.com/bitcoin/bitcoin/issues/14536 10:13 < jnewbery> I think it'd be really useful to have wallet files produced by lots of old versions of bitcoin core and tests for them 10:14 < ariard> jkczyz: have a look on 16624 listed issues in opening message, I think some behaviors aren't tested 10:14 < jnewbery> for PRs like 15931 and 16624, where there are changes in serialization (or at least in the way we deserialize and hold data at runtime), being able to do regression testing on old wallet files would be really useful 10:15 < jnewbery> lightlike: when you talk about 'callback mechanism', is that things like the BlockConnected/BlockDisconnected/TransactionAddedToMempool stuff? 10:16 -!- bcribles [~bcribles@184-23-191-179.vpn.dynamic.sonic.net] has joined #bitcoin-core-pr-reviews 10:16 < jnewbery> first question I had was: An early version of this PR added an m_block_height field the wallet transaction serialization (comment). Why wouldn’t this work? 10:16 < jnewbery> any thoughts about that? 10:16 < nehan> breaks compatibility with old wallet.dat files? 10:17 < jnewbery> nehan: yes exactly 10:18 < provoostenator> I have a PR that tests both forward and backward compatibility of wallets, though not ancient: https://github.com/bitcoin/bitcoin/pull/12134 10:18 < jnewbery> serialization is a bit tricky with the wallet. We store wallet objects in bdb, which is a key value store. 10:18 < lightlike> jnewbery: yes, that's what I meant. Not the changes done, in this PR, which are clear, but how the whole mechanism really works level. 10:18 < lightlike> *at a low level 10:18 < jnewbery> The serialization for each individual object is defined in the object declaration in the header file 10:19 < jnewbery> eg https://github.com/bitcoin/bitcoin/blob/6dfa9efa3f558deaca0f01f673c79cce2b92a2b3/src/wallet/wallet.h#L492 10:19 < jonatack> provoostenator: nice! 10:19 < jnewbery> but there's also a bunch of code in walletdb.cpp that fiddles around with that serialization 10:20 < jnewbery> here: https://github.com/bitcoin/bitcoin/blob/6dfa9efa3f558deaca0f01f673c79cce2b92a2b3/src/wallet/walletdb.cpp#L200 10:21 < jnewbery> serialization just serializes various data elements from the wallet in a specified order: https://github.com/bitcoin/bitcoin/blob/6dfa9efa3f558deaca0f01f673c79cce2b92a2b3/src/wallet/wallet.h#L505 10:21 < jnewbery> which makes it very difficult to version the serialization code 10:21 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has quit [Read error: Connection reset by peer] 10:21 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 10:22 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 10:22 < michaelfolkson> I think we lost John 10:22 -!- jnewbery [~john@rrcs-184-74-240-156.nyc.biz.rr.com] has joined #bitcoin-core-pr-reviews 10:22 < bcribles> you didn't miss any discussion while disconnected 10:23 < michaelfolkson> What version does it test back to 10:23 < michaelfolkson> ? 10:23 < jnewbery> thanks bcribles! 10:24 < jonatack> jnewbery, ariard: how hard do you think it would be to clean up that serialization? 10:24 < jnewbery> jonatack: what do you mean by clean up? 10:25 < jonatack> untangle it to be easier to version 10:26 < dergigi> side note: i really like the tx status (suggested by ryanofsky) in 16624: unconfirmed/confirmed/conflicted/abandoned - the comments on that are very clear, good job on that ariard 10:26 < jnewbery> jonatack: quite difficult. We absolutely need to remain compatible with old wallet.dat files, and it's also important that new wallet.dat files are compatible with old versions, so we couldn't just change it completely 10:26 < lightlike> jnewbery: the serialization link https://github.com/bitcoin/bitcoin/pull/15931#discussion_r295434058 you gave pointed to CMerkleTx. Isn't that only for old wallet files, so why try to add block height there? Or did the change to CWalletTx happen only recently? 10:27 < ariard> could we version wallet.dat files in a backward compatible way ? 10:27 < jnewbery> if we were designing it from scratch, we might use a type-length-value scheme so wallet software could just ignore fields that it doesn't recognize, but we're constrained by all the existing wallet.dat files already in existence 10:28 < jnewbery> lightlike: the change to CWalletTx happened recently, resulting from a suggestion in this PR 10:28 < jnewbery> https://github.com/bitcoin/bitcoin/pull/16451 10:28 < lightlike> oh ok, then it makes sense. 10:29 < jnewbery> ariard: I'm not sure. Does the serialization for the wallet include the software version that wrote it? 10:29 < jnewbery> In any case, we'd need to leave the code for deserializing 'legacy' wallet.dat files in place for a long time 10:30 < fjahr> fg 10:31 < ariard> yes version with birthday would be cool, and pumping a message to user with which core version to use 10:31 < hugohn> brainstorming: perhaps one way to get around the lack of versioning in older Core versions is to write new stuff to a new .dat file? and start versioning in the new .dat file? 10:31 < hugohn> not sure if that's feasible 10:33 < ariard> let's say a new frontend serializer you may try to read first byte of files, if there is a magic number showing versioning support you use new serializer if not the old one 10:33 < jnewbery> lightlike: going back to your previous point: all of the wallet callbacks come from CValidationInterface functions. I think there are only 5 of those methods that are overridden by the wallet: TransactionAddedToMempool, TransactionRemovedFromMempool, BlockConnected, BlockDisconnected, UpdatedBlockTip, ChainStateFlushed 10:34 < jnewbery> it's worth looking at the NotificationsHandlerImpl in src/interfaces/chain and understanding how those interface methods are called 10:34 < ariard> jnewbery: it's a bit more complicated given we use Chain::Notifications as an adaptor between CValidationInterface and wallets 10:35 < jnewbery> ariard: that's right. It's a bit of indirection, but they're direct function calls from Chain::Notifications 10:35 < jnewbery> I haven't seen any answers to: The PR author offers two ways for the wallet to populate the wallet transactions’ heights (save the transaction height to disk or calculate the height for each transactions at wallet load time). What are the trade-offs? Which approach do you prefer? 10:35 < fjahr> sorry, wrong window :/ 10:36 < jnewbery> One potential disadvantage to calculating height at wallet load time is that it might impact performance for a large wallet 10:37 < jnewbery> ryanofsky thought it wouldn't be too much of a problem: https://github.com/bitcoin/bitcoin/pull/15931#issuecomment-519099417 10:38 < ariard> I think it's a performance hit at load time but a win at run time, you don't have to query chain anymore 10:38 < hugohn> saving tx height to disk seems to make more sense in the long-term, doesn't make sense to recompute the height each time the process starts, which doesn't change. 10:38 < jnewbery> ariard: I'm comparing doing the calculation at start up, or serializing it to disk. Both are a win at run time 10:39 < nehan> what is the likelihood that the heights have changed since the process was restarted? 10:39 < jkczyz> saving to disk would require a serialization change, right? 10:39 < jnewbery> jkczyz: exactly correct 10:39 < jnewbery> which we should be cautious about doing without thorough testing 10:40 < jnewbery> nehan: very slim, but possible. There would have to be a re-org over that block while the wallet was offline 10:41 < nehan> jnewbery: and if that happened, would there be a process to update the txn heights if they were loaded from disk? 10:41 < provoostenator> @michaelfolkson at the moment it tests back to 0.17.1, but more could be added. The main drawback is having too many switch statements in Python test framework to deal with ancient RPC stuff 10:42 < nehan> it seems to me it's not safe to rely on the serialized txn heights unless you're sure that on restart you'll get the appropriate notifications to update them (which you might. i'm not sure) 10:42 < michaelfolkson> : So how far do you think it could be reasonably extended back to? 10:42 < bcribles> possibly because I haven't read enough of the PR but the malleability of using height caused when a reorg happens (like nehan was saying) is something I need to work through 10:42 < ariard> nehan: yes you need others change before, where the wallet send a block locator of its current highest tip 10:42 < jnewbery> nehan: yes, the wallet knows its own best block hash and tries to find the point that it diverges from the main chain, then rescans from there 10:42 < hugohn> is it possible to save the tx height to a different field in wallet.dat? 10:42 < ariard> and if this one is on a forked branch, a reorg should happen by replaying old blocks 10:42 < jnewbery> https://github.com/bitcoin/bitcoin/blob/6dfa9efa3f558deaca0f01f673c79cce2b92a2b3/src/wallet/wallet.cpp#L4504 10:42 < ariard> and connecting new ones 10:42 < hugohn> without affecting existing CWalletTx serialization 10:43 < jnewbery> ariard: that's correct 10:44 < nehan> ariard: jnewbery: thanks 10:44 < jnewbery> to add some more detail: the wallet has a 'locator', which is a sparse list of blockhashes in its view of the block chain. It uses that to try to find a fork point with the nodes view of the block chain at startup 10:45 < provoostenator> michaelfolkson: in theory back to when regtest was added, in practice you'll just have to try one by one 10:45 < jnewbery> https://github.com/bitcoin/bitcoin/blob/6dfa9efa3f558deaca0f01f673c79cce2b92a2b3/src/primitives/block.h#L122 10:45 < bcribles> jnewbery e.g. a wallet would be able to reason about if the block a txn was confirmed in has experienced a reorg? 10:46 < jnewbery> Locators are also used in GETBLOCKS and GETHEADERS messages on the P2P network: https://btcinformation.org/en/developer-reference#getblocks 10:46 < hugohn> to me the decision to persist to disk or not probably shouldn't impact how the wallet reacts to reorgs - one is a memory management issue, one is a consensus issue. reorgs could happen _after_ we have recomputed the height at startup anyway, so the issues should be completely orthogonal. but I could be wrong. 10:47 < jnewbery> bcribles: not directly, but it would know that there had been a re-org since it went offline, and therefore rescan the block chain from a height where it knows it shares history with the node 10:48 < jnewbery> if a re-org happened when the wallet is online, we'd expect the node to inform the wallet of the blocks being rewound with BlockDisconnected calls, followed by BlockConnected calls for the new chain 10:48 < nehan> hugohn: agreed! i just wanted to understand how invariants are maintained, and expectations on how a newly loaded wallet will be updated 10:48 < jnewbery> next question: How does the wallet learn about new transactions in the mempool or included in blocks? 10:49 < jnewbery> (should be easy, since we've already talked about it) 10:49 < hugohn> nehan: yeah, great question! 10:49 < hugohn> CValidationInterface? 10:49 < digi_james> ValidationInterface Callbacks 10:49 < jkczyz> It registers for notifications from the chain and implementing interfaces::Chain::Notifications 10:50 < jnewbery> good, you've all been paying attention :) 10:50 < jnewbery> ok, final question: What are the wallet’s expectations about block notifications? Is it a problem if the wallet is informed of the same block more than once? If blocks arrive in the wrong order? If a block is skipped? If a block is re-orged out of the main chain? 10:51 -!- peevsie_ [peevsie@gateway/vpn/privateinternetaccess/peevsie] has joined #bitcoin-core-pr-reviews 10:52 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has quit [Ping timeout: 250 seconds] 10:52 -!- peevsie [peevsie@gateway/vpn/privateinternetaccess/peevsie] has quit [Ping timeout: 258 seconds] 10:52 -!- peevsie_ [peevsie@gateway/vpn/privateinternetaccess/peevsie] has quit [Client Quit] 10:52 < jnewbery> ok, it's a difficult question to answer without reading a lot of code! 10:52 < jkczyz> It would seem that AddToWalletIfInvolvingMe handles many of these cases 10:53 < jnewbery> your entry points are those ValidationInterface methods 10:53 < jnewbery> jkczyz: exactly. AddToWalletIfInvolvingMe() is key 10:53 < jnewbery> https://github.com/bitcoin/bitcoin/blob/6dfa9efa3f558deaca0f01f673c79cce2b92a2b3/src/wallet/wallet.cpp#L1204 10:54 < jnewbery> how does a wallet know if a transaction is interesting to it? 10:54 < hugohn> by calling IsMine() 10:54 < hugohn> or IsFromMe() 10:55 < jnewbery> hugohn: yes, IsMine() will determine if that transaction sends money to the wallet 10:55 < jnewbery> hugohn: and yes, IsFromMe() determines if the transaction sends money from the wallet 10:56 < jnewbery> important to note: IsFromMe() can only work if the transaction input is spending from a transaction that the wallet already knows about 10:56 < hugohn> the legacy IsMine() logic is quite involving, it does pattern-matching and recursive lookups to determine if something belongs to the wallet 10:56 < jnewbery> say tx A sends money to a wallet, and tx B spends an output from tx A 10:56 -!- TonySanak [68253fa0@104-37-63-160.dyn.novuscom.net] has quit [Remote host closed the connection] 10:56 -!- Zenton [~user@unaffiliated/vicenteh] has quit [Ping timeout: 248 seconds] 10:57 < jkczyz> It seems odd that BlockDisconnected uses this with a null block hash / position. At very least it's hard to follow what the code is doing in this case given the reuse for adding blocks 10:57 < jnewbery> if the wallet doesn't know about tx A, then it has no way of knowing that tx B is a spend from the wallet 10:57 < nehan> jnewbery: it doesn't look like AddToWalletIfInvolvingMe() handles blocks in the wrong order 10:57 < jnewbery> nehan: right, the wallet's expectation is that blocks are served sequentially 10:58 < nehan> jnewbery: good that aligns with my understanding of CValidationInterface 10:58 < lightlike> A comment in ValidationInterface says that it is guaranteed that calls arrive in the same order as events are generated in validation - in that case, how is it possible that a block is skipped? 10:58 < jnewbery> jkczyz: that's needed so the wallet can change confirmed transactions to unconfirmed in the case of a reord 10:58 < jnewbery> lightlike: it shouldn't be possible 10:59 < hugohn> could the arrive-in-order assumption still hold true once we have node/wallet process separation? and block notifications served over IPC? 10:59 < jnewbery> however, I believe the wallet should be fine if it receives a block more than once 10:59 < nehan> jnewbery: re "What are the wallet’s expectations about block notifications?..." I think the wallet does not expect or handle any of that 10:59 < digi_james> hugohn: I had the same question 10:59 < nehan> jnewbery: oh 10:59 < jnewbery> hugohn: blocks would still need to arrive in the correct order 11:00 < lightlike> isn't the arrive-in-order assumptions broken during IBD? 11:00 < ariard> lightlike: do you mean receiving block C before A and B ? 11:00 < jnewbery> because there's not really the concept of a 'from address' in a transaction, the wallet needs to know about all prior transactions to know if a new tx is spending from it 11:01 < lightlike> ariard: yes. 11:01 < jnewbery> I have to run now. Sorry we've run out of time. 11:01 < jkczyz> jnewbery: ah, I think the fUpdate flag threw me off 11:01 < jnewbery> Feel free to continue chatting in here. 11:02 < jnewbery> Thanks for everyone's input. Great discussion this week! 11:02 < lightlike> thanks! 11:02 < ariard> lightlike: it shouldn't if ActivateBestChain is right 11:02 < lightlike> ariard: you are right, I guess blocks can be downloaded out of order, but obviously not connected out of order 11:02 < nehan> jkczyz: what did you mean when you said that AddToWalletIfInvolving me handleds many of these cases? What cases does it handle? 11:03 < ariard> lightlike: exactly 11:03 -!- pinheadmz [~matthewzi@209.209.238.185] has joined #bitcoin-core-pr-reviews 11:05 -!- jonatack [~jon@jau64-1-88-171-168-34.fbx.proxad.net] has joined #bitcoin-core-pr-reviews 11:05 < jkczyz> nehan: Possibly informed of the same block more than once (BlockConnected) and re-org (vis BlockDisconnected), though I'm not sure if the former happens in practice 11:06 < jonatack> ugh, lost connection for last 15 11:06 < hugohn> `bool fExisted = mapWallet.count(tx.GetHash()) != 0` 11:06 < hugohn> `if (fExisted && !fUpdate) return false;` 11:06 < hugohn> this logic^ should take care of redundant block? 11:07 < hugohn> AddToWalletIfInvolving bails if it's already seen the txn 11:08 -!- coisinho [uid386002@gateway/web/irccloud.com/x-dldjqerttewcwaby] has joined #bitcoin-core-pr-reviews 11:09 < hugohn> unless it's a forced update (fUpdate = true when rescanning) 11:11 -!- michaelfolkson [~textual@109.226.49.47] has quit [Quit: Sleep mode] 11:12 < hugohn> not sure of behavior when blocks are skipped 11:19 -!- jonatack [~jon@jau64-1-88-171-168-34.fbx.proxad.net] has quit [Ping timeout: 276 seconds] 11:20 -!- jonatack [~jon@2a01:e35:8aba:8220:6627:dad:d967:649d] has joined #bitcoin-core-pr-reviews 11:24 -!- dergigi [~dergigi@95.211.188.11] has quit [Remote host closed the connection] 11:46 < jnewbery> jonatack: logs are up: https://bitcoin-core-review-club.github.io/15931.html 11:59 -!- michaelfolkson [~textual@109.226.49.150] has joined #bitcoin-core-pr-reviews 12:15 -!- csknk [~csknk@unaffiliated/csknk] has quit [Quit: leaving] 12:17 -!- thoragh [5db14b9a@93.177.75.154] has quit [Remote host closed the connection] 12:48 -!- pinheadmz [~matthewzi@209.209.238.185] has quit [Quit: pinheadmz] 12:58 -!- Zenton [~user@unaffiliated/vicenteh] has joined #bitcoin-core-pr-reviews 13:22 -!- pinheadmz [~matthewzi@209.209.238.185] has joined #bitcoin-core-pr-reviews 13:30 -!- michaelfolkson [~textual@109.226.49.150] has quit [Quit: Sleep mode] 13:49 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has quit [Remote host closed the connection] 13:49 -!- sdaftuar [~sdaftuar@gateway/tor-sasl/sdaftuar] has joined #bitcoin-core-pr-reviews 13:51 -!- michaelfolkson [~textual@109.226.49.150] has joined #bitcoin-core-pr-reviews 14:04 -!- michaelfolkson [~textual@109.226.49.150] has quit [Quit: Sleep mode] 14:15 -!- hanhua [68840054@104.132.0.84] has quit [Remote host closed the connection] 14:56 -!- bcribles [~bcribles@184-23-191-179.vpn.dynamic.sonic.net] has quit [Quit: bcribles] 16:08 -!- TheRec [~toto@drupal.org/user/146860/view] has quit [] 17:31 -!- pinheadmz [~matthewzi@209.209.238.185] has quit [Quit: pinheadmz] 17:56 -!- Netsplit *.net <-> *.split quits: MarcoFalke, fjahr, provoostenator, coisinho, harding, tinova, hebasto, Nebraskka, hardforkthis, peltre, (+49 more, use /NETSPLIT to show all of them) 18:13 -!- Netsplit over, joins: gwillen, Nebraskka, molz, qinfengling, dmkathayat, RubenSomsen, e4xit, d3spwn, hkjn, usil (+3 more) 18:13 -!- illlicit_ [uid109953@gateway/web/irccloud.com/session] has joined #bitcoin-core-pr-reviews 18:13 -!- Netsplit over, joins: Zenton, aj, akionak 18:13 -!- coisinho_ [uid386002@gateway/web/irccloud.com/x-towzydwvtaeuqogf] has joined #bitcoin-core-pr-reviews 18:13 -!- peltre_ [sid268329@gateway/web/irccloud.com/x-zaaqcrfkocmwsrcb] has joined #bitcoin-core-pr-reviews 18:13 -!- petezz4 [uid2429@gateway/web/irccloud.com/session] has joined #bitcoin-core-pr-reviews 18:13 -!- Netsplit over, joins: jonasschnelli, jonatack 18:13 -!- lightlike_ [~lightlike@2001:16b8:57fc:bf00:70c1:d04:c7c8:bb6b] has joined #bitcoin-core-pr-reviews 18:13 -!- schmidty_ [sid297174@gateway/web/irccloud.com/x-vohqmufybdifcwge] has joined #bitcoin-core-pr-reviews 18:13 -!- rrybarczyk [sid364924@gateway/web/irccloud.com/x-curjcvtjncyrswpw] has joined #bitcoin-core-pr-reviews 18:13 -!- fanquake [sid369002@gateway/web/irccloud.com/x-jupgkcdrrufypwdn] has joined #bitcoin-core-pr-reviews 18:13 -!- wumpus [~ircclient@2001:bc8:3bec:100::1] has joined #bitcoin-core-pr-reviews 18:13 -!- Netsplit over, joins: achow101, nothingmuch, designwish, harding, wallet42, fjahr, Lauda, hardforkthis, ariard, digi_james (+15 more) 18:14 -!- petezz4 [uid2429@gateway/web/irccloud.com/session] has quit [Changing host] 18:14 -!- petezz4 [uid2429@gateway/web/irccloud.com/x-adhkffcjfpgjuyhy] has joined #bitcoin-core-pr-reviews 18:14 -!- illlicit_ [uid109953@gateway/web/irccloud.com/session] has quit [Changing host] 18:14 -!- illlicit_ [uid109953@gateway/web/irccloud.com/x-vcusrrhcifgnbygx] has joined #bitcoin-core-pr-reviews 18:14 -!- Netsplit over, joins: MasterdonX 18:14 -!- Netsplit over, joins: sdaftuar 18:14 -!- Netsplit over, joins: hebasto, luke-jr, bad_duck, tinova 18:17 -!- fanquake is now known as Guest95396 18:21 -!- Guest95396 [sid369002@gateway/web/irccloud.com/x-jupgkcdrrufypwdn] has quit [] 18:21 -!- fanquake [sid369002@gateway/web/irccloud.com/x-wldabmtwklcqgksi] has joined #bitcoin-core-pr-reviews 18:23 -!- lightlike_ [~lightlike@2001:16b8:57fc:bf00:70c1:d04:c7c8:bb6b] has quit [Quit: Leaving] 18:24 -!- midnightmagic [~midnightm@unaffiliated/midnightmagic] has joined #bitcoin-core-pr-reviews 19:07 -!- emilengler [~emilengle@unaffiliated/emilengler] has quit [Ping timeout: 248 seconds] 19:11 -!- emilengler [~emilengle@unaffiliated/emilengler] has joined #bitcoin-core-pr-reviews 19:38 -!- davterra [~none@209.58.185.33] has quit [Quit: Leaving] 20:09 -!- pinheadmz [~matthewzi@209.209.238.185] has joined #bitcoin-core-pr-reviews 20:15 -!- pinheadmz [~matthewzi@209.209.238.185] has quit [Ping timeout: 258 seconds] 20:18 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 21:04 -!- pinheadmz [~matthewzi@c-73-92-181-51.hsd1.ca.comcast.net] has quit [Quit: pinheadmz] 21:10 -!- pinheadmz [~matthewzi@198.8.81.52] has joined #bitcoin-core-pr-reviews 21:19 -!- hebasto [~hebasto@95.164.65.194] has quit [Remote host closed the connection] 21:33 -!- pinheadmz [~matthewzi@198.8.81.52] has quit [Quit: pinheadmz] 21:34 -!- pinheadmz [~matthewzi@198.8.81.52] has joined #bitcoin-core-pr-reviews 22:17 -!- elichai2 [uid212594@gateway/web/irccloud.com/x-virceihzfqfvnmqe] has quit [Quit: Connection closed for inactivity] 22:20 -!- pinheadmz [~matthewzi@198.8.81.52] has quit [Quit: pinheadmz] 23:19 -!- pinheadmz [~matthewzi@198.8.81.52] has joined #bitcoin-core-pr-reviews