--- Day changed Wed May 13 2020 00:08 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has quit [Quit: ZNC 1.7.5 - https://znc.in] 00:10 -!- harrigan [~harrigan@ptr-93-89-242-235.ip.airwire.ie] has joined #bitcoin-core-pr-reviews 00:40 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 256 seconds] 01:00 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews 01:05 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 260 seconds] 01:12 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 256 seconds] 01:28 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews 01:45 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 01:57 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 256 seconds] 01:59 -!- sr_gi [~sr_gi@128.red-79-154-168.dynamicip.rima-tde.net] has joined #bitcoin-core-pr-reviews 02:12 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 246 seconds] 02:24 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 02:29 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds] 03:43 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds] 03:55 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 04:00 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Remote host closed the connection] 04:06 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 04:16 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Ping timeout: 240 seconds] 04:21 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 04:24 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has joined #bitcoin-core-pr-reviews 04:25 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 04:30 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 264 seconds] 04:48 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 246 seconds] 05:11 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has joined #bitcoin-core-pr-reviews 05:22 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 05:33 -!- theStack [~honeybadg@vps1648322.vs.webtropia-customer.com] has joined #bitcoin-core-pr-reviews 05:35 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has quit [Ping timeout: 260 seconds] 05:42 -!- luke-jr [~luke-jr@unaffiliated/luke-jr] has joined #bitcoin-core-pr-reviews 06:12 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has quit [Remote host closed the connection] 06:13 -!- molakala [~sumanth.b@2601:192:4701:87f0:248b:8db4:3f75:79fa] has joined #bitcoin-core-pr-reviews 06:18 -!- sipa [~pw@gateway/tor-sasl/sipa1024] has joined #bitcoin-core-pr-reviews 06:26 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 06:30 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 240 seconds] 06:53 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has quit [Quit: jonatack] 07:30 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 07:33 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 246 seconds] 07:35 -!- mol_ [~mol@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 08:27 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 08:32 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 265 seconds] 08:42 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews 08:52 -!- mol [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 08:53 < ryanofsky> review club meeting for sethdseed inactive keypool topup https://bitcoincore.reviews/17681.html should start in around one hour (17:00) 08:55 < jonatack> yes... a reminder was tweeted 3 hours ago https://twitter.com/BitcoinCorePRs/status/1260554899810586624 09:11 -!- shaunsun [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has joined #bitcoin-core-pr-reviews 09:12 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 09:15 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 09:21 -!- nehan [~nehan@41.213.196.104.bc.googleusercontent.com] has quit [Quit: leaving] 09:21 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 09:21 -!- nehan [~nehan@41.213.196.104.bc.googleusercontent.com] has joined #bitcoin-core-pr-reviews 09:21 -!- pinheadm_ [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 09:21 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 09:21 -!- pinheadm_ [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Client Quit] 09:21 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Read error: Connection reset by peer] 09:21 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 09:22 -!- pinheadmz [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 09:23 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 09:23 -!- vasild_ is now known as vasild 09:27 -!- lightlike [~lightlike@p200300C7EF1A0F00E55E228E8DC90D42.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews 09:35 -!- soup [494e6362@c-73-78-99-98.hsd1.co.comcast.net] has joined #bitcoin-core-pr-reviews 09:39 -!- seven_ [~seven@2a00:ee2:410c:1300:5d75:db96:5379:1e71] has joined #bitcoin-core-pr-reviews 09:39 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews 09:50 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:79fa:9bdf:6272:bed7] has joined #bitcoin-core-pr-reviews 09:52 -!- b10c [~b10c@2001:16b8:2e12:8c00:6403:a492:9cfe:36ca] has joined #bitcoin-core-pr-reviews 09:55 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 09:56 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 09:56 -!- seven_ [~seven@2a00:ee2:410c:1300:5d75:db96:5379:1e71] has quit [Remote host closed the connection] 09:56 -!- seven_ [~seven@2a00:ee2:410c:1300:5d75:db96:5379:1e71] has joined #bitcoin-core-pr-reviews 09:59 -!- Smeier [cfacdb18@207.172.219.24] has joined #bitcoin-core-pr-reviews 09:59 -!- kory [54112956@84.17.41.86] has joined #bitcoin-core-pr-reviews 10:00 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 10:00 < jnewbery> #startmeeting 10:00 < kanzure> hi 10:00 < andrewtoth> hi 10:01 < troygiorshev> hi 10:01 < raj_149> hi 10:01 < theStack> hi 10:01 < vasild> hi 10:01 < ryanofsky> hi 10:01 < soup> :hello: 10:01 < fjahr> hi 10:01 < jnewbery> hi! 10:01 < jkczyz> hi 10:01 < lightlike> HI 10:01 < jnewbery> ryanofksy is hosting today. Notes in the usual place: https://bitcoincore.reviews/17681.html 10:01 < ryanofsky> Welcome to the meeting on the sethdseed key topup PR: https://github.com/bitcoin/bitcoin/pulls/17681, https://bitcoincore.reviews/17681.html 10:01 < jonatack> hi 10:01 -!- driver [~driver@195.181.160.175.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 10:01 < ryanofsky> if any one has any questions about the pr, feel free to ask, or we can go through questions at https://bitcoincore.reviews/17681.html 10:02 < ryanofsky> first question there is who's reviewed the pr? (y/n) 10:02 < jnewbery> y 10:02 < andrewtoth> n 10:02 < fjahr> y 10:02 < troygiorshev> n 10:02 < vasild> n 10:02 < jonatack> y 10:02 < soup> n 10:02 < raj_149> y 10:02 < jkczyz> y 10:02 < nehan> hi 10:03 < theStack> n (only Concept ACK) 10:03 < lightlike> n 10:03 < ryanofsky> That's good, next question is the purpose of the PR clear? How does it help users to top up keypools for inactive HD seeds? What is not good about the behavior before this PR? 10:03 -!- r251d [~r251d@162-196-59-192.lightspeed.irvnca.sbcglobal.net] has joined #bitcoin-core-pr-reviews 10:04 -!- michaelf_ [~textual@2a00:23c5:be01:b201:85d2:62f:72a7:cf48] has joined #bitcoin-core-pr-reviews 10:04 < raj_149> it helps to keep track of inactive chains, top it up properly if addresses are querried. Not sure about the bad behaviour part. 10:05 < jonatack> I think the src/wallet/scriptpubkeyman.h::L50 CKeyPool documentation is helpful on this 10:05 < fjahr> for whatever reason funds could still be sent to addresses of these inactive seeds. Those should not be missed by the wallet. 10:05 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:79fa:9bdf:6272:bed7] has quit [Ping timeout: 246 seconds] 10:05 -!- michaelf_ [~textual@2a00:23c5:be01:b201:85d2:62f:72a7:cf48] has quit [Client Quit] 10:06 < jnewbery> raj_149: it's not about querying addresses. It's if addresses from the keypool are used up if a transaction output is sent to them 10:06 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:85d2:62f:72a7:cf48] has joined #bitcoin-core-pr-reviews 10:06 < andrewtoth> it's a bit tricky, but i think the bug is that if a highly used seed is replaced by sethdseed, but lots of new txs come into that seed, and then the entire wallet is restored from an old backup but using the new seed, then txs sent to new addresses in the old seed won't be added to the wallet 10:06 < raj_149> jnewbery: oh, i thought it tops up whenever adress is generated. Where it is checking if the address actually have incoming transaction? 10:07 < ryanofsky> andrewtoth, yeah that kind of scenario (old backups) seems more likely to cause a bug, than someone intentionally changing hd seed on their wallet and expecting to receive old funds from new keys generated externally 10:07 < andrewtoth> ahh i see, i didn't consider keys generated externally by that replaced seed. makes sense thanks 10:08 < jnewbery> raj_149: yes, you're right that the keypool tops up when an address is given out. It also tops up if we receive a transaction output to one of the addresses in the keypool. See the MarkUnusedAddresses() function. 10:09 < jnewbery> in this case, we're not giving out new addresses from the old HD seed 10:09 -!- LarryRuane [62f5cc94@c-98-245-204-148.hsd1.co.comcast.net] has joined #bitcoin-core-pr-reviews 10:09 < vasild> So, if I create a wallet, generate 1100 addresses, send them to some people, receive funds on 500 of them, replace the seed with sethdseed, for sure the 500 already received will be accounted for, later the remaining 600 addresses also receive funds. Would none of those 600 be accounted or just the last 100 (over 1000)? 10:09 < raj_149> jnewbery: right, missed that part. thanks.. 10:10 < ryanofsky> vasild, yes that should be right 10:10 -!- sonnet [5f5bd65f@ip5f5bd65f.dynamic.kabel-deutschland.de] has joined #bitcoin-core-pr-reviews 10:10 < vasild> which one? none of the new 600 are accounted or only the last 100 are not accounted? 10:10 < raj_149> my guess is it would miss the last 100. is that correct? 10:11 < jnewbery> vasild: in that scenario, the keypool would already have up to index 2100 (when you give out address 1100). I think a crucial part is that this happens only when you restore from an old backup 10:11 < theStack> raj_149: that would be my guess too 10:11 < troygiorshev> I guess it won't miss any of those 1100 10:12 < ryanofsky> oh, misread, yeah, none of the 1100 would be lost, it's the keys that come after those 1100, which might exist because the wallet is old backup, or there's another active wallet out with the same seed 10:12 < jonatack> * In the unlikely case where none of the addresses in the `gap limit` are 10:12 < jonatack> * used on-chain, the look-ahead will not be incremented to keep 10:12 < jonatack> * a constant size and addresses beyond this range will not be detected by an 10:12 < jonatack> * old backup. 10:12 < vasild> I see 10:12 < ryanofsky> good, so that's the general motivation for this pr, but next question is about the specific motivation 10:13 -!- michaelf_ [~textual@2a00:23c5:be01:b201:6c2c:5858:1258:3ac8] has joined #bitcoin-core-pr-reviews 10:13 < ryanofsky> 3. The original motivation for the PR was to remove the restriction that setting a new HD seed wasn’t allowed during Initial Block Download. What was the reason for this restriction? 10:13 < ryanofsky> link to the relevant code is https://github.com/bitcoin/bitcoin/commit/769b03a83c2aa2b97f344b58dc689be26c6e08e5 10:14 < achow101> hi everyone, ping me if you have questions 10:14 < raj_149> ryanofsky during IBD the old chain can get some new tx that wont be accounted for if it is replaced? 10:14 < ryanofsky> hi achow101 10:14 < jonatack> hi achow101 10:15 < jkczyz> It wasn't clear to me from the PR description 10:15 < andrewtoth> sethdseed requires a rescan right? 10:15 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:85d2:62f:72a7:cf48] has quit [Ping timeout: 265 seconds] 10:15 < ryanofsky> raj_149, right, it can happen outside of ibd, but it's more likely keypool will run out during ibd with all the historical blocks being processed 10:15 < jnewbery> I think the easiest scenario to think about where this could be a problem is something like: 10:15 < jnewbery> 1. new wallet 10:15 < jnewbery> 2. create backup 10:15 < jnewbery> 3. give out 1001 addresses. keyopol tops up to index 2001 (assuming it's not locked) 10:15 < jnewbery> 4. receive funds to 1001 addresses 10:15 < jnewbery> ... 10:15 < jnewbery> 5. restore old backup on node - only the first 1000 keypool keys are regenerated 10:15 < ryanofsky> andrewtoth, sethdseed for an existing seed can require a rescan, for a new seed shouldn't require 10:15 < jnewbery> 6. set new hd seed before the wallet has sync'ed to tip 10:15 < jnewbery> 7. the first 1000 payments are received, but payment 1001 is not because we're not able to top-up the keypool for the old seed after setting a new seed. 10:16 -!- michaelf_ [~textual@2a00:23c5:be01:b201:6c2c:5858:1258:3ac8] has quit [Client Quit] 10:17 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:6c2c:5858:1258:3ac8] has joined #bitcoin-core-pr-reviews 10:17 < ryanofsky> jkczyz, your question about this is answered? 10:17 < raj_149> ryanofsky: i couldn't follow the rescan part. where the rescan logic is triggered by sethdseed? 10:18 < ryanofsky> raj_149, normally when sethdseed is called you are just creating a brand new hdseed, so no rescan is required 10:19 < achow101> sethdseed doesn't rescan. you would rescan separately afterwards if you have/want to 10:19 < ryanofsky> if you are setting an pre-existing seed that may have transactions associated, you have to rescan manually with a separate rpc call 10:19 < raj_149> oh i see. 10:19 < ryanofsky> it might have made sense (or might make sense) to add a rescan option to sethdseed or key birthday option like we have for other imports 10:19 < ryanofsky> but that kind of leads to next question 10:20 < ryanofsky> 4. Why was the sethdseed RPC added? Are there uses for having multiple HD seeds in the same wallet? Does this change to sethdseed affect new descriptor wallets as well as existing wallets? 10:20 < troygiorshev> the fact that it can still happen after IDB is even more motivation for this PR. Part of the point of HD wallets was that you can have a "master" wallet that can spend funds and then put a receive-only wallet on a vulnerable machine. But if you change the hd seed on the master and the receive-only receives a lot of new transactions (and generates more than 1000 new addresses) then 10:20 < troygiorshev> the master will lose them 10:20 < troygiorshev> ah sorry a little late there, did I get that right? 10:21 < raj_149> troygiorshev: that seems right as the topop wont happen for old chains currently. 10:21 < ryanofsky> troygiorshev, that makes sense in principle, but in practice we don't support a receive only wallet part that can top up, because we use hardened keys 10:22 < troygiorshev> ryanofsky: ah ok 10:22 < raj_149> ryanofsky: recieve only wallet part can remian is some other system like a merchant website. 10:22 < ryanofsky> raj_149, oh good, point actually that does seem like a good motivation then 10:23 < jonatack> As troygiorshev's and jnewbery's examples show, it's not a great idea, as a user, to reduce the gap limit. 10:23 < ryanofsky> or wait, isn't that precluded because of the hardened keys? 10:23 < jnewbery> achow101: for descriptor wallets do we still only use hardened keys, or is it possible to use unhardened keys too? 10:24 < achow101> jnewbery: descriptor wallets will use unhardened derivation 10:24 < jkczyz> ryanofsky: yes, I believe so. My main qualm is the reason that "we no longer need to wait for IBD to finish before sethdseed can work" is not given 10:24 < ryanofsky> (question 4 is about future of sethdseed and descriptor wallets) 10:25 < jnewbery> if anyone is unfamiliar with hardened/unhardened key derivation, it's defined in BIP32 here: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions 10:25 < jonatack> achow101: by default we only make descriptors that use unhardened derivation, yes? 10:25 < ryanofsky> jkczyz, yeah it think that may be phrased as a binary statement when it was always a matter of precaution. but with this change there is no longer any benefit to disabling sethdseed during ibd 10:26 -!- michaelf_ [~textual@2a00:23c5:be01:b201:88e5:5024:9021:20fc] has joined #bitcoin-core-pr-reviews 10:26 < achow101> jonatack: yes. descriptor wallets use bip44/49/84 10:26 < jnewbery> specifically of note is that for hardened keys it's not possible to derive a child public key from a parent public key: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#public-parent-key--public-child-key 10:26 -!- kory [54112956@84.17.41.86] has quit [Remote host closed the connection] 10:26 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:6c2c:5858:1258:3ac8] has quit [Ping timeout: 244 seconds] 10:26 < ryanofsky> because after this change, no matter when sethdseed is call, during ibd or before, behavior for the previous hdseed will be the same 10:26 < raj_149> ryanofsky: that seems correct. Corollary question, what one needs to do to have a recieve only wallet in a merchant site and still get it tracked by core? 10:27 < achow101> raj_149: you have to export tons of addresses and constantly do that as addresses get used 10:28 -!- michaelf_ [~textual@2a00:23c5:be01:b201:88e5:5024:9021:20fc] has quit [Client Quit] 10:28 < jnewbery> achow101: why wouldn't you be able to use the xpub for an unhardened descriptor? 10:29 < jonatack> raj_149: i think it will finally become possible to export an xpub to, say, btcpayserver from a bitcoin core wallet, with descriptor wallets 10:29 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:88e5:5024:9021:20fc] has joined #bitcoin-core-pr-reviews 10:29 -!- michaelfolkson is now known as michaelfolkson2 10:29 < raj_149> jonatack: is there any specific reason why core never creates unhardened keys and only hardened ones? 10:30 < achow101> jnewbery: exports aren't implemented for descriptors yet. but once that's done, yes, you can use the xpub 10:30 < jonatack> raj_149: see https://bitcoin.stackexchange.com/questions/90135/how-do-i-export-an-xpub-from-bitcoin-core-for-use-in-btcpayserver 10:30 < achow101> but also sethdseed is disabled for descriptor wallets 10:31 < raj_149> jonatack: thanks, answers it. 10:31 < ryanofsky> just to wrap up question 4, I couldn't think of real uses cases for sethdseed anymore, and didn't check but suspect it was just added before there was multiwallet support 10:31 < jonatack> raj_149: for security reasons, bitcoin core legacy wallets don't support xpub key derivation because if an attacker knows the private key of any of the child keys and the xpub, he can compute the private key of all child keys 10:31 < jonatack> right 10:32 -!- michaelfolkson2 is now known as michaelfolkson 10:32 < achow101> ryanofsky: it's original purpose was for the upgrade from non-HD to HD, but that got cut at some point 10:32 < ryanofsky> anybody want to summarize purpose of CHDChain, CKeyMetadata, and KeyOriginInfo for question 5? 10:33 < ryanofsky> achow101, oh that makes sense. and of course it makes a lot of sense this is dropped for descriptor wallets 10:34 < raj_149> CHDChain: contans data to construct the full chain. CKeyMetadata: Contains metadata for a particular key KeyOriginInfo: contains origin and path to get to this key. 10:35 < ryanofsky> yeah important fields in CHDChain for this are nExternalChainCounter and nInternalChainCounter 10:35 < jonatack> if helpful to anyone, achow101 wrote a really helpful doc lately about descriptor wallets: https://gist.github.com/achow101/94d889715afd49181f8efdca1f9faa25 10:35 < jonatack> achow101: hope it wasn't a secret doc :p 10:35 < raj_149> ryanofsky: my idea is they just contains the max used index for the chain. is that correct? 10:36 < ryanofsky> and the key indices in KeyOriginInfo::path field are used to derive these on startup 10:36 < raj_149> jonatack: cant help anymore if it was, already starred it. :P 10:36 < ryanofsky> raj_149, right they're the max indices that summarize the state of the keypool 10:37 < ryanofsky> question 6 is about differences between the new TopUpInactiveHDChain method and existing TopUp method, anything notable there? 10:37 < ryanofsky> https://github.com/ryanofsky/bitcoin/blob/review.17681.5/src/wallet/scriptpubkeyman.cpp#L293 vs https://github.com/ryanofsky/bitcoin/blob/review.17681.5/src/wallet/scriptpubkeyman.cpp#L1194 10:38 < ryanofsky> or opinions on whether it's good to have such similar methods or merge them? 10:38 < raj_149> ryanofsky: the new one has the bool internal parameter, so tops up only a single chain. 10:39 < jnewbery> ryanofsky: The call to AddKeypoolPubkeyWithDB() has been removed from TopUpInactiveHDChain() in the latest version 10:40 < jonatack> jnewbery: that change thanks to recent review :D 10:40 < ryanofsky> raj_149, that's interesting, yeah i guess it gets to a basic difference that TopUpInactive is told specifically what to top up, while TopUp just figures it out itself 10:41 < ryanofsky> jnewbery, oh actually forgot I was pasting links to an old version 10:41 < raj_149> ryanofsky: if TopUp can have some way to know the chain in context, cant it handle the inactive topup part also? 10:42 < ryanofsky> raj_149, yes to merge the functions in an efficient way TopUp would have to be passed more information 10:42 < jkczyz> Maybe pass kp_size and chain to a common helper? Haven't looked in detail if it is possible 10:42 < achow101> iirc there was an iteration (before the pr was opened) where TopUp was being used for both, but it ended a bit too messy 10:43 < ryanofsky> yeah, point of the question was just get people looking at these functions, and see how new code compares to existing 10:43 < ryanofsky> if these are more or less clear we can go on to AddInactiveHDChain in question 7 10:43 < ryanofsky> 7. When is AddInactiveHDChain called? Where do the inactive HD chain objects come from? Is AddInactiveHDChain called everywhere it needs to be called? 10:44 < ryanofsky> there's a twist to this question in that I think there's a bug in the current version of the pr related to this 10:45 < jonatack> Good question. Only called from LoadWallet atm. 10:46 < jnewbery> ryanofsky: should this be called from sethdseed too? 10:46 < ryanofsky> jnewbery, yes that's what i was thinking at least 10:48 < jnewbery> I think you're right 10:48 < ryanofsky> is it clear to people what AddInactiveHDChain does and when it should be called? anyone want to summarize? 10:48 -!- sonnet [5f5bd65f@ip5f5bd65f.dynamic.kabel-deutschland.de] has quit [Remote host closed the connection] 10:49 -!- soup [494e6362@c-73-78-99-98.hsd1.co.comcast.net] has quit [Ping timeout: 245 seconds] 10:49 < raj_149> ryanofsky: it adds a chain to m_inactive_hd_chains. But if sethdchain requires also m_inactive_hd_chains to be updated and its not right now. How the test is passing? 10:51 < fjahr> the test does us loadwallet to check persistence 10:51 < ryanofsky> right the test isn't just calling sethdseed, it's also loading & unloading which could mask the bug 10:52 < ryanofsky> maybe skip through some other questions, unless people have thoughts / opinions on them 10:52 < jnewbery> right. The test doesn't do any operations or checking between sethdseed and unload/reload wallet 10:53 < ryanofsky> one question that might be interesting is 10: A previous version of this PR had a subtle bug on this line. What was the bug and what were the effects? 10:53 < ryanofsky> previous pr version with bug: https://github.com/ryanofsky/bitcoin/commits/review.17681.4 10:53 < ryanofsky> previous line of code with the bug: https://github.com/ryanofsky/bitcoin/blob/review.17681.4/src/wallet/walletdb.cpp#L446 10:54 < raj_149> ryanofsky: it was a bit wise and instead of or. Resulting into nothing happening. Is that correct? 10:54 < fjahr> yes but I thing only internal keys would not be extended because of this bug 10:55 < jonatack> raj_149: yes. a good catch by ryanofsky. i missed it during my first review. 10:55 < ryanofsky> raj_149 & fjahr that's right, shows why it's good to look carefully at arithmetic operations, and new code cleans this up quite a bit 10:55 < fjahr> test fails if the two lines with unload and load are commented out btw 10:56 < fjahr> *the functional test I mean, going back to previous question 10:56 < raj_149> can anyone give a one liner on what happens at loading and unloading of wallet? 10:57 < jnewbery> raj_149: there's a lot going on during load/unload. I don't think it's possible to summarize in one line 10:58 < ryanofsky> raj_149, i'd say the important thing happening is that m_inactive_hd_chains is populated on loading, by looping over all the keys and finding the max internal/external indices 10:58 -!- sonnet [5f5bd65f@ip5f5bd65f.dynamic.kabel-deutschland.de] has joined #bitcoin-core-pr-reviews 10:59 < ryanofsky> maybe people have thoughts in general on the code / pr 10:59 < ryanofsky> my thoughts are that i'm glad this stuff is going away with descriptor wallets :) 10:59 < jnewbery> ryanofsky: +1 11:00 < raj_149> ryanofsky: i need to understand descriptor wallet better. But overall the motivation made sense. The wallets need to handle old chains sensibly. 11:00 < jnewbery> ok, that's time. Thanks for hosting ryanofsky! 11:00 < fjahr> Thanks ryanofsky, great job! 11:00 < jnewbery> if anyone wants to host in the coming weeks, please message me 11:00 < ryanofsky> Thanks everyone! 11:00 < jonatack> thanks ryanofsky! 11:00 < andrewtoth> thanks ryanofsky! 11:01 < troygiorshev> thanks ryanofsky! 11:01 < theStack> thanks ryanofsky 11:01 < michaelfolkson> Thanks! 11:01 < raj_149> thanks everybody. It was a great session. For the first time it felt like i understood enough to have meaningful conversation. thats motivating. :) 11:01 < jonatack> PSA: special review club session tomorrow at the same hour 11:01 < theStack> jonatack: \o/ 11:01 < jonatack> BIP157 session 2, hosted by jnewbery 11:02 < raj_149> jonatack: whats that? and details? 11:02 -!- sonnet [5f5bd65f@ip5f5bd65f.dynamic.kabel-deutschland.de] has left #bitcoin-core-pr-reviews [] 11:02 < jonatack> raj_149: https://bitcoincore.reviews/18960 11:02 < jnewbery> right. BIP 157 part 2 is same time, same place tomorrow 11:02 < jonatack> notes and questions are up 11:03 -!- r251d [~r251d@162-196-59-192.lightspeed.irvnca.sbcglobal.net] has quit [Quit: r251d] 11:03 < andrewtoth> what makes it a special session? why not have it next week? 11:03 < michaelfolkson> I don't think John wants to commit to doing 2 review club sessions every week ;) 11:03 < jonatack> andrewtoth: i think because it's a series of sessions about bip157 that are being held in parallel 11:04 < jonatack> on thursdays 11:04 < michaelfolkson> Maybe if we get enough hosts we could 11:04 < jnewbery> andrewtoth: I'm hosting a series on BIP 157. I realise that some people aren't interested in that, so I didn't want to monopolize the review club meetings for the next 5 weeks 11:04 < andrewtoth> ok thanks 11:05 < jnewbery> the idea for review club is to expose people to a wide variety of topics, and be a place for new contributors to learn about the project as a whole. The BIP 157 series didn't seem to fit with that goal 11:05 < michaelfolkson> Makes sense 11:09 -!- theStack [~honeybadg@vps1648322.vs.webtropia-customer.com] has quit [Quit: Lost terminal] 11:22 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 11:29 -!- troygiorshev [~troy@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has left #bitcoin-core-pr-reviews [] 11:31 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 11:32 < thomasb06> wasn't the PR review today? 11:32 -!- Smeier [cfacdb18@207.172.219.24] has quit [Ping timeout: 245 seconds] 11:37 < michaelfolkson> Yes finished 30 mins ago 11:38 < thomasb06> damn, I was working on my phd and i forgot... 11:57 -!- LarryRuane [62f5cc94@c-98-245-204-148.hsd1.co.comcast.net] has quit [Ping timeout: 245 seconds] 12:40 -!- b10c [~b10c@2001:16b8:2e12:8c00:6403:a492:9cfe:36ca] has quit [Quit: Leaving] 13:11 < ja> thomasb06: what is your phd thesis on and where can i read more about it? 13:12 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:88e5:5024:9021:20fc] has quit [Ping timeout: 256 seconds] 13:15 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 13:19 < thomasb06> ja: we are trying to adapt this article to another numerical scheme: https://www.researchgate.net/publication/282328790_FDTD_transient_analysis_of_grounding_grids_A_comparison_of_two_different_thin_wire_models 13:20 < thomasb06> the idea is to compute the electromagnetic field radiated by an antenna with refining the mesh around the wire 13:21 < thomasb06> this article more precisely: https://www.infona.pl/resource/bwmeta1.element.ieee-art-000006107532 13:23 < thomasb06> This the original scheme: https://en.wikipedia.org/wiki/Finite-difference_time-domain_method 13:24 < thomasb06> and ours: https://en.wikipedia.org/wiki/Transmission-line_matrix_method 13:24 < thomasb06> *without refining the mesh around the wire 13:33 -!- lightlike [~lightlike@p200300C7EF1A0F00E55E228E8DC90D42.dip0.t-ipconnect.de] has left #bitcoin-core-pr-reviews ["Leaving"] 13:40 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 13:40 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 13:43 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Client Quit] 13:44 < thomasb06> here you have the computation of how our numerical scheme works: https://x0.at/UNB.pdf 13:45 < thomasb06> (it's not very popular, though. It's not intuitive at all) 13:47 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 13:47 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 13:49 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 13:53 -!- michaelfolkson [~michaelfo@2a03:b0c0:1:e0::23d:d001] has joined #bitcoin-core-pr-reviews 13:55 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 13:57 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:04 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 14:04 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:07 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Client Quit] 14:08 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:11 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Client Quit] 14:11 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:13 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Client Quit] 14:13 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:20 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 14:20 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:26 -!- molakala [~sumanth.b@2601:192:4701:87f0:248b:8db4:3f75:79fa] has quit [Ping timeout: 244 seconds] 14:29 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 14:29 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:41 -!- slivera [~slivera@116.206.228.243] has joined #bitcoin-core-pr-reviews 14:41 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 14:42 < ja> thanks for the links, i have some reading to do :P 14:49 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 14:53 < jonatack> thomasb06: there is a special review club session tomorrow at the same time, if you like... details at https://bitcoincore.reviews/18960 :) 14:56 < thomasb06> (ja: but I didn't redo the computation myself...) 14:58 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Quit: Textual IRC Client: www.textualapp.com] 14:58 < thomasb06> jonatack: thank you, I'll try to be there tomorrow. And still no news from Archwiki admins, maybe they are trying to optimise the compilation for Arch: https://wiki.archlinux.org/index.php/User_talk:Lahwaacz#[Bitcoin_core]_build_the_code_and_run_the_test_suites 15:01 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has joined #bitcoin-core-pr-reviews 15:34 -!- michael__ [~textual@2a00:23c5:be01:b201:51f:ad4a:41c4:e367] has joined #bitcoin-core-pr-reviews 15:34 -!- michaelf_ [~textual@2a00:23c5:be01:b201:807:4839:1991:9221] has quit [Ping timeout: 272 seconds] 15:38 -!- michael__ [~textual@2a00:23c5:be01:b201:51f:ad4a:41c4:e367] has quit [Client Quit] 15:58 -!- dfmb_ [~dfmb_@unaffiliated/dfmb/x-4009105] has quit [Read error: Connection reset by peer] 16:36 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Quit: ERC (IRC client for Emacs 26.3)] 16:36 -!- shaunsun [shaunsun@gateway/vpn/privateinternetaccess/shaunsun] has quit [Ping timeout: 260 seconds] 17:48 -!- ghost43_ [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 17:49 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Ping timeout: 240 seconds] 18:39 -!- seven_ [~seven@2a00:ee2:410c:1300:5d75:db96:5379:1e71] has quit [Read error: Connection reset by peer] 19:37 -!- shesek [~shesek@unaffiliated/shesek] has quit [Read error: Connection reset by peer] 19:38 -!- shesek [~shesek@185.3.145.28] has joined #bitcoin-core-pr-reviews 19:38 -!- shesek [~shesek@185.3.145.28] has quit [Changing host] 19:38 -!- shesek [~shesek@unaffiliated/shesek] has joined #bitcoin-core-pr-reviews 20:55 -!- mol_ [~mol@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 20:55 -!- mol [~mol@unaffiliated/molly] has quit [Ping timeout: 272 seconds] 21:19 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 21:23 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 21:23 -!- vasild_ is now known as vasild 22:44 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 260 seconds] 23:14 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has joined #bitcoin-core-pr-reviews 23:28 -!- rjected [~dan@pool-71-184-77-198.bstnma.fios.verizon.net] has quit [Ping timeout: 260 seconds] 23:36 -!- rjected [~dan@pool-71-184-77-198.bstnma.fios.verizon.net] has joined #bitcoin-core-pr-reviews 23:36 -!- jesseposner [~jesseposn@c-67-188-220-154.hsd1.ca.comcast.net] has quit [Ping timeout: 265 seconds]