< 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
< 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
< 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 50.254.73.145
< 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
< 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
< 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] 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.
< 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)
< 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...
< 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.
< 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
< 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)
< 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!
< 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?
< 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.
< 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
< 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