< phantomcircuit>
jimpo, can you make it so the individual commits in 12254 actually work independently?
< phantomcircuit>
pr's like that where the individual commits result in broken code make things like bisect less useful
< jimpo>
I thought they do. What one breaks the build?
< phantomcircuit>
jimpo, hmm yeah you're right that it doesn't actually, my mistake
< phantomcircuit>
still kind of odd to define interfaces in one commit and then implement them in several others i think
< phantomcircuit>
but maybe that's just me
< jimpo>
Yeah, that's just sort of what made the most sense to me as far as trying to communicate why code is happening. Basically, it's just a lot of new code and I wanted to break it up somehow.
< mryandao>
basically just summarising the actual stuff i'm contributing less the changes made in all the commits i've squashed?
< r251d>
I'm not sure exactly what to change about it but I think it may just be a bit verbose. Contributing.md says this:
< r251d>
> Commit messages should be verbose by default consisting of a short subject line (50 chars max), a blank line and detailed explanatory text as separate paragraph(s), unless the title alone is self-explanatory (like "Corrected typo in init.cpp") in which case a single title line is sufficient. Commit messages should be helpful to people reading your code in the future, so explain the reasoning for
< r251d>
your decisions. Further explanation here.
< gmaxwell>
mryandao: Its not clear to me what you're being asked to do there. Feel free to ask.
< Randolf>
mryandao: You're new too? And you got Squashing working? You're ahead of me then. :)
< mryandao>
the instructions were easy to follow.
< mryandao>
way more inituitive than git rebase --help really.
< Randolf>
I think I got rebasing working today. Nothing showed up on GitHub.com though, so I figured it got queued or something and I'll check again later.
< mryandao>
you gotta do a git push -f
< mryandao>
and it should show in github.com after the push is done.
< Randolf>
I did that, it asked for my username and password, and then it finally reported that everything is up to date.
< bitcoin-git>
[bitcoin] kallewoof opened pull request #12257: [wallet] Use destination groups instead of coins in coin select (master...feature-addrgrouped-coinselect) https://github.com/bitcoin/bitcoin/pull/12257
< ProfMac>
Is there a channel for people who want to study the code, but aren't strong developers?
< Randolf>
ProfMac: There is a #bitcoin-dev channel.
< gmaxwell>
kallewoof: very cool
< gmaxwell>
kallewoof: did you consider/try having the non-avoidpartialspends mode first try to see it to see if there is a solution? I assume everyone would prefer an avoidpartial solution if it was otherwise about as good.
< luke-jr>
kallewoof: that doesn't avoid reuse, and has security issues in the event of QC (but we have enough issues there already, so not a big deal), but seems useful anyway
< gmaxwell>
kallewoof: it doesn't avoid reuse, but it reduces one of its major harms.
< luke-jr>
yes, hence useful anyway
< gmaxwell>
right now even a small amount of reuse results in your entire wallet being common-input linked.
< luke-jr>
my point was only that it isn't an "avoid address reuse" category PR as it seemed to be suggesting it was ;)
< gmaxwell>
luke-jr: we could probably do a lot wrt reuse with two small features: when you send to an address you've sent to before, put up a dialog that asks if you're making a mistake. (we've certantly seen people mess up by double paying addresses); and put a 'reuse' icon on any transaction paying you that pays to an address which has already been paid.
< kallewoof>
gmaxwell: you mean it should try to avoid partial spending and if it finds a fairly optimal solution it picks it over the per-coin selector?
< kallewoof>
gmaxwell: I'm not sure what would be a good factor for determining success in that case, but it does sound like a good strategy. (Maybe simply # of inputs with enabled vs disabled? If same, use the no-partial-spends variant.)
< kallewoof>
also, as noted in the PR, it is a fix to an issue raised in a different PR which marks addresses dirty. The two PRs together would avoid reuse completely, I believe. (Correct me if I'm missing something, though!)
< kallewoof>
luke-jr: ^
< kallewoof>
s/issue raised/issue addressed/
< gmaxwell>
kallewoof: the decision criteria is a complicated question that is cropping up in other areas too... generally if the current feerate is low compared to what you expect in the future you want to be spending more inputs, if it's high, fewer-- though solutions without change are preferable.
< kallewoof>
gmaxwell: ahh.. that's a good point. I really like the idea of testing both cases and using the avoidpartialspends variant if it's only marginally worse.
< gmaxwell>
well at a minimum you can prefer the avoidpartial solution if it pays equal or less total fees, I think.
< gmaxwell>
Though I know that isn't ideal, but I think it's strictly better than not trying.
< kallewoof>
If the fee rate is decided on coin selection (I don't think it is, but have to check), it could be used to consider more inputs as a better solution, maybe. I think I'll do the 'equal or less fee' fix right away though (but will it, ever? won't the same solution be found by the less restricted variant?)
< kallewoof>
I mean, if the fee rate is really low, it could see it as an opportunity to group up inputs. I think that's flawed, though, so nevermind.
< gmaxwell>
Fee rate is known then, so the software could decide to prefer more vs fewer, the challenge is how do you know if a specific feerate is a high vs a low one?
< gmaxwell>
as far as "won't it find it" -- unlikely, the stochastic solver only evaluates a tiny fraction of the search space... imagine you have 10 addresses, and 10 payments each, its unlikely the ungrouped solver will ever consider spending just two groups.
< gmaxwell>
I wouldn't be surprised if the grouped solution we seldom lower fees, though it might be equal not that infrequently.
< kallewoof>
Okay... yeah, I haven't considered the more complex cases. I'm mostly thinking of this from the "one actual payment with a bunch of tiny dust payments to track you" scenario.
< gmaxwell>
I was about to say that with feerates at least tracking dust is less of an issue ... but then I remembed the last couple days...
< gmaxwell>
fun fact: I noticed recently that some obvious tracking dust I got years ago is now worth $10 each. :)
< kallewoof>
lol
< kallewoof>
gmaxwell: the only straightforward way I could think of to check if the fee drops is the # of coins selected (I assume every input + signature will cost the same, which is untrue with segwit vs p2pkh, but I think that's not a big issue in the long run)
< gmaxwell>
well one might have change and the other not...
< gmaxwell>
you could compare their weight instead?
< kallewoof>
I'm checking for change by seeing if nValueFromPresetInputs + nValueRet2 == nTargetValue. That's not precise though, due to dust limits and such, I guess.
< kallewoof>
I didn't know you could get the weight for coins. Will look.
< gmaxwell>
oh I see.
< gmaxwell>
right you're working with selections, not transactions.
< kallewoof>
Okay, I think I understand what you mean -- make both transactions and compare their weights and pick the avoid-partial one if it's reasonable in comparison to the non-avoiding one.
< gmaxwell>
yes, I was thinking that you were working with transactions for some reason.
< wumpus>
promag: I agree it makes sense to separate it out and discuss it separately, not making it hold up 0.16
< promag>
ok, I'll adjust
< wumpus>
I agree with his points, also that we really don't want to be introducing new #ifdef ENABLE_WALLET, but in any case let's get the other two commits in
< wumpus>
it's doubly interesting because this code within ENABLE_WALLET doesn't use any wallet, implying, if we want this, that there is some utility code that needs to be moved out of the wallet
< wumpus>
going to merge #11415 first, seems it's ready
< bitcoin-git>
bitcoin/master 596c446 Sjors Provoost: [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH...
< bitcoin-git>
bitcoin/master 9594139 Wladimir J. van der Laan: Merge #12119: [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH...
< bitcoin-git>
[bitcoin] laanwj closed pull request #12119: [wallet] use P2WPKH change output if any destination is P2WPKH or P2WSH (master...bech32-change) https://github.com/bitcoin/bitcoin/pull/12119
< promag>
wumpus: #12213, rebased and removed createmultisig change