< tryphe>
particularly 'have' and 'NUM_OS_RANDOM_BYTES' are int
< tryphe>
is this intended for some reason, or a bug?
< sipa>
it's a constant, it doesn't matter
< tryphe>
but why?
< tryphe>
size_t is unsigned, int is signed
< sipa>
32 is 32
< tryphe>
right but it doesn't really answer my question
< gmaxwell>
tryphe: use of an unsigned value would potentially risk random unsigned promotions elsewhere.
< gmaxwell>
Which is a common source of gnarly bugs.
< gmaxwell>
32 fortunately fits in every type (except bool. :P )
< tryphe>
gmaxwell, makes sense, it's that i would think doing that kind of thing might cause platform issues down the road, maybe similar to the way "word" became ambiguous
< luke-jr>
tryphe: I don't think C++ allows it to be ambiguous
< sipa>
tryphe: thabkfully we have a lot of control over supported platforms; right now, the code already relies on int being at least 32 bits iirc
< tryphe>
luke-jr, sipa, those are good points, i guess it just makes me think how ints were 16 bits on 16-bit processors and how over time there were problems because interpretation became loose
< luke-jr>
tryphe: int is guaranteed to be at least 16-bit
< tryphe>
what about using an already defined fixed-width type like int32_t?
< tryphe>
would it be preferred over int?
< luke-jr>
it really depends on what the code requires
< luke-jr>
if it's just counting from 1 to 3, int is fine
< luke-jr>
if it's iterating over an array, it should use size_t
< luke-jr>
etc
< luke-jr>
fixed-width types should only be used when there is a need for an exact width
< tryphe>
but if you know it's 32 bits, then that's why C++11 standard made fixed-width types, for that exact reaason
< tryphe>
it just seems strange that you would use int to me, that's all
< luke-jr>
`int` is the preferred integer type when 16-bits is sufficient
< luke-jr>
the compiler will choose whatever width is best for the target platform
< luke-jr>
eg, it will use 32-bit if 16-bit would entail extra overflow checks/handling
< tryphe>
i thought you guys only supported platforms where it was 32 bits, though
< sipa>
yes
< tryphe>
luke-jr, but from what sipa said, it can only be 32, hence the confusion
< tryphe>
so it can be 16 sometimes?
< sipa>
tryphe: C++ only guarantees that ints are at least 16 bits
< sipa>
bitcoin core only support platforms where it is 32
< tryphe>
gotcha
< tryphe>
i still don't see why you would want to use int, seems like a future rat's nest of problems
< sipa>
really, it is not a big deal
< sipa>
int is big enough for the constant 32, hence, it is enough
< tryphe>
i see why you would want to use that for promotion to an unsigned type, but i'm just thinking hypothetically i guess :p
< tryphe>
maybe some guy in 100 years compiles an old bitcoin build on the new C++50 standard where ints are 128 bytes, or something, lol
< tryphe>
bits even
< luke-jr>
and it will still work
< luke-jr>
because 32 is still 32 in 128-bit
< tryphe>
yeah, you could even safely promote it to size_t_128 or something i guess :p
< tryphe>
thanks, that makes sense
< tryphe>
wasn't trying to nag, just didn't understand it
< tryphe>
i'm thankful for bitcoin because watching the development process helps amatuers like me not have such terrible C++ habits :o
< jb55>
gcc bugs... spooky
< gwillen>
tryphe: one interesting note about int, is that it is the smallest type for which C/C++ will not perform integer promotion behind your back unless the arguments have different types
< gwillen>
if you're smaller than int, you get promoted to int no matter what
< gwillen>
once you're at int, you get left alone unless you need a promotion to make the operation possible
< tryphe>
gwillen, neat, thanks!
< tryphe>
gwillen, i guess i had noticed that before, but never had a real explanation for it.
< bitcoin-git>
bitcoin/master 2ee811e João Barbosa: wallet: Track scanning duration
< bitcoin-git>
bitcoin/master 90e27ab João Barbosa: wallet: Track current scanning progress
< bitcoin-git>
bitcoin/master d3e8458 João Barbosa: rpc: Show scanning details in getwalletinfo
< bitcoin-git>
[bitcoin] laanwj merged pull request #15730: rpc: Show scanning details in getwalletinfo (master...2019-04-getwalletinfo-scanning) https://github.com/bitcoin/bitcoin/pull/15730
< bitcoin-git>
[bitcoin] meshcollider opened pull request #15957: Show "No wallets available" in open menu instead of nothing (master...201905_openwallet_empty) https://github.com/bitcoin/bitcoin/pull/15957
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #15959: refactor: Work around gcc compiler bug 90348 (master...1905-gccBugWorkaround) https://github.com/bitcoin/bitcoin/pull/15959
< bitcoin-git>
bitcoin/master ba534cc John Newbery: [tests] log thread names by default in functional tests
< bitcoin-git>
bitcoin/master 7b29ec2 John Newbery: [tests] Comment for why logging config is set as command-line args.
< bitcoin-git>
bitcoin/master 8ec7121 MarcoFalke: Merge #15927: [tests] log thread names by default in functional tests
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #15927: [tests] log thread names by default in functional tests (master...2019-04-log-threads-tests) https://github.com/bitcoin/bitcoin/pull/15927
< bitcoin-git>
[bitcoin] jnewbery opened pull request #15963: [tests] Make random seed logged and settable (master...2019-05-test-deterministic-randomness) https://github.com/bitcoin/bitcoin/pull/15963
< tryphe>
if the default maximum open file descriptors on Mac OS 10 is 256 ('ulimit -n' == 256), and the leveldb default is 1000, what would a reasonable leveldb default be so that bitcoind + leveldb won't exceed 256 fds total?
< tryphe>
on one hand, if it goes too low, it could hurt performance, but on the other hand, we don't want it to exceed 256, which seems low for having tons of files around for compaction.
< tryphe>
i guess i should clarify: it's 256 per process, not system-wide, of course.
< gwillen>
tryphe: have you actually hit the limit?
< luke-jr>
is it really? :/
< gwillen>
I'm asking because I'm on OS X and I'm kind of suprised I never have
< gwillen>
luke-jr: yes, it sucks
< luke-jr>
including mmaps?
< gwillen>
I had to change one of the linter tests because it tried to open every file in the tree at once, and it would crash on OS X
< gwillen>
mm, I don't know about mmaps, I wouldn't think so
< gwillen>
maybe that's why I've never hit it?
< tryphe>
^ i was looking at #14870 and realized this was most likely the culprit after running ulimit
< tryphe>
but it's hard to know how many files will need to be read by the compaction mechanism... i doubt it's identical across different configurations and whatnot
< gwillen>
which says (like luke-jr was just asking): "On 64-bit POSIX hosts LevelDB will open up to 1000 file descriptors using mmap(), and it does not retain an open file descriptor for such files."
< tryphe>
gwillen, i didn't hit the limit personally, but my Mac box is mostly just for testing, not for full node running
< gwillen>
so I wonder if something else is actually the culprit
< gwillen>
I regularly run full nodes on my box for testing and real use
< gwillen>
and I've _never_ hit this
< gwillen>
although I guess these days I wouldn't, since I upped my default at some point... if I did that before 12495 was merged, I might be a bad test case
< gwillen>
but I would expect that if the leveldb file count were the culprit, LOTS of people would be affected, and I believe the theory that mmaps should not count
< tryphe>
yeah, i'm not sure. my hunch was that leveldb would approach that limit, and bitcoind opening files could cause the issue to appear, without leveldb being the sole cause (depends how many files bitcoind is also accessing)
< tryphe>
which would make sense i guess, 256 isn't insanely high
< sipa>
tryphe: on 64-bit systems, LevelDB uses mmap which doesn't need many simultaneous file descriptors
< tryphe>
and on linux, it's less than 1024, which is the default, so it does nothing
< gwillen>
right, if leveldb isn't taking up fds then the whole leveldb thing is a red herring and the cause must be something else
< tryphe>
sipa, i see, but what about too many concurrent mmap() calls? assuming you have a bunch of 2MB files at one level, it seems to me like you could potentially still be opening hundreds of handles at once
< tryphe>
sipa, what i mean by that is, the fd isn't closed until mmap returns the mapping
< gmaxwell>
tryphe: yes, but they are created sequentially.
< sipa>
tryphe: i don't think those things happen in separate threads
< tryphe>
i see, thanks. maybe i'm thinking of rocksdb, where it happens in a thread pool.
< gmaxwell>
the comment about RPC in the reporter is a pretty strong indication that they're likely hammering the RPC...
< tryphe>
gmaxwell, yeah, but usually you don't have leveldb throw that error unless something is implemented wrong, it's a common problem
< gmaxwell>
tryphe: the error being reported by the user is not from leveldb.
< gmaxwell>
It's from libevent-- which handles the rpc.
< tryphe>
gmaxwell, look at the first instance
< gmaxwell>
tryphe: it looks to me like they're hammering the rpc and used up all the FDs that way, and leveldb is unable to get a single FD to open a file.
< tryphe>
gmaxwell, 2018-12-04T17:09:42Z leveldb: Compaction error: IO error: /Users/matt/Library/Application Support/Bitcoin/testnet3/indexes/txindex/012589.ldb: Too many open files
< sipa>
maybe we should increasy fds also proportionally to -rpcthreads ?
< gmaxwell>
sipa: yes, I was just looking for where we did that, seems we don't.
< gmaxwell>
we might want to add some instrumentation that runs during tests, and checks to see if the total open fds is ever greater than we pass to RaiseFileDescriptorLimit.
< tryphe>
if it raises the fds to 283 if it's <283, but the default is 1024 on most linux/unix systems (except mac os), would a sane thing to be just to increase the fds to 1024 on mac os? (i think the max per process is 10240)
< gmaxwell>
otherwise we're going to keep ending up with bugs that are invisible when RaiseFileDescriptorLimit does nothing.
< gmaxwell>
tryphe: well that would just be covering up the bug that RaiseFileDescriptorLimit isn't being increased by the rpc thread count.
< tryphe>
gmaxwell, yeah, i'm not even sure how to reproduce it, but if that's one glaring difference, it might make sense to increase it?
< gmaxwell>
tryphe: then the problem will just show up again when users set rpcthreads even higher
< gmaxwell>
if you don't understand an issue, just changing things blindly risks introducing new issues, and not fixing them.
< gmaxwell>
But I think we do understand it-- RaiseFileDescriptorLimit should be getting the rpcthreads added.
< sipa>
but by how much?
< tryphe>
gmaxwell, i don't see how adding in the number of rpcs would do anything if RaiseFileDescriptorLimit() doesn't do anything if my fd limit is 1024 by default
< tryphe>
rpcs/rpc threads
< bitcoin-git>
[bitcoin] zenosage opened pull request #15965: check bad-prevblk is right error for invalid descendants (master...bad-prevblk) https://github.com/bitcoin/bitcoin/pull/15965
< gmaxwell>
tryphe: go look at the code in init.cpp. It works out how many FDs we can actually use.
< sipa>
tryphe: what if you have 2000 rpc threads? :)
< gmaxwell>
Right now it doesn't account for the RPC threads (they're just assumed to be part of MIN_CORE_FILEDESCRIPTORS, but that doesn't hold if someone changes the setting)
< gmaxwell>
it should account for RPC threads.
< gmaxwell>
Which would be the actual bug.
< tryphe>
gmaxwell, i already looked at it before i told you anything
< sipa>
tryphe: i think you're missing something, but i don't see what
< tryphe>
i see why you would want to add the number of rpc threads, but not why you would want different fd max values on different systems
< sipa>
what are you talking about with "different fd max values on different systems" ?
< gmaxwell>
sipa: he's referring to the fact that we don't raise it if it's already more than we need.
< tryphe>
sipa, on mac os 10, if 'limit -n' gives 256, what would RaiseFileDescriptorLimit() set the value to?
< tryphe>
sipa, likewise, on linux, what would it do?
< gmaxwell>
sipa: and on OSX the default is lower than we need (with defaults), but on linux it's higher.
< sipa>
it tries to raise it if needed
< tryphe>
the difference is a value of ~700
< sipa>
if it's not needed, there is nothing to be done
< sipa>
yes, so the number needed is miscomputed, and we should fix that
< gmaxwell>
tryphe: The limit is compltely irrelevant so long as it is greater than our need. If our caclulated need is incorrect, then we will have problems in some configurations.
< sipa>
that fact that it's already 1024 only helps mask the problem
< gmaxwell>
If we set the limit higher than we think we need, it won't fix any bugs-- the program will still fail in some configurations-- but it will make some bugs less visible.
< gmaxwell>
So instead of failing on mac when you adjust rpc threads at all, it'll still fail, but everwhere when you set rpcthreads to 700. (or whatever)
< tryphe>
right but <300 seems pretty low, especially if they are running bitcoin-qt which accesses a bunch of files, not to mention future features
< sipa>
tryphe: so we should fix the computation that incorrectly determines 300 is enough
< gmaxwell>
Any new features that use FDs need to increase the calculated limit.
< sipa>
i don't understand what you're arguing for
< tryphe>
sipa, because it's a glaring difference, and you're blowing it off like it's nothing
< gmaxwell>
No fixed amount is safe, only computing the required amount is safe.
< sipa>
tryphe: no...
< sipa>
i'm not
< tryphe>
for every 1 github issue there's 1000 morons that ignored it and stopped using it
< sipa>
i
< tryphe>
for the same reason
< gwillen>
tryphe: we are statically computing the exact number of file descriptors we need, and raising the limit to that exact value, which we will never exceed, so there is never a reason for it to be higher
< gwillen>
in this case, there is a bug, which is that we compute the wrong number
< gwillen>
once the bug is fixed, there will again be no reason to ever raise the number
< gmaxwell>
tryphe: I think we're failing to communicate here. I think we should fix the problem, which is now known in part thanks to your exploration. We should not, however, paper over misbehavior as that can create much more serious issues.
< gwillen>
it sounds like you believe that Core will sometimes open other file descriptors not accounted for here, but that is not supposed to ever happen, and would represent a bug
< gmaxwell>
Fixing the calculation will fix the bug. Blindly increasing the value on OSX will paper over the bug and any similar bug in the future but not actually fix it.
< bitcoin-git>
[bitcoin] hebasto opened pull request #15966: net: Drop the ancient miniUPnPc API version support (master...20190506-drop-ancient-miniupnpc-api) https://github.com/bitcoin/bitcoin/pull/15966
< gmaxwell>
what gwillen says. All FDs _must_ be accounted for.
< gwillen>
I think this is unusual, and it's much more typical for people to just give a high number and hope for the best
< tryphe>
sipa, gmaxwell, gwillen, i see. that makes sense.
< gwillen>
cool :-)
< sipa>
yeah, we try to account for all resource usage
< gmaxwell>
(it's a little less crtical now that we don't use select on linux, but they still need to be accounted for)
< sipa>
disk, memory, cpu, fds
< gwillen>
could we actually _lower_ it from 1024 on linux, instead of leaving it alone?
< gwillen>
that would make this kind of thing show up much faster
< gwillen>
rather than only happening on oddball os x
< sipa>
yeah, perhaps
< gwillen>
and it would satisfy tryphe's very reasonable desire that it be the same across platforms, which I think is a good thought :-)
< gmaxwell>
we should at least in some test builds.
< tryphe>
i do think it's sort of a hinderance that mac os is somehow bundled in with other "linux/unix" compile targets and has some funny differences, though.
< gwillen>
yeah
< gwillen>
as one of the few people actively developing on OS X, I agree ;-)
< tryphe>
but yeah i think raising it is silly especially with mmap
< gmaxwell>
like make one of the debug builds reduce fd limit to the computed amount.
< tryphe>
i didn't intend to suggest raising it as a key argument, just nitpicking differences between systems i guess
< gmaxwell>
difference between systems are good though-- without this one we wouldn't have found this issue until some other change increased usage to the point where it was making 1024 limit get busted.
< gmaxwell>
which might have happened across the whole network at once
< gwillen>
heh, that's an interesting point
< tryphe>
yeah, i would think lowering it and dynamically setting it across all platforms would make sense, as you only really need to increase it if you are running some insane file server or something
< tryphe>
and that's when you -need- overkill
< tryphe>
here, you know what you need
< sipa>
what does file server have to do with it?
< gwillen>
I raised it in my .profile, I should maybe take that out :-\
< gwillen>
since it masks any problem like this
< sipa>
this limit is per process, not system wide
< gwillen>
I did it so I could get the tests to run, but then I fixed the tests
< tryphe>
sipa, it just as an example of extremes where people blindly raise the limit to 10000 or whatever :p
< tryphe>
sipa, but in those cases they are maybe succombing to future DoS problems and whatnot
< tryphe>
gmaxwell, do you think i should give it a test with a lower MIN_CORE_FILEDESCRIPTORS value, and add the rpc threads in? maybe use 0 for MIN_CORE_FILEDESCRIPTORS instead of 150
< sipa>
We certainly need a few FDs independent of RPC handling (including those for accessing block files, undo files, ...)
< tryphe>
right, i'll give it a try! thanks
< sipa>
i also don't know how many FDs we need per RPC, and it's hard to experimentally determine the upper bound as it depends on the type of RPCs
< sipa>
but there is at least 1 for the TCP connection itself (except on windows, where sockets don't count as FDs...), and maybe 1 or 2 more for files that may be accessed for handling the RPC
< tryphe>
i see, because an rpc might trigger a file to be read?
< sipa>
yes
< bitcoin-git>
[bitcoin] practicalswift closed pull request #15962: Add locking annotations for RewindBlockIndex and GetNetworkHashPS. Add missing locks. (master...RewindBlockIndex) https://github.com/bitcoin/bitcoin/pull/15962
< tryphe>
sipa, i wonder if an "rpc thread fd factor" could be used, perhaps to replace MIN_CORE_FILEDESCRIPTORS which is a value of 150. 20 fds per thread *8 threads=160, so you'd get 160, which is roughly the same, but a little more
< sipa>
tryphe: we still have a number of fds independent of RPCs
< sipa>
probably the majority
< tryphe>
sipa, right, that's true
< tryphe>
good point
< jamesob>
anyone think it'd be a good idea to prefix class member function calls with `this->` for clarity?
< luke-jr>
jamesob: no, that's why they're supposed to be named m_*
< jamesob>
that only applies to class *data* members afaict
< luke-jr>
oh, methods
< luke-jr>
`this->` seems ugly, but I don't know that I have a better solution, and I agree it can at times be unclear
< jamesob>
luke-jr: agreed
< sipa>
jamesob: the explicit way to do that is writing Classname::Method(...)
< sipa>
but that's also ugly
< tryphe>
i think it makes a little sense if you consider how you destruct this->~Class();
< tryphe>
but maybe adds clutter
< luke-jr>
what?
< luke-jr>
you destruct with delete..
< jamesob>
sipa: yeah that's wordy
< * luke-jr>
wonders why `this` is a pointer and not a reference anyway
< sipa>
Because "this" was introduced into C++ (really into C with Classes) before references were added. Also, I chose "this" to follow Simula usage, rather than the (later) Smalltalk use of "self".
< tryphe>
luke-jr, they are identical, but you can use delete to destruct an array
< sipa>
tryphe: no, you need delete[] to destruct an array
< sipa>
and invoking a destructor does not release its memory
< sipa>
it only destroys the object allocated in it
< tryphe>
sipa, that's what i said
< sipa>
1) "but you can use delete to destruct an array", that's wrong, you need to use delete[]
< tryphe>
sipa, yeah.... i meant delete or delete[]
< sipa>
2) "they are identical", that's wrong; deleting an object invokes the relevant destructors as needed, but actually cleans up allocated memory
< tryphe>
just 'delete' as the keyword
< * luke-jr>
wonders if there's ever a case where calling this->~class directly actually makes sense. :x
< sipa>
luke-jr: if you're constructing objects objects in self-allocated space
< tryphe>
sipa, luke-jr, but i zero out the memory in the destructor. that's what it's for...
< sipa>
tryphe: the destructor is invoked automatically when destroying the object (when it goes out of scope, when delete is called, ...); you usually don't need to invoke it yourself
< luke-jr>
sipa: hmm, I figured since new can be used with preallocated space, so could delete, but I guess not
< sipa>
the only counterexample i know is when you're writing your own container classes that manually manage memory
< bitcoin-git>
[bitcoin] hebasto closed pull request #15966: net: Drop the ancient miniUPnPc API version support (master...20190506-drop-ancient-miniupnpc-api) https://github.com/bitcoin/bitcoin/pull/15966
< tryphe>
sipa, right, it's just hard to explain how i'm using it, it's a thread cleanup at the end of an application. if i invoke the destructor inside of itself, i don't have to call 'delete' from main() and the pointer gets popped off of the stack when main unwinds
< tryphe>
sipa, i guess i wrote "this->Class()" because i have "delete" all over the place and wanted to mark "i'm exiting the thread here"
< tryphe>
well, thread manager*
< sipa>
ah, this is starting to get off topic :)
< tryphe>
sipa, lol, sorry <3
< jeremyrubin>
I happened to be reviewing VerifyScript, and wanted to check that it is the intended behavior that if CastToBool(witnessprogram) == false that validation fails?
< jeremyrubin>
I just couldn't find it in the documentation (practically speaking it doesn't exclude very many scripts...)
< sipa>
jeremyrubin: yes
< sipa>
"not very many" is really "practically none", as the witness program is a hash
< jeremyrubin>
Yep it's fine, just wanted to clarify as it's not documented that it is intentional.
< jeremyrubin>
The same sort of thing is true for P2SH
< jeremyrubin>
My reason for looking at it is refactoring the VerifyScript code so that I understand it better.