< midnightmagic> In src/core_write.cpp:ScriptToAsmStr() there is a check for an integer printout with 0 <= vch.size() <= 4 -- otherwise it prints out a hex string. But for decodescript 6900, (decoding to OP_VERIFY 0) the vch.size() I believe for the second byte is 0, rather than an actual 0x00, and so the decode works apparently by accident..? Am I missing something?
< bitcoin-git> [bitcoin] sipa pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/3908fc472805...e126d0c12ca6
< bitcoin-git> bitcoin/master 997a98a Gregory Maxwell: Replace FindLatestBefore used by importmuti with FindEarliestAtLeast....
< bitcoin-git> bitcoin/master 4b06e41 Suhas Daftuar: Add unit test for FindEarliestAtLeast
< bitcoin-git> bitcoin/master e126d0c Pieter Wuille: Merge #9490: Replace FindLatestBefore used by importmuti with FindEarliestAtLeast....
< bitcoin-git> [bitcoin] sipa closed pull request #9490: Replace FindLatestBefore used by importmuti with FindEarliestAtLeast. (master...fix_find_latest_before) https://github.com/bitcoin/bitcoin/pull/9490
< cfields> sipa: good idea. I'm not exactly sure how to execute it though. I'd rather not trip up someone's manual efforts to enable sanitizers. Maybe add a --enable-sanitizers config option, off by default, that tries to turn on as many non-conflicting ones as possible?
< cfields> BlueMatt: woohoo! congrats on merge.
< sipa> cfields: we may need 2 or 3 separate builds with sanitizers, unfortunately
< sipa> cfields: but yes, optional sounds right
< cfields> sipa: right. Maybe there's no realistic way to add to configure, then? We can just handle with cflags for travis/gitian :(
< sipa> you wouldn't want to enable sanitizers on production binaries, obviously
< cfields> right, it'd build both sets
< cfields> I suppose just doing it for travis would be a start
< cfields> we already have a slow, minimal build with debug cranked up for libstdc++.
< sipa> the sets you can do are 1) -fsanitize=address -fsanitize=undefined -fno-omit-frame-pointer 2) -fsanitize=thread 3) -fsanitize=memory (clang only, and requires rebuild of all dependencies)
< sipa> i've only played around with the first for now
< cfields> is no-omit-frame-pointer needed for correctness? or just helpful backtraces?
< sipa> i am not sure
< cfields> that one might be tricky since it's enabled at -O2, iirc. Not sure what "-fno-omit-frame-pointer -O2" turns into
< cfields> ok
< sipa> i think individual -f things override the -O defaults
< cfields> i would hope so. My fear is that it's order-based, like warnings.
< sipa> ah
< sipa> seems likely
< cfields> if so, that'd be tricky, since -Ox sneak in all over the place.
< cfields> will mess around with 'em
< BlueMatt> cfields: yea, feels good to get a few things in for 0.14....do feel bad that I havent done enough review this week and am holding up things like bumpfee :'(
< BlueMatt> will at least finish this review before going out :/
< * BlueMatt> hopes ryanofsky has time to spare this weekend
< cfields> heh. it'd be a rookie move to make plans the weekend before freeze/release :)
< BlueMatt> well he is a rookie, after all :p
< sipa> like 'participating in the mit mystery hunt'
< BlueMatt> (at least in bitcoin)
< sipa> or 'agreeing to review workshop papers with deadline the 15th' ?
< BlueMatt> heh, yea, what were you and gmaxwell thinking, sipa????
< BlueMatt> :P
< cfields> sipa: ahah, that's this weekend?
< sipa> i'll be so happy when i get back to california on tuesday
< sipa> all this vacation thing is too stressful
< cfields> heh
< cfields> jeremyrubin: are you still pushing for your checkqueue rewrite? I'm rebasing the interruptible threads, and just remembered how terribly inefficient that is. There are some easy gains for sure. But I won't mess with it if it's being nuked anyway.
< gmaxwell> cfields: we should be careful with '--enable' because there are users that run ./configure and then turn on every enable.
< luke-jr> sipa: note that -fsanitize=memory also has weird LLVM/kernel matching issues, and is basically impossible on Travis currently (I used to use it)
< cfields> gmaxwell: well --enable-debug would already be hurting them pretty badly :)
< sipa> luke-jr: ah, good to know
< gmaxwell> cfields: yea agreed we should probably fix that -- though at least that one says 'debug'.
< luke-jr> sipa: cfields: also, we absolutely should NOT use -fsanitize=x with prod binaries. they are not intended for that, and add exploits
< sipa> luke-jr: absolutely!
< bitcoin-git> [bitcoin] practicalswift opened pull request #9549: [net] Avoid possibility of NULL pointer dereference in MarkBlockAsInFlight(...) (master...avoid-potential-null-pointer-dereference-in-markblockasinflight) https://github.com/bitcoin/bitcoin/pull/9549
< gmaxwell> maybe making them -dev-debug -dev-sanatize-foo etc would be sufficient.
< gmaxwell> can someone tell him that if he thinks he actually has a possible null reref in code like that he should send it privately?
< gmaxwell> oh the description is okay
< gmaxwell> (ignore me)
< sipa> ideally i think those sanitize things should build separate binaries (bitcoind_san_X, test_bitcoin_san_X, ...)
< sipa> if that's at all possible
< sipa> maybe it's easier with a wrapper script around configure/make rather than inside the system
< luke-jr> is there a reason to special-case it at all? what's wrong with just passing CXXFLAGS?
< cfields> luke-jr: read the discussion above :)
< sipa> ideally, make check etc... build the multiple configurations and run all tests on all
< gmaxwell> As another side, we use the oneliners in our release notes but often our choice of them is really poor for this purpose. Case in point ^ sounds like a serious crasher or even exploitable.
< luke-jr> cfields: I did skim it, but didn't see a rationale
< sipa> luke-jr: if we do it as a wrapper around (and just re-run configure and test for each of the sanitizers), it will just be passing CXXFLAGS/LDFLAGS
< cfields> luke-jr: i was kinda thinking same as you. Not sure it makes sense to handle in the build-system. as sipa said, maybe a wrapper makes more sense
< sipa> luke-jr: but it would be nice if it was completely integrated, so that the multiple tests/builds can run in parallel just from make, for exampe
< luke-jr> oh, for make check basically?
< sipa> right
< sipa> but make check should also run pull-tester/rpc-tests.py then
< sipa> i guess we can just start by doing it as a wrapper over the build system
< luke-jr> making make check run rpc tests shouldn't be hard, unlike sanitizers :P
< cfields> sipa: hmm, how do they interact with lto? it'd be nice if some of them could be enabled at link-time
< sipa> cfields: they play well with lto, but you need to pass them both at compile and at link time, i think
< midnightmagic> Is a verbose transaction decode available through the rpc block decode interfaces..?
< sipa> decoderawtransaction?
< midnightmagic> sipa: The function interface has a txDetails bool which defaults to false but .. I don't know where it can be called with true through the rpc. Only the REST interface I think..?
< midnightmagic> blockToJSON() I mean.
< achow101> midnightmagic: you mean with something like getblock?
< midnightmagic> Yeah.
< achow101> no. but I have an open pr for that
< midnightmagic> ah, nice. \o thanks. I'll look for your PR then instead of doing my own.
< bitcoin-git> [bitcoin] gmaxwell opened pull request #9550: Trim down the XP notice and say more about what we support. (master...we_got_it_already) https://github.com/bitcoin/bitcoin/pull/9550
< achow101> midnightmagic: #8704
< gribble> https://github.com/bitcoin/bitcoin/issues/8704 | [RPC] Transaction details in getblock by achow101 · Pull Request #8704 · bitcoin/bitcoin · GitHub
< midnightmagic> excellent.
< sipa> </mrburns>
< gmaxwell> luke-jr: I really wish make check ran the rpc tests though the rpc tests have more dependencies than the oridnary builds.
< gmaxwell> But I think we need to, because increasingly the useful tests are in rpctests.
< gmaxwell> I think current make check has a poor ratio of excution time to sensitivity.
< BlueMatt> morcos: getbalance("*", 1, True) is defined as a synonym of getbalance(), but bumpfee breaks that
< BlueMatt> (note that I think bumpfee also breaks some account balance shit, but I didnt bother to audit that - do we care?)
< BlueMatt> morcos / sdaftuar / ryanofsky: please add the following test-case https://0bin.net/paste/m91nHUQbD39EXaQ6#9fJ3bvAfAib5xWWwVdh3Ck0v7jQht1r8iDMQKQDpfTC
< gribble> https://github.com/bitcoin/bitcoin/issues/9 | Fix for GUI on Macs and latest wxWidgets by gavinandresen · Pull Request #9 · bitcoin/bitcoin · GitHub
< BlueMatt> no gribble
< BlueMatt> (it currently fails on the last line of the test)
< BlueMatt> note that if you move that test to be the last one run you'll get fun results - getbalance("*", 1, True) is negative!
< instagibbs> BlueMatt, https://github.com/bitcoin/bitcoin/issues/8183 Perhaps still the case
< instagibbs> they haven't been the same in a number of cases already :(
< BlueMatt> hum, ok, that seems shit
< BlueMatt> can we tag #8183 for 0.14, then, that is insane
< gribble> https://github.com/bitcoin/bitcoin/issues/8183 | getbalance comment incorrect · Issue #8183 · bitcoin/bitcoin · GitHub
< morcos> BlueMatt: I had assumed there might be an issue like that after you brought up your GUI question, and it was on my list to look into...
< BlueMatt> we either need to fix it or fix the docs in getbalance
< BlueMatt> they significantly imply that if you specify "*" as account you'll get sane results
< morcos> i guess in my head i sometimes confuse getbalance "*" with something that has to do with accounts
< morcos> but quite honestly getbalance "*" is a shit show
< morcos> why do we even need it
< instagibbs> yeah it's really bad
< instagibbs> :)
< instagibbs> i mean :(
< morcos> i would suggest it should be deprecated with accounts
< BlueMatt> getbalance "*" should throw an exception as "FUCK YOU, WE DEPRECATED THIS FOREVER AGO"
< BlueMatt> it is marked deprecated the way i read the docs
< BlueMatt> the fact that it returns insanity is gonna confuse someone, and it has been deprecated, so it should throw an exception as its known-unsafe and we likely wont fix it
< morcos> From #9167 : "Note that the fee reported in the details and any other function which depends on ListTransactions is not changed as getbalance("*") depends on having incorrect negative fees calculated on mixed debit transactions in order to track the right balances."
< gribble> https://github.com/bitcoin/bitcoin/issues/9167 | IsAllFromMe by morcos · Pull Request #9167 · bitcoin/bitcoin · GitHub
< BlueMatt> I mean it may be deprecated, but that doesnt mean it gets to return insanity
< BlueMatt> if its gonna return insanity, it needs to die
< morcos> anyway got to go
< BlueMatt> when you have to google C++ operator precedence in the process of review, something has gone horribly wrong :'(
< gmaxwell> BlueMatt: time to study operator precidence more! :)
< BlueMatt> clearly
< fanquake> cfields Made the required changes in 9469, and pulled in your commits. Any chance while your looking at Qt stuff, you can take a look at #9126. Have you seen that issue at all?
< gribble> https://github.com/bitcoin/bitcoin/issues/9126 | An error has occurred and has been logged. Please contact this bot's administrator for more information.
< BlueMatt> I think #9484 is ready for merge
< gribble> https://github.com/bitcoin/bitcoin/issues/9484 | Introduce assumevalid setting to skip validation presumed valid scripts. by gmaxwell · Pull Request #9484 · bitcoin/bitcoin · GitHub
< BlueMatt> (just thought I'd mention it so it doesnt slip 0.14)
< BlueMatt> gmaxwell: you were saying you'd be super dissapointed if some of the net fixes didnt make 0.14...well there is one bunch left from your pr that is at risk - #9535
< gribble> https://github.com/bitcoin/bitcoin/issues/9535 | Split CNode::cs_vSend: message processing and message sending by TheBlueMatt · Pull Request #9535 · bitcoin/bitcoin · GitHub
< * BlueMatt> does solomly swear all helgrind issues will be thoroughly debugged prior to release :p
< BlueMatt> 9535 wont add any, but I know of a few still left
< * sipa> tests 9535 with -fsanitize=thread
< BlueMatt> sipa: thar be dragons
< BlueMatt> (not cause by 9535)
< BlueMatt> d
< sipa> yes, i expect it to fail (on master)
< BlueMatt> can I buy a "d"?
< BlueMatt> I can give you a list of places it will fail :p
< BlueMatt> alright folks, I'm all reviewed-out...and likely wont have time tomorrow. apologize for those I didnt get to, but will try again on sunday
< sipa> thanks!
< sipa> ok, bitcoin-cli fails with -fsanitize=thread... i think there is some build config failure
< bitcoin-git> [bitcoin] 2HCHO opened pull request #9551: disable out of sync warnings for regtest network (master...ca4b90519c3c210f) https://github.com/bitcoin/bitcoin/pull/9551
< cfields> sipa: yes, i'd expect that to be a bloodbath if it works. But there should be no new issues, i think
< cfields> sipa: specifically, anything that uses CNodeStats is very racy.
< sipa> cfields: i think it inteferes with some fortify/aslr/... we're using
< sipa> bitcoin-cli has no threads
< cfields> sipa: oh, yes, for sure
< cfields> i read somewhere that FORTIFY is incompatible with some, sec for link
< sipa> in fact, the compiler detector fails with -fsanitize=thread ("C++ compiler cannot produce binaries")
< cfields> fanquake: will have a look at 9126
< cfields> (at some point this weekend)
< fanquake> cfields cheers. Interesting build error on just the osx build of 9469. "Qt requires a C++11 compiler and yours does not seem to be that."
< fanquake> Thought I might have messed up passing -c++std c++11. But that looks ok. https://travis-ci.org/bitcoin/bitcoin/jobs/191837557#L895
< sipa> -std=c++11 ?
< cfields> fanquake: hmm, strange. it built fine for me locally
< fanquake> cfields yes me too.
< cfields> fanquake: ah, it's the damn .mm again
< cfields> fanquake: i noticed something at one point and suspected that might happen
< cfields> trying to remember
< cfields> ah, right
< cfields> mind nuking that line and giving it a try?
< fanquake> cfields Have pushed that change up to GH.
< fanquake> Will start a new build here also.
< cfields> thanks
< fanquake> Just realised I forgot you had two commits on top of mine, so that change is now part of your translations build commit.
< cfields> fanquake: for the reasoning, see https://github.com/bitcoin/bitcoin/blob/master/configure.ac#L62
< fanquake> Can fixup later if the builds passed. Otherwise np.
< cfields> fanquake: np. Though if this fixes, we probably want to commit it as a one-liner. It's a good backport candidate.
< fanquake> cfields good point. Will split it out post builds passing.
< cfields> fanquake: only reason i mention is that i had a theory at one point that it was the cause of the 10.7 back-compat breakage. Since it's responsible for intoducing c++03 abi objects into the 0.13 build.
< fanquake> cfields ok. Just made note of it in a comment in that PR.
< cfields> fanquake: thanks :)
< fanquake> cfields I'll stop pestering now, you can go and enjoy the rest of your saturday heh
< cfields> fanquake: heh, np. i meant to test that a long time ago, glad it came up either way
< cfields> besides, this is what i enjoy :)
< cfields> woohoo, success
< fanquake> Awesome. I'll fixup that commit then.
< cfields> great, thanks again!
< bitcoin-git> [bitcoin] jamesmacwhite opened pull request #9552: Add IPv6 support to qos.sh (master...qos-ipv6) https://github.com/bitcoin/bitcoin/pull/9552
< jerryco> hello
< jerryco> how to install bitcoin-core centos ?
< bitcoin-git> [bitcoin] practicalswift opened pull request #9553: Use z = std::max(x - y, 0) instead of z = x - y; if (z < 0) z = 0; (master...std-max) https://github.com/bitcoin/bitcoin/pull/9553
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/e126d0c12ca6...8daf103fa138
< bitcoin-git> bitcoin/master 988d300 practicalswift: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64)
< bitcoin-git> bitcoin/master 8daf103 MarcoFalke: Merge #9528: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64)...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #9528: [qt] Rename formateNiceTimeOffset(qint64) to formatNiceTimeOffset(qint64) (master...rename-formateNiceTimeOffset) https://github.com/bitcoin/bitcoin/pull/9528
< bitcoin-git> [bitcoin] MarcoFalke pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/8daf103fa138...23281a4dc3af
< bitcoin-git> bitcoin/master b348287 Alex Morcos: Clarify that prioritisetransaction remains supported
< bitcoin-git> bitcoin/master 34ede12 Alex Morcos: Document fee estimation changes
< bitcoin-git> bitcoin/master 23281a4 MarcoFalke: Merge #9531: Release notes for estimation changes...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #9531: Release notes for estimation changes (master...relnotes) https://github.com/bitcoin/bitcoin/pull/9531
< sipa> cfields: -fsanitize=thread "runs" with LDFLAGS="-no-pie"
< luke-jr> sipa: only with newer compilers.. neither Gentoo-stable compiler supports sanitize=thread without PIE
< luke-jr> and with PIE requires either a newer LLVM or older kernel
< luke-jr> (neither of which are available on Travis)
< luke-jr> the LLVM fix backports only so far as 3.8 FWIW (porting to 3.7 seemed difficult, at least)
< sipa> luke-jr: using gcc 6.2
< luke-jr> ok, just pointing out that isn't a reasonable min requirement ;)
< * luke-jr> peers at Gentoo have 6.3 masked, but no other 6.x at all
< bitcoin-git> [bitcoin] practicalswift opened pull request #9554: [test] Avoid potential NULL pointer dereference in addrman_tests.cpp (master...avoid-null-pointer-dereference-in-addrman_tests) https://github.com/bitcoin/bitcoin/pull/9554
< bitcoin-git> [bitcoin] practicalswift opened pull request #9555: [test] Avoid triggering undefined behaviour in tx_invalid-test (transaction_tests.cpp) (master...avoid-ub-in-tx_invalid-test) https://github.com/bitcoin/bitcoin/pull/9555
< Netmage> Hi, I use bitcoint-qt. The size of the block chain is overall 113 GB large. Is this a normal value for the current block chain ?
< btcdrak> Netmage: sadly yes.
< Netmage> ok, is there a good way to decrease the size ?
< Netmage> Without having problems afterwards
< BlueMatt> yes, luke-jr, Netcraft confirmed: gentoo is dead
< BlueMatt> Netmage: look at the docs for the -prune option
< luke-jr> Netmage: depends on if you need to restore old wallet backups etc
< BlueMatt> luke-jr: ok, sooooo utack for #9499?
< gribble> https://github.com/bitcoin/bitcoin/issues/9499 | Use recent-rejects, orphans, and recently-replaced txn for compact-block-reconstruction by TheBlueMatt · Pull Request #9499 · bitcoin/bitcoin · GitHub
< luke-jr> BlueMatt: I did not complete review of all the code; I'm not sure it's worth doing as-is, but I don't object to it.
< BlueMatt> huh? folks have reported on the order of 10% fewer rtts!
< BlueMatt> how is that not worth doing?
< BlueMatt> (or more)
< luke-jr> it just seemed like a lot of complexity for what appeared to be random caching; maybe I'm wrong
< luke-jr> maybe in practice it happens to work out to be very effective
< BlueMatt> most of that pr's "complexity" is copying one block of code.....
< luke-jr> I did review that block
< BlueMatt> but, yes, three independant benchmarks posted on the pr suggested its a huge win
< luke-jr> ok, maybe it is! I'm not objecting
< luke-jr> if my review matters for the merge, I can go back and finish it
< BlueMatt> all reviews matter
< BlueMatt> :)
< luke-jr> maybe I should be doing this one commit-by-commit
< BlueMatt> yes
< BlueMatt> that tends to make most reviews easier :p
< luke-jr> I'd be a lot more comfortable that there isn't an implicit mask in rebasing code, if plTxnReplaced were at the end of the function signature
< luke-jr> s/mask/cast/
< BlueMatt> I believe gcc would complain in any way you fuck that up?
< luke-jr> checking..
< BlueMatt> it should complain if you try to pass a bool into that pointer?
< BlueMatt> i suppose unless, for some reason, you want to fOverrideMempoolLimit based on whether not a std::vector<CTransactionRef>* is non-null
< BlueMatt> but......
< luke-jr> yeah, probably safe
< luke-jr> BlueMatt: c73554042886fb63fb48edf29cf827951edde341 needs a #include "netprocessing.h" in init.cpp I think
< BlueMatt> init has an include net_processing?
< luke-jr> right, nm
< BlueMatt> cfields: is there a reason CConnman::ForNode calls the provided function with cs_vNodes still locked? that seems bad?
< bitcoin-git> [bitcoin] practicalswift opened pull request #9556: Remove redundant semicolons (master...remove-redundant-braces) https://github.com/bitcoin/bitcoin/pull/9556
< Chris_Stewart_5> If anyone is interested in #8469 and wants to get some internet points on SO: http://stackoverflow.com/questions/41539334/including-header-file-in-cpp-causes-nasty-error
< gribble> https://github.com/bitcoin/bitcoin/issues/8469 | [POC] Introducing property based testing to Core by Christewart · Pull Request #8469 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] practicalswift opened pull request #9557: Use static_cast<new_type>(...) instead of deprecated new_type(...) (functional cast) (master...avoid-functional-cast-expression) https://github.com/bitcoin/bitcoin/pull/9557
< bitcoin-git> [bitcoin] TheBlueMatt opened pull request #9558: Clarify assumptions made about when BlockCheck is called (master...2017-01-blockcheckeddocs) https://github.com/bitcoin/bitcoin/pull/9558
< bitcoin-git> [bitcoin] 2HCHO closed pull request #9551: disable out of sync warnings for regtest network (master...ca4b90519c3c210f) https://github.com/bitcoin/bitcoin/pull/9551
< bitcoin-git> [bitcoin] practicalswift opened pull request #9559: [net] Avoid possibility of NULL pointer dereference in ProcessMessage(...) (master...avoid-null-pointer-deref-in-processmessage) https://github.com/bitcoin/bitcoin/pull/9559
< bitcoin-git> [bitcoin] practicalswift opened pull request #9560: [rpc] Avoid possibility of NULL pointer dereference in getblocktemplate(...) (master...avoid-null-pointer-dereference-in-rpc-blockchain) https://github.com/bitcoin/bitcoin/pull/9560
< BlueMatt> man practicalswift needs to calm down a bit with this....bad timing, yo
< sipa> release's on fire, yo
< BlueMatt> yea, as in a bunch of stuff is cruisin' to miss :(
< BlueMatt> sipa: how's mystery hung going?
< BlueMatt> t
< sipa> BlueMatt: first team finished at 4am this morning, shortest hunt ever
< sipa> BlueMatt: our team finished early afternoon today
< BlueMatt> oh shit, wow
< bitcoin-git> [bitcoin] TheBlueMatt opened pull request #9561: Wake message handling thread when we receive a new block (master...2017-01-wakeup-on-new-block) https://github.com/bitcoin/bitcoin/pull/9561
< BlueMatt> ^ one-line change provides 100ms improvement in block-relay latency for non-compact-block peers :p