< bitcoin-git>
[bitcoin] gmaxwell opened pull request #9344: Do not run functions with necessary side-effects in assert() (master...assert_no_sideeffects) https://github.com/bitcoin/bitcoin/pull/9344
< BlueMatt>
again? :(
< gmaxwell>
looks like we missed some.
< sipa>
or 2.
< gmaxwell>
or 4.
< BlueMatt>
missed, or re-added?
< gmaxwell>
the flush ones are from 2013, didn't check the others.
< gmaxwell>
looking
< gmaxwell>
BlueMatt: the other two you introduced in 9c837d54.
< gmaxwell>
You have have the brownpaper bag when 2013-sipa is done with it.
< BlueMatt>
lol, oops
< * luke-jr>
wonders if it would be useful to have a prune-to-slow-storage mode.
< adam3us>
djb has a database called cdb (used in djbdns) that guarantees key->value lookup in 2 disk hits. cost is it is slow to update (batch job)
< luke-jr>
I just mean for block data. Instead of deleting, move it to a USB drive or smth
< luke-jr>
maybe even a location that can be offline when you don't need it
< luke-jr>
so when you want to restore an old backup or whatever, you just plug it in
< adam3us>
yes. i am talking about something only semi-related. that utxo access latency (with sig checking disabled) is increasing due to less spam creating lower cache hit rates, and larger utxo hitting disk more.
< adam3us>
so then maybe a three tier strategy could be old stuff in cdb, more recent (eg last 2months?) in current disk layout, plus memory cache. once per month do the batch to move another 1month chunk to the cdb compact/efficient lookup format.
< adam3us>
cdb has constant time lookup, where general lookup is log n.
< Lightsword>
are there any known issues getting master to compile on the latest osx?
< Lightsword>
nvm was protobuf version issue
< luke-jr>
why is secp256k1.c +x now?
< luke-jr>
main_impl.h also
< phantomcircuit>
instagibbs, are wallet transactions loaded into the mempool on restart?
< phantomcircuit>
i didn't think they were
< phantomcircuit>
wouldn't that break #9262
< gribble>
https://github.com/bitcoin/bitcoin/issues/9262 | Prefer coins that have fewer ancestors, sanity check txn before ATMP by instagibbs · Pull Request #9262 · bitcoin/bitcoin · GitHub
< gmaxwell>
phantomcircuit: yes, they are.
< phantomcircuit>
gmaxwell, hmm
< gmaxwell>
(assuming the mempool would take them).
< gmaxwell>
they do you say it would break 9262?
< phantomcircuit>
they're not rebroadcast though right?
< gmaxwell>
they're rebroadcast if they get into the mempool.
< phantomcircuit>
if they're not in the mempool then checking their ancestor depth will always fail of course
< gmaxwell>
Yea, I see what you're saying-- they're put into the mempool. (and IIRC the wallet will not try to spend them if they're unconfirmed and not in the mempool)
< phantomcircuit>
hmm
< gmaxwell>
(but I could be wrong there and instead remembering the behavior it should have instead of does have... :) )
< bitcoin-git>
bitcoin/master f692fce Gregory Maxwell: Make RelayWalletTransaction attempt to AcceptToMemoryPool....
< bitcoin-git>
bitcoin/master 57e337d Pieter Wuille: Merge #9290: Make RelayWalletTransaction attempt to AcceptToMemoryPool....
< bitcoin-git>
[bitcoin] sipa closed pull request #9290: Make RelayWalletTransaction attempt to AcceptToMemoryPool. (master...resend_retries_mempool) https://github.com/bitcoin/bitcoin/pull/9290
< phantomcircuit>
gmaxwell, iirc SelectMinCoins can be supppper expensive
< phantomcircuit>
er SelectCoinsMinConf
< gmaxwell>
phantomcircuit: it has linear cost in the number of spendable coins below the amount you're sending.
< phantomcircuit>
ok
< phantomcircuit>
was that fixed from something much worse in the past?
< gmaxwell>
wumpus: fell off my radar before I finished review; I will review later today (your time).
< gmaxwell>
phantomcircuit: it used to call the IsFromMe stuff that could be O(terrible).
< sipa>
morcos: so, initial report (see https://github.com/sipa/bitcoin/commits/pertxoutcache)... gmaxwell benchmarked it, and it seems to make normal operation somewhat faster, but the cache memory usage larger resulting in more frequent flushes, and the flushes themselves are maybe 3x slower
< sipa>
so for large dbcache it seems to be a win, while for low dbcache sizes it clearly is not
< gmaxwell>
basically for largeish dbcache it looked ~15% faster than master until the moment where it flushed, then it took 2.5x longer... the cache also used about 20% more memory, so 20% less cache in a given size... so flushing more often and the further it went the performance change tended to a large slowdown. :( The cache using more memory is perhaps something of an illusion of an effect, since a m
< gmaxwell>
ajor motivator of the change was to eliminate the existance of modified entries in the cache, which would make a good replacement policy possible instead of dumbly dumping the whole thing.
< gmaxwell>
so if the issue were only the 20% reduction in cache size, that much could easily be recovered by smarter cache handling that it makes possible.
< morcos>
I didn't properly think through notifications that would no longer happen because SyncTransaction was no longer being called
< sipa>
where else do we need notifications for conflicts?
< morcos>
I think there are 3 things that won't happen 1)zmqnotifications (but we dont' do those for other things that leave the mempool, so maybe doesn't matter)
< morcos>
2) UI notifications or whatever NotifyTransactionChanged does (haven't dived into this yet, but I also think this doesn't matter)
< morcos>
3) -walletnotify notifications
< morcos>
3) is tricky, b/c it might make more sense to call those from MarkConflicted in the wallet except for the case of someone paying you
< sipa>
but what we need for all of those is not conflicts
< sipa>
maybe we need a class of "reorged" transactions
< morcos>
someone paying you is not something you would identify in wallet conflict code as now being conflicted, but something you might want to know about in walletnotify
< morcos>
whats the definition of that if not conflict
< wumpus>
morcos: well that can happen, at least it's not one of the backported ones :)
< sipa>
reorged is previously confirmed transactions that are now no longer confirmed
< morcos>
sipa: you still are notified on those
< sipa>
conflicted is previously unconfirmed transactions that are now inconsistent with the tip
< morcos>
this is just mempool transactions that are now conflicted (actually nothing to do with a reorg, just a new block that conflicts with it)
< morcos>
yeah sorry
< sipa>
yes, i'm questioning whether we need that
< sipa>
their confirmation count is not changing
< sipa>
oh, maybe we need it for wiping a cached balance?
< morcos>
i think it might be nicer to just have -walletnotify for any wallet transactions that left the mempool... then it might not be necessary to distinguish if you can mark it conflicted or not
< morcos>
but thats difficult
< morcos>
sipa: i don't think so... it doesn't add to your credit if its not from you, and if it spends your money, then well... i suppose it could be a mixed debit tx that you don't identify... but imo thats in the class of things we could never fully get anyway
< morcos>
i think my thinking was just about our own wallet code, which i think is still ok... its the external notifications i hadn't considered
< sipa>
i didn't even know we sent notifications for left-mempool
< morcos>
we don't we send them for SyncTransaction
< sipa>
?
< morcos>
which we were calling for things that left the mempool via conflict
< morcos>
we send them from SyncTransaction for anything that is InvolvingMe
< morcos>
I was saying if you had a generic way of calling SyncTransaction for everything that left the mempool (for reasons other than being included in a block which is already covered) then that might be the most logical thing...
< morcos>
And I don't think it would be all that important to distinguish the reason it left, at least our own wallet isn't smart about that.
< morcos>
But that is not easy to do I don't think.
< sipa>
hmm, we could have a callback installed in the mempool
< morcos>
So maybe its important to still call SyncTransaction for txConflicted (the mempool conflict detection i removed) just so we get the -walletnotify for payments to us that have now been double spent / replaced / conflicted, what have you
< sipa>
ok
< morcos>
believe me i don't like txConflicted.. but i'm worried about accidentally making a change that somebody might be relying on for zero-conf doublespend detection
< sipa>
so you'll revert 9240 entirely, or keep txConflicts but remove its use in wallet conflicts checking
< morcos>
sipa: i think thats the same thing. it wasn't doing anything in wallet conflicts checking. so reverting 9240 entirely will not affect our wallet.
< morcos>
i think we should either revert it, or decide that there is a not that complicated better solution.
< sipa>
i see
< morcos>
perhaps its worth spending an hour thinking about whether SyncTransaction called from mempool.remove is the right thing to do regardless.. if not.. then revert 9240, if so, then do that before 0.14.
< sdaftuar>
i'd be hesitant to make changes without thinking through a clear API for how all these callbacks ought to work, and documenting that
< bitcoin-git>
[bitcoin] Christewart opened pull request #9350: Adding label for amount inside of tx_valid/tx_invalid.json (master...master) https://github.com/bitcoin/bitcoin/pull/9350
< morcos>
sipa: wumpus: It turned out to be quite easy to track all mempool removals and call SyncTransaction on them. However there are some tricky issues around edge cases involving reorgs where you might call the Sync for its removal after the Sync for it entering a block.
< morcos>
I don't believe this actually causes any issues.. but it'll take some time to think through it carefully
< morcos>
It would be my preference to do that, and try to document this code better.. As I think calling SyncTransaction (and thus walletnotify) on things that leave the mempool makes sense
< morcos>
And I think reverting 9240 is a step backwards towards cleaning this stuff up
< morcos>
But it depends on how risk averse you are for leaving it in its current state for a little while.
< morcos>
To summarize, I think the only regression is that -walletnotify isn't fired on transactions that were in the mempool and then evicted due to a tx in a new block conflicting them