< gmaxwell>
the pr explains, they were getting relayed.
< gmaxwell>
(or so says the pr)
< BlueMatt>
hum, that seems strange to me?
< BlueMatt>
i mean that was a long time ago, though, i guess before CCoins
< BlueMatt>
CCoinsViewCache stuff should have fixed this?
< sipa>
yes
< sipa>
i believe the ConnectInputs code at the time may not have been able to catch this
< BlueMatt>
yes, this sounds correct to me
< sipa>
it had an fBlock which was false for mempool txn, and it disabled part of the checks
< gmaxwell>
removal trumps optimizing, though if you want to optimize there, I think I found a 1ms-scale speedup by fusing varrious loops over every transaction in the block into a single loop.
< BlueMatt>
you mean in CheckBlock?
< BlueMatt>
theres only two over-tx loops
< BlueMatt>
no, 3
< gmaxwell>
there are three, coinbase check, checktransaction and sigops check.
< BlueMatt>
yea, should merge that, though the sigops loop is ~free
< BlueMatt>
well, its well under 1ms
< BlueMatt>
well, well under
< gmaxwell>
IIRC I benchmarked it an it was about a 1ms change.
< * BlueMatt>
-> dinner
< BlueMatt>
hum, that would surprise me (or your system was slower than mine :p)
< gmaxwell>
well slower probably, -- probably benchmarked it on my laptop.
< gmaxwell>
BlueMatt: or I flubbed the meaurement.
< gmaxwell>
But I think the opterations can basically be fused into CheckTransaction... e.g. takes a flag that indicates if its allowed to be a coinbase, and returns the sigops count.
< gmaxwell>
BlueMatt: in 9045 what initilizes data_hash?
< BlueMatt>
gmaxwell: default-initialized to null
< BlueMatt>
its a class
< BlueMatt>
gmaxwell: anyway, i'll look at optimizing it a bit tomorrow...see if i can write a bench_bitcoin thinggy for it
< BlueMatt>
CheckBlock is not-insignificant in terms of time from wire to udp broadcast (which is right before disk write in AcceptBlock - same place compact block announcement will end up)
< BlueMatt>
ofc actual block deserialization is really the largest time sink
< sipa>
gmaxwell: ints are default not initialized
< gmaxwell>
oh, matt was saying the uint256 constructor is initing it. I thought he was saying because CNetMessage is a class all its members are initilized.
< sipa>
yes, it is
< sipa>
CNetMessage is a class, so all its members have their default constructor called
< gmaxwell>
Yes, I'd just missed that the hash was a uint256. (or rather that a uint256 behaves differently from a primitive type. :) )
< sipa>
we could in theory not initialize the member chars in a uint5
< BlueMatt>
sipa: I prefer adding an ifdef than having a segfault (also that was already merged in another pr)
< GitHub117>
[bitcoin] sdaftuar opened pull request #9048: [0.13 backport] Fix handling of invalid compact blocks (0.13...fix-invalid-cb-ban-0.13) https://github.com/bitcoin/bitcoin/pull/9048
< sipa>
BlueMatt: 8977 it seems
< BlueMatt>
gmaxwell: so i went and combined everything into one loop of txn in a block (and even combined the sigops count loops pushed into checktransaction, etc...also removed the duplicative GetSerializeSize check in CheckTransaction (which is duplicative of CheckBlock) and the only part of that whole excersize that made any noticeable performance difference was removing the duplicate-input check, which made for a significant performance
< BlueMatt>
increase (of around 1.5ms in the 3-4ms function...)
< BlueMatt>
nvm, 0.7ms
< BlueMatt>
on my laptop: 8ms to deserialize, 11.1 -> 10.5ms for deserialize + checkblock
< sipa>
BlueMatt: cool, so let's just get rid of the duplicates check
< BlueMatt>
sipa: not so easy due to network partition risk, as sdaftuar and I just found out :(
< sipa>
uh?
< sipa>
ah.
< BlueMatt>
oops, no, nvm, just makes it tricky-er to do pre-relay of compact blocks
< BlueMatt>
since then CheckBlock becomes network-ban-partition-consensus
< BlueMatt>
(0.13.1 will already ban you for this, so it cant get worse...)
< BlueMatt>
sipa: re: #9026
< gmaxwell>
bump the protocol version number. then you can decide on your behavior based on it.?
< BlueMatt>
true, probably need to do that, though still really want to do for 0.13.2
< BlueMatt>
and then the problem is we dont want to change too often because you would just skip pre-relay for people not on your protocol version
< sipa>
BlueMatt: i'm really curious what typo you made that got corrected into "I did not Rome the hashing"
< BlueMatt>
dunno how my phone converted benchmark to rome, but it appears to have
< GitHub112>
[bitcoin] TheBlueMatt opened pull request #9049: Remove duplicatble duplicate-input check from CheckTransaction (master...2016-10-bench-checkblock) https://github.com/bitcoin/bitcoin/pull/9049
< sipa>
duplicatble duplicate-input ?
< GitHub106>
[bitcoin] theuni opened pull request #9050: net: make a few values immutible, and use deterministic randomness for the localnonce (master...connman-const) https://github.com/bitcoin/bitcoin/pull/9050
< gmaxwell>
cfields_: I haven't looked yet, but does that fix the current race condition, where if you connect to yourself then someone else connects to you then you get your own nonce, you'll not detect the connect to self?
< cfields_>
gmaxwell: i believe that was fixed a while back with the initial CConnman merge.
< gmaxwell>
yup! that looks good. Okay, I'd missed that.
< gmaxwell>
cfields_: whats the motivation for making the nonce determinstic?
< cfields_>
gmaxwell: none really, other than it's one of the few (the only?) users of Rand in there. Eliminating them could ease replay for testing.
< gmaxwell>
uh. wait... you mean this is completely determinstic?
< * gmaxwell>
goes to read the patch.
< cfields_>
gmaxwell: really not much though, i just figured if it was immutible and per-connection, may as well make it deterministic if possible
< cfields_>
gmaxwell: heh, no. It's seeded at startup
< cfields_>
wait, i hope...
< cfields_>
yes, sorry. It's a per-connman seed.
< cfields_>
you made me doubt myself for a sec :)
< gmaxwell>
K. yes, I see that. Just the 'replay for testing' made me wonder!
< gmaxwell>
so if someone manages to make enough connections to wrap the peer_id ... it'll repeat. OTOH if someone manages to do that lots of other things will probably break. :P
< cfields_>
heh, yes. NodeId is signed, so things could go wonky way before that
< cfields_>
err
< gmaxwell>
sipa: cfields_ just had a spurrious travis failure caused by the overly jumpy rand_bits test in libsecp256k1.
< gmaxwell>
sipa: IMO we should ifdef out the RNG tests and just run them once per release as part of a prerelease checklist or something.
< sipa>
iirc i started a replacement for that at some point, but it remained too spurious
< BlueMatt>
cfields_: hum, hashing isnt much of a bottleneck here...
< gmaxwell>
sipa: I believe its the only rest with spurrious failures, and the rng tests are the only ones which _must_ have spurrious failures.
< sipa>
right, but it should be doable to bring that down to 1 in a billion runs or so
< BlueMatt>
well, the compact blocks stuff is somewhat spurious
< BlueMatt>
though i suppose it could effect regtest during semi-normal usage
< sipa>
BlueMatt: this is about libsecp256k1
< BlueMatt>
oh, sorry
< BlueMatt>
get your own channel!
< BlueMatt>
(jk)
< cfields_>
BlueMatt: i just can't think of any good reason for handling it on the socket thread, which lags more with each added connection, as opposed to on the message thread(/pool), potentially lazily, and only as needed
< BlueMatt>
cfields_: oops, see my respond on github
< BlueMatt>
its a latency question...our current hasher should (mostly) be able to keep up with 1Gbps of shit (ok, maybe more like 500Mbps), but putting it in the processing logic adds latency to block processing
< gmaxwell>
also we do no other computation in the thread that handles the incoming data... so this should better let it run concurrently with processing given the current setup.
< cfields_>
hmm
< sipa>
i would expect that in the future it's also easier to parallellize networking than it is to parallellize processing
< sipa>
and the bip151 hasher should be a few times faster
< cfields_>
sipa: heh, i argued the exact opposite in the PR. At the select/epoll/etc. level anyway. Though I suppose you could split the nodes into batches.
< BlueMatt>
cfields_: if you're bored, #8969 can probably get another ack and a merge, then we're only about 3 commits from splitting main, altough the next one will probably have to wait until the compact block fix goes in
< * BlueMatt>
realizes he hasnt re-reviewed 8708, oops
< cfields_>
BlueMatt: you can save the poke, i owe you an ack on that one already :)
< BlueMatt>
heh, ok
< BlueMatt>
just trying to move things in since its gonna be a painful merge cycle to get this all in for 0.4
< sipa>
cfields_: also, 8708 mentions constness... i'm addressing that to some extent in #8580, which now is queued after #9039 :)
< BlueMatt>
lol, so much to review
< sipa>
9039 grew a bit beyond what i originally planned to address, but i think the result is worth it
< BlueMatt>
yea, i saw there was some back-and-forth, but, indeed, I'll bet thats worth it
< BlueMatt>
serialization can always use cleaning
< cfields_>
sipa: ack. Your serialization changes are next on my list to review. As it relates to 8708, i removed the const hack that i justified with "but we already do this elsewhere!", so my changes won't undo your fixes.
< BlueMatt>
we need an irc bot that posts pr titles after numbers are mentioned :p
< sipa>
BlueMatt: yes!
< BlueMatt>
where is nanotube when you need him?
< cfields_>
BlueMatt: heh, yes, and links to commit ids
< BlueMatt>
gribble: help me out bro
< cfields_>
BlueMatt: i used that in another channel, iirc it was easy enough to setup
< BlueMatt>
I'm sure, but I'm lazy...prefer if someone who's already running a bot in here does it :p
< BlueMatt>
I mean its easy...4 digit number starting with a # and link to github.com/bitcoin/bitcoin/issue/#
< BlueMatt>
cfields_: I'll try to get you another review on 8708 tonight, otherwise tomorrow morning
< sipa>
BlueMatt, cfields_: can you two fight over the order in which 8708 and 8969?
< BlueMatt>
should be minimal conflicts?
< sipa>
ok
< BlueMatt>
or at least easy-to-resolve ones?
< sipa>
so much easier :)
< sipa>
i assumed they'd conflict a lot
< BlueMatt>
8969 is some random trivial cleanups in preparation for the big split (tm)
< BlueMatt>
so souldnt conflict with much of anything
< sipa>
cfields_: 9039 touches pretty much every line that deals with serialization, so it'll conflict with the beginning of 8708
< BlueMatt>
sipa: I think 8708 is further along?
< cfields_>
sipa: np, i don't mind rebasing that one. It'll need one more set of changes on top, in fact, so i'll take the burden there
< BlueMatt>
I think cfields_ and I probably agree on it now, just need to review
< BlueMatt>
heh, ok
< sipa>
ok
< sipa>
i'll review 8708 in more detail too hen
< sipa>
then
< cfields_>
yea, it should be ready, i think, but it'll touch PushMessage again.
< cfields_>
sipa: if it makes more sense to get the serialization changes in first, I could go ahead and make the other change in 8708, rather than breaking it out, that way it can be reviewed in parallel, then just rebase on the serialization changes
< cfields_>
either way, doesn't matter at all to me.
< sipa>
cfields_: i think it'll just boil down to removing "nType, nVersion, " everywhere in your patch
< cfields_>
sipa: ah ok, that's trivial then.
< cfields_>
BlueMatt: btw, i'm just a few steps away from splitting CConnman out of net, but I'll wait for the main split :)
< BlueMatt>
cfields_: cool
< BlueMatt>
if we get all this shit in 0.14 I'm gonna be really impressed
< BlueMatt>
cfields_: you missed one of my nits - you need to move the assert in EndMessage up above the first WriteLE32
< BlueMatt>
cfields_: while you're at it, just change it to assert size >= HEADER_SIZE
< cfields_>
BlueMatt: hm, thought I moved it. sorry about that. will make that change.
< BlueMatt>
cfields_: while you're at it, you changed the LogPrint on sending messages
< BlueMatt>
it used to read "sending: ..." you removed the sending
< BlueMatt>
and you got rid of the SanitizeString() call there, though i doubt that matters
< cfields_>
BlueMatt: grr, not sure why those got dropped. will fix.
< cfields_>
crap, got to go help with dinner. bbl.
< BlueMatt>
cfields_: ok, I'll queue them in review...gotta run as well, so might have to finish tomorrow