19:00:25 <meshcollider> #startmeeting
19:00:25 <lightningbot> Meeting started Fri Aug 30 19:00:25 2019 UTC.  The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:25 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:30 <meshcollider> #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
19:00:33 <instagibbs> hi
19:00:35 <achow101> hi
19:00:35 <kanzure> hi
19:00:44 <provoostenator> hi
19:00:51 <meshcollider> Topics this week?
19:00:52 <awesome-doge> hi
19:00:55 <wumpus> hi
19:01:13 <sipa> hi
19:01:20 <instagibbs> might need some assist reasoning about safety of new arg in #11413
19:01:23 <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:01:51 <instagibbs> I'd like to use the X sat/B etc style as well for my bumpfee PR
19:02:22 <jtimon> I guess #16524 could be on-topic for this meeting?
19:02:24 <gribble> https://github.com/bitcoin/bitcoin/issues/16524 | Wallet: Disable -fallbackfee by default by jtimon · Pull Request #16524 · bitcoin/bitcoin · GitHub
19:02:52 <sipa> i'll try to have a first miniscript pr ready today
19:02:59 <achow101> woo miniscript
19:03:01 <meshcollider> instagibbs: is that a request for comment in the PR or do you want to discuss it here?
19:03:26 <meshcollider> sipa: exciting! :)
19:03:52 <instagibbs> meshcollider, the thread I guess? Unless someone has Strong Opinions
19:04:17 <instagibbs> problem with that PR is that it's hijacking the conf_target arg, which already has meaning without unit
19:04:19 <provoostenator> Probably good to discuss here,
19:04:34 <instagibbs> ok I propose as topic then
19:04:53 <meshcollider> #topic explicit feerate option (instagibbs)
19:05:12 <provoostenator> The easiest would be to make it mandotary to specify, but that is a breaking change.
19:06:05 <instagibbs> Right, it's really hard to do it safely without that, even if you don't re-purposed conf_target(unless you have 0 mean not using it?)
19:06:46 <provoostenator> I'm going to guess that most automated systems already set the estimate_mode param
19:07:01 <provoostenator> So those won't be affected.
19:08:31 <provoostenator> If we deprecate & remove the "UNSET" (default) the error will at least be very clear and easy to detect if someone doesn't read the release notes.
19:08:39 <provoostenator> But maybe that's already going to far.
19:08:57 <instagibbs> that means really basic usage will break
19:09:05 <instagibbs> forcing -cli users to put in tons of args
19:09:24 <provoostenator> Forcing -cli users to specifiy what they mean with the fee rate seems reasonable to me.
19:09:44 <instagibbs> and the comment, comment-to, subtract fee from dest...
19:10:22 <achow101> named args gets around having to specify everything else
19:10:34 <provoostenator> I don't know if we can change the order to mvoe the fee rate first, in such a way that old invocations are guaranteed to fail
19:10:54 <achow101> I would rather not requiring users to have to specify the fee rate
19:11:23 <provoostenator> Ideally I would like "sendtoaddress AMOUNT ADDRESS FEE_RATE sat/B"
19:11:41 <instagibbs> new RPC command time? :P
19:11:46 <instagibbs> time to scope creep
19:12:01 <achow101> provoostenator has some ultimate send rpc pr
19:12:07 <instagibbs> hmm that's true
19:12:08 <provoostenator> #16378
19:12:10 <gribble> https://github.com/bitcoin/bitcoin/issues/16378 | [WIP] The ultimate send RPC by Sjors · Pull Request #16378 · bitcoin/bitcoin · GitHub
19:12:10 <provoostenator> I was tempted
19:12:21 <achow101> maybe we should deprecate all the existing send rpcs and just make one that doesn't suck?
19:12:29 <provoostenator> But problem is, it'll be nice for a year or two, and then it sucks again.
19:12:48 <provoostenator> But I'm happy to reopen.
19:12:57 <sipa> mandatory xkcd: https://xkcd.com/927/
19:13:01 <achow101> new rpcs: sendv1, sendv2, sendv3...
19:13:19 <provoostenator> Having a new RPC that's safe to use for newbies and useful for advanced forlks, might be worth doing.
19:14:20 <instagibbs> maybe brainstorm on how to make changes easier in the future. e.g., require units in args rather than implicit, I don't know
19:14:33 <provoostenator> It'd be useful to have an idea of what extra stuff we want that RPC to do in light of miniscript and multisig.
19:14:41 <provoostenator> Hopefully "nothing"
19:15:28 <instagibbs> If no one has objections to the "Xsat/B" type format I can roll with that for my PR at least
19:15:44 <achow101> ideally we would also have fee per weight unit too
19:15:46 <provoostenator> We could do something pedanctic like a 5 second delay that the user can abort with ctrl + c, if and only if the user didn't set an explicit argument.
19:16:01 <instagibbs> s/Wu :) c-lightning feelz
19:16:04 <achow101> I would prefer a single arg that specifies the unit at the end
19:16:42 <provoostenator> single arg with unit at the end has my preference too. As well as consistency between RPC calls.
19:17:17 <instagibbs> ok, I'll at least adapt my PR with a "standard" proposal
19:17:17 <meshcollider> Yes that seems clean
19:17:30 <instagibbs> then we can parse them all the same way in the future
19:17:33 <meshcollider> So what about kalle's PR
19:17:33 <instagibbs> with same code
19:17:40 <instagibbs> that's complicated. I don't really know.
19:17:52 <instagibbs> deprecate non-specified is the safe behavior
19:18:26 <instagibbs> non-specified unit*
19:19:52 <instagibbs> maybe kallewoof will have something to say when he awakes
19:20:21 <instagibbs> end topic I guess
19:21:09 <meshcollider> #topic disable -fallbackfee (jtimon)
19:21:33 <meshcollider> #16542
19:21:35 <gribble> https://github.com/bitcoin/bitcoin/issues/16542 | Return more specific errors about invalid descriptors by achow101 · Pull Request #16542 · bitcoin/bitcoin · GitHub
19:21:38 <meshcollider> Oops
19:21:55 <meshcollider> #16524
19:21:57 <gribble> https://github.com/bitcoin/bitcoin/issues/16524 | Wallet: Disable -fallbackfee by default by jtimon · Pull Request #16524 · bitcoin/bitcoin · GitHub
19:22:22 <jtimon> so what the PR does is having the same defualt for regtest as it has on mainnet, simply decoupling it from the chainparams stuff
19:23:13 <achow101> isn't the fallbackkfee useful though?
19:23:21 <instagibbs> it's useful to just have tests "Go"
19:23:49 <jtimon> it can still be used, you just don't have a different default for regtest
19:24:14 <instagibbs> so this is mostly just to pull it out of the chainparams?
19:24:42 <jtimon> I mean, I assume at some point we may want to change mainnet's default, but I think that's kind of orthogonal
19:24:50 <jtimon> instagibbs: yes
19:25:30 <meshcollider> Well it's not really "in" chainparams
19:26:20 <instagibbs> I guess I'm -0 on it, unless I see "where it's going" with follow-on changes? network-specific behavior outside of consensus params aren't going away..
19:26:33 <jtimon> well, it is, it was just renamed to IsTestChain to couple it with another chainparams that says whether your chain allows you to set acceptnonstdtxs to true or not
19:27:26 <jtimon> I have 2 follow up PRs, but they're more related to chainparams than to the wallet: https://github.com/bitcoin/bitcoin/pull/16527 and https://github.com/bitcoin/bitcoin/pull/16526
19:28:35 <jtimon> I mean, I guess this is removign a feature, but is it really that important? is it that harsh to ask regtest and testnet users to set -fallbackfee explicitly?
19:28:50 <instagibbs> it's one sticking point that requires a restart
19:29:09 <instagibbs> that basically any one-off regtest users is going to have to remember to do
19:29:21 <instagibbs> anyways, maybe im off base
19:29:32 <meshcollider> Regtest will rarely have enough data for fee estimation so it is kinda annoying to have to set it every time but I'm -0 too I guess
19:29:44 <jtimon> I guess you won't like #16527 either then
19:29:46 <gribble> https://github.com/bitcoin/bitcoin/issues/16527 | B: Get rid of Params().RequireStandard() by jtimon · Pull Request #16527 · bitcoin/bitcoin · GitHub
19:30:11 <instagibbs> well that won't help/stop "sendtoaddress" from working
19:30:22 <instagibbs> which the fallback PR would
19:30:56 <instagibbs> I think it's case by case
19:31:02 <provoostenator> It wouldn't be too bad if "1 sat/B" exists
19:31:04 <jtimon> so if we don't want to do this, can we at least rename it back to what it was instead of coupling it with other unrelated things with IsTestChain?
19:31:30 <instagibbs> could also just decouple it yeah
19:33:12 <jtimon> any more thoughts? if people agree with instagibbs I think that's it
19:33:22 <jtimon> that's it forf this topic, I mean
19:33:32 <meshcollider> Yeah I agree with instagibbs here
19:34:03 <meshcollider> sipa: did you want to talk about miniscript or was that just a PSA
19:34:11 <jtimon> cool, but to be clear, we agree on decoupling the IsTestChain stuff ?
19:34:12 <sipa> i guess
19:34:17 <instagibbs> jtimon, concept ACK
19:34:25 <instagibbs> (only speaking for self)
19:34:29 <jtimon> cool, thanks
19:35:04 <instagibbs> sipa, wasn't a yes or no question :P
19:35:27 <jtimon> sure, anyone feel free to express any nuances
19:35:40 <sipa> so there are really 4 different places miniscript can be integrated into: descriptors themselves (like, be able to write them, check them, compute addresses from), descriptor inference (finding a miniscript descriptor for a miniscript-compatible script), signing (including psbt update/finalize), and size estimation
19:35:52 <meshcollider> #topic miniscript (sipa)
19:36:20 <sipa> i'm working on a PR that includes just the first one (and maybe the second one), because the latter ones add quite a bit of complexity
19:36:28 <sipa> of course, it's also kinda pointless without the last ones
19:37:07 <achow101> I think 1 and 3 are the most useful to have first
19:37:21 <provoostenator> sipa: does miniscript (or your PR) help with the open issue of composing descriptors (as opposed to parsing string)?
19:37:46 <provoostenator> E.g. for hardware wallet support it'd be nice if we can get an xpub from the device and make a descriptor.
19:37:52 <provoostenator> Which currently involves hacking strings.
19:38:04 <sipa> no
19:38:08 <sipa> haven't looked at that
19:38:21 <meshcollider> provoostenator: open issue? Isn't that just an implementation feature
19:38:30 <provoostenator> Not literally an issue
19:38:38 <sipa> yeahh it's an imetail
19:38:43 <sipa> could be fixed with a bit of refactoring
19:38:50 <sipa> yeah it's an implementation detail
19:39:07 <sipa> why is my keyboard randomly dropping some letters
19:39:20 <provoostenator> Kind of. It gets more interesting with multisig
19:39:23 <sipa> achow101: 3 without 4 is a bit scary
19:39:34 <bitcoin-git> [13bitcoin] 15godfrey-cw opened pull request #16765: Documentation for `systemd` troubleshooting (06master...06master) 02https://github.com/bitcoin/bitcoin/pull/16765
19:39:50 <sipa> right now we do size estimation just using the dummy signer... which works fine for single sig and multisig because every witness has the same size
19:40:02 <gwillen> 16IBtbcikinvnffggjcutlcrevrijukb
19:40:11 <sipa> gwillen: no begging!
19:40:34 <sipa> but with more complicated things it would result in the lowest possible sizes across all possible witnesses
19:40:52 <sipa> so that may cause fee underestimation
19:40:56 <provoostenator> That's probably not actually gwillen.
19:41:58 <gwillen> oops, sorry
19:42:00 <gwillen> that's no an address
19:42:05 <gwillen> it's an OTP from a yubikey
19:42:09 <sipa> haha
19:42:11 <provoostenator> LOL
19:42:25 <sipa> anyway, there'll be more to discuss when i have code to show
19:42:45 <achow101> sipa: maybe temporarily limit signing to single key until size estimation is in?
19:42:56 <achow101> like have it all there, but put some gate in the caller
19:43:19 <achow101> s/single key/things we already sign for
19:43:53 <sipa> but it's an interesting question how to structure things; in particular, the internal satisfaction code is very useful to test other parts of the miniscript logic (e.g., do randomized tests that show the stack size never exceeds the statically predicted max stack size)
19:44:13 <sipa> even if it isn't exposed yet because of lack of 4
19:45:11 <meshcollider> sipa: sounds good yeah
19:46:01 <bitcoin-git> [13bitcoin] 15JeremyRubin opened pull request #16766: Make IsTrusted scan parents recursively (06master...06recursive-istrusted) 02https://github.com/bitcoin/bitcoin/pull/16766
19:46:06 <meshcollider> Ok we wait for pr then
19:46:10 <meshcollider> Any other topics?
19:47:06 <jeremyrubin> Well it is wallet related, but if anyone wants to look at the PR I just opened would be thankful.
19:47:51 <jeremyrubin> Trying to patch in covenant detection for OP_SECURETHEBAG so that you know if you can trust funds have been paid
19:48:09 <jeremyrubin> But the lack of recursive trust seems like a bug because it breaks transitivity
19:48:58 <sipa> jeremyrubin: can you give an example of how parents could be untrusted but a child is false marked as trusted?
19:49:37 <jeremyrubin> Sure
19:50:11 <jeremyrubin> Suppose I have a txn B which I generated from a output A.0
19:50:38 <jeremyrubin> A.0 ISMINE_SPENDABLE
19:51:10 <jeremyrubin> But transaction A has an input Z.0 which is not ISMINESPENDABLE
19:51:41 <sipa> which of these are confirmed?
19:52:04 <jeremyrubin> None have to be I think
19:54:02 <sipa> right so you have Z,A,B in the mempool, and A will be correctly marked as !IsTrusted(), but B will be marked as IsTrusted
19:54:44 <jeremyrubin> Yeah that sounds right
19:55:48 <meshcollider> I thought transactions in the mempool are always untrusted
19:55:51 <jeremyrubin> Z also has to be a wallettx I think, which can be accomplished by having a Z.10 which ismine (so we're tacking it)
19:56:40 <meshcollider> Dw ignore me
19:56:45 <jeremyrubin> No, if the txn ISMINE_SPENDABLE then in theory only we can spend it (we should probably be checking ISMINE and NOT IS_SOMEONEELSES or something too for complex scripts)
19:57:17 <sipa> jeremyrubin: multisig is currently never ISMINE_SPENDABLE, unless you literally have all the keys in your wallet (not even just the threshold)
19:57:23 <jeremyrubin> Cool
19:57:25 <jeremyrubin> Wasn't sure
19:57:34 <sipa> i think that's a horrible mistake, though :)
19:57:48 <sipa> ownership of funds shouldn't be tied to control over keys
19:58:22 <jeremyrubin> In any case, I think that the recursive check fixes the edge condition
19:58:46 <sipa> the performance impact is scary
19:59:18 <sipa> you can construct a graph of transactions that makes this computation explode without caching
19:59:31 <jeremyrubin> I think it's OK given the limits on longchain in the mempool
19:59:40 <sipa> i'm not sure
19:59:55 <jeremyrubin> It would also be OK to construct a cache per call to IsTrusted
20:00:07 <jeremyrubin> but caching across calls to IsTrusted is a bit finicky
20:00:20 <jeremyrubin> Would that sort of caching be happier sipa?
20:01:10 <sipa> yeah
20:01:32 <jeremyrubin> e.g., you can optionally pass a unordered_set<uint256> trustedParents to the call and you query against that and pass it on recursion
20:02:00 <jeremyrubin> k I will modify it to work that way
20:02:21 <meshcollider> #endmeeting