< jeremyrubin>
For IsTrusted, can someone explain why we don't recursively call istrusted on the parent?
< jeremyrubin>
Currently we iterate over the inputs and check IsMine != Spendable
< jeremyrubin>
Is that sufficient if we're spending from an unconfirmed tx?
< jamesob>
can anyone hear me yet?
< achow101>
jamesob: shout louder
< jamesob>
achow101: hah thanks
< jamesob>
sipa: apologies, tried replying to your question but freenode had me muted (or something). haven't tried sparsehash for chainstate but in general it looked not to perform as well as the robin_hood map. is there some reason in particular you think it'd be worth trying?
< instagibbs>
jeremyrubin, I *think* it's by induction? if the previous output with !IsTrusted parent exists, you won't spend, right?
< instagibbs>
if somehow you did, say raw API, well SFYL I guess
< meshcollider>
#startmeeting
< lightningbot>
Meeting started Fri Aug 30 19:00:25 2019 UTC. The chair is meshcollider. Information about MeetBot at http://wiki.debian.org/MeetBot.
< provoostenator>
The easiest would be to make it mandotary to specify, but that is a breaking change.
< 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?)
< provoostenator>
I'm going to guess that most automated systems already set the estimate_mode param
< provoostenator>
So those won't be affected.
< 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.
< provoostenator>
But maybe that's already going to far.
< instagibbs>
that means really basic usage will break
< instagibbs>
forcing -cli users to put in tons of args
< provoostenator>
Forcing -cli users to specifiy what they mean with the fee rate seems reasonable to me.
< instagibbs>
and the comment, comment-to, subtract fee from dest...
< achow101>
named args gets around having to specify everything else
< 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
< achow101>
I would rather not requiring users to have to specify the fee rate
< provoostenator>
Ideally I would like "sendtoaddress AMOUNT ADDRESS FEE_RATE sat/B"
< instagibbs>
new RPC command time? :P
< instagibbs>
time to scope creep
< achow101>
provoostenator has some ultimate send rpc pr
< provoostenator>
Having a new RPC that's safe to use for newbies and useful for advanced forlks, might be worth doing.
< 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
< 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.
< provoostenator>
Hopefully "nothing"
< instagibbs>
If no one has objections to the "Xsat/B" type format I can roll with that for my PR at least
< achow101>
ideally we would also have fee per weight unit too
< 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.
< instagibbs>
s/Wu :) c-lightning feelz
< achow101>
I would prefer a single arg that specifies the unit at the end
< provoostenator>
single arg with unit at the end has my preference too. As well as consistency between RPC calls.
< instagibbs>
ok, I'll at least adapt my PR with a "standard" proposal
< meshcollider>
Yes that seems clean
< instagibbs>
then we can parse them all the same way in the future
< meshcollider>
So what about kalle's PR
< instagibbs>
with same code
< instagibbs>
that's complicated. I don't really know.
< instagibbs>
deprecate non-specified is the safe behavior
< instagibbs>
non-specified unit*
< instagibbs>
maybe kallewoof will have something to say when he awakes
< 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
< achow101>
isn't the fallbackkfee useful though?
< instagibbs>
it's useful to just have tests "Go"
< jtimon>
it can still be used, you just don't have a different default for regtest
< instagibbs>
so this is mostly just to pull it out of the chainparams?
< jtimon>
I mean, I assume at some point we may want to change mainnet's default, but I think that's kind of orthogonal
< jtimon>
instagibbs: yes
< meshcollider>
Well it's not really "in" chainparams
< 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..
< 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
< 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?
< instagibbs>
it's one sticking point that requires a restart
< instagibbs>
that basically any one-off regtest users is going to have to remember to do
< instagibbs>
anyways, maybe im off base
< 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
< jtimon>
I guess you won't like #16527 either then
< instagibbs>
well that won't help/stop "sendtoaddress" from working
< instagibbs>
which the fallback PR would
< instagibbs>
I think it's case by case
< provoostenator>
It wouldn't be too bad if "1 sat/B" exists
< 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?
< instagibbs>
could also just decouple it yeah
< jtimon>
any more thoughts? if people agree with instagibbs I think that's it
< jtimon>
that's it forf this topic, I mean
< meshcollider>
Yeah I agree with instagibbs here
< meshcollider>
sipa: did you want to talk about miniscript or was that just a PSA
< jtimon>
cool, but to be clear, we agree on decoupling the IsTestChain stuff ?
< sipa>
i guess
< instagibbs>
jtimon, concept ACK
< instagibbs>
(only speaking for self)
< jtimon>
cool, thanks
< instagibbs>
sipa, wasn't a yes or no question :P
< jtimon>
sure, anyone feel free to express any nuances
< 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
< meshcollider>
#topic miniscript (sipa)
< 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
< sipa>
of course, it's also kinda pointless without the last ones
< achow101>
I think 1 and 3 are the most useful to have first
< provoostenator>
sipa: does miniscript (or your PR) help with the open issue of composing descriptors (as opposed to parsing string)?
< provoostenator>
E.g. for hardware wallet support it'd be nice if we can get an xpub from the device and make a descriptor.
< provoostenator>
Which currently involves hacking strings.
< sipa>
no
< sipa>
haven't looked at that
< meshcollider>
provoostenator: open issue? Isn't that just an implementation feature
< provoostenator>
Not literally an issue
< sipa>
yeahh it's an imetail
< sipa>
could be fixed with a bit of refactoring
< sipa>
yeah it's an implementation detail
< sipa>
why is my keyboard randomly dropping some letters
< provoostenator>
Kind of. It gets more interesting with multisig
< 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
< gwillen>
16IBtbcikinvnffggjcutlcrevrijukb
< sipa>
gwillen: no begging!
< sipa>
but with more complicated things it would result in the lowest possible sizes across all possible witnesses
< sipa>
so that may cause fee underestimation
< provoostenator>
That's probably not actually gwillen.
< gwillen>
oops, sorry
< gwillen>
that's no an address
< gwillen>
it's an OTP from a yubikey
< sipa>
haha
< provoostenator>
LOL
< sipa>
anyway, there'll be more to discuss when i have code to show
< achow101>
sipa: maybe temporarily limit signing to single key until size estimation is in?
< achow101>
like have it all there, but put some gate in the caller
< achow101>
s/single key/things we already sign for
< 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)
< sipa>
even if it isn't exposed yet because of lack of 4
< meshcollider>
sipa: sounds good yeah
< bitcoin-git>
[bitcoin] JeremyRubin opened pull request #16766: Make IsTrusted scan parents recursively (master...recursive-istrusted) https://github.com/bitcoin/bitcoin/pull/16766
< meshcollider>
Ok we wait for pr then
< meshcollider>
Any other topics?
< jeremyrubin>
Well it is wallet related, but if anyone wants to look at the PR I just opened would be thankful.
< jeremyrubin>
Trying to patch in covenant detection for OP_SECURETHEBAG so that you know if you can trust funds have been paid
< jeremyrubin>
But the lack of recursive trust seems like a bug because it breaks transitivity
< sipa>
jeremyrubin: can you give an example of how parents could be untrusted but a child is false marked as trusted?
< jeremyrubin>
Sure
< jeremyrubin>
Suppose I have a txn B which I generated from a output A.0
< jeremyrubin>
A.0 ISMINE_SPENDABLE
< jeremyrubin>
But transaction A has an input Z.0 which is not ISMINESPENDABLE
< sipa>
which of these are confirmed?
< jeremyrubin>
None have to be I think
< 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
< jeremyrubin>
Yeah that sounds right
< meshcollider>
I thought transactions in the mempool are always untrusted
< 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)
< meshcollider>
Dw ignore me
< 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)
< sipa>
jeremyrubin: multisig is currently never ISMINE_SPENDABLE, unless you literally have all the keys in your wallet (not even just the threshold)
< jeremyrubin>
Cool
< jeremyrubin>
Wasn't sure
< sipa>
i think that's a horrible mistake, though :)
< sipa>
ownership of funds shouldn't be tied to control over keys
< jeremyrubin>
In any case, I think that the recursive check fixes the edge condition
< sipa>
the performance impact is scary
< sipa>
you can construct a graph of transactions that makes this computation explode without caching
< jeremyrubin>
I think it's OK given the limits on longchain in the mempool
< sipa>
i'm not sure
< jeremyrubin>
It would also be OK to construct a cache per call to IsTrusted
< jeremyrubin>
but caching across calls to IsTrusted is a bit finicky
< jeremyrubin>
Would that sort of caching be happier sipa?
< sipa>
yeah
< 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
< jeremyrubin>
k I will modify it to work that way
< meshcollider>
#endmeeting
< lightningbot>
Meeting ended Fri Aug 30 20:02:21 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< sipa>
a graph of transactions consisting of 25 levels, with 2 2-in-2-out transactions per level, each spending an output from each of the previous level's transactions
< sipa>
i think that would fit in the mempool
< sipa>
but cost 2^26 IsTrusted calls
< sipa>
with more outputs and inputs the base of the exponentiation goes up
< jeremyrubin>
Would you also prefer the code to be a non recursive loop?
< jeremyrubin>
I think the recursion isn't too big a deal since we're at most up 25 levels -- lmk if you feel differently. I also decided to only cache the ones we detect as being trusted, but we could cache the untrusted ones as well
< jeremyrubin>
Caching untrusted seems less a big deal because it works out to be at most 25 (assuming a fully loaded trusted cache)
< bitcoin-git>
[bitcoin] kristapsk opened pull request #16767: test: Check for codespell in lint-spelling.sh (master...check-for-codespell) https://github.com/bitcoin/bitcoin/pull/16767
< bitcoin-git>
[bitcoin] kristapsk opened pull request #16768: test: Make lint-includes.sh work from any directory (master...lint-includes-anydir) https://github.com/bitcoin/bitcoin/pull/16768