< sdaftuar> cfields: i think it'd also get tricky because the CValidationState passed in to ProcessNewBlock can be referencing a different block than where it might be used in ActivateBestChainStep
< sipa> sdaftuar: good point
< cfields> sdaftuar: yes, makes sense. thanks.
< bitcoin-git> [bitcoin] TheBlueMatt closed pull request #9014: Fix block-connection performance regression (master...2016-10-fix-tx-regression) https://github.com/bitcoin/bitcoin/pull/9014
< BlueMatt> jtimon: so i think i need to revert #9087 for #9075
< gribble> https://github.com/bitcoin/bitcoin/issues/9087 | RPC: why not give more details when "generate" fails? by jtimon · Pull Request #9087 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/9075 | Decouple peer-processing-logic from block-connection-logic (#3) by TheBlueMatt · Pull Request #9075 · bitcoin/bitcoin · GitHub
< BlueMatt> because 9074 removes the state parameter from ProcessNewBlock - it only is used if CheckBlock fails, not if there is an actual failure
< jtimon> #9087 is just something I found while trying to fix the blocksigning branch, but it surprises me that you need to revert it
< gribble> https://github.com/bitcoin/bitcoin/issues/9087 | RPC: why not give more details when "generate" fails? by jtimon · Pull Request #9087 · bitcoin/bitcoin · GitHub
< BlueMatt> well it already doesnt work - the state object to ProcessNewBlock is only used if CheckBlock fails, not if the block is invalid for any other reason
< BlueMatt> the only way to get useful info from ProcessNewBlock is to register, then de-register a CValidationInterface, already
< BlueMatt> so I was removing it
< sipa> or if processblock doens't run... if someone another new block was received?
< sipa> *somehow
< jtimon> oh, I see, and in my case it was failing on checkblockheader
< jtimon> so with 9087 I was able to see "high hash"
< BlueMatt> if someone actually cares, I can fix it by doing the register, de-register hack
< BlueMatt> but....its a dirty hack
< jtimon> btw, after you matt took the time check out checkblockheader is just like checkpow but putting a message on the validation state and with and absurd bool arg
< sipa> jtimon: assuming the block wasn't reorged out, for what reason other than high hash would generate fail?
< jtimon> feel free to revert the change, I can always have it locally, it shouldn't normally fail anyway, just because I'm doing another version of checkpow for the block signing stuff
< sipa> jtimon: with matt's change, processnewblock doesn't take a CValidateState anymore
< jtimon> I just thought there was no harm and maybe was useful in more cases besides mine, seriously, no need to pass CValidateState to processnewblock only for rpc/mining
< sipa> BlueMatt, jtimon: perhaps it can be fixed by invoking AcceptNewHeader explicitly from RPC, and report the result from that?
< BlueMatt> ehh, please no...CheckBlock
< BlueMatt> not Accept*Header
< sipa> ah
< sipa> ok
< jtimon> I can do it locally if I need to, but right now I know where the things are failing, just don't understand why, please feel free to revert 9087, np
< BlueMatt> ok
< jtimon> it just felt kind of free to upstream
< BlueMatt> yupyup, all right decisions, just randomly gets in the way :)
< jtimon> ok, good, same page it seems
< jtimon> sipa: the last idea may be interesting for mining::generate though
< jtimon> but yeah, later
< jtimon> does it make sense for me to try that the custom chain (by default just like regstes, just with a different genesis block and a different directory) passes all rpc python tests just like regtest does?
< jtimon> never mind, I'll try first, ask later
< rebroad> gmaxwell, wumpus, sipa, I think I may have found a bug that can crash bitcoin nodes
< rebroad> potential DoS
< Victorsueca> rebroad: in latest version?
< rebroad> yes
< rebroad> or at least, someone else found it and used it against my node
< rebroad> but it's easy to fix
< jonasschnelli> rebroad: Does it crash or do you just get an exception?
< rebroad> crashes
< rebroad> well, both
< jonasschnelli> I guess we are talking about 9110
< jonasschnelli> PR #9110
< gribble> https://github.com/bitcoin/bitcoin/issues/9110 | CInv::GetCommand(): type=1373179482 unknown type · Issue #9110 · bitcoin/bitcoin · GitHub
< rebroad> yes
< rebroad> only if debug=net though
< rebroad> the "got inv" debug messsage
< jonasschnelli> Okay...
< Victorsueca> sounds like bitcoin core fails some assertion in there
< paveljanik> please add the info about debug=net to the PR...
< Victorsueca> rebroad: it crashes only with debug=net enabled? or it still crashes anyway but without log exception when disabled?
< rebroad> I have changed the code to simply strprintf an unknown and I think this should be an ok fix.. not tested sufficiently yet (as I don't have code to generate invalid inv messages yet)
< rebroad> Victorsueca, I suspect it'll only crash if debug=net but not tested with it not set yet
< rebroad> but someone out there is already sending these invalid invs... the address it came from was
< Victorsueca> if that's the case then it would be a bug at generating the exception warning message or writing it to the log
< jonasschnelli> I though ProcessMessages() executes always in a try/catch...
< rebroad> it's is possible that I have made a change that is making it vulnerable
< jonasschnelli> I think so...
< jonasschnelli> Going to test this now...
< rebroad> I don't think I have, but it's not beyond belief
< Victorsueca> maybe somebody should make a honeypot and wireshark it to see what message is causing this
< jonasschnelli> rebroad: you could try to reproduce the issue on master with a simple new RPC test (maybe called invalidinv.py)
< jonasschnelli> Victorsueca: it's simple to test... no need for a honeypot
< paveljanik> rebroad, and in every case: responsible disclosure next time, please...
< Victorsueca> ^ yeah, this things should be PGPed to the team
< jonasschnelli> Indeed (if you think your right)
<@wumpus> I'm fairly sure tI've tried to do this before, though probably not with debug=net
<@wumpus> it'd need to call CInv::ToString or CInv::GetCommand to get this exception, indeed something that only happens with debugging on
<@wumpus> I think it's somewhat unexpected for a ToString call to raise an exception
<@wumpus> however the exception should not crash the program
< Victorsueca> the log says something about St12out_of_range, that sounds like something that was asserted to be within certain range is not
< Victorsueca> maybe the type number?
< fanquake> wumpus Yes you have. See #1625
< gribble> https://github.com/bitcoin/bitcoin/issues/1625 | Bitcoin-Qt: exception in CInv::GetCommand() when using -onlynet="IPv6" · Issue #1625 · bitcoin/bitcoin · GitHub
< fanquake> So we've seen this/a similar issue before.
<@wumpus> managed to get the message in the log, however no crash
< bitcoin-git> [bitcoin] fanquake opened pull request #9111: Remove unused variable UNLIKELY_PCT from fees.h (master...remove-unused-unlikely-pct) https://github.com/bitcoin/bitcoin/pull/9111
< bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/924de0bd75a7...e9847303e708
< bitcoin-git> bitcoin/master addfdeb Andrew Chow: Multiple Selection for peer and ban tables...
< bitcoin-git> bitcoin/master 1077577 Andrew Chow: Fix auto-deselection of peers
< bitcoin-git> bitcoin/master e984730 Wladimir J. van der Laan: Merge #8874: Multiple Selection for peer and ban tables...
< rebroad> apologies for the less than ideal disclosure... I purposefully did not mention the issue I'd raised on here for that reason (bad internet here, otherwise I'd already have wiped the issue the moment it got mentioned)
< bitcoin-git> [bitcoin] laanwj opened pull request #9112: Avoid ugly exception in log on unknown inv type (master...2016_11_invtype_debugging) https://github.com/bitcoin/bitcoin/pull/9112
< bitcoin-git> [bitcoin] rebroad opened pull request #9113: Return the type when it's unknown instead of throwing an exception (master...ReturnNotThrow) https://github.com/bitcoin/bitcoin/pull/9113
< bitcoin-git> [bitcoin] fanquake opened pull request #9114: [depends] Set OSX_MIN_VERSION to 10.8 (master...depends-min-osx-10-8) https://github.com/bitcoin/bitcoin/pull/9114
< jonasschnelli> I have started with the BIP151 implementation and are trying to split it into different PRs, though, not sure how easy that is.
<@wumpus> indeed, for next time: if you suspect a remote-triggerable crash bug, even if you're not sure, please don't immediately throw it in here, but use private ways of contacting
<@wumpus> </panicmode>
<@wumpus> jonasschnelli: at least coordinate with cfields on the network level changes
< bitcoin-git> [bitcoin] paveljanik opened pull request #9115: Mention reporting security issues responsibly (master...20161109_secissues) https://github.com/bitcoin/bitcoin/pull/9115
< bitcoin-git> [bitcoin] ondrejsika opened pull request #9116: Allow getblocktemlate for not connected regtest node (master...master) https://github.com/bitcoin/bitcoin/pull/9116
< jonasschnelli> wumpus: Yes. Good idea. Not sure if a PR with the implementation of the new message format without the actual encryption itself could make sense.
< bitcoin-git> [bitcoin] laanwj pushed 10 new commits to master: https://github.com/bitcoin/bitcoin/compare/e9847303e708...e81df49644c2
< bitcoin-git> bitcoin/master 50e8a9c Pieter Wuille: Remove unused ReadVersion and WriteVersion...
< bitcoin-git> bitcoin/master c2c5d42 Pieter Wuille: Make streams' read and write return void...
< bitcoin-git> bitcoin/master fad9b66 Pieter Wuille: Make nType and nVersion private and sometimes const...
< bitcoin-git> [bitcoin] laanwj closed pull request #9039: Various serialization simplifcations and optimizations (master...simpleserial) https://github.com/bitcoin/bitcoin/pull/9039
< bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/e81df49644c2...e0477f6d2072
< bitcoin-git> bitcoin/master fd5654c Pavel Janík: Check and enable -Wshadow by default.
< bitcoin-git> bitcoin/master 359bac7 Pavel Janík: Add notes about variable names and shadowing
< bitcoin-git> bitcoin/master e0477f6 Wladimir J. van der Laan: Merge #8794: Enable -Wshadow by default...
< bitcoin-git> [bitcoin] laanwj closed pull request #8794: Enable -Wshadow by default (master...20160922_Wshadow_enable) https://github.com/bitcoin/bitcoin/pull/8794
< jonasschnelli> cfields: net.h what's the advantage of using a Callable (ForEachNodeContinueIf(Callable&& func)) instead of just passing in a std::function?
< cfields> jonasschnelli: so that lambdas are overhead-free
< jonasschnelli> cfields: can you explain me what kind of overhead? (Sorry to bother you with basic c++)
< cfields> jonasschnelli: np. std::function necessarily does an allocation, is copyable, etc. But lambdas can essentially create inline code.
< cfields> by using a callable, you can pass in a lambda for "free", or pass in a std::function for flexibility. It gives the flexibility of both.
< cfields> (maybe I should've mentioned that earlier. You can pass a std::function into it if you'd like, it'll work as you'd want it to)
< jonasschnelli> cfields: here I pass in Lambda https://github.com/bitcoin/bitcoin/pull/9076/files#diff-b2bb174788c7409b671c46ccc86034bdR3824 ... the function structure defines the parameter as std::function, does this always result in a copy for the lambda?
< cfields> jonasschnelli: yes. a std::function is constructed there.
< jonasschnelli> Okay. So changing it to a callable would be more efficient.. right?
< jonasschnelli> (reducing a copy)
< cfields> jonasschnelli: yep, assuming you can neatly use a template in the definition
< cfields> jonasschnelli: think of it like something "printable". You could pass the constant "foo" into a "Print(std::string s) { printf("string" %s, s); }, which will construct a std::string just to throw it away
< jonasschnelli> cfields: Thanks. I think I see you point now.
< cfields> but if you used "template <typename T> Print(T&& s) { std::cout << s; }, you can pass in a raw string for free
< fanquake> paveljanik I'm probably wrong, but the hex/line numbers was the first thing that jumped out at me.
< paveljanik> fanquake, sure. But this is probably some issue on the builder side as it does not touch leveldb at all...
< MarcoFalke> I think I was hitting the assert in net.h https://github.com/bitcoin/bitcoin/blob/e9847303e708ab71b4d2c22ceb7d65c89615e18a/src/net.h#L772 ... I couldn't reproduce and debug.log only says ProcessMessages(version, 102 bytes) FAILED peer=10 http://pastebin.ubuntu.com/23451144/
< cfields> MarcoFalke: yes, i assumed that would cause a few crashes for the first few days
< cfields> MarcoFalke: can you run with -debug for a while?
< MarcoFalke> sure
< cfields> MarcoFalke: the issue is: we continue to send messages, even after the handshake has failed and we've decided to disconnect. I think it's a good thing to find out what those cases are.
< MarcoFalke> cfields: Prob when expected != offered. See debug.log: https://paste.fedoraproject.org/476493/87085851/
< cfields> MarcoFalke: perfect, thanks
< cfields> MarcoFalke: hmm, seems to be in sending the reject...
< MarcoFalke> And what is the feefilter in the bt?
< cfields> MarcoFalke: aha! I didn't see the bt
< cfields> MarcoFalke: yep, that's the problem.
< cfields> MarcoFalke: I'll pr a quick/dirty fix in a few min, and I'm working on a more robust solution
< cfields> MarcoFalke: quick fix: http://pastebin.com/raw/E8ZXsKYa
< MarcoFalke> that was quick :)
< cfields> MarcoFalke: well like i said, I expected it to point out a few issues. They should all be very easy to fix individually.
< cfields> (as long as there's a nice log/backtrace, that is :)
< bitcoin-git> [bitcoin] theuni opened pull request #9117: net: don't send feefilter messages before the version handshake is complete (master...feefilter-assert) https://github.com/bitcoin/bitcoin/pull/9117
< sipa> jonasschnelli: i was planning to work on bip151, but i'm not going to touch anything before the ongoing net refactors are done :)
< gmaxwell> cfields: if the issue in 9117 is feefilter before handshake, should it not be checking fSuccessfullyConnected?
< cfields> jonasschnelli and i just discussed a bit, i'm making sure that the current refactors at least make bip151 not harder, but hopefully easier too
< cfields> gmaxwell: i suppose that would work too in this case. just that fDisconnected happens to also catch the case when we've just decided to disconnect after a successful handshake
< MarcoFalke> gmaxwell: I think the handshake was done in the case I saw above: https://paste.fedoraproject.org/476493/87085851/raw/
< cfields> er, at any time in the session after the handshake is done
< cfields> MarcoFalke: no, it's halfway done. We got their version and set it, but didn't like it, so didn't set a sendVersion
< gmaxwell> but it wouldn't catch cases where we weren't done but also weren't planning on disconnecting them?
< cfields> gmaxwell: i don't think that's a possibility
< cfields> gmaxwell: i'm cleaning up the disconnect logic atm so that it's hopefully more clear. I believe the only case where we'd want to send a message after we've set fDisconnect is for reject messages, no?
< gmaxwell> I don't really think we should be sending any messages after flipping that flag.
< cfields> hmm, may be able to make that work
< morcos> sipa: Did you intend for fImporting to be set during LoadMempool? It is now, but I thought that was a bug, indeed it causes pruning.py to fail b/c sync isn't happening during the potentially long process of loading the mempool. (fImporting causes IBD to be true)
< morcos> I was going to PR a fix, but I realize now that fee estimation conveniently ignores the LoadMempool txs due to this, so now it requires a lot more thinking about what the correct behavior is
< sipa> morcos: hmm, IBD should probably not be true during LoadMempool
< gmaxwell> the build is now a _flood_ ofr shadowing warnings for me.
< gmaxwell> thousands of them.
< paveljanik> gmaxwell, what compiler? Can you upload the log please?
< gmaxwell> gcc version 6.1.1 20160802 (Debian 6.1.1-11)
< paveljanik> old gcc?
< sipa> that's a very new gcc
< sipa> i also get those warning
< Chris_Stewart_5> Is the CHECKSIG operation missing in BIP141 for P2WPKH & P2WPKH nested inside of P2SH? https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#p2wpkh
< paveljanik> can you please upload the log somewhere?
< paveljanik> There is #8808 but maybe we need even more for new gcc
< gribble> https://github.com/bitcoin/bitcoin/issues/8808 | Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin · GitHub
< paveljanik> fFlushOnClose is fixed there
< paveljanik> gmaxwell, looks like you are good candidate for ACK on #8808 8)
< gribble> https://github.com/bitcoin/bitcoin/issues/8808 | Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin · GitHub
< jonasschnelli> sipa: Okay. That's great news. I can re-focus then on the SPV mode.
< gmaxwell> Where did we turn on these Wshadow warnings?
< gmaxwell> oh, I'd already pulled it off while bisecting. 8794
< paveljanik> yes, but 8808 is missing.
< paveljanik> and I'm now testing with a bit older gcc than yours...
< sipa> i can test with 6.2.0
< paveljanik> great! I will install the VM with some newer one later
< phantomcircuit> gmaxwell: yeah we shadow things all over the place
< sipa> let's fix it
<@wumpus> strange, I had no shadow warnings at all before I merged that
<@wumpus> or maybe one in leveldb or so, but certainly no flood
<@wumpus> I don't get it
<@wumpus> going to revert
<@wumpus> maybe we shouldn't do it at all, it's too compiler-dependent
< sipa> i think all the difficulty we have in figuring out how to resolve the -Wshadow warning exactly shows what problems actual unintentional shadowing could have
< sipa> and there is a clear upper bound on what -Wshadow can mean... i'm a bit surprised there even is compiler dependence here
< gmaxwell> In C it's utterly unambigious and fine, I'm less sure about C++ but I expect it's good to enable. But we should eliminate all the warnings first. Very weird that you weren't seeing them!
< MarcoFalke> I could check #8808 with 6.2.1
< gribble> https://github.com/bitcoin/bitcoin/issues/8808 | Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin · GitHub
< gmaxwell> (by all I mean almost all, of course, obviously I don't care about one in leveldb)
<@wumpus> I'm not going to bother with it anymore, at least
<@wumpus> not just me *multiple people* tested it here: https://github.com/bitcoin/bitcoin/pull/8794
< bitcoin-git> [bitcoin] laanwj pushed 1 new commit to master: https://github.com/bitcoin/bitcoin/commit/f445d886126c00814c71b855fc2f36fdd7e11098
< bitcoin-git> bitcoin/master f445d88 Wladimir J. van der Laan: Revert "Check and enable -Wshadow by default."...
<@wumpus> seems just needless busywork, how many 'prevent shadowing' commits have we had and it's still a problem
< gmaxwell> Coding convention wise, I notice that 8808 adds _ to many local variables. I think there moderately common convention for using _ on function parameters.
< gmaxwell> Many other things are wshadow with no issues, but we have classes with many flag generic top level names. like "bits", "flags", "nversion"... and a codebase that hasn't had the warning for a long time.
<@wumpus> but those were supposed to be fixed already
<@wumpus> where are you getting all these warnings?
<@wumpus> can you maybe pastebin and post in that issue?
<@wumpus> maybe paveljanik would like to know
< gmaxwell> Glad to help squash them all.
<@wumpus> jdid't we get rid of nVersion?
< sipa> of the serialize.h-based nVersion
< sipa> but there are still some nVersion variables scattered
<@wumpus> nah, I think there's more important things
<@wumpus> I thought this was done, but if there's another load of that bullshit
< gmaxwell> I really would like to understand how compliler differences came into play.
<@wumpus> probably they consider slightly different scopes
< * paveljanik> back
< paveljanik> I first done Wshadow clean build on OS X/clang
< paveljanik> then tested on newer clang
< paveljanik> than newer clang on Linux
< paveljanik> then old gcc on Linux.
< paveljanik> I'm now Wshadow clean on all clangs available to me and one gcc version (after 8808).
< paveljanik> There are millions of differences in them emitting warnings.
<@wumpus> I tried with the gcc that comes with Ubuntu 16.04 (5.4.0) as well as clang 4.0 master git on Linux
< MarcoFalke> I think gcc is more aggressive and also complains if a local var shadows a function name (such as size() or one of our own functions)
< paveljanik> exactly!
<@wumpus> haven't tested gcc 6.x, maybe that is uselessly agressive
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/f445d886126c...fb156100f9b4
< bitcoin-git> bitcoin/master d8edf03 fanquake: Remove unused var UNLIKELY_PCT from fees.h
< bitcoin-git> bitcoin/master fb15610 Wladimir J. van der Laan: Merge #9111: Remove unused variable UNLIKELY_PCT from fees.h...
< bitcoin-git> [bitcoin] laanwj closed pull request #9111: Remove unused variable UNLIKELY_PCT from fees.h (master...remove-unused-unlikely-pct) https://github.com/bitcoin/bitcoin/pull/9111
< bitcoin-git> [bitcoin] laanwj pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/fb156100f9b4...faec09bc7f03
< bitcoin-git> bitcoin/master e5d682f jnewbery: Fix mininode version message format
< bitcoin-git> bitcoin/master faec09b Wladimir J. van der Laan: Merge #8894: [Testing] Include fRelay in mininode version messages...
< bitcoin-git> [bitcoin] dooglus opened pull request #9118: Remove requireGreater argment from TxConfirmStats::EstimateMedianVal() (master...remove-dead-code) https://github.com/bitcoin/bitcoin/pull/9118
< bitcoin-git> [bitcoin] laanwj closed 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
< morcos> wumpus: is it critical to be doing all this clean up on fee estimation right now?
< morcos> i'd at least like a chance to think about it before you merge all these changes
< morcos> i understand those variables are unused now
< morcos> but they may still be useful functionality for further extension of the fee estimation code
<@wumpus> morcos: no, imo it's not necessary, but I haven't heared you complain about it before
<@wumpus> you could just NACK those pulls
<@wumpus> if no one complains then dead code cleanups seems like a no-brainer
< morcos> i guess i just panicked b/c i saw one of them got merged before i even saw it
< morcos> within a 12 hours
< morcos> but that one actually was trivial
<@wumpus> that was just a one-liner
< morcos> i will comment on the other
< morcos> i hadn't actually looked at the changes on either of them yet
<@wumpus> also our usual policy is to remove dead code, we're using git to make it easy to bring back things if necessary
< morcos> but yes i'm in favor of the priority removal that was done of course, but the require greater logic was tricky to get right the first time
< morcos> i'd rather leave it until we're sure we won't want it again
<@wumpus> I'll just stop merging things now, only caused problems today I feel :/
<@wumpus> just too many pulls...
< morcos> wumpus: i'm impressed you keep up as well as you do.. my head is spinning... i started today trying to doing a posthumous ACK to the serialization cleanups, and i still haven't gotten around to looking at the 2nd commit.
<@wumpus> I also have a harder and harder time keeping up
< morcos> i suppose its a good problem to have
< gmaxwell> wumpus: re the Wshadow warning, is it possible that the files aren't dirty when you change the flag so make isn't rebuilding them?
< gmaxwell> I was rebasing a branch from a distant version when I noticed the warngings, so my tree had been effectively make cleaned.
< bitcoin-git> [bitcoin] laanwj closed pull request #9118: Remove requireGreater argment from TxConfirmStats::EstimateMedianVal() (master...remove-dead-code) https://github.com/bitcoin/bitcoin/pull/9118
<@wumpus> gmaxwell: no, I cleaned both the tree and the ccache, for both compilers
<@wumpus> but I don't want to bother with the Wshadow warnings anymore, let's focus the rare time we have on more important things
< paveljanik> gmaxwell, can you please try http://tmp.janik.cz/q.diff?
< paveljanik> it should clear the nVersion from chain.h and others.
< gmaxwell> paveljanik: the only ones remaining with that are leveldb and bench/lockedpool.cpp
< paveljanik> gmaxwell, great!
< paveljanik> bench/lockedpool is clear IIRC.
< paveljanik> and leveldb is upstream 8)
< paveljanik> so fixed? ;-)
< gmaxwell> paveljanik: I see you're renaming local's to _local. That is a violation of the common convention in other codebases that function paramters get _prefixes.
< paveljanik> but I'll first retest on some rolling upstream Linux distro
< paveljanik> we used that even before and people agreed on it.
< paveljanik> some parts of the code use _, some _in, some In postfix..
< gmaxwell> Were we really using it for _local_ variables before, not just function parameters?
< paveljanik> yes
< paveljanik> I was inspired by it.
< paveljanik> but sometimes, it was better to rename the variables...
< paveljanik> and thus I did so
< paveljanik> e.g. data -> vData to follow the typeName conventions used
< paveljanik> I understand it is not sexy to read all those diffs though 8)
< paveljanik> but please be patient
< gmaxwell> paveljanik: for example, in 8808 src/streams.h
< paveljanik> yes, read -> _read.
< gmaxwell> you change read to _read it would be more ideomatic to change it to someghing like nBytes or bread or nread or something like that.
< paveljanik> no problem, just comment there and I will change it ;-)
< gmaxwell> When you make these updates are you checking to make sure the existing code wasn't buggy?
< paveljanik> sometimes. I even found some bugs, yes.
< gmaxwell> paveljanik: Good. I'll gladly leave some comments.
< bitcoin-git> [bitcoin] UdjinM6 opened pull request #9120: bug: Missed one "return false" in recent refactoring in #9067 (master...fixExitCodesBitcoinTx) https://github.com/bitcoin/bitcoin/pull/9120
< paveljanik> gmaxwell, thank you!
< jtimon> morcos: no, I'm still getting my weird error on walletbackup even with #9058, really killall bitcoind ; rm -rf ./qa/cache ; doesn't seem enough
< gribble> https://github.com/bitcoin/bitcoin/issues/9058 | Fixes for p2p-compactblocks.py test timeouts on travis (#8842) by ryanofsky · Pull Request #9058 · bitcoin/bitcoin · GitHub
< morcos> jtimon: not for me right?
< jtimon> reminder: I only get it by running all rpc tests, not running walletbackupt individually
< jtimon> you told me 9058 may help the other day, but it seems is unrelated :(
< jtimon> I'm locally getting errors on your branch, but I was getting them already and at this point I'm just doing something wrong with rpc tests somehow, so yeah, sorry for the too long explanation
< jtimon> btw, general thoughts on moving some fast extended tests to the common ones and maybe also some slow ones from the common ones to the extended ones?
< jtimon> re warnings you can still fix some shadowing even if you don't put the new flag yet
< jtimon> I'm starting to think it may not be just be but actually something between 1adf82ac and faec09bc
< jtimon> s/just be/just me/