< gmaxwell>
::sigh:: https://www.reddit.com/r/Bitcoin/comments/6pu13q/whats_up_with_bitcoin_core/ so github seems to be distributing tar files for our tags that have nothing to do with our distribution, and it sounds like netbsd's stuff was using them and is not happy that the checksums change (presumably because file order or compressor changed)
< sipa>
they've complained about that before
< sipa>
there was an issue about that years ago
< jonasschnelli>
Can we disable tar download via github? I guess no...
< wumpus>
we've been creating stable tarballs for years now, for distributions and such, what's wrong with them?
< wumpus>
more than two years, seeing that comment
< gmaxwell>
yea, just read. For some reason it seems that after nagging the crap out of us, they didn't switch to using the tarballs they asked us to provide.
< sipa>
almost 4 years ago
< wumpus>
well if there's something wrong with the tarballs from their pov we'd like to hear, but wtf post it to reddit
< gmaxwell>
well the person posting is probably an enduser, I guess
< luke-jr>
gmaxwell: it's because git decided to change git-archive output in some recentish version
< luke-jr>
they made the substituted hash abbreviation longer
< luke-jr>
wumpus: the stable tarballs are missing a bunch of stuff still :/
< sipa>
luke-jr: like?
< gmaxwell>
how could they be missing stuff ... they're just make dists
< luke-jr>
gmaxwell: `make dist` is missing the files
< gmaxwell>
Only in from-git/: .gitattributes
< gmaxwell>
Only in from-git/: .gitignore
< gmaxwell>
don't belong in the distribution
< sipa>
how does git checkdist succeed if things are missing?
< sipa>
or distcheck
< gmaxwell>
but the readmes and stuff are not good.
< sipa>
ah, agree
< gmaxwell>
should be trivial to add DISTFILES to the autotools stuff...
< luke-jr>
IIRC there's a bunch of contrib now missing
< luke-jr>
like init scripts and such
< luke-jr>
haven't done a full check recently
< gmaxwell>
why didn't you PR a fix
< luke-jr>
I don't know a good solution
< gmaxwell>
luke-jr: you list the files in EXTRA_DIST in makefile.am
< gmaxwell>
(though don't add the git specific stuff)
< luke-jr>
it's just going to get outdated again :/
< gmaxwell>
luke-jr: thats what review is for.
< luke-jr>
isn't there some way to get git to build the list?
< gmaxwell>
among other things, we don't want every file in git to end up in the dist.
< luke-jr>
the .git* files maybe not, but everything else we do
< gmaxwell>
luke-jr: in some other projects I've worked on, there were some machine generated source files that weren't checked in but were just generated and included as extra_dist, and some of the build scripts for those didn't get extra_disted.
< gmaxwell>
luke-jr: does our dist tarball even build?!
< cfields>
travis builds from tarball
< cfields>
(or something close to it)
< luke-jr>
talking with cfields, I'm thinking list the files individually, and add a script for Travis to complain if we add a new file that isn't either included or explicitly excluded from dist
< cfields>
that seems like more work to me, i'd rather just add "check for excluded files" as a step in the release process
< luke-jr>
cfields: I'll do it
< gmaxwell>
cfields: human steps get missed. (see also the last N RCs where versions didn't get bumped. :) )
< cfields>
heh
< wumpus>
luke-jr: which files are missing that would be required for a distro?
< wumpus>
and yes, the dist tarball builds
< wumpus>
it works fine, there have been no complaints about it at all, except from luke-jr apparently
< wumpus>
stuff not included is not part of 'make install', so generally not relevant to distros
< wumpus>
(such as developer tools)
< gmaxwell>
well there are files that are missing which should be included, including internal documentation, and contrib stuff like linearize. (docs including things like INSTALL.md and README.md that tell you things about building the software)
< wumpus>
if that's so, it should probably be part of 'make install'
< wumpus>
if it's not installed somewhere, how aer people normally supposed to use it?
< gmaxwell>
normally build instructions don't end up in make install though.. right.
< gmaxwell>
er right?
< wumpus>
that's maybe the only exception
< gmaxwell>
development script would be another.
< wumpus>
but linearize? I'm not sure that's generally useful to end-users, even if it should be, it's not in a fool proof state suited for that
< gmaxwell>
yes, linearize should get installed.
< wumpus>
then it should be maintained and tested too
< gmaxwell>
ya, and if it's not fool proof enough, thats a reason to not install it right now I guess.
< wumpus>
most developer scripts make no sense if you don't have a git checkout
< wumpus>
a lot even rely on it explicitly (to find the root for pulling transifex translations, for example)
< wumpus>
in any case I think being somewhat selective about what to include in the dist is fine
< wumpus>
though I agree just a git tarball would make things a lot easier
< gmaxwell>
verify-binaries, are quite useful and very much part of our source. I don't really think we gain anything by leaving something out even if it might not be that useful, except for pure git things like gitignore files.
< wumpus>
well then
< wumpus>
thinking of it, why don't we just skip the make dist and include everything
< wumpus>
zip up the git repository
< wumpus>
the tarball is the smallest file
< gmaxwell>
the make dist eliminates the endusers need for a full and compatible autotools setup.
< wumpus>
it's not like we have to worry about a few extra source files in the downlaod or storage
< wumpus>
wouldn't running autogen.sh before zipping uphave the same effect?
< wumpus>
(maybe with some special options)
< gmaxwell>
Yes +/- some temp files I think autogen leaves behind.
< wumpus>
we efiniltely don't need the selective part of 'make dist' then
< wumpus>
yes, well that's not too bad
< gmaxwell>
I don't think I have know any counterarguments to that point.
< gmaxwell>
maybe someone who knows autotools better would be aware of something we're missing.
< wumpus>
it would be the minimum energy expenditure to finally getting this discussion out of the way at least
< wumpus>
I am kind of surprised verify-binaries isn't included
< wumpus>
cfields: any reason not to do the above? e.g. make the tarball simply a repo dump + build system autogenerated files
< gmaxwell>
looking at the list, I think its just accidental. stuff got added without anyone throwing it into extra dist because it's easy to forget. seems there are things in libsecp256k1 where we'd do that too.
< cfields>
wumpus: i suppose that's fine
< wumpus>
otherwise we're bound to forget things and have this "the tarball is useless!" discussion again and agian
< cfields>
the 'make dist' stuff is kinda outdated. It's intended to be an explicit list of files so that the release tarball doesn't get polluted by the builder's working dir...
< cfields>
but with our build process, it's really kinda moot
< wumpus>
which could also be avoided by taking the snapshot before starting building
< wumpus>
yeah
< cfields>
yea, tar it up after bootstrap but before anything else
< wumpus>
I understand the reasoning behind being selective about what to include, but we don't have the people nor attention to carefully decide that every time, so erring on the side of 'include everything' makes sense for this project. For others (like secp256k1) which are more focused it's probably different.
< luke-jr>
fixing make dist is proving to be a real pain (hi depends/ … ) so I like the idea of not using it
< wumpus>
depends/ is another joy in that regard, yes :)
< cfields>
agreed
< wumpus>
sigh, no, I still don't think it's acceptable to mention bitcoin-cli specifically in RPC error messages, it should be aimed at people using the JSON-RPC API, not people using bitcoin-cli only
< wumpus>
there's no point in talking about a specific client in server error messages
< wumpus>
that's just wrong, and won't help someone struggling with this interface in say python-bitcoinrpc
< wumpus>
why is it so much to ask that documentation for bitcoin-cli arguments belongs in bitcoin-cli?
< wumpus>
it doesn't belong in two places, no it just belongs in bitcoin-cli
< wumpus>
gah, coredev.tech is september, scaling bitcoin in november, both in SF area, would have been better to plan it closer together :/
< wumpus>
phew, at least 'make dist' includes the function test suite
< sipa>
wumpus: just stay :)
< wumpus>
sipa: if it was a two weeks in between instead of two months I would
< sipa>
yeah :)
< wumpus>
indeed, the ESTA allows staying 90 days, so theoretically it'd be possible, but just too much hassle
< jonasschnelli>
wumpus: Yeah. The SB guy contacted me after I have booked the room.
< jonasschnelli>
He asked to move it closer... :)
< jonasschnelli>
But was already too late.
< jonasschnelli>
Too bad he did not contact me before announcing the date
< jonasschnelli>
I bet the knew weeks ago
< bitcoin-git>
[bitcoin] promag opened pull request #10941: Add blocknotify functional test (master...2017-07-blocknotify-functional-test) https://github.com/bitcoin/bitcoin/pull/10941
< promag>
#10941 makes sense right? should add tests for -walletnotify too right?
< promag>
refactors can impact tests, for instance, a test uses a lib which has new signatures..
< promag>
in terms of functional tests, yes you're right
< wumpus>
you could have tests for e.g. optimizations, that check benchmarks for inner functions such as sha256 etc, but it'd require a stable dedicated platform, certainly not on our current infrastructure
< wumpus>
also something that automatically syncs from a local node and keeps timings, daily w/ most recent master could be useful, but it'd certailny need a deterministic environment or there'd be too much noise
< promag>
right
< promag>
I'm for sure interested in improve that
< promag>
it's a great way to get deeper in the project
< wumpus>
it'd need infrastructure, not so much coding work
< wumpus>
with an available server it'd be quite easy to set it up
< jonasschnelli>
promag: I could provide you the necessary infrastructure (server hosted remotly, ideally for test/benchmarks)
< bitcoin-git>
bitcoin/master 72f0060 Dag Robole: Replace traditional for with ranged for in primitives
< bitcoin-git>
bitcoin/master ba1bbb0 Wladimir J. van der Laan: Merge #10892: Replace traditional for with ranged for in block and transaction primitives...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10892: Replace traditional for with ranged for in block and transaction primitives (master...20170721-rangedfor-primitives) https://github.com/bitcoin/bitcoin/pull/10892
< wumpus>
re: installing linearize, I think the main thing that needs to be done to make it more user friendly is to make it one command, instead of the split between generating the blocks list and writing them out. These could be subcommands/options for when doing them separately.
< wumpus>
at the least cookie authentication was already implemented, that's nice, used to be that it required always manually setting up the authentication
< wumpus>
"In many organisms, a significant fraction of the genomic DNA is highly repetitive, with over two-thirds of the sequence consisting of repetitive elements in human." well at least that means it could be compressed efficiently :p
< BlueMatt>
???
< wumpus>
probably responding to the genesis block that gets added again and again
< gmaxwell>
yes
< BlueMatt>
ah
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jul 27 19:00:27 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< achow101>
I checked last week that this was still a problem
< wumpus>
ok, so this still needs to be fixed?
< gmaxwell>
cfields: are you working on that? if you don't have time, someone else can pick it up
< BlueMatt>
its assigned cfields....
< achow101>
it should probably be fixed, but I don't know if it is a blocker or how urgent a fix would be required
< morcos>
He's only had 8 months cut him a break
< gmaxwell>
Looks like it should be a trivial fix (famous last words)
< achow101>
it was already pushed from 0.14 to 0.15, so I'm guessing it isn't all that urgent
< wumpus>
I don't think uninitalized state in a test is important enough to block 0.15, though it could result in intermittent faliures which are always annoying
< jonasschnelli>
Yes. Moving to 0.16 would be possible.
< wumpus>
though it might hide real issues
< wumpus>
(if the tests are not valgrind clean)
< gmaxwell>
I'm guessing cfields and sipa are not in the meeting today due to timezones.
< gmaxwell>
I'll follow up and make sure it gets fixed (either extract something cfields hasn't submitted yet or fix it myself)
< wumpus>
ok, thanks
< gmaxwell>
(or realize it isn't actually trivial and ask for a punt to the next version)
< wumpus>
#topic sendtoaddress subtractfeefromamount=true does not respect paytxfee values (#10034)
< morcos>
Are we going to do the wiki thing again for release notes? If so can we put a link to it on the top of #9889. I kept having trouble finding that link last release
< wumpus>
I'm not sure about this one, don't think it has been looked at much
< wumpus>
morcos: yes, good idea
< gmaxwell>
Lets decide if we're doing the wiki or not, last time the wiki merge killed release notes written directly. :)
< wumpus>
but it helped a lot with actually getting release notes written
< gmaxwell>
I've held off on writing release notes due to lack of clarity on this point.
< wumpus>
and getting the style consistent etc
< gmaxwell>
I'm fine with doing it, I just need to know where to put them.
< wumpus>
well right now you simply have to create a PR
< gmaxwell>
I don't want to write any outside of the wiki if we're using the wiki.
< wumpus>
as soon as the wiki is open, do it there
< morcos>
I recommend we do the wiki, but we need a good code of conduct, where people should not make potentially controversial changes without flagging them somehow
< gmaxwell>
wumpus: I did that last time and they got stomped by the wiki merge. :)
< morcos>
Let's do the wiki.
< jonasschnelli>
+1
< wumpus>
gmaxwell: I know, we should avoid that, but that's no reason to completely abandon a good idea
< gmaxwell>
So on this feefrom amount, that sounds like the known overpayment bug with no change that we fixed though I don't recall if we fixed it in the feefrom amount case.
< gmaxwell>
wumpus: I'm all for the wiki.
< achow101>
we should do the wiki and probably open it now
< gmaxwell>
Just someone point me to it and I'll write a bunch of release notes.
< wumpus>
gmaxwell: I think we should put the commit id that the wiki 'forked' from on that page, so that it can be compared if there haven't been changes since when mering it back
< achow101>
+1
< gmaxwell>
that would help too.
< wumpus>
(this was missing last time, so I had no reference point)
< kanzure>
wumpus: if you mean github wiki, those are actually git repositories, so they do literally have forks/branches (even if github tosses them).
< achow101>
is #10034 actually a problem or is it just an instance of an already known issue?
< morcos>
I'll take a look at 10034, but there have been multiple changes to this code for 0.15, so not clear that even if there was a bug it would still be there
< wumpus>
kanzure: true, you can fork and edit locally, but I mean the bitcoin core master revision
< wumpus>
morcos: thanks
< jonasschnelli>
Also, if 10034 is still a bug, it does not deserve the "medium" tag IMO
< gmaxwell>
achow101: I think 10034 is the known thing that we fixed at least in the non-fee-from-value case.
< gmaxwell>
jonasschnelli: Do you believe it deserves high.. overpaying fees should be high. IMO.
< morcos>
gmaxwell: it seems to have some determinism in the description that bears a closer look.. but i'm not expecting to find anything
< wumpus>
the dirty secret is that we don't really use the priority tags for issues/prs for anything :)
< jonasschnelli>
heh
< gmaxwell>
yea...
< gmaxwell>
morcos: thanks for looking at it.
< jtimon>
priority medium seems specially useless, if it's not tagged low or high, then it's medium, no?
< wumpus>
#topic Force on-the-fly compaction during pertxout upgrade UTXO Db and Indexes (#10526)
< gmaxwell>
jtimon: untagged is a possible state (not evaluated yet).
< wumpus>
jtimon: I think in practice only priority high could be useful, if someone paid attention to it
< gmaxwell>
wumpus: as morocos noted on the PR, it's not shrinking on its own.
< gmaxwell>
$ du -sh .
< gmaxwell>
5.8G .
< wumpus>
jtimon: then again, I think a year ago I checked "high priority" issues and some were >5 years old
< jtimon>
gmaxwell: right, but then you have to tag them all with something about priority once you evaluate them
< BlueMatt>
see my most recent comment on it: that pr makes upgrade time a bunch faster for me, but also doesnt actually save disk space directly (but its done later upon next-db-reopen cause i guess leveldb realizes it has a bunch of tables it doesnt need)
< BlueMatt>
gmaxwell: I believe it may clean it up over time, but not quickly
< gmaxwell>
So I think we really need to do that... but IIRC we had a report from wumpus saying that it didn't work for him.
< BlueMatt>
anyway, yes, we should take 10526 for 15 imo, morcos had one code issue on it, but it doesnt appear to change behavior for me
< gmaxwell>
BlueMatt: I've had two weeks and many startup and shutdowns on this node... so ... really not quickly.
< wumpus>
gmaxwell: it doesn't work reliably for me, no
< gmaxwell>
wumpus: could it be what bluematt says.. it works but doesn't take effect until after another restart?
< BlueMatt>
gmaxwell: see my later comment, i meant that it'll do it when tables fill, ie when you connect more blocks and so
< wumpus>
gmaxwell: sometimes after running upgrade on that test set it it is 2.1G sometimes 4.3G, apparently random, after another restart it's always down
< wumpus>
gmaxwell: yes, that could well be
< gmaxwell>
okay, well thats still better than behavior in master.. which I've tried on several hosts and it results in a persistant 5.8 gb state.
< morcos>
well if it consistently shrinks at the very latest after another restart, that's a clear improvement
< wumpus>
well the, we should merge it, better is better
< BlueMatt>
if we want we can probably simulate restart by closing and re-opening db :p
< morcos>
needs a bug fix though, commented on pr
< wumpus>
yes
< achow101>
I hope it does actually work. my node running master has a chainstate of 8.2 GB right now (it was upgraded from a previous 0.14 master)
< gmaxwell>
BlueMatt: yes though that doesn't get rid of the peak usage, we'll need to release note that upgrading will require an extra 3GB free disk space.
< achow101>
I can test it too
< gmaxwell>
uh.. 5GB disk space.
< wumpus>
achow101: yes, please test
< gmaxwell>
achow101: well it doesn't fix already bloated ones.
< BlueMatt>
gmaxwell: yes, please note that on the release notes todo
< morcos>
how'd you get 8.2, that's yuuuge
< achow101>
i don't know
< gmaxwell>
morcos: don't make fun of how I pronounce huge. :(
< BlueMatt>
heh
< gmaxwell>
Maybe we should sneak in a hidden rpc to compact the whole database. (I think there is just a leveldb call to do that)
< wumpus>
8.2 is definitely abnormal, if it's from a very old version maybe something left behind
< BlueMatt>
gmaxwell: seems reasonable to me
< achow101>
wumpus: its the same datadir from a 0.10 master that has been periodically upgraded over the years
< achow101>
to different random master builds
< wumpus>
(leveldb changed some things also, over time, e.g. the extension for database files at some point even)
< gmaxwell>
achow101: it would be interesting to see if leveldb compaction shrinks it, or if it actually has zombie records in it due to some kind of corruption.
< wumpus>
achow101: can you send me a pastebinned 'ls'?
< wumpus>
zombie records, or zombie files even
< gmaxwell>
achow101: you should also try gettxoutsetinfo to compare its hash with another non-bloaty host.
< wumpus>
adding a hidden RPC to compact databases is fine with me
< gmaxwell>
I think we know what to do to go forward on this one then.
< wumpus>
ye
< wumpus>
#topic Fix some chainstate-init-order bugs (#10758)
< gmaxwell>
10882 I need to sync up with the current state, it looked like it was going in a good direction. I see the latest comments have some improvements still needed.
< wumpus>
jtimon: no, of course not, the purpose is that they can copy it and change the attribution and claim it's theirs :-)
< gmaxwell>
but it seems like people know what to do...
< gmaxwell>
wumpus: lol
< jtimon>
wumpus: lol
< achow101>
lol
< morcos>
gmaxwell: i think 10882 is pretty much done
< BlueMatt>
its very close, yes
< gmaxwell>
\O/
< gmaxwell>
I'll review today.
< jonasschnelli>
Great
< gmaxwell>
(as an aside, I'm really happy that people found solutions to issues I raised that I had no idea how to fix)
< wumpus>
ah the instructions for manual wallet top-up were added, great
< wumpus>
yeah
< gmaxwell>
(in particular the two thresholds, so you can bypass critical shutdown)
< wumpus>
lol github things the diff of wallet.cpp is so huge it hides it by default
< gmaxwell>
I hate that feature.
< jonasschnelli>
Yes. Make searching really painful
< jonasschnelli>
*Makes
< gmaxwell>
Caused me to screw up reviews before.
< gmaxwell>
(because I searched for X and then complained a PR couldn't work because it didn't touch X)
< BlueMatt>
lol just dont review on github directly :)
< BlueMatt>
next pr?
< wumpus>
gmaxwell: well they got complaints from people like me that browsers crashed/run out of memory if the page becomes very large, but I agree this is suboptimal too
< gmaxwell>
obviously we need to split that file. :P
< BlueMatt>
to get the first one merged cause people didnt want me to have a huge ever-growing pile of fixes in one pr
< jtimon>
probably obvious but why not symlinks?
< wumpus>
jtimon: berkeleydb doesn't gracefully support them, especially if you link outside the context directory
< jtimon>
thanks
< wumpus>
so it's very, very dangerous
< wumpus>
BlueMatt: ok
< wumpus>
#topic segwit wallet release (gmaxwell)
< gmaxwell>
So, segwit is going to activate. It would be nice to get a version out
< gmaxwell>
soon that exposes a segwit wallet to the user, and maybe supports sending
< gmaxwell>
to BIP173 style addresses. Maybe we could consider short cycling 0.16
< gmaxwell>
with primarily just this functionality, or something like that?
< gmaxwell>
Since we technically have support but don't wire it up by default, (except
< gmaxwell>
via the RPCs) I believe it's a short effort; mostly a testing concern.
< gmaxwell>
Another alternative would be to branch, off a 0.15+segwit and cut
< gmaxwell>
expiremental releases for people.
< gmaxwell>
There are some political implications, since getting adoption of SW
< gmaxwell>
will tone down some mania in the industry.
< jonasschnelli>
Agree
< BlueMatt>
seems reasonable
< BlueMatt>
i vote for at 15.segwit
< achow101>
perhaps start off by giving out the p2sh nested addresses first and then move on to bech32?
< wumpus>
no problem with doing 0.16 faster (create a specific segwit-themed feature release, if it is realistic to do it in less than 6 months but still takes significant time (say, 3 months)
< gmaxwell>
Well I threw out two ideas: 0.16 fast and 0.15+segwit as a seperate downloadable thing...
< achow101>
why not just a 0.15.x release?
< jonasschnelli>
So 0.15.1 + 0.15.1+SW?
< gmaxwell>
achow101: the point on 173 is support for sending to. We likely wouldn't actually issue those addresses for >1 year.
< wumpus>
achow101: it's a feature
< * BlueMatt>
would be somewhat dissapointed to see a full release number for such a small set of changes
< BlueMatt>
15.segwit seems reasonable, while working on 16 still
< gmaxwell>
If we did 0.15+sw as a full release we should call that 0.16. But thats just a naming thing.
< wumpus>
so is it really a small set of changes? or are you underestimating it?
< BlueMatt>
well, ok
< jtimon>
achow101: yeah, wondering the same
< gmaxwell>
but we could also release it as 0.15+expiremental segwit, an explicit alternative.
< BlueMatt>
point being lets not freeze master again soon just for this
< BlueMatt>
gmaxwell: all you're saying is support sending to segwit addresses, right?
< BlueMatt>
not gui showing them as a receive address?
< gmaxwell>
BlueMatt: if we do BIP173 support it would just be sending to.
< wumpus>
yeah full GUI support is significant work
< BlueMatt>
wumpus: hmm?
< BlueMatt>
i mean we're just talking about a send-only address format, no?
< gmaxwell>
There are two questions: segwit wallet, and sending to BIP173. I consider the second less important, it's also more work (because it isn't already dormant in the codebase).
< wumpus>
it's very easy to underestimate the changes needed, the tests, reviews, iterations
< achow101>
gmaxwell: what do you mean by segwit wallet?
< jtimon>
well, with the short release (let's say, 3 months) we don't necessarily need to "freeze master", those would be just high priority features for the release but we can still keep doing other things, no?
< gmaxwell>
Segwit wallet basically means making getnewaddress return p2sh embedded segwit addresses.
< gmaxwell>
(and the GUI of course)
< BlueMatt>
mmm, yea, thats...trickier
< gmaxwell>
BlueMatt: well technically it's just a few lines changed, if we ignore the testing burden. :)
< BlueMatt>
heh, yea, well testing
< wumpus>
building it on top of 0.15 and calling it 0.16 that's possible too, though a big change from how releases have always been done
< gmaxwell>
because the wallet does support segwit today... just doesn't use it without dorking with the rpc.
< wumpus>
would be the first major release not branched from master
< BlueMatt>
i mean its not a 2 week project, but i also dont think its worth freezing master to do it (and given it'd be nice to get it out, it'd be nice to have *only* it as changes)
< wumpus>
and it still needs to land in master anyhow, no way around that
< * BlueMatt>
is fine with calling it 0.15.1, fwiw
< gmaxwell>
And in _theory_ the segwit support in the wallet is "tested".
< jtimon>
yeah, nack on working on 0.15 and calling it 0.16, why not simply 0.15.1 ?
< gmaxwell>
So there is a scope question and a naming question. Lets not conflate them.
< BlueMatt>
fair, scope wise lets not creep
< BlueMatt>
which means keep it on the 15 branch
< BlueMatt>
naming, i dont feel strongly about, either way wfm
< wumpus>
ok
< jonasschnelli>
would the 0.15.SW always generate p2sh embedded sw addresses or optional?
< BlueMatt>
by default, i assume
< gmaxwell>
So there are two things I think are potential for scope, SW wallet (getnewaddress returns p2sh embedded SW), and BIP173 sending. We don't have to do them at the same time but it would be nice to be able to say that any SW enabled wallet can send to 173 style addresses.
< gmaxwell>
jonasschnelli: I assume we'd make it gated by a config option like usehd.
< gmaxwell>
Unless we ran into hairballs that we couldn't resolve.
< jtimon>
if the scope is smal it seems reasonable to do it on 0.15.1/0.15.segwit
< wumpus>
it certainly needs to be optional, default behavior can't change in a minor version
< jonasschnelli>
BIP173 sending only is kinda hard(er) to test without the rec. part
< jonasschnelli>
wumpus: Yes. That makes sense
< wumpus>
sending obvs is optional in itself, you don't *need* tos send to BIP173 addresses, and if you do you know what you're doing. I mean about receiving.
< gmaxwell>
jonasschnelli: we can recieve BIP173 payments with some hidden options. (I believe the code is still there-- we have tests for that already)
< gmaxwell>
jonasschnelli: BIP173 is native segwit, we have tests for native segwit... what BIP173 adds is just an address encoding for it, so that it can be made user accessible.
< jonasschnelli>
Indeed
< gmaxwell>
But actually generating 173 addresses won't be useful for a long time... since you don't want to do that until basically everyone can send to it, took 2-3 years for p2sh.
< wumpus>
creating bip173 receiving addresses should be optional, to not confuse too much, especially as long as vrutally no other wallets have support for it
< gmaxwell>
I could see 173 adoption going faster, but it'll still take a while, and I don't see any reason to push it. But we should get started soon.
< wumpus>
yes
< gmaxwell>
(also pushing for rapid 173 adoption has a downside of making people think it's needed for segwit, it's not)
< wumpus>
so anyhow, we'll branch off a special branch from 0.15 after the 0.15.0 release, for this work to be done?
< gmaxwell>
I would be okay with that. We don't need to decide quite yet, but I wanted to get people thinking about it.
< jtimon>
so what about 0.15.1 -> segwit wallet, 0.15.2 - > send to bip173, optionally receive ?
< BlueMatt>
wumpus: that seems reasonable to me, ultimately i dont care too much and its up to you
< wumpus>
or, I guess development could still be done on master w/ backports as normal
< BlueMatt>
oh, yea, I'd vote backports
< gmaxwell>
wumpus: that would be better IMO
< BlueMatt>
whether its on the 15 branch or on a 15.segwit branch I dont care
< achow101>
ack backports
< wumpus>
(in the other case you'd have to forward-port)
< achow101>
so we can have it in 0.16 too
< wumpus>
yes
< gmaxwell>
I really think the segwit-wallet part will be pretty minimal effort, except perhaps for more testing... just because it's virtually already done.
< jtimon>
2 mins
< wumpus>
I was thinking people wanted to work on the 0.15 branch for a moment, which would make sense if we expect master/0.16 to drift apart very quickly, but I don't think that's very true for the wallet (though there's more multi-wallet changes waiting for merge after the branch)
< gmaxwell>
But who knows whatever dusty corners we'll expose.
< gmaxwell>
wumpus: well it might favor delaying some 0.16 development to happen after this.
< wumpus>
but forward and backporting isn't much different effort and it has to end up in master too
< gmaxwell>
but I don't think there are many PRs already open that would get in the way.
< jonasschnelli>
What about delaying 0.15 for P2SH(P2WPKH) support?
< wumpus>
gmaxwell: in that case we're doing the 0.16 freeze that BlueMatt didn't want :)
< gmaxwell>
jonasschnelli: I'd rather not.
< wumpus>
I'd also prefer not
< wumpus>
0.15 is almost ready
< gmaxwell>
jonasschnelli: if we wanted to do that, I'd rather we just do 0.15 and then release 0.16 a couple weeks later with it.
< wumpus>
0.15.1 is better
< jonasschnelli>
Okay
< wumpus>
would be very bad to delay 0.15.0 on a *feature* now, so long after the feature freeze
< wumpus>
espeically one that doens't even have a PR open
< BlueMatt>
*DONG*
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jul 27 20:01:16 2017 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< BlueMatt>
well, clearly we have failed, we should give up having meetings
< gmaxwell>
that meeting was invalid, gonna have to drop it and make a new one
< achow101>
so about the large chainstate I have, here's the pastebin in case you missed it: https://pastebin.com/hAeYMjRW
< wumpus>
we should have a meeting about better timing of meetings
< wumpus>
achow101: ah yes, thanks, forgot about that one
< gmaxwell>
BlueMatt: so why do you think it's not a two week project.. barring testing, I think we just shim getnewaddress to add the witness address. And then it will just work(tm).
< wumpus>
achow101: .fuse_hidden files?!
< achow101>
dunno wtf those are
< BlueMatt>
gmaxwell: i dunno, i dont expect anything to get done in 2 weeks
< BlueMatt>
gmaxwell: just review of wallet stuff is usually slow, plus testing and release cycle
< wumpus>
achow101: what size do you get if you only measure LOG and .ldb files?
< gmaxwell>
BlueMatt: well sure, but I think it's like a 1 day change plus N weeks of discussion and tweaking.
< wumpus>
achow101: I think those are your junk files
< wumpus>
achow101: fuse_hidden files are part of filesystem management anyhow, not of the leveldb database
< BlueMatt>
gmaxwell: yea, that
< gmaxwell>
BIP173 is worse becuase it's just not implemented at all... I wouldn't even suggest it except for the nice property of being able to say "any segwit version can send to BC addresses".
< wumpus>
achow101: sometimes they're left behind
< jcorgan>
what fs is dir in
< achow101>
oh
< wumpus>
sshfs?
< achow101>
it's 5.8 of .ldb files
< achow101>
*5.8 GB
< wumpus>
achow101: that's more or less the normal size after upgrade and before compaction
< gmaxwell>
oh you said 8GB before, which was concerning.
< achow101>
yeah, that makes sense
< achow101>
gmaxwell: that's the entire chainstate folder
< gmaxwell>
hm my entire chainstate folder is 5.8
< achow101>
jcorgan: I think its an NTFS drive
< jcorgan>
ah ok
< jcorgan>
so ntfs-fuse then
< achow101>
(I use it with windows occasionally)
< BlueMatt>
ntfs? eww
< achow101>
it started out as a windows drive
< wumpus>
gmaxwell: it turns out to be full of fuse fs ghost files
< achow101>
too lazy to change it
< jcorgan>
those .fuse_hidden files are files deleted but someone still has an open file handle to it
< wumpus>
I also had my datadir for one node on a ntfs-formatted external HDD for a long time, it seems to work surprisingly well
< gmaxwell>
zoinks, well thats one mystery solved.
< achow101>
jcorgan: can they be safely deleted?
< wumpus>
jcorgan: yep, and sometimes it forgets about them and leaves them behind
< jcorgan>
yes and yes
< wumpus>
achow101: yes, maybe close bitcoind first just in case
< achow101>
too late
< wumpus>
(nothing *else* will be holding on handles to the chainstate files)
< jcorgan>
the dates on those were from last september, you've probably rebooted since then :)
< wumpus>
lol, yes
< achow101>
I probably have. uptime says 43 days
< wumpus>
or at least restarted bitcoind
< jcorgan>
yeah, then they were just a bunch of fuse turds
< achow101>
so how about compacting the database after upgrade (and not upgrading with 10526)?
< achow101>
or is that "you did something stupid so deal with it yourself" kind of thing?
< wumpus>
achow101: gmaxwell was talking about adding a hidden RPC to do database compactions, seems like a reasonable idea
< achow101>
would it be a bad idea to just compact every startup?
< wumpus>
yes, slow
< achow101>
I figured
< wumpus>
leveldb does it automatically once in a while anyway
< wumpus>
(depending on some metric)
< wumpus>
normally you shouldn't want to bother with it, unless you just rewrote the entire database like with the upgrade
< morcos>
Murch: how deterministic is the existing coin selection algorithm? if you call it twice with the same nValueToSelect, how likely is it to return the same inputs?
< Murch>
morcos: I have not explicitly checked that, but since it heavily biases towards the largest input available and removes the last when it overshoots, my gut feeling would be that the vast majority of the 1000 attempts would get the same result.
< Murch>
or at least the vast majority would reach a very small set of results.
< morcos>
ok, thanks, i'm just looking at #10034 and there is a some braindead behavior where we stupidly call SelectCoins a second time for no reason if we are subtracting from recipient
< Murch>
~50% of the results include the first utxo below target for starters. 25+% include the second utxo (depending on whether the first and second both fit at the same time)
< morcos>
but i'm not seeing why it would consistently lead to significant overpaying
< Murch>
morcos: The overpaying behavior comes from locking in the expected fees from the previous run.
< morcos>
Murch: yes i'm aware of that problem, but that should be quite rare, not something that is seen over and over again
< morcos>
and should be even less likely with subtractfeefromamount, b/c then you are trying to select the same value on the second run, so unlikly to choose far fewer inputs
< Murch>
morcos: every time when one round fails for selecting too little fees and then the next one succeeds with fewer inputs.
< sipa>
so i think #10924 should be tagged for 0.15; i'll update with some more information
< gribble>
https://github.com/bitcoin/bitcoin/issues/10924 | [RPC][Wallet][Segwit] Bug: Transaction sent to imported P2WSH does not appear in listtransaction. · Issue #10924 · bitcoin/bitcoin · GitHub
< sipa>
it's possible to make gettransaction and listtransactions output diverge
< Murch>
I think you changed something that took a minimal amount of fee from the change output when too little fee was selected. That should fix it. right?
< sipa>
by importing a native segwit output script with importaddress, it getd detected as an ismine transaction, but the receiving end is treated as change
< morcos>
Murch: my fixes didn't apply when you were subtracting fees from recipients, but in theory that case is the simplest to solve, just subtract fee from recipients. :) But not sure we need that now for 0.15 or not
< sipa>
the fix for #10924 is easy: we need to add P2WSH/P2WPKH to CTxDestination, which is needed anyway for bip173 support
< gribble>
https://github.com/bitcoin/bitcoin/issues/10924 | [RPC][Wallet][Segwit] Bug: Transaction sent to imported P2WSH does not appear in listtransaction. · Issue #10924 · bitcoin/bitcoin · GitHub
< Murch>
I seem to remember that you fixed at least the accumulative fee issue, but I'm not sure.
< bitcoin-git>
[bitcoin] morcos opened pull request #10942: Eliminate fee overpaying edge case when subtracting fee from recipients (master...subtractfee) https://github.com/bitcoin/bitcoin/pull/10942
< promag>
morcos: is fSubtractFeeFromAmount something really used?
< gmaxwell>
Yes, people use it.
< gmaxwell>
It's a pretty useful way, for example, to transfer funds to another account you control; like an exchange, when you want to spend a given amount.
< promag>
ah I see, ty
< promag>
however we could do the same without having that hassle in the algorithm: do as normal and then subtract the fee from those outputs and add it to the change
< gmaxwell>
promag: one of the applications is completely emptying a wallet.
< gmaxwell>
without subtract fee from amount there is basically no way to do that (other than manually with raw transactions)
< promag>
I surrender
< gmaxwell>
:)
< gmaxwell>
It's a good instinct to want to simplify.
< morcos>
promag: irrelevangt of whether this fixes a bug or not, its a better design to just pick the inputs once and then subtract the fee. That's what this PR accomplishes
< morcos>
granted it does it in a kind of roundabout way, but all this code is going in the rubbish bin the day 0.15 is released
< morcos>
so no reason to go crazy trying to make it pretty right now
< promag>
yeah dejavu
< morcos>
even if it doesn't look better, the logic is far clearer and smarter now than it was in 0.14