--- Day changed Wed Apr 08 2020 00:41 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 00:44 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 00:44 -!- vasild_ is now known as vasild 01:12 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 01:37 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Ping timeout: 240 seconds] 03:03 -!- Julian69Eichmann [~Julian69E@ns334669.ip-5-196-64.eu] has joined #bitcoin-core-pr-reviews 03:08 -!- Julian69Eichmann [~Julian69E@ns334669.ip-5-196-64.eu] has quit [Ping timeout: 265 seconds] 03:50 -!- molly [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 03:54 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 256 seconds] 04:08 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 04:19 -!- febrocas [~pedrocas@a95-92-178-39.cpe.netcabo.pt] has joined #bitcoin-core-pr-reviews 04:19 -!- febrocas [~pedrocas@a95-92-178-39.cpe.netcabo.pt] has quit [Read error: Connection reset by peer] 04:26 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Ping timeout: 240 seconds] 05:24 -!- dr-orlovsky [~dr-orlovs@xdsl-188-155-161-135.adslplus.ch] has joined #bitcoin-core-pr-reviews 06:06 -!- ghost43_ [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 06:07 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Ping timeout: 240 seconds] 06:13 -!- ghost43_ [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 06:15 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 06:15 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 06:23 -!- dr-orlovsky [~dr-orlovs@xdsl-188-155-161-135.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 06:35 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 07:03 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 07:05 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 07:20 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 07:21 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 07:42 -!- molly [~molly@unaffiliated/molly] has quit [Ping timeout: 265 seconds] 08:04 -!- hebasto [~hebasto@95.164.65.194] has quit [Quit: ZNC 1.6.6+deb1ubuntu0.2 - http://znc.in] 08:07 -!- hebasto [~hebasto@95.164.65.194] has joined #bitcoin-core-pr-reviews 08:07 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 08:08 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 08:25 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 08:26 -!- ghost43 [~daer@gateway/tor-sasl/daer] has quit [Remote host closed the connection] 08:26 -!- ghost43 [~daer@gateway/tor-sasl/daer] has joined #bitcoin-core-pr-reviews 09:06 < willcl_ark> Meeting in 55 minutes, right? 09:12 < pinheadmz> willcl_ark: yeah think so ! 09:15 -!- vindard [~vindard@190.83.165.233] has quit [Ping timeout: 250 seconds] 09:16 < jonatack> willcl_ark: right :) 09:18 -!- vindard [~vindard@190.83.165.233] has joined #bitcoin-core-pr-reviews 09:23 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:24 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 09:24 < pinheadmz> was hoping to tune into blockstream webinar first but they are running so late ! 09:29 -!- andrewtoth_ [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 09:31 -!- theStack [~theStack@81.223.165.6] has joined #bitcoin-core-pr-reviews 09:43 < jonatack> pinheadmz: it might be posted on youtube afterward? 09:44 < pinheadmz> its cool, I hung in as long as i could but i found it a little boring :-) 09:44 < pinheadmz> running multiwallet tests now :-) 09:46 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 09:49 < jonatack> pinheadmz: nice :) 09:49 < jonatack> well get started in about 10 minutes 09:50 < jonatack> since the country went into forced lockdown 22 days ago, the internet has really become slow 09:50 < jonatack> if i end up being cut off, just carry on :) 09:54 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:24e5:9ce9:b92f:4c1b] has joined #bitcoin-core-pr-reviews 09:56 -!- ecurrencyhodler [68ac287f@cpe-104-172-40-127.socal.res.rr.com] has joined #bitcoin-core-pr-reviews 09:57 -!- danny [ac5c056c@172.92.5.108] has joined #bitcoin-core-pr-reviews 09:58 -!- danny [ac5c056c@172.92.5.108] has left #bitcoin-core-pr-reviews [] 09:59 -!- robot-visions [ac5c056c@172.92.5.108] has joined #bitcoin-core-pr-reviews 09:59 -!- kevin_gislason [49777ce7@c-73-119-124-231.hsd1.ma.comcast.net] has joined #bitcoin-core-pr-reviews 10:00 < jonatack> #startmeeting 10:00 < jonatack> Hi all! Welcome to this week's episode of the Bitcoin Core PR Review club. 10:00 < jonatack> Feel free to say "hi" to let everyone know you're here, even if you arrive in the middle of the meeting. 10:00 < jnewbery> hi 10:00 < jonatack> Special welcome to everyone here for the first time today. 10:00 < emzy> Hi 10:00 < kevin_gislason> hi 10:00 < robot-visions> hi 10:00 < pinheadmz> hi! 10:00 < jonatack> I'd like to encourage you to not be shy. Jump in at any time to comment or ask questions. There are no dumb questions here. We're all here to learn. 10:00 < jkczyz> hi 10:00 < jonatack> topic This week, we are looking at PR #18453, which for the moment is called "rpc, cli: add multiwallet balances rpc, use it in -getinfo, no longer depend on getwalletinfo balance." 10:00 < vasild> Hi 10:00 < theStack> hi 10:00 < michaelfolkson> hi 10:00 < jonatack> Notes and questions in the usual place: https://bitcoincore.reviews/18453 10:00 < ecurrencyhodler> hi 10:00 < kanzure> hi 10:00 < jonatack> This week's PR is relatively approachable to review. 10:00 < andrewtoth> hi 10:00 < fjahr> hi 10:00 < jonatack> It should be easy to manually test by building, launching bitcoind, and then running `bitcoin-cli -getinfo` on the command line, preferably with multiple wallets loaded (you can use the `createwallet` and `loadwallet` RPCs to do that, if needed). 10:00 < nehan_> hi 10:00 < jonatack> Yet the PR also brings up several practical topics on Bitcoin Core API process and design. 10:00 < jonatack> - API removal: what is the process for deprecating APIs? 10:00 < willcl_ark> hi 10:01 < jonatack> - Adding APIs: when should we add an RPC call versus extending an existing one? 10:01 < jonatack> - Multiwallets: how should we extend the API to handle multiple wallets? 10:01 < jonatack> Today's PR is a great example of Thing Leads To Another (TM). 10:01 < jonatack> Experienced review is scarce, and one consequence of that is when a particular section of code is being touched and reviewed, it is often an opportune moment to make -- or be asked to make -- changes touching the same code, while effort and eyes are on it. 10:01 < jonatack> This PR originally began as a single commit to enable `-getinfo` to call `getbalances`, in order to no longer depend on the `getwalletinfo` balance response that was marked as deprecated a year earlier. 10:01 < jonatack> Then Bitcoin Core maintainer laanwj proposed, while this code was being updated, to add displaying the balance per wallet. The review comment is here: https://github.com/bitcoin/bitcoin/pull/18453#issuecomment-605431806 10:01 < jonatack> Bitcoin CLI calls are in src/bitcoin-cli.cpp: https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp 10:01 < jonatack> The -getinfo code is in the GetinfoRequestHandler class starting at line 224: https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L224 10:01 < jonatack> Under the hood, `-getinfo` performs multiple requests to various RPC calls (namely `getnetworkinfo`, `getblockchaininfo`, and `getwalletinfo`) and presents the results in a (hopefully) user-friendly output. 10:01 < jonatack> While adding multiwallet balances to -getinfo, it seemed to make sense to do it by adding a new dedicated RPC call. 10:01 < jonatack> Now... changing a CLI call is one thing; but changing the RPC API is quite another: the latter is subject to API stability constraints 10:02 < jonatack> which anyone who has ever maintained a legacy codebase with a large user base long enough to have battle scars from the experience can understand ;) 10:02 < jonatack> Let's get started! 10:02 < jonatack> Who had a chance to review the PR this week? Did you build and test it manually with multiple wallets? 10:02 < robot-visions> (y) 10:02 < pinheadmz> yeah, but having issues with multiwallet 10:02 < vasild> y 10:02 < fjahr> y and y 10:02 < jnewbery> 0.5 y review, but no testing 10:02 < willcl_ark> y and y 10:03 < vasild> pinheadmz: what issues? 10:03 < nehan_> a little review, started testing but didn't get far yet 10:03 < pinheadmz> well i created a second wallet, restarted bitcoind with -wallet=a adn -wallet=b 10:03 < pinheadmz> but when i call -getingo it shows me NO balances 10:03 < pinheadmz> unless i specify -rpcwallet=a or b 10:03 < pinheadmz> then it only shows me the one balance 10:04 < vasild> maybe you need "loadwallet"? 10:04 < theStack> did code-review, but no testing yet 10:04 < jonatack> anyone else have issues with multiwallets? 10:04 < jonatack> Question 2: Is the `getunconfirmedbalance` RPC deprecated? How about the `getwalletinfo` RPC "balance" fields? Explain. 10:04 -!- Lola_Dam [56c00d0d@lfbn-ami-1-546-13.w86-192.abo.wanadoo.fr] has joined #bitcoin-core-pr-reviews 10:04 < vasild> getunconfirmedbalance: yes - DEPRECATED\nIdentical to getbalances().mine.untrusted_pending 10:04 < robot-visions> Both `getunconfirmedbalance` and the `getwalletinfo` balance fields have been "soft deprecated", because the documentation indicates that they are deprecated, but `-deprecatedrpc=` is not yet needed to use them. 10:04 < vasild> getwalletinfoybalance fields: yes - DEPRECATED. Identical to getbalances().mine.* 10:04 < jnewbery> pinheadmz: (forgive the basic question) are you definitely running a newly built bitcoin-cli? (I've made mistakes before where I've rebuilt bitcoind but accidentally called the old bitcoin-cli executable) 10:05 < pinheadmz> yes i thought of that and just re-maked 10:05 < pinheadmz> hm but might not be on my pth, hang on 10:05 < pinheadmz> jonatack: those are both deprecated RPCs 10:05 < jonatack> pinheadmz: yes, or sometimes from running the command from root and not /src 10:05 < pinheadmz> but so is `getinfo` -- so I'm curious why bother adding features to it ? 10:05 < jonatack> (at least i've done that) 10:06 < theStack> from what i found out the `getunconfirmedbalance` deprecation was introduced with the introduction of the `getbalances` rpc (https://github.com/bitcoin/bitcoin/pull/15930) 10:06 < jonatack> robot-visions: correct! 10:06 < theStack> s/introduced/done 10:06 < jonatack> the commit *says* they are deprecated 10:06 < pinheadmz> jonatack: thanks, src/bitcoin-cli and src/bitcoind totally my bad 10:06 < jonatack> and the *do* have warnings in the help 10:07 < jonatack> but they are still usable without the need to pass the deprecation flag in the .conf file or when launching 10:08 < jonatack> TBH this state can be a bit confusing 10:08 < sipa> when were they made deprecatedm 10:08 < sipa> ? 10:08 < sipa> also: hi! 10:08 < vasild> maybe somebody (some app) could be using them and never noticed they are deprecated because apps don't read the help 10:08 < jonatack> and I think one of three things should happen: 10:08 < jnewbery> in my experience, the best assumption to make is that no user will ever read the help text or documentation, nor will they change client behaviour if there's a warning about deprecation. I think writing "deprecated" in help text is next-to-useless if the command is not functionally deprecated. 10:09 < MarcoFalke> That is my fault. I should not have added DEPRECATED to the doc 10:09 < sipa> vasild: yeah, we don't remove deprecated things without first going through a -deprecatedrpc cycle 10:09 < willcl_ark> What is the general idea behind deprecated `getinfo` being moved to `-getinfo`? 10:09 < jnewbery> great question, willcl_ark! 10:09 < jonatack> (a) deprecate them for real, (b) remove the warnings, or (c) update the doc to mention possible 2-stage ("soft then hard") deprecation process for some calls (like entire calls) 10:10 < jonatack> *for some things (like entire calls) 10:10 < jonatack> sipa: MarcoFalke: hi! 10:10 < MarcoFalke> hi 10:10 < pinheadmz> jonatack: but why add new features like multiwallet balance statistics to a deprecated rpc ? 10:11 < jnewbery> `getinfo` used to be a server-side RPC method. It was a PITA because it accessed data from many different components (wallet, node, etc) and locked everything up while it did that 10:11 < ecurrencyhodler> So are you saying people tend to look at release notes only if their app breaks because an rpc has been removed? 10:11 < theStack> -getinfo as cli option is not deprecated as far as i see it? 10:11 < jnewbery> it was deprecated and finally removed from the server a few releases ago 10:11 < jonatack> willcl_ark: my guess is that it is because RPC calls are intended to be consumed by applications, not people 10:11 < sipa> theStack: it is not 10:11 < sipa> ecurrencyhodler: even that is a stretch ;) 10:12 < jnewbery> however, the functionailty is useful for people (and particularly developers and testers), so a bitcoin-cli client-side function called getinfo was added to deliver that functionality 10:12 < jonatack> willcl_ark: as so getinfo is better as a CLI call for use by humans which frees it from API stability constraints 10:12 < jnewbery> bitcoin-cli -getinfo makes multiple RPC requests to the server and presents a nice summary to the user 10:12 < pinheadmz> i see, so the server getinfo was removed, but on the CLI side, getinfo is still useful as a sort of macro that wraps several other calls 10:12 < willcl_ark> jnewbery: hmm ok. I see. I always liked the command so was happy it was not "removed" completely 10:12 < willcl_ark> thanks all 10:12 < jnewbery> pinheadmz: exactly 10:13 < jonatack> pinheadmz: RPC getinfo was deprecated in favor of CLI -getinfo (with a dash) 10:13 < jonatack> pinheadmz: yes 10:13 < robot-visions> In practice, does making `-getinfo` "non atomic" introduce any gotchas? 10:13 -!- Prayank [c6fc991c@198.252.153.28] has joined #bitcoin-core-pr-reviews 10:13 < willcl_ark> Ok I see it now, it makes 4 calls, and aggregates them 10:13 < kevin_gislason> What does a dash mean tbh? 10:14 < sipa> robot-visions: it may mean that the wallet balance isn't for the exact block reported 10:14 < theStack> (a little off-topic, but i always found arguments with more than one letter but *not* having two dashes confusing...) 10:14 < jnewbery> ecurrencyholder: that's my experience from bitcoind and other projects I've worked on 10:14 < sipa> theStack: feel free to use two dashes; i think that works too 10:14 < MarcoFalke> jup, should work as well 10:14 < jonatack> jnewbery: thanks, good point about why it was deprecated as an rpc 10:15 < theStack> sipa: MarcoFalke: ah, indeed it does 10:15 < ecurrencyhodler> thanks john and sipa for answering. 10:16 < jnewbery> It should be possible to make it 'atomic' - have every RPC call return the block that it's reporting info for, and then ensure that all of them are the same before aggregating the data. 10:16 < MarcoFalke> jnewbery: Is this a "good first issue"? :)) 10:16 < jnewbery> it could be! Probably worth asking around for concept ACKs first 10:17 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has quit [Remote host closed the connection] 10:17 < vasild> infinite loop (during IBD) 10:17 < MarcoFalke> Concept ACK 10:17 -!- kristapsk [~KK@gateway/tor-sasl/kristapsk] has joined #bitcoin-core-pr-reviews 10:17 < jonatack> Question 3: Give an example of a recent Bitcoin Core API deprecation? And removal? 10:17 < robot-visions> 3. The "size" field of mempool related RPCs was deprecated in [#15637](https://github.com/bitcoin/bitcoin/pull/15637) and will be removed in [#18493](https://github.com/bitcoin/bitcoin/pull/18493) 10:17 < MarcoFalke> Concept ACK on returning the block, no opinion on the cli change 10:17 < jnewbery> vasild: yeah, you'd need to be careful. Perhaps only retry a few times and then report that tip height is changing rapidly so data could be inconsistent? 10:17 < michaelfolkson> wallet generate deprecated and removed 10:17 < theStack> not sure if that counts as "recent", but i found that the `generate` RPC was deprecated in 0.18 and finally removed in 0.19.1 10:18 < jonatack> all good 10:18 < vasild> jnewbery: +1 10:18 < andrewtoth> labels were all deprecated fairly recently 10:19 < jonatack> rpc bumpfee's totalFee argument was removed e.g. no more fee bumping using total fee 10:19 < andrewtoth> getbalance for getbalances 10:19 < jonatack> andrewtoth: rpc getaddressinfo label yes, labels was modified 10:19 < andrewtoth> ah right 10:20 < willcl_ark> Is an "immature" balance "trusted" or "untrusted"? 10:20 < andrewtoth> willcl_ark could be either 10:20 < vasild> willcl_ark: neither one 10:20 < vasild> :) 10:20 < willcl_ark> either and neither :) 10:20 < jonatack> label was superseded by labels, and labels went from being a JSON object containing name and purpose, to an array containing the name 10:20 < jnewbery> There have been many deprecations and removals over the last 3 or 4 releases, I think mainly because for years things were marked 'DEPRECATED' and then never actually deprecated/removed so there was a backlog and we're finally paying down that debt 10:20 < sipa> immature balances are unspendable, by consensus rules 10:21 < sipa> trusted/untrusted ones are (but with the untrusted ones you need to be careful) 10:21 < willcl_ark> thanks sipa. I don't think I've ever seen one of those. I guess it's mainly coinbase transactions then? 10:21 < jonatack> willcl_ark: run rpc getbalances 10:21 -!- Prayank [c6fc991c@198.252.153.28] has quit [Remote host closed the connection] 10:21 < sipa> willcl_ark: it is solely coinbase transactions 10:21 < sipa> willcl_ark: with leas than 100 confirmatiins 10:22 < willcl_ark> That explains why then. 10:22 < vasild> nLockTime? 10:22 < sipa> (sorry, phone typing) 10:22 < theStack> vasild: not having anything to do with nLockTime as far as i know 10:22 < jonatack> willcl_ark: ah, misunderstood your question 10:22 < sipa> vasild: non-final transactions are something else 10:22 -!- hanhua [63687d99@99-104-125-153.lightspeed.sntcca.sbcglobal.net] has joined #bitcoin-core-pr-reviews 10:23 < sipa> vasild: they can't enter the mempool until they are minable 10:23 -!- hanhua [63687d99@99-104-125-153.lightspeed.sntcca.sbcglobal.net] has quit [Remote host closed the connection] 10:23 -!- Prayank [310e82b5@49.14.130.181] has joined #bitcoin-core-pr-reviews 10:23 < vasild> I see 10:23 < MarcoFalke> In which case they won't enter the wallet either, or at least the wallet doesn't "count" them 10:25 < jonatack> re jnewbery's point about debt: 10:25 < sipa> arguably CSV transaction outputs to our wallet could be given a similar treatment - but since those aren't (yet) supported it isn't an issue 10:25 < vasild> So, coinbase transactions cannot be spent if they have less than 100 confirmations, by consensus rules? 10:25 < sipa> vasild: correct 10:26 < vasild> ack 10:26 < sipa> it may be 101, unsure 10:27 < jonatack> how much of the accumulated debt was a result of backlash from the community on deprecating too quickly? 10:27 < jonatack> (versus just no one bothering with it) 10:28 < sipa> for a very long time in bitcoin core's history, rpc were never ever deprecated or removed or broken in impactful ways 10:28 < MarcoFalke> I don't think we have great feedback mechanisms for this kind of thing in general. Though, account removal had a great backlash 10:28 < sipa> i don't think that was as a result of user feedback, just an abundance of caution 10:28 < andrewtoth> ah so an immature balance can never become untrusted, but can mature into trusted? 10:28 < jnewbery> jonatack: I think it was a healthy caution about breaking things for users. We didn't have the -deprecatedrpc staged deprecation framework until ~0.16 10:29 -!- Prayank [310e82b5@49.14.130.181] has quit [Remote host closed the connection] 10:29 < sipa> andrewtoth: correct 10:29 < kevin_gislason> Only tangentially related, but why doesn't the bitcoin core RPC use encryption? 10:30 < sipa> kevin_gislason: it's only intended to be exposed to local, trusted, clients 10:30 < jonatack> sipa: MarcoFalke: jnewbery: thanks 10:30 < sipa> encrypting it (which was supported for a while) made people incorrectly assume it was safe to expose to the internet 10:30 < vasild> what if a miner sends his reward to me after 50 confirmations, would that tx be allowed in mempool? 10:30 < pinheadmz> kevin_gislason: bcoin and btcd offer rpc over SSL, but to sipas point its not very safe 10:30 < sipa> vasild: no 10:31 < vasild> ok 10:31 < kevin_gislason> sipa why wouldn't encrypted rpc be safe to expose to the internet tbh? 10:32 < sipa> kevin_gislason: because RPC is not safe to expose to untrusted parties 10:32 < emzy> Also you would need again SSL/TLS in bitcoind. 10:32 < sipa> it may have DoS vulnerabilities 10:32 < sipa> it's just not designed to be used by untrusted parties 10:32 < andrewtoth> it's not safe to expose period, and encrypting gave a false sense that it was 10:32 < sipa> you can use stunnel if you really want to encrypt it 10:32 < michaelfolkson> But DoS are the only vulnerabilities? 10:33 < sipa> michaelfolkson: maybe? 10:33 < nehan_> jonatack: how are #18451 and #18453 related? Did you start with the idea of fixing up deprecations? 10:33 < kevin_gislason> Hmm interesting, makes sense 10:33 < MarcoFalke> michaelfolkson: It used to be possible to guess the rpcpassword with a timing attack. Though now that is fixed. 10:34 < MarcoFalke> And obvioulsy some users use rpcpassword=123456 10:34 < MarcoFalke> No need for a timing attack there 10:34 < michaelfolkson> Interesting, thanks. Yup that is a concern 10:34 < ecurrencyhodler> Quick question back to the get-info PR. 10:34 < jonatack> nehan_: yes! i (and 2 other contributors) tripped up a bit on the deprecation process lately 10:35 < sipa> vasild: generally the mempool only accepts transactions that would be legal to include in the subsequent block 10:35 < ecurrencyhodler> Do I have this correct? bitcoin-cli -getinfo means it's client side and so user can access it via terminal. But bitcoin-cli getinfo is on the server side and intended for apps. bitcoin-cli getinfo command included several other rpc calls which prevented the app from accessing them again until the task was completed. 10:35 < willcl_ark> So will this PR get held up waiting for an "across all wallets" RPC naming convention (favoured in the comments), or is it fine to go in and get modified with that later change? 10:35 < sipa> ecurrencyhodler: i don't understand the second half of your sentence 10:36 < sipa> willcl_ark: well, up to reviewers! 10:36 < pinheadmz> ecurrencyhodler: think you might be confusing RPC calls to bitcoind and CLI commands 10:36 < ecurrencyhodler> Ah ic 10:36 < willcl_ark> everyone got plenty of time for bikeshedding whilst under lockdown :) 10:36 < jonatack> willcl_ark: i think that depends if the rpc should be public 10:36 < ecurrencyhodler> Okay will need to read more. Thanks. 10:37 < jonatack> ISTM that once an RPC is part of the public API, stability constraints are in force 10:37 < sipa> ecurrencyhodler: the getinfo RPC commamd was just one RPC, but it reached into various otherwise unrelated subsystems in the code, which was messy 10:38 < ecurrencyhodler> sipa: Thanks for clarifying that for me. 10:38 < jnewbery> ecurrencyhodler: bitcoin-cli is a very simple client-side application. The starting point in the code is src/bitcoin-cli.cpp 10:38 < sipa> ecurrencyhodler: so it was split into various more single-purpose RPCs (getblockchaininfo, getwalletinfo, getnetworkinfo, ...), and a CLI command (-getinfo) was added that just calls all those RPCs and aggregates the result, inside bitcoin-cli 10:38 < jonatack> Question 4: *You are the PR author:* how would you personally implement laanwj's request to add multiwallet balances to -getinfo? 10:39 < jonatack> Would you have done it in bitcoin-cli.cpp? 10:39 < vasild> The "getbalances" output is already fixed to display a single/default wallet data. I would introduce a new multi-wallet RPC that would eventually replace "getbalances". 10:39 < jnewbery> jonatack: a general principal in software projects is that it's *much* *much* harder to remove something than to add it, so I think we should generally be cautious about adding functionality to the RPC interface 10:39 < jonatack> jnewbery: +1 10:40 < nehan_> jnewebery: +1 10:40 < nehan_> jnewbery: +1 10:40 < robot-visions> 4. It looks like `getbalances` makes some assumptions that depend on working with a single wallet: (i) the result doesn't have any fields that support multiple baances, (ii) the RPC errors if multiple wallets are loaded and you don't specify one. I don't yet see a good case for completely overhauling `getbalances` to handle multiple wallets, so I 10:40 < robot-visions> agree with jonatack's approach of adding a new RPC. 10:40 < vasild> in bitcoin-cli.cpp - no because then it cannot be reused by RPC users 10:40 < jnewbery> once users start relying on a feature, even if it's marked 'experimental' or 'debug-only' or whatever, they'll be upset if you try to remove or change it 10:41 < nehan_> jonatack: is your next step here to actually remove usage of getwalletinfo() in the tests? 10:41 < robot-visions> (That being said, I also agree with jnewbery's caveat about adding vs. removing so I'm not sure in the end) 10:41 < willcl_ark> jonatack: yes. It seems to make sense to have small, logical RPC commands, then implement more complex stuff in bitcoin-cli.cpp 10:41 < theStack> i agree to vasild that bitcoin-cli.cpp is not the proper place, for the same reason 10:41 < jnewbery> vasild: this functionality is precisely for bitcoin-cli users. I don't think we want it re-used by other applications 10:42 < willcl_ark> I think, if you are interfacing with the RPC you are able to (easily?) re-create any cli-specific stuff you want 10:42 < jonatack> nehan_: i think getunconfirmedbalances and the getwalletinfo balance fields should be deprecated 10:42 < vasild> jnewbery: you mean getting the balances of all wallets? 10:43 < jnewbery> vaslid: yes. In the current PR the new RPC is hidden precisely because we don't want other clients to use it 10:43 < jonatack> jnewbery: yes 10:44 < nehan_> jonatack: ah, just those fields, right 10:44 < jonatack> while getting feedback i also didn't see any point in adding documentation unless people want a public rpc 10:44 < jnewbery> jonatack: my preferred approach is to implement this entirely in bitcoin-cli by looping over the wallets, rather than making changes to the RPC server 10:44 < vasild> "get balances of all wallets" looks like a good RPC candidate to me, why not? 10:45 < jonatack> nehan_: and maybe(?) getbalance at some point (superseded by getbalances) 10:45 < jonatack> jnewbery: like what promag suggested in the PR? 10:46 < jnewbery> your proposed new RPC `getwalletbalances` is the only RPC that exposes multiple wallets. There's `listwallets` but that only lists the wallets and whether they're loaded, rather than interact with wallet functionality. 10:46 < vasild> I think either completely in bitcoin-cli.cpp or a public documented RPC. I don't like the idea of a "hidden" RPC because people will find it and will use it. 10:47 < sipa> jnewbery: i don't know why it's intended to be hidden from other applications? 10:47 < jonatack> jnewbery: is your approach to begin this way and attempt adding multiwallet rpcs separately? or that it there is no need for an rpc at all 10:47 < MarcoFalke> Does it scale if we add for each wallet call another call that does the same for all wallets? 10:47 < jnewbery> (there's no way for you to know this since it's not documented anywhere, but) some people would like to add per-wallet permissions (https://github.com/bitcoin/bitcoin/pull/10615), and there's another proposal to move the wallet to a separate process with its own RPC server (https://github.com/bitcoin/bitcoin/pull/10102). I think both would be more difficult if we started introducing RPCs 10:47 < jnewbery> that access multiple wallets. 10:47 < willcl_ark> (once you understand it) it seems easier to reason that each `wallet` command refers to a single wallet. Although the comments on the PR about adding specific `allwallets*` commands or endpoint also seems reasonable 10:48 < theStack> so the alternative idea would be that users make one RPC call per wallet and assemble the informations themselves? 10:48 -!- vindard [~vindard@190.83.165.233] has quit [Ping timeout: 264 seconds] 10:48 < jnewbery> so I think the current model of RPCs only being able to access the wallet that they were called for is something we should strive to keep 10:49 < sipa> jnewbery: in that case maybe it's better to not have a separate RPC at all, and have bitcoin-cli assembly the information? 10:49 < wumpus> I'd also prefer this to be a client-only feature in bitcoin-cli at most 10:49 < jnewbery> sipa: yes. That's what I'm proposing 10:49 < wumpus> +1 sipa that's how I initially meant it as well 10:50 < sipa> the per-wallet permissions/endpoints/separation argument would favor that approach 10:50 < wumpus> I'm not sure why people suddenly want to change the server side, this was about -cli imporovements 10:50 < jnewbery> MarcoFalke: I'd be a concept NACK on that for the permissions/endpoints argument 10:50 < willcl_ark> so 27b81b25ab6c176ba84f69bf9c00fed13c2dca30 should move getbalances logic to bitcoin-cli.cpp? 10:50 < jnewbery> wumpus: it seems like a cleaner solution if you don't have the context of permissions/endpoints 10:51 < wumpus> I think it's useful if bitcoin-cli can show the balances for multiple wallets, if the complexity of this prohibitive, please just drop the entire idea 10:51 < jonatack> wumpus: i thought the PR as-is it quite simple (except for the additional rpc, heh) 10:52 < jnewbery> wumpus: I don't think that's it's too complex. We're using this meeting to explore different ideas 10:53 < vasild> how about per wallet permissions and bitcoin-cli.cpp "get all wallet balances"? How would it behave? 10:53 < wumpus> I know but I"m alomost sorry for suggesting it now 10:53 < jonatack> maybe can revert back to the initial commit, and explore doing it in bitcoin-cli.cpp in a follow-on PR 10:53 < willcl_ark> Apart form making more RPC calls, I don't see any difference about having it in bitcoin-cli.cpp vs rpcwallet.cpp? 10:53 < jonatack> wumpus: i think in the worse case it's a good opportunity for everyone to learn/discuss 10:54 < nehan_> so, to summarize (do i have everything?) in favor of new rpc: cleaner and simpler in favor of bitcoin-cli: server-side might make future wallet/server separation harder; might want to add permissioned wallets, 10:54 < michaelfolkson> Yup agreed. I'm enjoying the discussion 10:54 < jnewbery> jonatack: +1 10:54 < ecurrencyhodler> jonatack: +1 I'm learning a lot. 10:55 < robot-visions> nehan_: one more point in favor of bitcoin-cli, avoid adding to the API in a way that's hard to remove later? 10:55 < jnewbery> willcl_ark: for nodes with multiple wallets, those wallet's RPC methods are accessed on a separate RPC endpoint. That maps nicely to the permissions/multiprocess model - if you used this endpoint, then you can access this wallet. 10:55 < jonatack> robot-visions: indeed, i think that's the main worry 10:55 < nehan_> robot-visio: good point! at least it's a hidden rpc. 10:56 < jnewbery> the proposed RPC in this PR adds an RPC method to the global node endpoint which can access all the wallets. That breaks the mapping above (which again, is not well documented anywhere so there's no reason jonatack should have known this) 10:56 < nehan_> it occurred to me that doing it bitcoin-cli side might make it harder to get consistency across wallets, though I'm not sure that's really true (the try again method could be used there; if at some point someone actually *wants* to lock the server to get a consistent snapshot that could only be done with a server-side rpc) 10:56 < wumpus> the idea of a hidden RPC with specific API for our client is a bit strange, other people are bound to start using it too, if it can be avoided that would be preferable 10:57 < pinheadmz> this hidden rpc is still accessible say from curl: 10:57 < wumpus> do we really need this kind of 'private APIs' 10:57 < pinheadmz> curl a:b@127.0.0.1:18443 -X POST --data '{"method":"getwalletbalances"}' 10:57 < jonatack> agreed -- i think it should be public or not at all 10:57 < jnewbery> wumpus: we already have hidden RPCs, but I guess those are mostly for testing? 10:57 < wumpus> jnewbery: they are for testing only 10:58 < wumpus> even there I don't really like them, but testing is really important 10:58 < jonatack> and it would need careful thinking... so doing it in bitcoin-cli.cpp is certainly much less risk 10:58 < nehan_> if my summary is roughly correct i'd vote cli side. 10:58 < wumpus> (it's hard to avoid that) 10:58 < wumpus> also the significant ones of the testing calls only work for regtest I think? 11:00 < jonatack> any last thoughts/questions? 11:00 < jonatack> #endmeeting 11:00 < ecurrencyhodler> Thanks jonatack for hosting! 11:00 < robot-visions> What are some common RPC use cases to consider (block explorers, 3rd-party wallets, etc.)? Who are the clients that will be affected by hard deprecations / removals? 11:00 < jonatack> thanks everyone! 11:00 < vasild> Thanks! 11:00 < jnewbery> nehan_: Good summary, although "server side - cleaner and simpler" is a bit of a subjective call. We could argue that minimizing the changes to the server is simpler/cleaner. 11:00 < pinheadmz> thanks jon, great work and thanks for your time 11:00 < kevin_gislason> Thanks all, it's been interesting 11:00 < nehan_> thanks jonatack! 11:00 < theStack> thanks for hosting jon 11:00 < michaelfolkson> 4 seconds for last thoughts and questions? ;) 11:00 < andrewtoth> thanks jonatack! 11:00 < nehan_> jnewbery: true! 11:00 < jonatack> michaelfolkson: sure 11:00 < robot-visions> Thanks jonatack, this was my first PR Review Club, I found it very interesting and educational! 11:01 < michaelfolkson> Haha thanks jonatack. Great meeting 11:01 < emzy> Thanks jonatack for hosting 11:01 < jonatack> robot-visions: keep coming! hope to see you at the next ones 11:01 < willcl_ark> thansk jonatack 11:01 -!- kevin_gislason [49777ce7@c-73-119-124-231.hsd1.ma.comcast.net] has quit [Remote host closed the connection] 11:01 < wumpus> yes thanks jonatack 11:01 < jonatack> if anyone is interested in hosting, or has ideas for good PRs to cover 11:02 < jonatack> don't hesitate to post here or to comment in 11:02 < jnewbery> We need a host for next week. Don't be shy! 11:02 < jonatack> https://github.com/bitcoin-core-review-club/website/issues/14 11:03 -!- ecurrencyhodler [68ac287f@cpe-104-172-40-127.socal.res.rr.com] has quit [Remote host closed the connection] 11:03 < jonatack> +1 (and you learn much more from hosting!) 11:03 -!- Lola_Dam [56c00d0d@lfbn-ami-1-546-13.w86-192.abo.wanadoo.fr] has quit [Remote host closed the connection] 11:05 < vasild> Hmm, for some reason I thought that https://github.com/bitcoin/bitcoin/pull/17994 "validation: flush undo files after last block write" was already discussed here, but it has not been. 11:06 < vasild> maybe we can do it some of the next weeks, I played with it somewhat. 11:08 -!- robot-visions [ac5c056c@172.92.5.108] has quit [Remote host closed the connection] 11:09 < MarcoFalke> jnewbery: So I can't host? :( 11:13 < jonatack> vasild: i've been wanting to add a kallewoof pr for some time... don't hesitate to host one! 11:16 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 250 seconds] 11:17 -!- vindard [~vindard@200.7.90.162] has joined #bitcoin-core-pr-reviews 11:22 < jnewbery> MarcoFalke: I thought you wanted to host on April 15? 11:23 < MarcoFalke> Isn't that next week? 11:23 < MarcoFalke> Today is April 8th, no? 11:24 < jnewbery> oh yeah 11:24 < jnewbery> losing track of days here 11:24 < MarcoFalke> heh 11:24 < sipa> MarcoFalke: on this side of the world, yes 11:24 < MarcoFalke> Australia is on April 9th 11:25 < jonatack> heh. all the days seem the same in confinement 11:28 < MarcoFalke> jonatack: True. Haven't figured out yet whether that is good or bad. 11:28 < MarcoFalke> jnewbery: So I can host? :) pls kthxby 11:28 < jnewbery> MarcoFalke: yes of course. I'll try to get your notes merged today 11:30 < jonatack> MarcoFalke: ah cool, hadn't seen the notes yet 11:34 < MarcoFalke> jnewbery: Filed a good first issue about the "wallet calls include block hash" thing: https://github.com/bitcoin/bitcoin/issues/18567 11:35 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:24e5:9ce9:b92f:4c1b] has quit [Quit: Sleep mode] 11:49 < vasild> jnewbery: jonatack: so, maybe schedule the the flush PR #17994 for Apr 22 or some subsequent date. If kallewoof (the PR author) is unavailable, then I can host it. 11:50 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 11:50 < jonatack> vasild: kallewoof is in asia so he is never present at this meeting, but he often hosts a second one in asia a few hours later 11:50 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:24e5:9ce9:b92f:4c1b] has joined #bitcoin-core-pr-reviews 11:51 < jonatack> vasild: for an example, see https://bitcoincore.reviews/17824#meeting-log--asia-time-zone 11:51 < vasild> I see 11:52 < jonatack> vasild: so you could host this meeting and he could host the second one 11:52 < vasild> yes 11:52 < jonatack> great 11:52 < vasild> \o/ 11:53 < jonatack> vasild: you may have already seen it, but here are the tips on hosting :) https://github.com/bitcoin-core-review-club/website/blob/master/CONTRIBUTING.md 11:55 < vasild> nope, thanks for the link, I will get up to speed with that 11:55 < jonatack> also, kallewoof previously proposed to co-write the notes and questions, so once jnewbery confirms the date and PR, definitely contact kallewoof to let him know 11:56 < vasild> yes 12:01 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:24e5:9ce9:b92f:4c1b] has quit [Quit: Sleep mode] 12:16 -!- vindard [~vindard@200.7.90.162] has quit [Read error: Connection reset by peer] 12:19 -!- vindard [~vindard@190.83.165.233] has joined #bitcoin-core-pr-reviews 12:33 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 258 seconds] 12:38 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 12:40 < jonatack> pinheadmz: re-reading the meeting, a propos your comment that "this hidden rpc is still accessible"... yes, it is only hidden in the help, to make it not callable (i think) the following line would need to be removed: 12:40 < jonatack> https://github.com/bitcoin/bitcoin/pull/18453/commits/27b81b25ab6c176ba84f69bf9c00fed13c2dca30#diff-df7d84ff2f53fcb2a0dc15a3a51e55ceR4289 12:41 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 12:41 < jonatack> which is mentioned/linked to in the notes "but it can be called from the command line to use and test it" 12:42 < jonatack> Meeting log is up at https://bitcoincore.reviews/18453.html#meeting-log 12:44 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 12:44 -!- vasild_ is now known as vasild 12:51 < pinheadmz> jonatack: thanks for confirming! 12:52 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:24e5:9ce9:b92f:4c1b] has joined #bitcoin-core-pr-reviews 13:21 -!- michaelfolkson [~textual@2a00:23c5:be01:b201:24e5:9ce9:b92f:4c1b] has quit [Quit: Sleep mode] 13:55 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has joined #bitcoin-core-pr-reviews 13:57 -!- pinheadmz [~matthewzi@pool-100-33-69-78.nycmny.fios.verizon.net] has quit [Quit: pinheadmz] 14:35 -!- thesonofhan [~sonofhan@172.241.251.164] has joined #bitcoin-core-pr-reviews 14:39 -!- sonofhan [~sonofhan@ip72-193-7-145.lv.lv.cox.net] has quit [Ping timeout: 264 seconds] 14:45 -!- thesonofhan [~sonofhan@172.241.251.164] has quit [Ping timeout: 264 seconds] 15:41 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 16:14 -!- willcl_ark [~quassel@cpc123762-trow7-2-0-cust7.18-1.cable.virginm.net] has quit [Ping timeout: 272 seconds] 16:18 -!- willcl_ark [~quassel@cpc123762-trow7-2-0-cust7.18-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews 16:22 -!- AlistairMann [~am@host86-180-28-103.range86-180.btcentralplus.com] has quit [Ping timeout: 250 seconds] 16:28 -!- AlistairMann [~am@host31-52-25-35.range31-52.btcentralplus.com] has joined #bitcoin-core-pr-reviews 16:31 -!- pinheadmz [~matthewzi@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 17:31 -!- belcher [~belcher@unaffiliated/belcher] has quit [Quit: Leaving] 19:29 -!- molly [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 19:32 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 265 seconds] 20:05 -!- molz_ [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 20:08 -!- molly [~molly@unaffiliated/molly] has quit [Ping timeout: 264 seconds] 20:13 -!- molly [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 20:16 -!- molz_ [~molly@unaffiliated/molly] has quit [Ping timeout: 260 seconds] 20:17 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 20:20 -!- molly [~molly@unaffiliated/molly] has quit [Ping timeout: 264 seconds] 20:24 -!- molly [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 20:26 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 260 seconds] 22:11 -!- mol [~molly@unaffiliated/molly] has joined #bitcoin-core-pr-reviews 22:13 -!- molly [~molly@unaffiliated/molly] has quit [Ping timeout: 265 seconds] 22:45 -!- mol [~molly@unaffiliated/molly] has quit [Ping timeout: 260 seconds]