< bitcoin-git>
bitcoin/master dbf6bd6 Wladimir J. van der Laan: Merge #11071: Use static_assert(…, …) (C++11) instead of assert(…) where appropriate...
< bitcoin-git>
[bitcoin] laanwj closed pull request #11071: Use static_assert(…, …) (C++11) instead of assert(…) where appropriate (master...static_assert) https://github.com/bitcoin/bitcoin/pull/11071
< gmaxwell>
kallewoof: copying the mempool ids worries me, what happens when it has a million tiny transactions in it... that will be pretty slow and blow out all the caches. Would it have been possible to use a closure like structure to carry the required operations under a single grab of the lock. (maybe thats worse, haven't benchmarked)
<@wumpus>
sounds like something that definitely needs benchmarks
< kallewoof>
Not sure what you mean by ids. Right now the PR I submitted will make a snapshot copying over the txs in the list as CTxMempoolEntry's. That is one single iteration over mapTx in the mempool, comparing each to the given std::set. I think this is faster than iterating mapTx once per hash, but not sure. Yes, proper benchmarks are probably needed...
< gmaxwell>
kallewoof: it's just that there can be a million-ish entries in mapTx. So I think that in extreme cases your snapshot could be quite slow. Though I certantly believe it's faster with the typically small mempools.
< bitcoin-git>
bitcoin/master f9ca0fe Jonas Nick: Fix combinerawtransaction RPC help result section
< bitcoin-git>
bitcoin/master f355883 Wladimir J. van der Laan: Merge #11083: Fix combinerawtransaction RPC help result section...
< bitcoin-git>
[bitcoin] laanwj closed pull request #11083: Fix combinerawtransaction RPC help result section (master...fix-combinerawtransaction-help) https://github.com/bitcoin/bitcoin/pull/11083
< kallewoof>
gmaxwell: I realized I am not stopping iteration ever, so I switched the code a bit (1. it will now erase found entries from set, and 2. it will break when set is empty). I will do benchmarks to see how mempool size impacts speed there.
< kallewoof>
... I *am* assuming that iterating over mapTx once is better than simply doing mapTx.find() for each hash. I probably shouldn't assume that.
< sipa>
iterating is O(1) per ste
< sipa>
find is O(log n)
< kallewoof>
sipa: Thanks. So with big mempool and small set, my approach is bad. Will fix.
< bitcoin-git>
bitcoin/0.15 1c4b9b3 Wladimir J. van der Laan: Merge #11076: 0.15 release-notes nits: fix redundancy, remove accidental parenthesis & fix range style...
< kallewoof>
gmaxwell: Updated. With k=vInvTx.size(), n=mapTx.size(), pre: O(k log n); post: O(k log n). This is mapTx stuff only. That's the part you were concerned about, right?
< gmaxwell>
okay, thats more interesting.
< gmaxwell>
its late, I must be confused... because I don't understand how this doesn't violate the locking on the multiindex. But I never did understand all the multiindex details all that well.
< kallewoof>
The snapshot is created under lock. Then it's used single-thread-like. How would that violate multiindex locking?
< gmaxwell>
Though.. couple extra points: when it's going to access all the vInvTx it would probably be a lot faster to pull all the things that get compared out at once instead of building a CTxMemPool::indexed_transaction_set.
< gmaxwell>
kallewoof: because there are references being copied, who owns the underlying data?
< gmaxwell>
e.g. what happens when a mempool entry is deleted while you've dropped the lock but are accessing the snapshot.
< kallewoof>
I am pretty sure the CTxMemPoolEntry is copied, not referenced.
< gmaxwell>
the mempool entry contains references.
< kallewoof>
if deleted, the inv may contain the hash of a deleted tx but that would have been the case if you had arrived a fraction of a second earlier, too, so doesn't seem unacceptable
< gmaxwell>
(and it better, you sure as heck don't want to copy all the txn! :) )
< kallewoof>
Oh, yes. But these are accessed without locking already.
< kallewoof>
Or at the least, they are made available after lock is released.
< sipa>
what is accessed?
< kallewoof>
tx, for example.
< sipa>
the mempool contains shared pointers to constant transactions
< sipa>
you can copy the shared pointers under lock
< sipa>
and then lose the lock
< sipa>
and then deref the shared pointers to txn
< sipa>
but nothing else
< kallewoof>
Yes. And if I have a separate structure with pointers to those txs, I can do whatever I want with it, right?
< sipa>
yes
< sipa>
txn are immutable
< kallewoof>
Then I think I'm good.
< kallewoof>
gmaxwell: forgot to reply regarding the vInvTx optimization you mentioned; I'm not entirely sure what you mean. Do you mean I should pull out the TxMempoolInfo objects directly instead of doing the snapshot middle step?
< kallewoof>
I kind of intended for the snapshot class to be useful in other places but if this is the only one that might be better.
< gmaxwell>
The only things that get accessed in the objects IIRC are their depth (and if they're in there or not), which means that you can just fetch all the data that the sort will later access in advance, instead of copying the whole CTxMemPoolEntry object.
< gmaxwell>
Which will probably be very time and memory inefficient (e.g. copying shared pointers)
< kallewoof>
Ahh.. that makes sense.
< gmaxwell>
(also their feerate, perhaps. vague recolletion here, in any case, it's just a couple narrow things. We do something like that for the node sorts for eviction)
< * kallewoof>
nods
< bitcoin-git>
[bitcoin] dooglus opened pull request #11085: Add 'sethdseed' RPC to initialize or replace HD seed. (master...set_hd_seed) https://github.com/bitcoin/bitcoin/pull/11085
< bitcoin-git>
bitcoin/master 07685d1 Jonas Schnelli: Add length check for CExtKey deserialization
< bitcoin-git>
bitcoin/master 9f60b37 Wladimir J. van der Laan: Merge #11081: Add length check for CExtKey deserialization (jonasschnelli, guidovranken)...
< bitcoin-git>
bitcoin/master bea8e9e practicalswift: Document the preference of nullptr over NULL or (void*)0
< bitcoin-git>
bitcoin/master 9e00a62 Wladimir J. van der Laan: Merge #11066: Document the preference of nullptr over NULL or (void*)0...
< bitcoin-git>
[bitcoin] laanwj closed pull request #11066: Document the preference of nullptr over NULL or (void*)0 (master...document-nullptr-preference) https://github.com/bitcoin/bitcoin/pull/11066
<@wumpus>
I like replacing .count() and [] with one find
<@wumpus>
incidentally, [] generates quite a lot of code
<@wumpus>
(when used with maps)
< promag>
there is a PR to replace that with .at()
< promag>
the idea there is just to avoid the 2nd lookup
<@wumpus>
yes
<@wumpus>
but the stanza of using count then [] (sometimes even multiple times) instead of using an iterator always annoyed me (but apparently not enough to patch it myself), happy to see it go
< luke-jr>
might be nice to do a explicittype& varname = it->second; though - but this is definitely an improvement
< luke-jr>
(it's hard to tell what it->second actually is here)
<@wumpus>
luke-jr: looks like that's what he does
<@wumpus>
luke-jr: only the iterator is assigned to an auto
< luke-jr>
I'm not talking about the iterator ;p
< promag>
yes, I almost did that, but I think the best is to switch those finds with GetWalletTx which is almost never used
<@wumpus>
luke-jr: I know, but he does "CWalletTx& wtx = it->second;" in many places
<@wumpus>
which matches "explicittype& varname = it->second; " right?
< luke-jr>
yes, it should be in the other places too
<@wumpus>
meh
< promag>
those "explicittype& varname = it->second; " were already there
<@wumpus>
yes. it's fine.
< promag>
luke-jr: I didn't add new vars
< luke-jr>
promag: I'm saying you should in this case
< luke-jr>
directly using it->second is poor for readability
< promag>
that will cause a bigger diff, out of scope IMO
<@wumpus>
please don't change it now, I've just reviewed it
<@wumpus>
yeah
<@wumpus>
the scope is "avoid second wallet lookup", which he did, The old code was "mapWallet[txin.prevout.hash].MarkDirty();" whichalso had no explicit type.
< luke-jr>
sure
< promag>
luke-jr: I almost did that, just thought those can be improved in a later PR
< promag>
cool
< luke-jr>
even with the nit, it's a utACK, not a NACK ;)
< bitcoin-git>
bitcoin/master e53615b Andrew Chow: Remove vchDefaultKey and have better first run detection...
< bitcoin-git>
bitcoin/master 2621673 Wladimir J. van der Laan: Merge #10952: [wallet] Remove vchDefaultKey and have better first run detection...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10952: [wallet] Remove vchDefaultKey and have better first run detection (master...remove-defaultkey) https://github.com/bitcoin/bitcoin/pull/10952
< luke-jr>
instagibbs: see #11089
< instagibbs>
luke-jr, ah sorry didn't notice that
< luke-jr>
instagibbs: if you want to take it back, I can close
< instagibbs>
nah, you rebased for me
< instagibbs>
thanks :P
< luke-jr>
although I cut out some of it
< instagibbs>
sure, doesn't have to be done all at once
< warren>
1) I don't think 'effective limit" is accurate in describing what is really a practical size compared to pre-segwit transaction uses, which these estimates were really referring to?
< warren>
2) And isn't 1.6MB-2MB the very old estimates, later found in a Bitfury study (?) to be more like 2.1MB? I wonder what it would be now.
< gmaxwell>
warren: luke posted an alaysis patch on reddit, it's about 1.9x IIRC.
< warren>
"effective limit" -> "effective size"?
< bitcoin-git>
[bitcoin] laanwj opened pull request #11091: test: Increase initial RPC timeout to 60 seconds (master...2017_08_test_wait_for_rpc) https://github.com/bitcoin/bitcoin/pull/11091
< luke-jr>
does per-TXO store the outpoint in both the key and the value? O.o
< sipa>
luke-jr: no, only in the key
< sipa>
COutPoint in the key, CTxOut in the value
< cfields>
luke-jr: grr, using git archive won't work either. Ironically, because the substitution from 'git archive' creates a diff.
< luke-jr>
>_<
< luke-jr>
well, we want that anyway I think, so maybe PR it for master at least
< luke-jr>
or at least don't discard it yet
< luke-jr>
cfields: so that leaves either fixing the tarball to build with the desired string, or making src/obj/build.h manually, right?
< cfields>
luke-jr: i don't think we can make it manually, it'll just get overwritten
< BlueMatt>
cfields: can you confirm there is nothing stopping switching sync.h primitives to std?
< cfields>
BlueMatt: sure, let me finish this thing up first though
< cfields>
BlueMatt: oh. at the very least, CSemaphore/CScheduler/CCheckqueue non-interruptible
< BlueMatt>
hmm?
< BlueMatt>
I dont think CSemaphore needs to be interruptible
< BlueMatt>
CScheduler and CCheckqueue use boost:: directiyl
< BlueMatt>
directly
< cfields>
BlueMatt: ok right, those were preventing the switch to std::thread.
< BlueMatt>
yes
< cfields>
luke-jr: heh
< cfields>
BlueMatt: yea, i guess CSemaphore is ok too. though didn't we decide at one point that the interruptions were saving us?
< BlueMatt>
on CSemaphore? no?
< cfields>
nm, was thinking of e007b24
< cfields>
i guess that commit shows we rely on posting :)
< BlueMatt>
yes, we rely on posting
< cfields>
ok neat, i guess the boost::mutex dep slowly dissolved
< BlueMatt>
=D
< cfields>
luke-jr: ok, want to just do that for gitian?
< BlueMatt>
well, no, not neat, really, we have locking primitives that have useful debug things in them, and a bunch of code is blindly using boost:: directly :(
< BlueMatt>
but oh well
< cfields>
that's really gross, as it completely defeats the purpose of all of the version stuff
< cfields>
but i guess it works for 0.15
< luke-jr>
cfields: personally, I prefer the "just get it fixed" approach, but if we don't have time for that, okay..
< cfields>
luke-jr: well the "just get it fixed" approach involves nuking most of our version handling, no?
< cfields>
i think we'll just take the complexity down to "the tarball injected the version" or "check git" ?
< luke-jr>
cfields: more like using genbuild on the git to generate the stuff used by the tar
< luke-jr>
what should it say, btw?
< cfields>
sure, that works
< cfields>
hmm?
< luke-jr>
I've never seen it without the git hash in the version ;)
< luke-jr>
Bitcoin Core RPC client version v0.15.0