< 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/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
< 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."
< 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?
< 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
< * 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")
< 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
< 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
< 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 #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