< bitcoin-git> [bitcoin] jimpo opened pull request #12254: BIP 158: Compact Block Filters for Light Clients (master...bip-158) https://github.com/bitcoin/bitcoin/pull/12254
< 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.
< bitcoin-git> [bitcoin] dongcarl opened pull request #12255: Update bitcoin.service to conform to init.md (master...patch-2) https://github.com/bitcoin/bitcoin/pull/12255
< mryandao> whats the general advice to "clean up commit body"?
< mryandao> does it mean I just have to rebase and re-write the commit message to reflect the final changes?
< mryandao> sorry, i'm still very new to contributing.
< gmaxwell> context?
< r251d> mryandao: I think "commit body" refers to the commit message at https://github.com/bitcoin/bitcoin/pull/12240/commits/acde12c2f651912fdd04e4e50638502d5de26e91
< 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.
< r251d> promag: could you please clarify your nit about commit body cleanup for mryandao at https://github.com/bitcoin/bitcoin/pull/12240 ?
< 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.
< bitcoin-git> [bitcoin] tviho opened pull request #12258: Change app name, datadir location/settings (master...master) https://github.com/bitcoin/bitcoin/pull/12258
< 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.
< kallewoof> gmaxwell: just not sure what to base the criteria on. Maybe that's a different PR. It's simply a matter of calling https://github.com/bitcoin/bitcoin/pull/12257/files#diff-b2bb174788c7409b671c46ccc86034bdR4228 with false once and true once and comparing.
< bitcoin-git> [bitcoin] fanquake closed pull request #12258: Change app name, datadir location/settings (master...master) https://github.com/bitcoin/bitcoin/pull/12258
< 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.
< kallewoof> I was hoping to do all this in SelectCoins. (My patch currently looks like this: https://github.com/bitcoin/bitcoin/pull/12257/commits/4593c6164891e5d766b33952702195288a241b2d)
< promag> mryandao: do you need help addressing my comment??
< gmaxwell> kallewoof: I think it's not unreasonable to do that.
< kallewoof> gmaxwell: I think I've got it. Not sure what reviewers will say though (I wrapped CWallet::CreateTransaction and called the original twice for the case where avoid-partial is unset)... https://github.com/bitcoin/bitcoin/pull/12257/commits/3af1fac480fa08f92cd611ce93885d4501acc676
< kallewoof> (It's failing in places; will fix & push update later.)
< bitcoin-git> [bitcoin] laanwj pushed 6 new commits to master: https://github.com/bitcoin/bitcoin/compare/b5e4b9b5100e...8470e64724cb
< bitcoin-git> bitcoin/master 8d0b610 Jonas Schnelli: Avoid pemanent cs_main/cs_wallet lock during wallet rescans
< bitcoin-git> bitcoin/master dbf8556 Jonas Schnelli: Add RAII wallet rescan reserver
< bitcoin-git> bitcoin/master bc356b4 Jonas Schnelli: Make sure WalletRescanReserver has successfully reserved the rescan
< bitcoin-git> [bitcoin] laanwj closed pull request #11281: Avoid permanent cs_main/cs_wallet lock during RescanFromTime (master...2017/09/rescan_locks) https://github.com/bitcoin/bitcoin/pull/11281
< promag> \o/
< bitcoin-git> [bitcoin] laanwj pushed 5 new commits to master: https://github.com/bitcoin/bitcoin/compare/8470e64724cb...6e89de5ba7ce
< bitcoin-git> bitcoin/master fb6f6b1 Matt Corallo: bluematt's testnet-seed now supports x9 (and is just a static list)
< bitcoin-git> bitcoin/master 51ae766 Matt Corallo: Use GetDesireableServiceFlags in static seeds, document this....
< bitcoin-git> bitcoin/master 62e7642 Matt Corallo: Fall back to oneshot for DNS Seeds which don't support filtering....
< bitcoin-git> [bitcoin] laanwj closed pull request #11512: Use GetDesireableServiceFlags in seeds, dnsseeds, fixing static seed adding (master...2017-10-seed-service-bits-cleanups) https://github.com/bitcoin/bitcoin/pull/11512
< promag> wumpus: #12213 do you agree with jnewbery?
< gribble> https://github.com/bitcoin/bitcoin/issues/12213 | Add address type to addmultisigaddress and createmultisig by promag · Pull Request #12213 · bitcoin/bitcoin · GitHub
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/11415 | [RPC] Disallow using addresses in createmultisig by achow101 · Pull Request #11415 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/6e89de5ba7ce...69ec021969a4
< bitcoin-git> bitcoin/master 1df206f Andrew Chow: Disallow using addresses in createmultisig...
< bitcoin-git> bitcoin/master 69ec021 Wladimir J. van der Laan: Merge #11415: [RPC] Disallow using addresses in createmultisig...
< bitcoin-git> [bitcoin] laanwj closed pull request #11415: [RPC] Disallow using addresses in createmultisig (master...createmultisig-no-addr) https://github.com/bitcoin/bitcoin/pull/11415
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/69ec021969a4...126000ba9e7f
< bitcoin-git> bitcoin/master ffffb10 MarcoFalke: qa: Rename cli.args to cli.options...
< bitcoin-git> bitcoin/master fae7b14 MarcoFalke: qa: Make TestNodeCLI command optional in send_cli
< bitcoin-git> bitcoin/master 126000b MarcoFalke: Merge #12089: qa: Make TestNodeCLI command optional in send_cli...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #12089: qa: Make TestNodeCLI command optional in send_cli (master...Mf1801-qaCliOptions) https://github.com/bitcoin/bitcoin/pull/12089
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/126000ba9e7f...95941396fff8
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/12213 | Add address type to addmultisigaddress and createmultisig by promag · Pull Request #12213 · bitcoin/bitcoin · GitHub
< wumpus> promag: thanks!
< promag> taking care of #12194 too
< gribble> https://github.com/bitcoin/bitcoin/issues/12194 | Add change type option to fundrawtransaction by promag · Pull Request #12194 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/95941396fff8...e37ca2be91bd
< bitcoin-git> bitcoin/master 04ededf Russell Yanofsky: Make CKey::Load references const...
< bitcoin-git> bitcoin/master e37ca2b Wladimir J. van der Laan: Merge #12250: Make CKey::Load references const...
< bitcoin-git> [bitcoin] laanwj closed pull request #12250: Make CKey::Load references const (master...pr/keyload) https://github.com/bitcoin/bitcoin/pull/12250
< promag> wumpus: pushed #12194
< gribble> https://github.com/bitcoin/bitcoin/issues/12194 | Add change type option to fundrawtransaction by promag · Pull Request #12194 · bitcoin/bitcoin · GitHub
< promag> those are the last 2 in 0.16 millestone
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/e37ca2be91bd...6e3fe936090d
< bitcoin-git> bitcoin/master 1e0d6e9 Wladimir J. van der Laan: tx: Update transifex slug for 0.16...
< bitcoin-git> bitcoin/master 6e3fe93 Wladimir J. van der Laan: qt: Update translation source file...
< achow101> Oh! 0.16 is almost ready. Only 2 PRs left
< wumpus> yep, I hope we can tag it after the meeting tomorrow
< wumpus> rc1, I mean
< wumpus> still need to some some release process things, such as updating hardcoded seeds
< sipa> wumpus: do we want #12253 in 0.16?
< gribble> https://github.com/bitcoin/bitcoin/issues/12253 | SegWit support for importmulti · Issue #12253 · bitcoin/bitcoin · GitHub
< wumpus> sipa: if possible, but I think it's getting a bit late for that
< sipa> i certainly won't have time until next week
< wumpus> I assume the code still needs to be written, and ofc it will need to go through a review cycle
< wumpus> right, so I guess that means no
< wumpus> should probably mention lack of support in importmulti it in the 'known issues' in the release notes
< promag> is that a thing for a 0.16.1?
< sipa> you think it's not essential?
< wumpus> no, I don't think it's essential
< promag> sipa: you think it is?
< sipa> signmessage may be worse, as it may silently break for people who didn't know we were switching to a new address format
< wumpus> but I think segwit address support is worth a release in itself, even if not all supporting features have made it in yet
< wumpus> so that one needs to be mentioned in the release notes as well
< sipa> okay
< wumpus> I can't be the only one that really wants a release with segwit wallet out?
< sipa> no, i'm sure you're not :)
< wumpus> promag: yes, could be 0.16.1
< ryanofsky> promag, fyi #12213 nulldummy.py test currently fails
< gribble> https://github.com/bitcoin/bitcoin/issues/12213 | Add address type to addmultisigaddress and createmultisig by promag · Pull Request #12213 · bitcoin/bitcoin · GitHub
< instagibbs> there's going to be a lot of cleanup regardless, 0.16.1 is going to happen
< promag> ryanofsky: ty
< wumpus> instagibbs: that's true for any major release, though :)
< instagibbs> I would look through release history to try and disprove, instead i shall just double-down, and log off
< instagibbs> ;)
< promag> is there anything deprecated in 0.15 to remove?
< bitcoin-git> [bitcoin] fwolfst opened pull request #12260: [Trivial] link mentioned scripted-diff-commit (developer-doc) (master...LINK_COMMIT_IN_DOC_DEVNOTES) https://github.com/bitcoin/bitcoin/pull/12260
< achow101> promag: I think all the currently deprecated things are for removal in 0.17 (except the accounts system)
< promag> so mark deprectated in X, remove in X+2?
< wumpus> depends on the feature and the reason for deprecation, so depends on what was discussed at the time of deprecating it
< achow101> no, the things currently deprecated were marked deprecated for 0.16
< promag> got it
< bitcoin-git> [bitcoin] laanwj opened pull request #12261: qt: Bump BLOCK_CHAIN_SIZE to 200GB (master...2018_01_block_chain_size) https://github.com/bitcoin/bitcoin/pull/12261
< promag> ryanofsky: fixed segwit
< promag> ^ test =)
< sipa> that sounds ominous
< promag> heh
< promag> wumpus: "Bump BLOCK_CHAIN_SIZE to 200GB" typo there s/_CHAIN//
< wumpus> promag: noooooooo
< sipa> hahaha
< promag> 100GB?
< wumpus> promag: I already had to juggle partitions to fit the current one on my dev machine
< Chris_Stewart_5> lol.
< Chris_Stewart_5> we can now fit our entire previous blockchain in two blocks. MUCH THROUGHPUT
< jcorgan> how about we embed the entire current blockchain in an OP_RETURN of a new genesis block?
< MarcoFalke> wumpus: What about #12251
< wumpus> of course, and after that the OP_RETURN policy limit goes from 80 bytes to 80 GB, so it's possible to embed entire DVD images
< gribble> https://github.com/bitcoin/bitcoin/issues/12251 | initwallet: Do not translate highly technical addresstype help by MarcoFalke · Pull Request #12251 · bitcoin/bitcoin · GitHub
< MarcoFalke> Asking since you pushed the translations recently
< promag> jcorgan: another typo, s/OP_RETURN/javascript
< wumpus> MarcoFalke: I think we should not translate option help messages at all
< promag> I guess project:bitcoin/bitcoin/8 will be updated in the next days?
< MarcoFalke> I know you proposed that #10962
< gribble> https://github.com/bitcoin/bitcoin/issues/10962 | Stop translating command-line option help? · Issue #10962 · bitcoin/bitcoin · GitHub
< wumpus> but yes it might be too late to do that for 0.16 so I'm ok with your change
< MarcoFalke> Yeah, I want to save our translators from trying to translate "segwit" or "p2sh"
< wumpus> agree
< bitcoin-git> [bitcoin] laanwj opened pull request #12262: net: Hardcoded seed update (master...2018_01_seed_update) https://github.com/bitcoin/bitcoin/pull/12262
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/6e3fe936090d...cc5870a4057f
< bitcoin-git> bitcoin/master fa7ecbf MarcoFalke: initwallet: Do not translate highly technical addresstype help
< bitcoin-git> bitcoin/master cc5870a Wladimir J. van der Laan: Merge #12251: initwallet: Do not translate highly technical addresstype help...
< bitcoin-git> [bitcoin] laanwj closed pull request #12251: initwallet: Do not translate highly technical addresstype help (master...Mf1801-walletNoTranslateInitHelp) https://github.com/bitcoin/bitcoin/pull/12251
< bitcoin-git> [bitcoin] jnewbery opened pull request #12264: Fix versionbits warning test (master...fix_versionbits_warning_test) https://github.com/bitcoin/bitcoin/pull/12264
< bitcoin-git> [bitcoin] jonasschnelli pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/cc5870a4057f...eadb2dacc3c6
< bitcoin-git> bitcoin/master 886a92f João Barbosa: [rpc] Add address type option to addmultisigaddress
< bitcoin-git> bitcoin/master f523c6b João Barbosa: [qa] Use address type in addmultisigaddress to avoid addwitnessaddress
< bitcoin-git> bitcoin/master eadb2da Jonas Schnelli: Merge #12213: Add address type option to addmultisigaddress...
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #12213: Add address type option to addmultisigaddress (master...2018-01-addmultisigaddress-address-type) https://github.com/bitcoin/bitcoin/pull/12213
< bitcoin-git> [bitcoin] jonasschnelli pushed 4 new commits to master: https://github.com/bitcoin/bitcoin/compare/eadb2dacc3c6...7abb0f0929bd
< bitcoin-git> bitcoin/master 31dbd5a João Barbosa: [wallet] Add change type to CCoinControl
< bitcoin-git> bitcoin/master 536ddeb João Barbosa: [rpc] Add change_type option to fundrawtransaction
< bitcoin-git> bitcoin/master 16f6f59 João Barbosa: [qa] Test fundrawtransaction with change_type option
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #12194: Add change type option to fundrawtransaction (master...2018-01-fundrawtransaction-changetype) https://github.com/bitcoin/bitcoin/pull/12194
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/7abb0f0929bd...f359afcc4104
< bitcoin-git> bitcoin/master ba490d2 Wladimir J. van der Laan: qt: Bump BLOCK_CHAIN_SIZE to 200GB...
< bitcoin-git> bitcoin/master f359afc MarcoFalke: Merge #12261: qt: Bump BLOCK_CHAIN_SIZE to 200GB...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #12261: qt: Bump BLOCK_CHAIN_SIZE to 200GB (master...2018_01_block_chain_size) https://github.com/bitcoin/bitcoin/pull/12261
< jonasschnelli> Our sighash tests do not exercise sigversion SIGVERSION_WITNESS_V0?
< jonasschnelli> Or to ask different, where can I find the testvectors for SIGVERSION_WITNESS_V0 sighash?
< Chris_Stewart_5> jonasschnelli: Isn't that tested by tx_{in}valid.json?
< jonasschnelli> Let me check...
< jonasschnelli> Chris_Stewart_5: I think your right,... was looking for bar sw sighash tests... thanks!
< Chris_Stewart_5> jonasschnelli: script_tests.json might have simpler tests for the serialization algorithm
< Chris_Stewart_5> there might be overhead to setting up the tx_valid.json test vectors
< jonasschnelli> Thanks!