1 2016-11-09 00:45:26	0|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
  2 2016-11-09 00:45:44	0|sipa|sdaftuar: good point
  3 2016-11-09 00:51:18	0|cfields|sdaftuar: yes, makes sense. thanks.
  4 2016-11-09 01:19:44	0|bitcoin-git|[13bitcoin] 15TheBlueMatt closed pull request #9014: Fix block-connection performance regression (06master...062016-10-fix-tx-regression) 02https://github.com/bitcoin/bitcoin/pull/9014
  5 2016-11-09 01:42:46	0|BlueMatt|jtimon: so i think i need to revert #9087 for #9075
  6 2016-11-09 01:42:48	0|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
  7 2016-11-09 01:42:49	0|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
  8 2016-11-09 01:43:06	0|BlueMatt|because 9074 removes the state parameter from ProcessNewBlock - it only is used if CheckBlock fails, not if there is an actual failure
  9 2016-11-09 01:45:23	0|jtimon|#9087 is just something I found while trying to fix the blocksigning branch, but it surprises me that you need to revert it
 10 2016-11-09 01:45:25	0|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
 11 2016-11-09 01:45:54	0|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
 12 2016-11-09 01:46:09	0|BlueMatt|the only way to get useful info from ProcessNewBlock is to register, then de-register a CValidationInterface, already
 13 2016-11-09 01:46:13	0|BlueMatt|so I was removing it
 14 2016-11-09 01:46:21	0|sipa|or if processblock doens't run... if someone another new block was received?
 15 2016-11-09 01:46:24	0|sipa|*somehow
 16 2016-11-09 01:46:42	0|jtimon|oh, I see, and in my case it was failing on checkblockheader
 17 2016-11-09 01:47:17	0|jtimon|so with 9087 I was able to see "high hash"
 18 2016-11-09 01:47:32	0|BlueMatt|if someone actually cares, I can fix it by doing the register, de-register hack
 19 2016-11-09 01:47:41	0|BlueMatt|but....its a dirty hack
 20 2016-11-09 01:48:29	0|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
 21 2016-11-09 01:48:31	0|sipa|jtimon: assuming the block wasn't reorged out, for what reason other than high hash would generate fail?
 22 2016-11-09 01:49:52	0|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
 23 2016-11-09 01:50:36	0|sipa|jtimon: with matt's change, processnewblock doesn't take a CValidateState anymore
 24 2016-11-09 01:51:03	0|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
 25 2016-11-09 01:51:12	0|sipa|BlueMatt, jtimon: perhaps it can be fixed by invoking AcceptNewHeader explicitly from RPC, and report the result from that?
 26 2016-11-09 01:51:31	0|BlueMatt|ehh, please no...CheckBlock
 27 2016-11-09 01:51:35	0|BlueMatt|not Accept*Header
 28 2016-11-09 01:51:40	0|sipa|ah
 29 2016-11-09 01:51:42	0|sipa|ok
 30 2016-11-09 01:52:22	0|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
 31 2016-11-09 01:53:07	0|BlueMatt|ok
 32 2016-11-09 01:53:28	0|jtimon|it just felt kind of free to upstream
 33 2016-11-09 01:53:41	0|BlueMatt|yupyup, all right decisions, just randomly gets in the way :)
 34 2016-11-09 01:54:14	0|jtimon|ok, good, same page it seems
 35 2016-11-09 01:56:15	0|jtimon|sipa: the last idea may be interesting for mining::generate though
 36 2016-11-09 01:56:51	0|jtimon|but yeah, later
 37 2016-11-09 01:59:07	0|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?
 38 2016-11-09 02:00:48	0|jtimon|never mind, I'll try first, ask later
 39 2016-11-09 09:16:35	0|rebroad|gmaxwell, wumpus, sipa, I think I may have found a bug that can crash bitcoin nodes
 40 2016-11-09 09:17:15	0|rebroad|potential DoS
 41 2016-11-09 09:17:45	0|Victorsueca|rebroad: in latest version?
 42 2016-11-09 09:17:48	0|rebroad|yes
 43 2016-11-09 09:18:18	0|rebroad|or at least, someone else found it and used it against my node
 44 2016-11-09 09:18:24	0|rebroad|but it's easy to fix
 45 2016-11-09 09:18:39	0|jonasschnelli|rebroad: Does it crash or do you just get an exception?
 46 2016-11-09 09:18:43	0|rebroad|crashes
 47 2016-11-09 09:18:47	0|rebroad|well, both
 48 2016-11-09 09:19:06	0|jonasschnelli|I guess we are talking about 9110
 49 2016-11-09 09:19:07	0|jonasschnelli|PR #9110
 50 2016-11-09 09:19:08	0|gribble|https://github.com/bitcoin/bitcoin/issues/9110 | CInv::GetCommand(): type=1373179482 unknown type · Issue #9110 · bitcoin/bitcoin · GitHub
 51 2016-11-09 09:19:16	0|rebroad|yes
 52 2016-11-09 09:19:31	0|rebroad|only if debug=net though
 53 2016-11-09 09:19:38	0|rebroad|the "got inv" debug messsage
 54 2016-11-09 09:20:09	0|jonasschnelli|Okay...
 55 2016-11-09 09:20:20	0|Victorsueca|sounds like bitcoin core fails some assertion in there
 56 2016-11-09 09:20:23	0|paveljanik|please add the info about debug=net to the PR...
 57 2016-11-09 09:21:19	0|Victorsueca|rebroad: it crashes only with debug=net enabled? or it still crashes anyway but without log exception when disabled?
 58 2016-11-09 09:21:27	0|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)
 59 2016-11-09 09:21:49	0|rebroad|Victorsueca, I suspect it'll only crash if debug=net but not tested with it not set yet
 60 2016-11-09 09:22:37	0|rebroad|but someone out there is already sending these invalid invs... the address it came from was 50.254.73.145
 61 2016-11-09 09:23:14	0|Victorsueca|if that's the case then it would be a bug at generating the exception warning message or writing it to the log
 62 2016-11-09 09:23:25	0|jonasschnelli|I though ProcessMessages() executes always in a try/catch...
 63 2016-11-09 09:23:53	0|rebroad|it's is possible that I have made a change that is making it vulnerable
 64 2016-11-09 09:24:02	0|jonasschnelli|I think so...
 65 2016-11-09 09:24:07	0|jonasschnelli|Going to test this now...
 66 2016-11-09 09:24:08	0|rebroad|I don't think I have, but it's not beyond belief
 67 2016-11-09 09:24:56	0|Victorsueca|maybe somebody should make a honeypot and wireshark it to see what message is causing this
 68 2016-11-09 09:25:06	0|jonasschnelli|rebroad: you could try to reproduce the issue on master with a simple new RPC test (maybe called invalidinv.py)
 69 2016-11-09 09:25:14	0|jonasschnelli|Victorsueca: it's simple to test... no need for a honeypot
 70 2016-11-09 09:25:40	0|paveljanik|rebroad, and in every case: responsible disclosure next time, please...
 71 2016-11-09 09:26:28	0|Victorsueca|^ yeah, this things should be PGPed to the team
 72 2016-11-09 09:26:53	0|jonasschnelli|Indeed (if you think your right)
 73 2016-11-09 09:27:58	0|wumpus|I'm fairly sure tI've tried to do this before, though probably not with debug=net
 74 2016-11-09 09:33:42	0|wumpus|it'd need to call CInv::ToString or CInv::GetCommand to get this exception, indeed something that only happens with debugging on
 75 2016-11-09 09:33:58	0|wumpus|I think it's somewhat unexpected for a ToString call to raise an exception
 76 2016-11-09 09:34:06	0|wumpus|however the exception should not crash the program
 77 2016-11-09 09:39:56	0|Victorsueca|the log says something about St12out_of_range, that sounds like something that was asserted to be within certain range is not
 78 2016-11-09 09:40:05	0|Victorsueca|maybe the type number?
 79 2016-11-09 09:41:05	0|fanquake|wumpus Yes you have. See #1625
 80 2016-11-09 09:41:06	0|gribble|https://github.com/bitcoin/bitcoin/issues/1625 | Bitcoin-Qt: exception in CInv::GetCommand() when using -onlynet="IPv6" · Issue #1625 · bitcoin/bitcoin · GitHub
 81 2016-11-09 09:41:28	0|fanquake|So we've seen this/a similar issue before.
 82 2016-11-09 09:41:50	0|wumpus|managed to get the message in the log, however no crash
 83 2016-11-09 09:54:03	0|bitcoin-git|[13bitcoin] 15fanquake opened pull request #9111: Remove unused variable UNLIKELY_PCT from fees.h (06master...06remove-unused-unlikely-pct) 02https://github.com/bitcoin/bitcoin/pull/9111
 84 2016-11-09 10:05:44	0|bitcoin-git|13bitcoin/06master 141077577 15Andrew Chow: Fix auto-deselection of peers
 85 2016-11-09 10:05:44	0|bitcoin-git|13bitcoin/06master 14addfdeb 15Andrew Chow: Multiple Selection for peer and ban tables...
 86 2016-11-09 10:05:44	0|bitcoin-git|[13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/924de0bd75a7...e9847303e708
 87 2016-11-09 10:05:45	0|bitcoin-git|13bitcoin/06master 14e984730 15Wladimir J. van der Laan: Merge #8874: Multiple Selection for peer and ban tables...
 88 2016-11-09 10:13:07	0|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)
 89 2016-11-09 10:16:37	0|bitcoin-git|[13bitcoin] 15laanwj opened pull request #9112: Avoid ugly exception in log on unknown inv type (06master...062016_11_invtype_debugging) 02https://github.com/bitcoin/bitcoin/pull/9112
 90 2016-11-09 10:25:20	0|bitcoin-git|[13bitcoin] 15rebroad opened pull request #9113: Return the type when it's unknown instead of throwing an exception (06master...06ReturnNotThrow) 02https://github.com/bitcoin/bitcoin/pull/9113
 91 2016-11-09 11:01:17	0|bitcoin-git|[13bitcoin] 15fanquake opened pull request #9114: [depends] Set OSX_MIN_VERSION to 10.8 (06master...06depends-min-osx-10-8) 02https://github.com/bitcoin/bitcoin/pull/9114
 92 2016-11-09 11:28:59	0|jonasschnelli|I have started with the BIP151 implementation and are trying to split it into different PRs, though, not sure how easy that is.
 93 2016-11-09 11:34:57	0|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
 94 2016-11-09 11:36:34	0|wumpus|</panicmode>
 95 2016-11-09 11:37:26	0|wumpus|jonasschnelli: at least coordinate with cfields on the network level changes
 96 2016-11-09 11:54:38	0|bitcoin-git|[13bitcoin] 15paveljanik opened pull request #9115: Mention reporting security issues responsibly (06master...0620161109_secissues) 02https://github.com/bitcoin/bitcoin/pull/9115
 97 2016-11-09 12:21:56	0|bitcoin-git|[13bitcoin] 15ondrejsika opened pull request #9116: Allow getblocktemlate for not connected regtest node (06master...06master) 02https://github.com/bitcoin/bitcoin/pull/9116
 98 2016-11-09 12:37:33	0|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.
 99 2016-11-09 12:52:32	0|bitcoin-git|[13bitcoin] 15laanwj pushed 10 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/e9847303e708...e81df49644c2
