< BlueMatt> cfields: hum, why do we even need fSuccessfullyConnected?
< BlueMatt> cfields: cant we just set nVersion at that location in ::VERSION processing and just keep using nVersion != 0
< BlueMatt> cfields: alternatively, shouldnt fSuccessfullyConnected be set in ::VERACK, not ::VERSION?
< cfields> BlueMatt: see the commit message. Sometimes we bail before fully finishing version
< BlueMatt> cfields: yes, I'm saying move that setting down
< cfields> BlueMatt: i think the confusion comes from trying to repurpose that var. What it really means is fCanSendToPeer
< cfields> how about renaming to that?
< BlueMatt> cfields: I'm asking if its redundant
< BlueMatt> the confusion is that it seems redundant
< cfields> BlueMatt: I suppose with the fDisconnect completely worked out, yes, it's redundant
< cfields> BlueMatt: the issue before was that sometimes we'd bail halfway through processing VERSION, leaving nVersion set, but we wouldn't want to send any new outgoing messages. But because some places didn't check for fDisconnected, outgoing messages got through anyway
< cfields> I believe that's fixed in that PR. So in that case, yes, they should be redundant. Will fix.
< BlueMatt> cfields: yes, i see that, but how hard is it to move the pfrom->nVersion = ... thing down 50 lines and do PushWithVersion for everything in between (is there anything)
< BlueMatt> ok
< BlueMatt> thanks
< BlueMatt> cfields: if you want to be extra super duper awesome for me you could even add a skip_bytes method in your CVectorWriter (see https://github.com/bitcoinfibre/bitcoinfibre/commit/70673283326f0ab7b542dfb16da32dd81f70176d) but thats not really a comment, just a note since I have a similar class in FIBRE and it would be useful
< sipa> BlueMatt: that just increments the write pointer?
< BlueMatt> essentially, yes
< sipa> the equivalent of writing zeroes, i guess?
< BlueMatt> sipa: yes
< BlueMatt> sipa: though for my use-case i couldnt care less if its 0s or garbage
< sipa> what is it used for?
< BlueMatt> though, i guess, sending un-init'd memory over the wire is generally frowned upon
< BlueMatt> cfields: didnt really read in too much detail, but the concept looks like what i was asking for
< sipa> BlueMatt: why do want to send (don't care) bytes over the wire?
< BlueMatt> sipa: pad transactions out so that they (often) start on packet/fec-chunk boundaries
< BlueMatt> you end up with null space
< BlueMatt> in an earlier implementation it even sent shorter packets to not send the null space, but you still use it for fec-coding
< BlueMatt> i think the current code sends the 0s just because it was annoying to try to code
< cfields> BlueMatt: hmm, it may make more sense to do that at the point where we're actually writing to the net?
< cfields> s/net/socket
< cfields> no problem adding it to CVectorWriter though, if that's the approach that makes sense
< cfields> BlueMatt: it surprises me that you'd see the benefits of padding like that though, considering how many layers of caching there are to get through on the OS side
< cfields> BlueMatt: and thanks for looking, btw
< gmaxwell> matt's data neeeds to be padded in memory, not for the net.
< gmaxwell> he seralizes the block into memory and then runs forward error correction over it. The padding is added so that transactions begin on FEC packet bundaries.
< gmaxwell> The reason for this is that when using the mempool for reconstruction when a txn is missing the whole packet containing is missing. To prevent miss amplification there is some padding to get txn onto packet boundaries.
< BlueMatt> cfields: note that if you've gotten to the point where you're sending tx data over the wire, you've failed horribly....at that point you've already sent a ton of fec data
< BlueMatt> also what gmaxwell said
< gmaxwell> if matt's FEC stuff was more optimized he'd probably never send the original packets at all ever. :P
< cfields> ah, i see
< BlueMatt> i mean i could just do that, but since i already have the data and all the plumbing for using it since the header stuff does...might as well send it :)
< cfields> completely misunderstood what you were getting at, thanks for the explanation
< luke-jr> I keep getting on master ./bench/data/block413567.raw.h:124989:40: error: redefinition of ‘const unsigned char block_bench::block413567 []’
< luke-jr> have to manually delete the file and try again
< luke-jr> the rules to generate it only append, shouldn't the first create anew?
< luke-jr> (and does make do the deletion of files when the rule fails, or must we?)
< luke-jr> looks like we need to do it..
<@wumpus> luke-jr: strange, I just copied that rule from the tests, so I'd expect it would work as-is. But indeed it should create the file anew, not append to it, that's weird
< luke-jr> is there one of these in the tests I should fix too?
<@wumpus> no, I think it's my fault, I removed the namespace{} around it, the first line in the test is probably ok
<@wumpus> should just change the first >> $@ to > $@
<@wumpus> are you going to do it or should I?
< luke-jr> well, if any of these steps fails, we need to delete the file too :x
< luke-jr> (or else make will think it's up to date next run)
<@wumpus> the best option then would be to write to a temporary file
<@wumpus> then at the end mv it over atomically
<@wumpus> eg write to $@.tmp
<@wumpus> in that case you need to do it in Makefile.test.include too
< luke-jr> hm, I was thinking http://codepad.org/SXhNyVzI
<@wumpus> meh, with my solution you never actually need to delete anything
<@wumpus> it's just not created if naything fails
< luke-jr> true
<@wumpus> or ... isn't that the case for yours too?
<@wumpus> you write the output to a pipe
<@wumpus> oh wait, yes
<@wumpus> the pipe doesn't 'wait'. SO yes a temporary file then atomic move is probably better
<@wumpus> yes
< bitcoin-git> [bitcoin] luke-jr opened pull request #9140: Bugfix: Correctly replace generated headers and fail cleanly (master...bugfix_genheaders) https://github.com/bitcoin/bitcoin/pull/9140
< bitcoin-git> [bitcoin] luke-jr closed pull request #7534: Minimal RPC & wallet support for CLTV-enabled multisig addresses (master...cltv_address) https://github.com/bitcoin/bitcoin/pull/7534
< bitcoin-git> [bitcoin] luke-jr closed pull request #8388: [0.13] mining: Optimise for typical mining use with blockmaxsize (0.13...blockmaxsize_opti-0.13) https://github.com/bitcoin/bitcoin/pull/8388
< BlueMatt> cfields: ouch, lets avoid any copies? I dont see why we should ever need that...once you PushMessage, PushMessage should just take the message, not the send code
< bitcoin-git> [bitcoin] jonasschnelli opened pull request #9141: Remove unnecessary calls to CheckFinalTx (master...2016/11/istrusted) https://github.com/bitcoin/bitcoin/pull/9141
< bitcoin-git> [bitcoin] jonasschnelli opened pull request #9142: Move -salvagewallet, -zap(wtx) to where they belong (master...2016/11/wallet_init) https://github.com/bitcoin/bitcoin/pull/9142
< Victorsueca> morning
< bitcoin-git> [bitcoin] jonasschnelli opened pull request #9143: Refactor ZapWalletTxes to avoid layer vialotions (master...2016/11/wallet_db_refactoring_1) https://github.com/bitcoin/bitcoin/pull/9143
< bitcoin-git> [bitcoin] fanquake opened pull request #9144: [Trivial] Correct waitforblockheight example help text (master...rpc-commands) https://github.com/bitcoin/bitcoin/pull/9144
< fanquake> Excuse the PRs/issue closing, going to try and clean up a lot of issues.
< paveljanik> fanquake, no need to apologise, good work!
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #9145: [qt] Make network disabled icon 50% opaque (master...Mf1611-qtNetworkIcon) https://github.com/bitcoin/bitcoin/pull/9145
<@wumpus> fanquake: yes, thanks a lot
< BlueMatt> someone wanna tag #9148 0.14?
< gribble> https://github.com/bitcoin/bitcoin/issues/9148 | Wallet RPCs can return stale info due to ProcessNewBlock Race · Issue #9148 · bitcoin/bitcoin · GitHub