< 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> const std::string& account_param = request.params[0].get_str();
< 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> const std::string* account = nullptr;
< 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 :/
< gribble> https://github.com/bitcoin/bitcoin/issues/10340 | Add harmless missing cs_wallet lock in qt CoinControlDialog by TheBlueMatt · Pull Request #10340 · bitcoin/bitcoin · GitHub
< * jonasschnelli> looking at 10340
< jonasschnelli> BlueMatt: seems reasonable to me (10340)
< jonasschnelli> BlueMatt: Ah.. know I now why you mentioned " you might kill me". Massive layer violation.
< bitcoin-git> [bitcoin] jtimon closed pull request #10119: Util: Remove ArgsManager wrappers: (master...0.14-args-wrappers) https://github.com/bitcoin/bitcoin/pull/10119
< 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
< bitcoin-git> [bitcoin] jnewbery opened pull request #10342: [tests] Improve mempool_persist test (master...improve_wait_until_test) https://github.com/bitcoin/bitcoin/pull/10342
< 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> oh, heh, missed that
< BlueMatt> k, will review
< bitcoin-git> [bitcoin] TheBlueMatt closed pull request #10340: Add harmless missing cs_wallet lock in qt CoinControlDialog (master...2017-05-fix-mapwallet-zap-runtime) https://github.com/bitcoin/bitcoin/pull/10340
< bitcoin-git> [bitcoin] practicalswift opened pull request #10343: Remove redundant on-the-same-line-repetition of type names (DRY): RepeatedTypeName foo = static_cast<RepeatedTypeName>(bar) (master...auto) https://github.com/bitcoin/bitcoin/pull/10343
< bitcoin-git> [bitcoin] jnewbery opened pull request #10344: [tests] Fix abandonconflict.py intermittency (master...fix_abandon_conflict) https://github.com/bitcoin/bitcoin/pull/10344
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/8855 | Use a proper factory for creating chainparams by jtimon · Pull Request #8855 · bitcoin/bitcoin · GitHub
< sipa> BlueMatt: ha
< sipa> RandAddSeedSleepSanity has no return statement
< luke-jr> sounds very random.
< BlueMatt> oh, well, i never said i had gotten a chance to test it yet
< BlueMatt> sipa: it seems a waste to call GetPerformanceCounter and *not* go ahead and add it to our random state
< sipa> BlueMatt: how about we add that after #10338 ?
< gribble> https://github.com/bitcoin/bitcoin/issues/10338 | Maintain state across GetStrongRandBytes calls by sipa · Pull Request #10338 · bitcoin/bitcoin · GitHub
< BlueMatt> sipa: initially it was built on both, but right now everything is pushed into openssl's prng state, so...meh?
< 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:
< gribble> https://github.com/bitcoin/bitcoin/issues/8 | RPC command to sign text with wallet private key · Issue #8 · bitcoin/bitcoin · GitHub
< jonasschnelli> No,... it does only lock a new cs_stats
< jonasschnelli> The mempool obtain it's own atomic caches in my current working branch
< jonasschnelli> Although I run with the GUI... but shoudln't make any different. It's a calm regtest peer anyways
< bitcoin-git> [bitcoin] sdaftuar opened pull request #10345: [P2P] Timeout for headers sync (master...2017-05-timeout-headers-sync) https://github.com/bitcoin/bitcoin/pull/10345
< paveljanik> BTW mempool - "standard" nodes are now running with ~300MB mempools...
< gmaxwell> paveljanik: yep, "mempoolminfee": 0.00002822
< 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