#bitcoin-core-dev log

19:01:26 <achow101> #startmeeting
19:01:26 <lightningbot> Meeting started Fri Oct 23 19:01:26 2020 UTC.  The chair is achow101. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:01:26 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:01:35 <jonatack> hi
19:01:37 <hebasto> hi
19:01:41 <michaelfolkson> hi
19:02:09 <achow101> #bitcoin-core-dev Wallet Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb moneyball ariard digi_james amiti fjahr jeremyrubin emilengler jonatack hebasto
19:02:10 <achow101> jb55 kvaciral ariard digi_james amiti fjahr jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
19:02:18 <achow101> topics?
19:02:24 <meshcollider> Hi
19:02:33 <achow101> oh you're awake
19:02:52 <jonatack> hey meshcollider
19:02:53 <meshcollider> One topic proposal standardize feerate unit on sat/vB (or sat/kvB... or sat/sipa...) (jonatack)
19:03:02 <sipa> you were trying to cross the border
19:03:12 <meshcollider> Yes :) but glad you're hosting because my internet may be intermittent
19:04:11 <achow101> #topic standardize feerate unit on sat/vB (or sat/kvB... or sat/sipa...) (jonatack)
19:04:32 <jonatack> #11413 was merged in June
19:04:38 <gribble> https://github.com/bitcoin/bitcoin/issues/11413 | [wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof · Pull Request #11413 · bitcoin/bitcoin · GitHub
19:05:09 <jonatack> and introduced an explicit feerate option, that overloads conf_target and estimate_mode
19:05:20 <jonasschnelli> would this break the RPC API... I mean would we change existing RPC parameters?
19:05:43 <jonatack> which has the disadnvantage of being unsafe and confusing to use
19:05:57 <sipa> jonatack: it uses a different unit than other feerate RPC arguments?
19:06:23 <jonatack> 11413 allows a choice between 2 units:
19:06:31 <jonatack> BTC/kB and sat/B
19:07:09 <jonatack> I've been using this IRL regularly... apart from bumpfee where it is not working (fix in #20220)
19:07:11 <gribble> https://github.com/bitcoin/bitcoin/issues/20220 | wallet, rpc: explicit feerate follow-ups by jonatack · Pull Request #20220 · bitcoin/bitcoin · GitHub
19:07:15 <jonatack> but it's scary
19:07:32 <sipa> how do you choose between them?
19:07:34 <jonatack> MarcoFalke created an issue at #19453
19:07:36 <gribble> https://github.com/bitcoin/bitcoin/issues/19453 | refactor: reduce DefaultRequestHandler memory allocations by jonatack · Pull Request #19453 · bitcoin/bitcoin · GitHub
19:07:42 <jonatack> ugh
19:07:49 <luke-jr> sipa: the unit is specified s a string
19:08:04 <jonatack> #19543
19:08:05 <gribble> https://github.com/bitcoin/bitcoin/issues/19543 | Normalize fee units for RPC ("BTC/kB" and "sat/B) · Issue #19543 · bitcoin/bitcoin · GitHub
19:08:15 <jonatack> discussion here ^
19:08:24 <luke-jr> in place of the conservative/etc fee estimate mode
19:08:38 <sipa> ok
19:08:44 <jonatack> the idea is to not release 0.21 with the overloaded conf_target and estimate_mode params
19:08:59 <jonasschnelli> 20231 would change the feeRate in fundrawtransaction from BTC/kB to sat/B. Isn't that potentially dangerous?
19:09:10 <luke-jr> jonatack: too late
19:09:49 <sipa> we can't change the interpretation of RPC arguments of existing RPCs in released versions
19:09:52 <jonatack> 6 RPCs are affected: sendtoaddress, sendmany, send, bumpfee, fundraw, and walletcreatefundedpsbt
19:10:16 <sipa> not without a command-line argument to opt into other semantics, at least
19:10:22 <jonasschnelli> I guess if we extend/add a feemode parameter in which the default is BTC/kB,.. its probably fine
19:10:31 <jonatack> per discussion, it's better to not avoid releasing as-is and fix it
19:10:32 <luke-jr> relevant #17356
19:10:34 <gribble> https://github.com/bitcoin/bitcoin/issues/17356 | RPC: Internal named params by luke-jr · Pull Request #17356 · bitcoin/bitcoin · GitHub
19:10:50 <sipa> jonatack: define 'fix it', what needs fixing?
19:10:57 <sipa> (just trying to understand the problem)
19:11:15 <jonatack> see #20220 and #19543
19:11:17 <gribble> https://github.com/bitcoin/bitcoin/issues/20220 | wallet, rpc: explicit feerate follow-ups by jonatack · Pull Request #20220 · bitcoin/bitcoin · GitHub
19:11:18 <gribble> https://github.com/bitcoin/bitcoin/issues/19543 | Normalize fee units for RPC ("BTC/kB" and "sat/B) · Issue #19543 · bitcoin/bitcoin · GitHub
19:12:29 <jonatack> there seems to be agreement to have a dedicated fee_rate option that uses a fixed unit of sat/vB
19:12:46 <jonatack> to replace overloading conf_target and estimate_mode
19:12:56 <jonatack> and it's not too hard to do
19:13:12 <jonatack> mostly test-writing to be sure all the plumbing is working
19:13:35 <sipa> so what would the changes be w.r.t 0.20 ?
19:14:09 <meshcollider> Concept ACK on fee_rate
19:14:10 <jonatack> either as-is with 11413 merged and fixups in 20220, e.g. overloading
19:14:27 <luke-jr> I don't understand the proposed solution
19:14:35 <jonatack> or stop overloading them asap before release
19:14:45 <jonatack> before we have to do a deprecation cycle
19:14:50 <jonatack> to change them
19:14:55 <luke-jr> overloading them isn't a problem, so long as it's properly documented?
19:15:05 <jonatack> luke-jr: it's fraught
19:15:06 <sipa> jonatack: sorry for all the questions, i haven't read all these issues... but i'd like to understand what the problem is and what is being changed
19:15:07 <achow101> sipa: I don't think explicit feerate is in 0.20?
19:15:08 <jonasschnelli> oh. Why do we overload "conf_target" with a feerate.. :/
19:15:24 <luke-jr> jonasschnelli: it's only for positional
19:15:32 <luke-jr> jonasschnelli: with named args, they have separate names
19:15:36 <sipa> mostly to make sure we're not doing anything that breaks compatibility
19:15:54 <jonatack> achow101: no. it was merged late june
19:15:59 <luke-jr> jonasschnelli: for positional, it's pretty reasonable
19:16:20 <sipa> jonatack: would the feerate argument to `settxfee` be affected?
19:16:33 <jonatack> luke-jr: it's mixing types, and look at the fundraw and createpsbt helps...
19:16:51 <luke-jr> jonatack: so the bug is documentation only
19:17:00 <jonatack> sipa: so far settxfee is not changed by 11413
19:17:18 <sipa> or existing RPCs that have feerates in their response?
19:17:37 <jonatack> luke-jr: mixed types, overloading, missing tests, broken bumpfee
19:18:30 <jonatack> luke-jr: i use it for positional, and it's scary. a bit more reassuring with -named
19:18:56 <jonatack> (have used it many times, i still re-check every time)
19:19:28 <jonatack> sipa: so far i've looked at input params/options, not yet at output
19:20:12 <sipa> if we're introducing a sat/vB option, wouldn't it be better to have it everywhere?
19:20:32 <jonatack> i think so, yes. there seems to be fairly strong agreement to migrate to it.
19:20:33 <sipa> otherwise people could be copying output from estimatesmartfee and have it be interpreted as a different unit
19:20:51 <sipa> "migrate" ?
19:21:01 <sipa> you can't break compatibility
19:21:15 <jonatack> so we have to stay with BTC/kB?
19:21:28 <luke-jr> isn't that the reason we use BTC at all in RPC?
19:21:29 <sipa> for existing RPCs, definitely
19:21:42 <sipa> but you could add extra output arguments, and extra input arguments
19:21:44 <luke-jr> otherwise it'd be better to have satoshis everywhere..
19:21:52 <sipa> feerate, feerate_satvb e.g.
19:22:05 <luke-jr> sipa: that breaks positional
19:22:15 <sipa> how so?
19:22:26 <luke-jr> sipa: positional doesn't have an arg name
19:22:41 <sipa> it'd be a completely separate argument
19:22:59 <luke-jr> that's terribly ugly
19:23:03 <sipa> yes
19:23:04 <jonatack> luke-jr: so far, in the rpcs where i've added feerate, i placed it just before verbose, which isn't dangerous
19:23:16 <jonatack> or could be after verbose as well, either way
19:23:22 <jonatack> e.g. last
19:23:23 <sipa> jonatack: so verbose moved?
19:23:46 <jonatack> sipa: no, this is what i'm looking at rn
19:24:24 <jonatack> adding "fee_rate" or "feerate_sat_vb" to sendtoaddress/sendmany/send etc
19:24:33 <luke-jr> btw why use vB at all, if not for API compatibility? ;)
19:24:38 <luke-jr> maybe it should be sat/WU ;)
19:25:06 <sipa> i think sat/vB makes sense, it's the most common unit used in practice for fees
19:25:12 <sipa> at least as an option
19:25:27 <sipa> but compatibility is a problem
19:26:06 <sipa> previous times when new units in RPC was brought up (mostly in the context of using sat instead of BTC for absolute amounts), it was suggested that this'd be done through a completely new version of the RPC API
19:26:26 <sipa> though that of course easily scope-creeps into discussions about what else to change
19:26:37 <jonatack> luke-jr: if it was me doing it greenfield, i might use sat/kvB (and call it sat/sipa for better marketing)
19:26:44 <jonatack> sipa: hm
19:26:49 <sipa> goh please no
19:26:52 <luke-jr> lol
19:27:09 <meshcollider> ugh, rpc versioning
19:27:32 <jonatack> for the 3 send rpcs, there is currently no feerate-like param
19:27:52 <luke-jr> can we split this topic up into two: what is actually _broken_ right now? 2) what is a future backward compat burden?
19:28:16 <jonatack> bumpfee, fundraw and WCFB do have them
19:28:18 <emzy> As long as minimum fee is 1 sat/vB, this unit makes sense.
19:28:40 <luke-jr> emzy: JSON-RPC does not treat integers as special
19:28:52 <sipa> many client libraries do, though
19:29:13 <sipa> but indeed, JSON only has a "number" type with no distinction between integers and floating-point
19:29:20 <jonatack> luke-jr: minimum fixes are in #20220, except maybe additional clarity in the help about the confusing edges, which there currently are
19:29:22 <gribble> https://github.com/bitcoin/bitcoin/issues/20220 | wallet, rpc: explicit feerate follow-ups by jonatack · Pull Request #20220 · bitcoin/bitcoin · GitHub
19:30:20 <jonatack> what MarcoFalke, wumpus, Murch, kallewoof and I have been discussing is not overloading conf_target and estimate_mode before it's too late and released
19:30:26 <luke-jr> sipa: I can't think of a sane way to prepare for broken client libs combined with sub-sat/WU fee rates :p
19:30:44 <michaelfolkson> What is WU?
19:30:59 <meshcollider> Weight unit
19:31:01 <luke-jr> weight units; ie, what consensus is really using
19:31:04 <michaelfolkson> Ah ta
19:31:32 <jonatack> Questions:
19:31:47 <luke-jr> jonatack: what is the difference between `int target = value.get_int();` and `const int target{value.get_int()};` that makes it so essential?
19:32:02 <jonatack> - move forward with fix to not overload those two params?
19:32:19 <luke-jr> I think they should be overloaded
19:32:25 <jonatack> - call it feerate or fee_rate?
19:32:41 <jonatack> luke-jr: it's WIP, no need to discuss style nits yet
19:32:47 <Murch> luke-jr: Do you think floats of sat/vB would be a problem?
19:33:04 <sipa> it is very late to still make changes to RPC arguments at this point
19:33:10 <luke-jr> Murch: JSON-RPC doesn't have floats, everything is precise decimal
19:33:37 <luke-jr> Murch: even if some clients might need to used floats for it, I don't see it likely to be a practical issue
19:33:44 <luke-jr> after all, floats have plenty of precision?
19:33:58 <Murch> So how do people input BTC/kB right now?
19:34:03 <jonatack> ISTM it can be simpler to add a fixed-unit feerate than keep overloading, and simpler code as well
19:34:24 <jonatack> above all, better for users
19:34:33 <luke-jr> jonatack: ?
19:34:56 <jonatack> and i've been dogfooding the current version quite a bit
19:35:19 <luke-jr> callers should be able to specify fee as an estimate mode or absolute feerate.. I see no case where overloading is weird
19:35:47 <sipa> Murch: 0.00123456 if you want 123.456 sat/vB
19:35:54 <jonatack> Has anyone actually used it?
19:36:16 <jonatack> The explicit feerate with the overloaded args.
19:36:43 <Murch> sipa: Is that then not a float?
19:36:55 <sipa> Murch: JSON doesn't have a concept of floats/integers, just "numbers"
19:37:30 <jonatack> It's awfully odd to send txns with a feerate set with conf_target=2 and estimate_mode="sat/vb"
19:37:44 <sipa> jonatack: but can't you use fee_rate=2 as well?
19:37:48 <luke-jr> jonatack: 20220 is long, and starting from the top looks like a bunch of completely unnecessary changes; I'm not opposed to them, but it's not helpful to understand what is in need of fixing
19:37:54 <Murch> I guess I'm missing the distinction, but I understand that it's not a problem to put in something with a value between 0 and 1
19:37:56 <jonatack> the names do not correspond at all to what is being done
19:38:19 <jonatack> luke-jr: 20220 mostly adds missing tests
19:38:36 <sipa> Murch: read the JSON spec, it just has a number type
19:39:00 <sipa> Murch: the problem is with client libraries, which may map the number type to floating point types, which is bad for currency reasons
19:39:05 <luke-jr> Murch: JSON-RPC numbers are decimal strings; floats are fixed-size approximations
19:39:27 <sipa> bitcoin core has no problem with this, it parses the decimals exactly from json, without conversion to a floating point type at any point
19:39:46 <Murch> luke-jr: Thanks, I see.
19:40:18 <sipa> i think it's a good idea to try to avoid numbers with a decimal in our RPC, but that's not worth breaking compatibility over
19:40:20 <jonatack> luke-jr: with an important fix that i noted with a review comment and some help documentation fixes
19:41:00 <jonatack> (some of the helps are currently wrong)
19:41:10 <Murch> My alternative proposal was sat/kvB, which gives us an additional 3 decimal of precision for numbers that are safe integers
19:41:45 <Murch> (which, though would also get parsed as floats in client libraries potentially, I guess)
19:41:47 <emzy> Murch: thats also msat/vB
19:41:56 <Murch> emzy: sure.
19:42:09 <sipa> Murch: the bitcoin core RPC also lets you pass any amount as a string, in case your client library can't produce JSON numbers without going through a floating-point type
19:42:23 <luke-jr> emzy: but msat isn't an existing unit (in Bitcoin/Core at least)
19:42:52 <emzy> luke-jr: but Lighting uses it already.
19:42:59 <sipa> so you can have integer sats internally in the application, and format them as "%i.%06i" % (sats // 1000000, sats % 1000000) for example (python like)
19:43:01 <luke-jr> emzy: unfortunately :p
19:43:57 <sipa> we'll add a command line option to rename msat/vB to sat/kvB for luke-jr
19:44:02 <sipa> ;)
19:45:05 <Murch> mh, my nickname is the only one with a capital letter. Are you all anti-capitalists?
19:45:07 <emzy> I don't like the 1/k I think it is confusing
19:45:12 <luke-jr> lol
19:45:39 <jonatack> I'm not sure what people would like to do here. What we have now does have some issues per #19543 and other issues I found while adding tests, but happy to not work on it further if it's not needed.
19:45:40 <gribble> https://github.com/bitcoin/bitcoin/issues/19543 | Normalize fee units for RPC ("BTC/kB" and "sat/B) · Issue #19543 · bitcoin/bitcoin · GitHub
19:46:01 <sipa> jonatack: i'm going to refrain from commenting further until i understand what the problem and proposed solution are
19:47:01 <jonatack> I'm afraid people will definitely complain about the overloaded options having functions that don't have much to do with the param names.
19:47:18 <jonatack> e.g. conf_target for the fee rate
19:47:34 <luke-jr> jonatack: if you're using named params, you *shouldn't* be specifying it as conf_target
19:47:39 <jonatack> and there is also a feeRate or fee_rate arg next to it in some rpcs
19:47:58 <luke-jr> it should be the same position, but named fee_?rate
19:48:11 <jonatack> that in addition does not work if you use estimate_mode
19:48:42 <luke-jr> without #17356, we can't enforce the correct name is used, but that's beside the point
19:48:44 <gribble> https://github.com/bitcoin/bitcoin/issues/17356 | RPC: Internal named params by luke-jr · Pull Request #17356 · bitcoin/bitcoin · GitHub
19:49:50 <Murch> presumably `conf_target` and `feerate` would be exclusive or one would supersede if both are provided (exclusive is cleaner, though)
19:50:15 <jonatack> Ok, in issue 19543 I thought there was a pretty clear direction, but it seems not, and I suspect no one has been dogfooding the current new feature.
19:50:19 <luke-jr> Murch: ideally, if both were provided, we'd error
19:50:40 <luke-jr> or rather, if the wrong one were provided
19:50:54 <luke-jr> estimate_mode (perhaps should be renamed fee_mode) specifies which one is correct
19:51:11 <jonatack> luke-jr: we cannot rename estimate_mode
19:51:12 <luke-jr> (and with "fee_mode" there, "fee_rate" seems obvious over "feerate")
19:51:16 <luke-jr> jonatack: why not?
19:51:25 <jonatack> it's used for the fee estimation since some time
19:51:29 <luke-jr> so?
19:51:33 <luke-jr> we support multiple names
19:51:46 <luke-jr> deprecate the old one and keep allowing it
19:51:55 <jonatack> This is a mess.
19:52:24 <jonatack> I thought we had a good solution, but it seems best for me to drop it.
19:52:36 <jonatack> I'm done.
19:52:57 <sipa> :(
19:53:21 <jonatack> luke-jr: I with you had chimed in on the discussion in 19543
19:53:33 <jonatack> we seemed in agreement I thought
19:54:03 <jonatack> I'll get back to catching up on the reviewing.
19:54:47 <achow101> any other topics for the last 5 minutes?
19:55:38 <michaelfolkson> Is that an effective Approach NACK luke-jr?
19:56:05 <luke-jr> michaelfolkson: I don't understand his proposed approach still :/
19:56:24 <sipa> i worry that i may have contributed to jonatack's frustration here by commenting without understanding the problem well
19:57:35 <michaelfolkson> Ok well if you both do look over it it would be good to get a formal Approach NACK if you really don't like it.
19:57:55 <michaelfolkson> I haven't understood it all either
19:58:07 <jonatack> sipa, no worries, I'm not frustrated, it's just clearly too early in that people haven't tripped on the issues yet.
19:59:11 <jonatack> achow101: did you want to do high priority?
19:59:12 <Murch> How come that parameters to RPC are defined separately instead of in a dictionary that applies to all uses of the same parameter?
19:59:33 <Murch> I.e. why is something like feerate not defined once for the whole codebase?
19:59:33 <achow101> jonatack: I think all the high priority is already listed in the milestone
19:59:44 <jonatack> achow101: that's true
19:59:56 <sipa> Murch: what do you mean with "defined once" ?
20:00:04 <sipa> code for parsing it?
20:00:27 <achow101> #endmeeting