< GitHub196> [bitcoin] luke-jr opened pull request #7340: Replace univalue subtree with proper dependency on external UniValue (master...sys_univalue) https://github.com/bitcoin/bitcoin/pull/7340
< kangx> hello.
< michagogo> cfields: detached sigs on the way?
< cfields> michagogo: signing this very second
< cfields> there's a snag with one of the descriptors, but nothing to worry about for rc1 i think
< michagogo> Cool, I'll see if I can kick off my script before I leave
< cfields> michagogo: you'll have to hand-edit one of the descriptors
< michagogo> Hm?
< michagogo> How so?
< cfields> i'll PR it in a sec, doing sanity checks on the build atm
< michagogo> (They're hashed, right?
< michagogo> )
< cfields> need to remove libc6:i386 out of the osx signer
< michagogo> Can you just push it to the 0.12 branch, and then I'll check that out instead of the tag?
< cfields> still need to build from the tag, just change the descriptor locally for the sig attach
< michagogo> But gitian keeps its own git repo and brings in the tag on its own
< cfields> yes, but the descriptor is local
< michagogo> So if that's the only change to the descriptors from the tag, the clone of the repo that I have outside gitian can be checked out to the branch
< michagogo> Er, can have the branch checked out
< michagogo> Since its ignored other than the descriptors
< cfields> you can copy the descriptor to /tmp, it doesn't matter where it lives or what's checked out
< michagogo> cfields: I know that. I'm saying, it's probably easiest if you just commit the fix to the 0.12 branch, since I assume that needs to be in rc2 anyway.
< cfields> oh
< cfields> <cfields> i'll PR it in a sec, doing sanity checks on the build atm
< cfields> :)
< michagogo> That way the fixed descriptor can easily be used by checking out 0.12 in the external copy of the repo
< michagogo> Yeah, just saying that makes the hand-editing unneeded as soon as that's in
< cfields> sure, gotcha
< GitHub160> [bitcoin] theuni opened pull request #7342: release: remove libc6 dependency from the osx signing descriptor (master...gitian-quickfix) https://github.com/bitcoin/bitcoin/pull/7342
< cfields> sigs pushed
< Luke-Jr> jgarzik: can you 1.0.2 univalue?
< Luke-Jr> although it might have a bug, so maybe we should figure that out first..
< jgarzik> Luke-Jr, sure
< Luke-Jr> jgarzik: any idea on how to determine the cause of https://travis-ci.org/bitcoin/bitcoin/jobs/102242679 ?
< Luke-Jr> I can't reproduce it.. :/
< cfields> Luke-Jr: it's a string being constructed from setNumStr(), i'd guess null pointer as input
< Luke-Jr> hmm
< Luke-Jr> if I hack the travis config to --enable-debug, think it'd give line numbers? <.<
< cfields> Luke-Jr: you can track it back with that, sec
< cfields> Luke-Jr: looks like it's internal to univalue, barring some race condition
< Luke-Jr> :x
< jgarzik> Luke-Jr, quickest debug solution is running it under gdb in a VM that mimics travis OS setup
< jgarzik> Smart to have such a VM lying around anyway
< Luke-Jr> that doesn't sound quick, considering Travis is closed source.. :/
< cfields> UniValue::setInt(int64_t val) -> setNumStr()
< jgarzik> the OS setup is easy to reproduce
< cfields> looks like your crash is at univalue.cpp:129
< jgarzik> setNumStr(oss.str());
< cfields> Luke-Jr: throw an assert(!oss.empty()) in there
< jgarzik> if css is empty, something is majorly broken with libstdc++
< * jgarzik> kicks autocorrect
< cfields> no clue how that would happen though
< cfields> jgarzik: yea, threading/shutdown issue seems much more likely
< Luke-Jr> ⁂ glibc detected ⁂ /home/travis/build/bitcoin/bitcoin/bitcoin-i686-pc-linux-gnu/src/bitcoin-cli: free(): invalid pointer: 0xb7774348 ⁂
< Luke-Jr> really doesn't suggest a NULL ptr thing
< Luke-Jr> IMO
< cfields> Luke-Jr: yea
< Luke-Jr> 1 hour download for old Ubuntu Server that Travis uses.. :/
< * Luke-Jr> kicks DSL
< Luke-Jr> at least I've confirmed Travis reproduces it..
< cfields> Luke-Jr: looking more closely, i think it's probably at univalue.cpp:118
< cfields> new guess is that the object is already destructed when that assign is attempted
< cfields> Luke-Jr: bitcoin-cli.cpp:214 as an example. Isn't that returning a bound temporary?
< Luke-Jr> const UniValue& reply = valReply.get_obj(); ?
< Luke-Jr> seems get_obj just throws or returns *this
< cfields> Luke-Jr: yes, but it's a temporary bound to valReply, which goes out of scope when the function returns.
< Luke-Jr> oh, right
< Luke-Jr> didn't realise it was being returned
< cfields> unless there's some funky RVO that makes that legal
< Luke-Jr> doubt it
< Luke-Jr> oh
< Luke-Jr> it's probably because the return type is not a reference
< Luke-Jr> so it makes a copy
< Luke-Jr> ?
< jgarzik> correct - return value is not a reference
< jgarzik> FYI - get_obj() is legacy and should not be used
< jgarzik> it's just a dumb type check with a nutty assignment style
< jgarzik> i.e. the purpose is simply to throw an exception if the type check fails
< cfields> erm, that still doesn't look right
< cfields> see the first exception under "Lifetime of a temporary"
< Luke-Jr> cfields: but it's not a temporary, it's scoped
< Luke-Jr> reply and valReply are literally the same thing
< Luke-Jr> lovely, ubuntu-12.04.5-server-amd64.iso doesn't work in KVM
< Luke-Jr> (and probably not at all)
< cfields> mm, ok
< Luke-Jr> put bitcoin-cli through valgrind for that rpc-test and it sees no issues
< jonasschnelli> Luke-Jr: re: removing univalue subtree:
< jonasschnelli> I guess UniValue is not available over pkg managers, so people need to install the dependency by checking out the git repo and compile?
< GitHub176> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/c079d79c9a9c...2be4bb51f37c
< GitHub176> bitcoin/master 3503a78 Cory Fields: release: remove libc6 dependency from the osx signing descriptor...
< GitHub176> bitcoin/master 2be4bb5 Wladimir J. van der Laan: Merge pull request #7342...
< GitHub199> [bitcoin] laanwj closed pull request #7342: release: remove libc6 dependency from the osx signing descriptor (master...gitian-quickfix) https://github.com/bitcoin/bitcoin/pull/7342
< GitHub96> [bitcoin] laanwj pushed 1 new commit to 0.12: https://github.com/bitcoin/bitcoin/commit/fbea2f64e23487636f2e490b6b0067c41545cd39
< GitHub96> bitcoin/0.12 fbea2f6 Cory Fields: release: remove libc6 dependency from the osx signing descriptor...
< GitHub64> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/2be4bb51f37c...e1060c56cca7
< GitHub64> bitcoin/master fa989fb MarcoFalke: [qt] coincontrol workaround is still needed in qt5.4 (fixed in qt5.5)
< GitHub64> bitcoin/master e1060c5 Wladimir J. van der Laan: Merge pull request #7334...
< GitHub186> [bitcoin] laanwj closed pull request #7334: [qt] coincontrol workaround is still needed in qt5.4 (fixed in qt5.5) (master...Mf1601-qt55Workaround) https://github.com/bitcoin/bitcoin/pull/7334
< sdaftuar> i was just looking over the release notes for 0.12 -- am i right in thinking we should highlight chainstate obfuscation and discuss downgrade issues that coudl result?
< sdaftuar> (i'm not that familiar with the topic but vaguely recall people talking about it before)
< Luke-Jr> jonasschnelli: or depends/ would presumably also work fine
< Luke-Jr> sdaftuar: agree
< jonasschnelli> Luke-Jr: you mean for univalue?
< Luke-Jr> jonasschnelli: yes, or any other dependency
< jonasschnelli> Yes. But it would be the first dependency that would require self-compiling.
< Luke-Jr> only until package managers pick it up *shrug*
< Luke-Jr> and only in the case of the user compiling in the first place :p
< Luke-Jr> cfields: test_bitcoin: key.cpp:305: void ECC_Start(): Assertion `secp256k1_context_sign == __null' failed.
< Luke-Jr> O.o
< jgarzik> Luke-Jr, Bloq (my newco) will be submitting Fedora and Debian packages upstream for univalue
< jgarzik> that's a long process though, maybe a year
< jgarzik> before it trickles down to a stable release
< Luke-Jr> jgarzik: cool
< jgarzik> It's not consensus critical so it makes sense as a package rather than subtree in the long term, IMO
< Luke-Jr> yeah, and we have a package manager bundled anyway
< Luke-Jr> in the meantime, here's a full stack trace with line numbers for the free bug http://codepad.org/bKXLXqia
< jgarzik> Luke-Jr, this is for a tree that is up to date with jgarzik/univalue master?
< Luke-Jr> no, this is 1.0.1
< jgarzik> Luke-Jr, any chance with http://codepad.org/u89EfIeE ?
< jgarzik> *change
< Luke-Jr> I don't have an easy way to test changes in univalue right now, so it will be a bit to answer
< jonasschnelli> I'm fine with keeping it at jgarzik/univalue ... I think we have moved it because we wanted to get a PR merged? not sure although.
< jonasschnelli> jgarzik: nice that you try to get upstream packages!
< jgarzik> jonasschnelli, basic open source upstreaming policy :)
< Luke-Jr> #10 0xf7fca7ca in UniValue::setNumStr (this=0xffffd348, val_=…) at lib/univalue.cpp:118
< Luke-Jr> 118 val = val_;
< Luke-Jr> prior to this, val is:
< Luke-Jr> (gdb) print val
< Luke-Jr> $4 = {static npos = <optimized out>, _M_dataplus = {<std::allocator<char>> = {<__gnu_cxx::new_allocator<char>> = {<No data fields>}, <No data fields>},
< Luke-Jr> _M_p = 0x567d93d4 ""}}
< jonasschnelli> ^^
< Luke-Jr> in this case the error is:
< Luke-Jr> ⁂ glibc detected ⁂ /home/ubuntu/bitcoin/src/bitcoin-cli: free(): invalid pointer: 0x567d93c8 ⁂
< cfields> Luke-Jr: i see that pretty often, iirc it happens if something goes wrong during startup
< cfields> Luke-Jr: mind doing a quick check on #7322 ?
< Luke-Jr> testing
< cfields> thanks
< cfields> <sdaftuar> i was just looking over the release notes for 0.12 -- am i right in thinking we should highlight chainstate obfuscation and discuss downgrade issues that coudl result?
< cfields> ^^ yes
< cfields> i accidentally ran an obfuscated chainstate on an older version, didn't go well
< Luke-Jr> it didn't just immediately fail? :x
< cfields> Luke-Jr: of course, but to a casual user the "why" wouldn't be obvious
< Luke-Jr> ah
< cfields> Luke-Jr: aha, i didn't notice the beginning of that stacktrace. facepalm.
< Luke-Jr> ?
< cfields> Luke-Jr: it's gone off the rails already in main()
< cfields> Luke-Jr: $1 says if you build univalue static, travis passes
< Luke-Jr> cfields: that doesn't fix the problem :p
< cfields> Luke-Jr: sure, just trying to narrow it down
< * Luke-Jr> stabs Ubuntu for valgrind not working
< cfields> Luke-Jr: btw, you can test on travis much quicker if you delete the other builders in your .travis file
< cfields> i've resorted to that once or twice
< Luke-Jr> I have it reproduced in a gitian VM :P
< cfields> oh, great
< Luke-Jr> making univalue static in bitcoin-cli results in bitcoind crashing instead
< Luke-Jr> so you're probably right
< cfields> init order issue?
< cfields> Luke-Jr: maybe there's an issue with cmdline args being parsed via univalue at startup? do you know what the passed-in args are?
< Luke-Jr> it doesn't seem to matter
< cfields> any args cause a crash?
< Luke-Jr> no, it needs at least rpcpassword and a method
< Luke-Jr> cfields: http://paste.ubuntu.com/14497754/ fixes it. :|
< cfields> wha?
< cfields> so something's assuming that it has a string value without checking it, i suppose?
< Luke-Jr> I was given this idea from https://gcc.gnu.org/bugzilla/show_bug.cgi?id=31368
< Luke-Jr> using : val() is a crash again
< GitHub11> [bitcoin] MarcoFalke opened pull request #7345: [0.12] Add backwards-compatibility issues (chainstate obfuscation) to release notes (0.12...Mf1601-docObfuscation) https://github.com/bitcoin/bitcoin/pull/7345
< Luke-Jr> cfields: : val("") also crashes
< cfields> Luke-Jr: can you build with -O0 and do a 'bt full' in gdb?
< cfields> i'm really curious now :\
< Luke-Jr> cfields: moving the constructor definition to the lib rather than inline, also seems to work
< cfields> Luke-Jr: yes, i was just about to suggest that
< cfields> Luke-Jr: i'm afraid it has to do with compile differences or locales
< Luke-Jr> ?
< Luke-Jr> crash remains with --disable-glibc-back-compat FWIW
< cfields> oh wait
< Luke-Jr> ?
< cfields> i'm re-reading the _GLIBCXX_DEBUG docs, trying to see if there are issues passing objects in/out of shared libs
< cfields> Luke-Jr: can you verify that bitcoin-cli.cpp is built with those flags?
< Luke-Jr> which flags?
< btcdrak> meeting in #bitcoin-dev now
< MarcoFalke> :( Looks like travis doesn't like the nightly builds. Can someone after the meeting check https://blog.travis-ci.com/2014-05-12-why-is-my-build-not-running/
< MarcoFalke> nightly is running for 2:20 h. (2 h is the max, iirc)
< GitHub142> [bitcoin] luke-jr opened pull request #7346: 0.12 release notes: Mining Policy Changes (0.12...0.12-release-notes-mining) https://github.com/bitcoin/bitcoin/pull/7346
< Luke-Jr> cfields: what flags?
< GitHub181> [bitcoin] morcos opened pull request #7347: Release notes about mining code changes (0.12...rnfixes2) https://github.com/bitcoin/bitcoin/pull/7347
< Luke-Jr> morcos: what's this nonsense about removal of priority code?
< Luke-Jr> I guess it predates your PR
< * Luke-Jr> fixes in #7346
< Luke-Jr> morcos: in any case, please rebase 7347 on top of that
< morcos> Luke-Jr: I did not add that. I reworded what already was in the release notes that were merged. I think if you read my changes, you'll agree they did not change meaning.
< Luke-Jr> morcos: yes, noticed that [20:23:38] <Luke-Jr> I guess it predates your PR
< morcos> oh, sorry, skipped over that
< Luke-Jr> sorry for having jumped to conclusions at first
< morcos> Well as you know I think that is what we should do, but I wouldn't have suggested to put it in there without clearly pointing it out, as I know there is not complete consensus on it
< Luke-Jr> sigh.. reddit.. downvoted for telling people not to use a literally random yum repository for Bitcoin Core ..
< paveljanik> and the answer was do not trust the source, read it first? ;-)
< PRab> I'm getting "cannot stat ‘base-trusty-amd64’: No such file or directory" when trying to run gbuild. Any ideas?
< Luke-Jr> cfields: http://paste.ubuntu.com/14499832/ also fixes the problem
< Luke-Jr> PRab: you need to build a trusty VM base
< cfields> Luke-Jr: so it's being instantiated two different ways?
< Luke-Jr> cfields: no, it seems to be a libstdc++ bug https://gcc.gnu.org/ml/libstdc++/2006-01/msg00162.html
< cfields> Luke-Jr: we don't use a custom string type, though?
< Luke-Jr> cfields: it doesn't seem to be restricted to only custom ones :/
< Luke-Jr> at least in this old version
< Luke-Jr> cfields: maybe bitcoin-cli is so simple that it doesn't instantise std::string sufficiently on its own
< cfields> Luke-Jr: i still suspect that it has to do with the STDCXX debug option on a shared lib, when compiled differently
< PRab> Luke-Jr: Thanks. I didn't realize I would need to redo any of that. In the past I was able to get away with just following the release-process instructions. I'm guessing it used an older version of ubuntu before.
< Luke-Jr> I don't understand what that means. I am compiling them the same afaik
< Luke-Jr> PRab: yes
< Luke-Jr> jgarzik: so it seems this issue is probably unrelated to univalue, so 1.0.2 tag seems good to go IMO?
< cfields> Luke-Jr: apparently libstdc++ uses a single address to notate that a string is "empty", and avoids freeing it in that case. If the addresses end up being different, an empty string allocated on the bitcoin side would attempt to be freed by the lib
< cfields> which would also explain why avoiding the assignment in the header would "fix" the issue
< Luke-Jr> cfields: hmm
< Luke-Jr> how is that single address determined? :x
< cfields> Luke-Jr: you use -fPIC and our hardening opts to build the lib?
< cfields> (and visibility)
< Luke-Jr> probably not.. those shouldn't affect ABI though :|
< cfields> PIC certainly does
< cfields> though visibility would get ugly, since univalue doesn't handle it
< Luke-Jr> well, libs are going to be PIC
< JackH> hi now that we are in bitcoin-core-dev channel, can I suggest something to the core dev's openly
< Luke-Jr> that last pastebin doesn't make sense with your theory, does it?
< Luke-Jr> jgarzik: actually, maybe the std::string workaround ought to be in univalue.h?
< cfields> Luke-Jr: i must admit that one makes no sense to me
< cfields> unless it's just avoiding them being defined differently
< cfields> Luke-Jr: i'm going to see if i can reproduce locally
< Luke-Jr> cfields: want me to just open my VM to you over the net?
< cfields> oh, sure. that'd be even easier :)
< cfields> Luke-Jr: not doubting the fix, just trying to understand what's going on
< Luke-Jr> cfields: I'm currently testing the idea of putting that line in the univalue.h fwi
< Luke-Jr> fwiw
< cfields> Luke-Jr: won't that cause nasty link problems?
< Luke-Jr> I guess we'll find out <.<
< Luke-Jr> might make the compile much slower though? :/
< Luke-Jr> perhaps moving the constructors to the lib is the best option
< Luke-Jr> certainly feels like this build is slower..
< Luke-Jr> cfields: ok, back to vanilla univalue; all yours
< Luke-Jr> in case you missed it, the rpc test passed with the template hack
< cfields> hmm, ok
< Luke-Jr> (Ctrl-A Esc to use pgup/pgdown keys to scroll in screen)
< Luke-Jr> (note it freezes the screen until you hit Esc again)