< luke-jr>
it seems that my system univalue returns a temporary, and for some reason Core is including that instead of the embedded univalue :|
< praxeology>
Not sure how "garbage collection" works with std::string. Does that line make a string array on the stack or is it allocated in heap?
< sipa>
praxeology: c++ has no garbage collection
< sipa>
when the variable goes out of scope, it is destroyed
< luke-jr>
what is supposed to add src/univalue/include/ to the include path? it's not there..
< sipa>
praxeology: and strings are always allocated on the heap (in practice), but that's not relevant
< sipa>
praxeology: there is no refcounting for references
< sipa>
some c++ standard libraries implement refcounting for strings internally, but that still only works with different _string_ objects with the same content, not multiple references to the same one
< luke-jr>
oh, system univalue is used by default, as expected, ok
< luke-jr>
so I guess options are: 1) require newer univalue to use system, or 2) rewrite this code to work either way
< luke-jr>
it's probably slightly optimised to do num 2
< luke-jr>
is it valid to assign the return std::string temporary to a reference? or is G++ just extra tolerant?
< gmaxwell>
'system univalue' dear lord, why would that exist?
< luke-jr>
gmaxwell: because that's the right way to do libraries
< sipa>
luke-jr: it is valid
< sipa>
(i commented on the PR)
< praxeology>
sipa: yea thought so... "it's taking a pointer to a temporary", not sure why you used the word "temporary" there because if allocated on the heap, its not temporary in any way.
< luke-jr>
which PR?
< luke-jr>
praxeology: it's not. if there's no "new", it's not on the heap
< gmaxwell>
luke-jr: if it were actually a library and not some unmaintained code dump that only we use...
< praxeology>
luke-jr: are you sure there is no "new" deeper in the library?
< sipa>
praxeology: 'heap' and 'stack' don't exist in the C++ spec, they're implementation details
< sipa>
praxeology: 'temporary' however exists in the spec
< sipa>
praxeology: a temporary is the result of an expression that returns something by-value, and isn't assigned to a variable
< bitcoin-git>
[bitcoin] luke-jr opened pull request #10341: rpc/wallet: Workaround older UniValue which returns a std::string temporary for get_str (master...rpcwallet_uv_workaround) https://github.com/bitcoin/bitcoin/pull/10341
< sipa>
praxeology: in practice, the std::string is allocated on the stack, but the string object itself allocates the string/length data on the heap
< sipa>
but again, that doesn't matter
< sipa>
the compiler may even optimize part out and not have such objects hit memory at all
< sipa>
what matters is that it's returning an object and not storing it in a variable
< praxeology>
Hm, yea I guess somehow with the automatic out-of scope clearing/freeing/deleting of the actual data... you need to assign the std::string to a stack variable for the duration you want it to exist
< sipa>
no, you don't
< sipa>
that's what i just pointed out
< sipa>
you can take a reference to a temporary, and in that case, the lifetime of the temporary is automatically extended as long as the reference exists
< sipa>
but that's done by static analysis by the compiler, not by reference counting
< praxeology>
its returning an object that becomes part of stack, but in order for that to happen you need to assign the object to a variable that exists in the stack
< sipa>
stop talking about stacks, that doesn't matter
< praxeology>
ok, scope somehow. but that is less clear to me how such works... I guess we don't have to discuss it here, I can look into it myself
< praxeology>
woops sorry I'll leave you guys alone now
< * luke-jr>
puts praxeology on the stack
< dcousens>
sipa: "its modifying rng_state, protected by the lock" --- I suppose, for an RNG, racing memcpy of random data isn't as much an issue? :D haha
< gmaxwell>
it's very much an issue, as it could cause arbritary corruption.
< dcousens>
gmaxwell: would it could corruption outside of rng_state though?
< gmaxwell>
Yes.
< dcousens>
OK, then yes, issue
< gmaxwell>
yea, I was just being stupid and thought that was the copy into the output too.
< dcousens>
gmaxwell: ooi, what could I lookup to understand the implications of that, I wasn't aware it could cause arbitrary corruption
< gmaxwell>
dcousens: in C/C++ the compiler is allowed to assume that it has exclusive access to any variable it writes to in a scope for the duration of it (the technical guarentee is more complicated and I don't fully understand it, but this approximation is enough). So the compiler could do optimizations like temporarily store the stack pointer into memory that it knows it's going to later overwrite. t
< gmaxwell>
hen copy it back out again before overwriting it.
< dcousens>
gmaxwell: answered as I asked
< dcousens>
cheers
< gmaxwell>
Its very rare for it to _in practice_ cause arbritary corruption, but it's not unheared of.. intel's compiler is a little more prone to crazy stunts that expose things like that than GCC is, but presumably future versions of GCC will keep getting smarter until nothing runs. :)
< dcousens>
ha
< jonasschnelli>
<*highlight>[22:47:52] <BlueMatt:#bitcoin-core-dev> jonasschnelli: you might kill me for #10340, sorry :/
< jonasschnelli>
Collecting stats over CScheduler with a interval of 2000ms (TBD), would it still make sense to collect a timestamp per sample of is CScheduler in general precise enough so we can calculate (index-of-sample * 2000ms) == timeoffset?
< rafalcpp>
dcousens: with any UB in c++, anything can happen. In theory a race condition in some debug code, could make your node start mining dogecoin instead
< dcousens>
rafalcpp: haha, I suppose I was simply affirming it was U/B, not just a product of something else I'd missed
< Chris_Stewart_5>
Is there a way to have two local regtest node share the same chain, while having different wallets?
< instagibbs>
Chris_Stewart_5, this is #bitcoin question, but yes
< BlueMatt>
jonasschnelli: ehh, I didnt feel like making needless copies of everything 3 times
< BlueMatt>
but i can do that if you prefer
< jonasschnelli>
BlueMatt: I don't like accessing the wallet's insides from outside of walletmodel. We can make an exception,... but I guess ryanofsky has to do more work in case we want process sep.
< BlueMatt>
well we can merge ryanofsky's pr first and then maybe extend it even further and never expose COutput outside of wallet
< BlueMatt>
ultimately we should move towards ^
< jonasschnelli>
Yes. Thats a good point. For intermediate steps, I think it's fine.
< BlueMatt>
I mean I'm more than happy to not take 10340 yet and do the real fix, as long as we get it in for 0.15
< BlueMatt>
it shouldnt be /too/ hard
< ryanofsky>
my pr that stops using coutput/cwallettx (#10244) is ready, just needs review
< gribble>
https://github.com/bitcoin/bitcoin/issues/10244 | [qt] Add abstraction layer for accessing node and wallet functionality from gui by ryanofsky · Pull Request #10244 · bitcoin/bitcoin · GitHub
< BlueMatt>
sipa: really? per-txout ends with a smaller dbcache? that's (marginally) surprising to me
< sipa>
BlueMatt: likewise
< sipa>
BlueMatt: in my memory the master dbcache usage for a full reindex was around 4GB or so, but apparently i haven't done one in a very long time...
< BlueMatt>
heh
< * BlueMatt>
would be curious to see the results of default-dbcache-on-tmpfs
< BlueMatt>
jtimon: do you feel so inclined to redo #9494 with scripted-diff?
< gribble>
https://github.com/bitcoin/bitcoin/issues/9494 | Introduce an ArgsManager class encapsulating cs_args, mapArgs and mapMultiArgs by jtimon · Pull Request #9494 · bitcoin/bitcoin · GitHub
< BlueMatt>
it would be kinda nice, though I'd understand if you dont want to redo it at this point
< jtimon>
BlueMatt: yeah, for some parts of it it would make a lot of sense. I think I may have broke something in the last edit from what travis said. I will fix that and will s/argsGlobal/gArgs/ and see if I can move some parts to scripte-diff without much effort
< BlueMatt>
sounds good
< BlueMatt>
cool
< jtimon>
BlueMatt: in the meantime #8855 remains very easy to review :p
< BlueMatt>
sipa: sure! RandAddSeed() also memory_cleanse()s, but I dont think thats actually useful
< jonasschnelli>
hmm... CScheduler seems to be innacurate. I've set up scheduler.scheduleEvery(fn,2000) (every 2000ms)... I often get a callback with a delta of 10 seconds between each other...
< BlueMatt>
huh?
< BlueMatt>
thats...not good
< jonasschnelli>
I need to look into it
< BlueMatt>
jonasschnelli: which version? cfields fixed something with sub-second precision not too long ago
< BlueMatt>
but i believe the symptom was high cpu usage
< jonasschnelli>
Hmm.. my Laptop runs pretty quite... i'll investigate
< jonasschnelli>
running in LLDB shoudln't cause such high offsets (i'm not halting the process)¨
< jonasschnelli>
BlueMatt: What if another Scheduler task requires a couple of seconds to complete? (not familiar with the internals of the Scheduler)
< BlueMatt>
sure, that could cause it, but the only thing in scheduler is wallet flush, really
< jonasschnelli>
hmm... I'm not sure if that is the right approach for collecting stats... well,.. at least I have to add a timestamp then to each sample
< BlueMatt>
hmm?
< sipa>
jonasschnelli: you'll have a timestamp on each sample regardless
< jonasschnelli>
sipa: You mean I should have one per sample?
< sipa>
yes
< sipa>
you can't guarantee that you'll always be able to gather any statistic at any given point in time at that accuracy
< * BlueMatt>
has no idea wtf is going on
< jonasschnelli>
Yes. I just removed it when I switch to useing CScheduler.. but I guess your right
< sipa>
BlueMatt: jonas is gather statistics
< BlueMatt>
stats for what?
< sipa>
every 2 seconds
< jonasschnelli>
BlueMatt: my stats collector PR (== mempool graph)
< sipa>
i assume things like memory usage, bandwidth, ...
< BlueMatt>
ohoh, that one
< jonasschnelli>
For now it does only collect mempool data..
< jonasschnelli>
but it would be trivial to expand it for bandwidth, etc.
< jonasschnelli>
sipa: you think it's worth keeping a int64 for the starttime and only uint32 deltas per sample?
< BlueMatt>
well if people want to start seriously using the scheduler we can give it another thread
< sipa>
jonasschnelli: i assume your resolution is never less than a second
< sipa>
jonasschnelli: in that case you can use int32 for the starttime and int16 for the deltas :)
< jonasschnelli>
sipa: I have now different precision levels and I wanted to keep it absolute per sample... but,... I can't remember why I have damned the idea of deltaes between samples..
< jonasschnelli>
Yes. I guess this would work now that I run over the Schedular (rather then collect "on event")
< sipa>
using absolute time and data is certainly easier... you can just delete entries to reduce resolution over time
< sipa>
but uses more memory
< jonasschnelli>
But still strange that sometimes the Scheduler fires the 2000ms interval after 10000ms (its an inaccuracy of x5)
< sipa>
that does not sound normal and should be investigated regardless
< sipa>
you're sure this is not due to lock contention on main or mempool, right?
< jonasschnelli>
Some sample output of a 2000ms scheduler:
< gmaxwell>
hurray, system is back to a healthy state.
< paveljanik>
not so often tested/run code parts are now being tested :-)
< BlueMatt>
gmaxwell: +1
< instagibbs>
what was happening before?
< instagibbs>
I missed all the excitement it seems
< gmaxwell>
instagibbs: nah just for the last couple months we were riding at the minimum mempoolminfee... everything was getting confirmed.
< instagibbs>
RIP sub-sat/byte confirms
< * paveljanik>
going to test dump/load mempool
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10346: Use range-based for loops (C++11) when looping over vector elements (master...range-based-for-loops) https://github.com/bitcoin/bitcoin/pull/10346
< bitcoin-git>
[bitcoin] practicalswift closed pull request #10346: Use range-based for loops (C++11) when looping over vector elements (master...range-based-for-loops) https://github.com/bitcoin/bitcoin/pull/10346
< bitcoin-git>
[bitcoin] practicalswift opened pull request #10347: Use range-based for loops (C++11) when looping over vector elements (master...range-based-for-loops) https://github.com/bitcoin/bitcoin/pull/10347
< phantomcircuit>
gmaxwell, odd
< phantomcircuit>
my bitcoind running patches to accept anything
< phantomcircuit>
is still "mempoolminfee": 0.00000000
< sipa>
"usage": 294825248,
< sipa>
"mempoolminfee": 0.00001801
< phantomcircuit>
"usage": 298733328,
< phantomcircuit>
oh
< phantomcircuit>
it's because i have maxmempool set to like infinity also
< phantomcircuit>
derp
< luke-jr>
lol
< phantomcircuit>
so it's just on the edge
< BlueMatt>
phantomcircuit: yea, well because most of your peers are limiting to 3MB its rare that your mempool ever gets much above 3mb
< BlueMatt>
whether you limit or not
< sipa>
3MB ??
< sipa>
300?
< BlueMatt>
oh, yes, 300
< phantomcircuit>
BlueMatt, i have peers that are before any kind of mempool limiting
< phantomcircuit>
but yeah
< BlueMatt>
well their peers :p
< BlueMatt>
even my 200-connection bitcoind seednode has only 307MB of txn