100 2016-11-09 12:52:33	0|bitcoin-git|13bitcoin/06master 1450e8a9c 15Pieter Wuille: Remove unused ReadVersion and WriteVersion...
101 2016-11-09 12:52:33	0|bitcoin-git|13bitcoin/06master 14c2c5d42 15Pieter Wuille: Make streams' read and write return void...
102 2016-11-09 12:52:34	0|bitcoin-git|13bitcoin/06master 14fad9b66 15Pieter Wuille: Make nType and nVersion private and sometimes const...
103 2016-11-09 12:52:42	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #9039: Various serialization simplifcations and optimizations (06master...06simpleserial) 02https://github.com/bitcoin/bitcoin/pull/9039
104 2016-11-09 13:12:42	0|bitcoin-git|[13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/e81df49644c2...e0477f6d2072
105 2016-11-09 13:12:43	0|bitcoin-git|13bitcoin/06master 14359bac7 15Pavel Janík: Add notes about variable names and shadowing
106 2016-11-09 13:12:43	0|bitcoin-git|13bitcoin/06master 14fd5654c 15Pavel Janík: Check and enable -Wshadow by default.
107 2016-11-09 13:12:44	0|bitcoin-git|13bitcoin/06master 14e0477f6 15Wladimir J. van der Laan: Merge #8794: Enable -Wshadow by default...
108 2016-11-09 13:12:53	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #8794: Enable -Wshadow by default (06master...0620160922_Wshadow_enable) 02https://github.com/bitcoin/bitcoin/pull/8794
109 2016-11-09 13:43:45	0|jonasschnelli|cfields: net.h what's the advantage of using a Callable (ForEachNodeContinueIf(Callable&& func)) instead of just passing in a std::function?
110 2016-11-09 13:51:37	0|cfields|jonasschnelli: so that lambdas are overhead-free
111 2016-11-09 13:55:19	0|jonasschnelli|cfields: can you explain me what kind of overhead? (Sorry to bother you with basic c++)
112 2016-11-09 13:57:14	0|cfields|jonasschnelli: np. std::function necessarily does an allocation, is copyable, etc. But lambdas can essentially create inline code.
113 2016-11-09 13:58:24	0|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.
114 2016-11-09 13:59:10	0|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)
115 2016-11-09 14:00:33	0|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?
116 2016-11-09 14:01:55	0|cfields|jonasschnelli: yes. a std::function is constructed there.
117 2016-11-09 14:02:49	0|jonasschnelli|Okay. So changing it to a callable would be more efficient.. right?
118 2016-11-09 14:02:57	0|jonasschnelli|(reducing a copy)
119 2016-11-09 14:03:53	0|cfields|jonasschnelli: yep, assuming you can neatly use a template in the definition
120 2016-11-09 14:06:37	0|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
121 2016-11-09 14:07:14	0|jonasschnelli|cfields: Thanks. I think I see you point now.
122 2016-11-09 14:07:21	0|cfields|but if you used "template <typename T> Print(T&& s) { std::cout << s; }, you can pass in a raw string for free
123 2016-11-09 14:19:30	0|fanquake|paveljanik I'm probably wrong, but the hex/line numbers was the first thing that jumped out at me.
124 2016-11-09 14:20:01	0|paveljanik|fanquake, sure. But this is probably some issue on the builder side as it does not touch leveldb at all...
125 2016-11-09 14:24:51	0|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/
126 2016-11-09 14:46:16	0|cfields|MarcoFalke: yes, i assumed that would cause a few crashes for the first few days
127 2016-11-09 14:47:54	0|cfields|MarcoFalke: can you run with -debug for a while?
128 2016-11-09 14:48:06	0|MarcoFalke|sure
129 2016-11-09 14:49:05	0|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.
130 2016-11-09 16:23:23	0|MarcoFalke|cfields: Prob when expected != offered. See debug.log: https://paste.fedoraproject.org/476493/87085851/
131 2016-11-09 16:23:36	0|MarcoFalke|or bt https://paste.fedoraproject.org/476491/08356147/
132 2016-11-09 16:23:52	0|cfields|MarcoFalke: perfect, thanks
133 2016-11-09 16:24:42	0|cfields|MarcoFalke: hmm, seems to be in sending the reject...
134 2016-11-09 16:25:15	0|MarcoFalke|And what is the feefilter in the bt?
135 2016-11-09 16:26:21	0|cfields|MarcoFalke: aha! I didn't see the bt
136 2016-11-09 16:27:08	0|cfields|MarcoFalke: yep, that's the problem.
137 2016-11-09 16:28:27	0|cfields|MarcoFalke: I'll pr a quick/dirty fix in a few min, and I'm working on a more robust solution
138 2016-11-09 16:29:11	0|cfields|MarcoFalke: quick fix: http://pastebin.com/raw/E8ZXsKYa
139 2016-11-09 16:29:21	0|MarcoFalke|that was quick :)
140 2016-11-09 16:30:54	0|cfields|MarcoFalke: well like i said, I expected it to point out a few issues. They should all be very easy to fix individually.
141 2016-11-09 16:31:09	0|cfields|(as long as there's a nice log/backtrace, that is :)
142 2016-11-09 18:19:51	0|bitcoin-git|[13bitcoin] 15theuni opened pull request #9117: net: don't send feefilter messages before the version handshake is complete (06master...06feefilter-assert) 02https://github.com/bitcoin/bitcoin/pull/9117
143 2016-11-09 18:23:51	0|sipa|jonasschnelli: i was planning to work on bip151, but i'm not going to touch anything before the ongoing net refactors are done :)
144 2016-11-09 18:26:31	0|gmaxwell|cfields: if the issue in 9117 is feefilter before handshake, should it not be checking fSuccessfullyConnected?
145 2016-11-09 18:27:03	0|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
146 2016-11-09 18:30:44	0|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
147 2016-11-09 18:31:08	0|MarcoFalke|gmaxwell: I think the handshake was done in the case I saw above: https://paste.fedoraproject.org/476493/87085851/raw/
148 2016-11-09 18:31:12	0|cfields|er, at any time in the session after the handshake is done
149 2016-11-09 18:31:50	0|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
150 2016-11-09 18:32:48	0|gmaxwell|but it wouldn't catch cases where we weren't done but also weren't planning on disconnecting them?
151 2016-11-09 18:34:35	0|cfields|gmaxwell: i don't think that's a possibility
152 2016-11-09 18:36:57	0|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?
153 2016-11-09 18:37:56	0|gmaxwell|I don't really think we should be sending any messages after flipping that flag.
154 2016-11-09 18:43:24	0|cfields|hmm, may be able to make that work
155 2016-11-09 18:53:20	0|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)
156 2016-11-09 18:54:02	0|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
157 2016-11-09 18:55:38	0|sipa|morcos: hmm, IBD should probably not be true during LoadMempool
158 2016-11-09 19:53:09	0|gmaxwell|the build is now a _flood_ ofr shadowing warnings for me.
159 2016-11-09 19:53:25	0|gmaxwell|thousands of them.
160 2016-11-09 19:53:57	0|paveljanik|gmaxwell, what compiler? Can you upload the log please?
161 2016-11-09 19:54:05	0|gmaxwell|gcc version 6.1.1 20160802 (Debian 6.1.1-11)
162 2016-11-09 19:54:07	0|paveljanik|old gcc?
163 2016-11-09 19:54:27	0|sipa|that's a very new gcc
164 2016-11-09 19:54:30	0|sipa|i also get those warning
165 2016-11-09 19:55:21	0|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
166 2016-11-09 19:56:20	0|paveljanik|can you please upload the log somewhere?
167 2016-11-09 19:57:11	0|paveljanik|There is #8808 but maybe we need even more for new gcc
168 2016-11-09 19:57:13	0|gribble|https://github.com/bitcoin/bitcoin/issues/8808 | Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin · GitHub
169 2016-11-09 19:58:38	0|gmaxwell|paveljanik: http://0bin.net/paste/C-XT2Ww24Gb0-oSk#gTvVzT2VMvRmjuKhVgAP25zWIpGHhJUbOKamb4KKFi-
170 2016-11-09 20:00:33	0|paveljanik|fFlushOnClose is fixed there
171 2016-11-09 20:01:28	0|paveljanik|gmaxwell, looks like you are good candidate for ACK on #8808 8)
172 2016-11-09 20:01:30	0|gribble|https://github.com/bitcoin/bitcoin/issues/8808 | Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin · GitHub
173 2016-11-09 20:01:43	0|jonasschnelli|sipa: Okay. That's great news. I can re-focus then on the SPV mode.
174 2016-11-09 20:04:00	0|gmaxwell|Where did we turn on these Wshadow warnings?
175 2016-11-09 20:06:18	0|gmaxwell|oh, I'd already pulled it off while bisecting. 8794
176 2016-11-09 20:15:14	0|paveljanik|yes, but 8808 is missing.
177 2016-11-09 20:15:32	0|paveljanik|and I'm now testing with a bit older gcc than yours...
178 2016-11-09 20:16:02	0|sipa|i can test with 6.2.0
179 2016-11-09 20:16:26	0|paveljanik|great! I will install the VM with some newer one later
180 2016-11-09 20:20:34	0|phantomcircuit|gmaxwell: yeah we shadow things all over the place
181 2016-11-09 20:21:47	0|sipa|let's fix it
182 2016-11-09 20:25:46	0|wumpus|strange, I had no shadow warnings at all before I merged that
183 2016-11-09 20:26:09	0|wumpus|or maybe one in leveldb or so, but certainly no flood
184 2016-11-09 20:26:12	0|wumpus|I don't get it
185 2016-11-09 20:26:14	0|wumpus|going to revert
186 2016-11-09 20:27:23	0|wumpus|maybe we shouldn't do it at all, it's too compiler-dependent
187 2016-11-09 20:28:08	0|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
188 2016-11-09 20:28:36	0|sipa|and there is a clear upper bound on what -Wshadow can mean... i'm a bit surprised there even is compiler dependence here
189 2016-11-09 20:28:44	0|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!
190 2016-11-09 20:29:06	0|MarcoFalke|I could check #8808 with 6.2.1
191 2016-11-09 20:29:07	0|gribble|https://github.com/bitcoin/bitcoin/issues/8808 | Do not shadow variables (gcc set) by paveljanik · Pull Request #8808 · bitcoin/bitcoin · GitHub
192 2016-11-09 20:29:15	0|gmaxwell|(by all I mean almost all, of course, obviously I don't care about one in leveldb)
193 2016-11-09 20:29:16	0|wumpus|I'm not going to bother with it anymore, at least
194 2016-11-09 20:29:38	0|wumpus|not just me *multiple people* tested it here: https://github.com/bitcoin/bitcoin/pull/8794
195 2016-11-09 20:29:40	0|bitcoin-git|13bitcoin/06master 14f445d88 15Wladimir J. van der Laan: Revert "Check and enable -Wshadow by default."...
196 2016-11-09 20:29:40	0|bitcoin-git|[13bitcoin] 15laanwj pushed 1 new commit to 06master: 02https://github.com/bitcoin/bitcoin/commit/f445d886126c00814c71b855fc2f36fdd7e11098
197 2016-11-09 20:30:14	0|wumpus|seems just needless busywork, how many 'prevent shadowing' commits have we had and it's still a problem
198 2016-11-09 20:31:55	0|gmaxwell|Coding convention wise, I notice that 8808 adds _ to many local variables.  I think there moderately common convention for using _ on function parameters.
199 2016-11-09 20:33:21	0|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.
200 2016-11-09 20:33:42	0|wumpus|but those were supposed to be fixed already
201 2016-11-09 20:33:47	0|wumpus|where are you getting all these warnings?
202 2016-11-09 20:34:13	0|wumpus|can you maybe pastebin and post in that issue?
203 2016-11-09 20:34:24	0|gmaxwell|I pastebinned the warnings above: http://0bin.net/paste/C-XT2Ww24Gb0-oSk#gTvVzT2VMvRmjuKhVgAP25zWIpGHhJUbOKamb4KKFi-
204 2016-11-09 20:34:26	0|wumpus|maybe paveljanik would like to know
205 2016-11-09 20:34:38	0|gmaxwell|Glad to help squash them all.
206 2016-11-09 20:34:59	0|wumpus|jdid't we get rid of nVersion?
207 2016-11-09 20:35:11	0|sipa|of the serialize.h-based nVersion
208 2016-11-09 20:35:22	0|sipa|but there are still some nVersion variables scattered
209 2016-11-09 20:35:33	0|wumpus|nah, I think there's more important things
210 2016-11-09 20:35:46	0|wumpus|I thought this was done, but if there's another load of that bullshit
211 2016-11-09 20:36:10	0|gmaxwell|I really would like to understand how compliler differences came into play.
212 2016-11-09 20:37:54	0|wumpus|probably they consider slightly different scopes
213 2016-11-09 20:42:34	0|paveljanik|I first done Wshadow clean build on OS X/clang
214 2016-11-09 20:42:39	0|paveljanik|then tested on newer clang
215 2016-11-09 20:42:46	0|paveljanik|than newer clang on Linux
216 2016-11-09 20:42:53	0|paveljanik|then old gcc on Linux.
217 2016-11-09 20:43:14	0|paveljanik|I'm now Wshadow clean on all clangs available to me and one gcc version (after 8808).
218 2016-11-09 20:43:36	0|paveljanik|There are millions of differences in them emitting warnings.
219 2016-11-09 20:44:02	0|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
220 2016-11-09 20:44:04	0|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)
221 2016-11-09 20:44:41	0|paveljanik|exactly!
222 2016-11-09 20:45:26	0|wumpus|haven't tested gcc 6.x, maybe that is uselessly agressive
223 2016-11-09 20:47:10	0|wumpus|http://stackoverflow.com/questions/2958457/gcc-wshadow-is-too-strict hah
224 2016-11-09 20:55:26	0|bitcoin-git|13bitcoin/06master 14d8edf03 15fanquake: Remove unused var UNLIKELY_PCT from fees.h
225 2016-11-09 20:55:26	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/f445d886126c...fb156100f9b4
226 2016-11-09 20:55:27	0|bitcoin-git|13bitcoin/06master 14fb15610 15Wladimir J. van der Laan: Merge #9111: Remove unused variable UNLIKELY_PCT from fees.h...
227 2016-11-09 20:55:37	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #9111: Remove unused variable UNLIKELY_PCT from fees.h (06master...06remove-unused-unlikely-pct) 02https://github.com/bitcoin/bitcoin/pull/9111
228 2016-11-09 21:07:26	0|bitcoin-git|[13bitcoin] 15laanwj pushed 2 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/fb156100f9b4...faec09bc7f03
229 2016-11-09 21:07:27	0|bitcoin-git|13bitcoin/06master 14e5d682f 15jnewbery: Fix mininode version message format
230 2016-11-09 21:07:28	0|bitcoin-git|13bitcoin/06master 14faec09b 15Wladimir J. van der Laan: Merge #8894: [Testing] Include fRelay in mininode version messages...
231 2016-11-09 21:07:36	0|bitcoin-git|[13bitcoin] 15dooglus opened pull request #9118: Remove requireGreater argment from TxConfirmStats::EstimateMedianVal() (06master...06remove-dead-code) 02https://github.com/bitcoin/bitcoin/pull/9118
232 2016-11-09 21:12:11	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #9048: [0.13 backport] Fix handling of invalid compact blocks (060.13...06fix-invalid-cb-ban-0.13) 02https://github.com/bitcoin/bitcoin/pull/9048
233 2016-11-09 21:12:31	0|morcos|wumpus: is it critical to be doing all this clean up on fee estimation right now?
234 2016-11-09 21:12:46	0|morcos|i'd at least like a chance to think about it before you merge all these changes
235 2016-11-09 21:12:57	0|morcos|i understand those variables are unused now
236 2016-11-09 21:13:18	0|morcos|but they may still be useful functionality for further extension of the fee estimation code
237 2016-11-09 21:14:40	0|wumpus|morcos: no, imo it's not necessary, but I haven't heared you complain about it before
238 2016-11-09 21:14:47	0|wumpus|you could just NACK those pulls
239 2016-11-09 21:15:04	0|wumpus|if no one complains then dead code cleanups seems like a no-brainer
240 2016-11-09 21:15:10	0|morcos|i guess i just panicked b/c i saw one of them got merged before i even saw it
241 2016-11-09 21:15:15	0|morcos|within a 12 hours
242 2016-11-09 21:15:21	0|morcos|but that one actually was trivial
243 2016-11-09 21:15:22	0|wumpus|that was just a one-liner
244 2016-11-09 21:15:23	0|morcos|i will comment on the other
245 2016-11-09 21:15:36	0|morcos|i hadn't actually looked at the changes on either of them yet
246 2016-11-09 21:15:53	0|wumpus|also our usual policy is to remove dead code, we're using git to make it easy to bring back things if necessary
247 2016-11-09 21:15:59	0|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
248 2016-11-09 21:16:08	0|morcos|i'd rather leave it until we're sure we won't want it again
249 2016-11-09 21:16:20	0|wumpus|I'll just stop merging things now, only caused problems today I feel :/
250 2016-11-09 21:18:55	0|wumpus|just too many pulls...
251 2016-11-09 21:22:21	0|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.
252 2016-11-09 21:25:09	0|wumpus|I also have a harder and harder time keeping up
253 2016-11-09 21:26:21	0|morcos|i suppose its a good problem to have
254 2016-11-09 21:26:48	0|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?
255 2016-11-09 21:27:19	0|gmaxwell|I was rebasing a branch from a distant version when I noticed the warngings, so my tree had been effectively make cleaned.
256 2016-11-09 21:27:21	0|bitcoin-git|[13bitcoin] 15laanwj closed pull request #9118: Remove requireGreater argment from TxConfirmStats::EstimateMedianVal() (06master...06remove-dead-code) 02https://github.com/bitcoin/bitcoin/pull/9118
257 2016-11-09 21:27:30	0|wumpus|gmaxwell: no, I cleaned both the tree and the ccache, for both compilers
258 2016-11-09 21:28:08	0|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
259 2016-11-09 21:31:02	0|paveljanik|gmaxwell, can you please try http://tmp.janik.cz/q.diff?
260 2016-11-09 21:31:15	0|paveljanik|it should clear the nVersion from chain.h and others.
261 2016-11-09 21:33:02	0|gmaxwell|paveljanik: the only ones remaining with that are leveldb and bench/lockedpool.cpp
262 2016-11-09 21:33:16	0|paveljanik|gmaxwell, great!
263 2016-11-09 21:33:41	0|paveljanik|bench/lockedpool is clear IIRC.
264 2016-11-09 21:33:47	0|paveljanik|and leveldb is upstream 8)
265 2016-11-09 21:33:50	0|paveljanik|so fixed? ;-)
266 2016-11-09 21:34:07	0|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.
267 2016-11-09 21:34:09	0|paveljanik|but I'll first retest on some rolling upstream Linux distro
268 2016-11-09 21:34:35	0|paveljanik|we used that even before and people agreed on it.
269 2016-11-09 21:34:54	0|paveljanik|some parts of the code use _, some _in, some In postfix..
270 2016-11-09 21:35:15	0|gmaxwell|Were we really using it for _local_ variables before, not just function parameters?
271 2016-11-09 21:35:22	0|paveljanik|yes
272 2016-11-09 21:35:35	0|paveljanik|I was inspired by it.
273 2016-11-09 21:36:17	0|paveljanik|but sometimes, it was better to rename the variables...
274 2016-11-09 21:36:23	0|paveljanik|and thus I did so
275 2016-11-09 21:36:38	0|paveljanik|e.g. data -> vData to follow the typeName conventions used
276 2016-11-09 21:37:51	0|paveljanik|I understand it is not sexy to read all those diffs though 8)
277 2016-11-09 21:38:02	0|paveljanik|but please be patient
278 2016-11-09 21:38:54	0|gmaxwell|paveljanik: for example, in 8808  src/streams.h
279 2016-11-09 21:39:38	0|paveljanik|yes, read -> _read.
280 2016-11-09 21:39:45	0|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.
281 2016-11-09 21:40:38	0|paveljanik|no problem, just comment there and I will change it ;-)
282 2016-11-09 21:41:20	0|gmaxwell|When you make these updates are you checking to make sure the existing code wasn't buggy?
283 2016-11-09 21:41:50	0|paveljanik|sometimes. I even found some bugs, yes.
284 2016-11-09 21:42:24	0|gmaxwell|paveljanik: Good. I'll gladly leave some comments.
285 2016-11-09 22:05:12	0|bitcoin-git|[13bitcoin] 15UdjinM6 opened pull request #9120: bug: Missed one "return false" in recent refactoring in #9067 (06master...06fixExitCodesBitcoinTx) 02https://github.com/bitcoin/bitcoin/pull/9120
286 2016-11-09 22:07:17	0|paveljanik|gmaxwell, thank you!
287 2016-11-09 22:26:45	0|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
288 2016-11-09 22:26:47	0|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
289 2016-11-09 22:27:21	0|morcos|jtimon: not for me right?
290 2016-11-09 22:43:18	0|jtimon|reminder: I only get it by running all rpc tests, not running walletbackupt individually
291 2016-11-09 22:43:49	0|jtimon|you told me 9058 may help the other day, but it seems is unrelated :(
292 2016-11-09 22:46:03	0|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
293 2016-11-09 22:51:01	0|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?
294 2016-11-09 22:56:19	0|jtimon|re warnings you can still fix some shadowing even if you don't put the new flag yet
295 2016-11-09 23:45:20	0|jtimon|I'm starting to think it may not be just be but actually something between 1adf82ac and faec09bc
296 2016-11-09 23:53:35	0|jtimon|s/just be/just me/