< GitHub73>
[bitcoin] sipa closed pull request #6776: Support -checkmempool=N, which runs checks once every N transactions (master...fraccheck) https://github.com/bitcoin/bitcoin/pull/6776
< gmaxwell>
Oh I think I know one reason we see corruption reports on windows.
< gmaxwell>
Stopnode often takes an awful long time, for example on my laptop running defaults it just took 12 seconds. In windows, on shutdown, tasks that don't stop pretty much immediately get killed.
< gmaxwell>
thats probably a maximally bad case for leveldb as it'll be in the middle of flushing out a bunch of cached updates.
< tripleslash>
gmaxwell: its specifically HungAppTimeout and is defaulted to 5 seconds.
< morcos>
test_bitcoin hangs for me a decent fraction of the time. It looks like it gets stuck at line 109 in scheduler_tests.cpp
< morcos>
ah, i see it is an issue, #6540
< wumpus>
gmaxwell: that will likely contribute to it, although most of the time corruption seems to happen on crashes / power failures, when there is no time to flush at all
< wumpus>
but if that is the case too then the flush+sync on windows is not essentially not working at all
< wumpus>
-first not
< gmaxwell>
someone was commenting that we were writing via mmap on windows and that the sync we were using there didn't work on maps; which sounds like the mac problem. I didn't verify these claims at all.
< wumpus>
didn't check that either
< dcousens>
wumpus: aye, 1 OOM and my chain was broke
< jcorgan>
cfields: did you ever make progress on #6681?
< jcorgan>
i guess that would be #6819 now
< cfields>
jcorgan: no, i stopped there. I just wanted to get it building so that someone who knows zmq could make it actually work
< jcorgan>
got it
< morcos>
gmaxwell: For this fast template generation on new block code. Are you envisioning you switch to a new empty template after a new most work header? Even if you haven't validated the block you're connecting yet? And then set some sort of timeout with which you'll switch back to not being willing to build off a headers only chain?
< morcos>
Once you've connected a new block, I'd say there is no reason not to wait the extra few ms to generate a block template with txs in it. Which can then be validated after its already been served to you.
< gmaxwell>
No I was not. This is less safe than many people think it is, and I think not needed if the other details are handled correctly.
< morcos>
Well thats where all the delay is right? Receiving and connecting the new best block.
< sipa>
how about building a new template, switching to working on it immediately, and then starting a validation for it
< morcos>
sipa: yes thats what i'm suggesting. but thats after you've connected the best block. if we're still requiring to wait for that, don't we think people will choose to short circuit it less safely on their own
< gmaxwell>
morcos: No, CNB latency is tens of times slower than validating normally.
< morcos>
gmaxwell: ok, i'll give you multiples, but probably less than 10x unless your mempool is really really big. and thats just compared to validating, what about waiting to receive?
< morcos>
i haven't looked at the time delay from receiving most work header to finishing connecting the block, any idea what that is typically?
< morcos>
i bet its long
< gmaxwell>
When the relay network is working normally about 80% of blocks are transmitted in a single packet.
< morcos>
ah yes, forgot about relay network
< morcos>
thats why i ask questions
< sipa>
it still makes sense to have numbers for the time between receive inv and CNB building a template on top
< gmaxwell>
Most of the delays in mining right now appear to be from outside of bitcoin core, actually.
< morcos>
sipa in a relay node connected case or regular node or both
< sipa>
morcos: in "reality"
< sipa>
:)
< morcos>
ok, well i'm almost ready to push a WIP branch. it still doesn't do it in a thread, but the gain is really rather limited at this point, and i'll save that i think for a second pull
< morcos>
but the question i want to resolve is what to do when TestBlockValidity fails
< morcos>
right now it throws an error
< gmaxwell>
hm. we've done something recently that slowed down connectblock (or the network behavior changes have)
< gmaxwell>
oh dear
< morcos>
connectblock has always been slow since i've measured it
< sipa>
how much is fetching inputs vs verifying inputs?
< gmaxwell>
wtf.
< gmaxwell>
since my node here's last update to master connect block's time has increased monotonically for every block.
< sipa>
gmaxwell: coincache being thrown out my mempool bloat?
< morcos>
gmaxwell, what block hash was that?
< morcos>
gmaxwell: are you still running mempool.check? it runs inside that timer
< gmaxwell>
morcos: before this latest rounds of attack this node was taking <100ms to connect block.
< gmaxwell>
morcos: must be the mempool checks then.
< gmaxwell>
morcos: it's all of them.
< sipa>
gmaxwell: turning on mempool checks are a certain way to blow away your cache every block
< morcos>
the blocks around that time for me took 500ms and then 1ms (only coinbase)
< gmaxwell>
in any case, debug logs ran this thing out of space a couple hours ago, so I've restarted it. I'll run without mempool checks to get some good timings.
< gmaxwell>
Numers I posted from shortly before the MTL event were about 80ms.
< morcos>
80ms?? hmm...
< sipa>
so with a 200 MB mempool i seem to need a 700-1200 MB coincache
< sipa>
with matt's latest patch
< sipa>
i wonder if we should (as a short term hack) treat some factor of the size-of-pulled-in-memory of a tx as its txsize
< morcos>
gmaxwell: i looked at some old numbers of mine (couple of months ago) and they were like 500ms on average (during fairly busy time, mid July)
< gmaxwell>
morcos: Interesting!
< morcos>
sipa: so what do you mean "need"?
< morcos>
a 200MB mempool isn't really the right measure right
< sipa>
morcos: how do you mean?
< morcos>
because as you go from 0 - 200MB you'll pull in a certain amount of txin's. but as you keep running, youre mempool stays at 200MB, but you'll i think tend to pull in more txin's? or does matt's patch actually remove txin's no longer requested when a tx gets mined?
< sipa>
when a tx gets mined it goes into the cache with dirty bit on
< sipa>
so it can't be removed from the cache anymore
< sipa>
until a flush
< sipa>
but just a 700 MB coincache sounds pretty painful already
< morcos>
yes i think 700MB is pretty painful, but we need to think a bit about how to be smarter about it
< sipa>
per-txout cache...
< morcos>
yeah if it was pertxout, then you'd solve that problem
< morcos>
every so often you don't flush your cache, you just write out the dirty entries
< morcos>
we could still do that now, except we don't know if that tx should still be in cache b/c of other mempool txs
< sipa>
well an lru or random eviction of the utxo set could work too
< sipa>
the cache would just become less effective
< morcos>
so when we flush the cache, we have to write everything correct? so its consistent. i think if we could jsut get smarter about what we wiped from the cache at that point, then maybe we could jsut flush a lot more often, or do you think that would be bad.
< sipa>
yes, a flush shouldn't wipe everything
< morcos>
tell me if this would be too cumbersome. i think it might be pretty fast, how long does a flush take?
< morcos>
after you write everything, you quickly scan the top 10MB of txs in the mempool, and insert all of their txin.prevout.hash's into a set, and then you iterate throught the cachemap erasing anything thats not one of those
< morcos>
so actually flushing takes over a second right? i think you might be able to do something like i suggested on the order of 10's of ms, but not sure
< morcos>
i don't know if it's bad (or maybe its even good) to flush more regularly. but if we did something like that, we wouldn't even need to worry about matt's patch. we could just "flush" every time the cache was getting too big.
< morcos>
re: TestBlockValidity failing. I think I'm going to log an error and return NULL. Seems better than throwing an error. I'd like to reuse FormatStateMessage (in main.cpp), should I move it to a different file, or just declare it in main.h?
< Luke-Jr>
morcos: ⁇? if the block is invalid you do NOT want to use the template ever
< Luke-Jr>
morcos: pretty sure you would entirely break proposals too
< morcos>
Luke-Jr: I know you don't want to use it, the question is how to handle that case.
< morcos>
It means code is broken somewhere, so human intervention is going to be required at some point
< morcos>
The existing code would have thrown an error. I chose to return NULL and log the error (which will cause getblocktemplate to throw a now misnamed JSONRPC error)
< morcos>
Another option would be to try to return a template with no tx's instead (since the likely bug is mempool consistency was broken)
< morcos>
I'm about to PR as a WIP... , would be great if you want to take a look
< gmaxwell>
morcos: I vaguly recall something that if createnewblock can fail there is a crash elsewher.e
< gmaxwell>
or maybe I also fixed that in anticipation of people making it possible to fail again.
< morcos>
It took a long time to make it produce the exact same blocks as the old code, but helped work out a couple of bugs.
< Luke-Jr>
IMO human intervention is preferable to silently mining empty blocks
< Luke-Jr>
and debug.log is not non-silent
< gmaxwell>
Most miners will never see anything in debug.log.
< morcos>
I really hate the ugliness around hacking in the ability to still do priority space in the blocks.
< Luke-Jr>
morcos: if that requires ugliness, the old code was better ?/
< Luke-Jr>
:\
< morcos>
well, I'd call the old code ugly as well.
< gmaxwell>
Luke-Jr: the old code is very ugly.
< Luke-Jr>
slow != ugly
< morcos>
Also I didn't clean up the whole thing, I just am putting this out there for proof of concept.
< Luke-Jr>
but I cant see the new code to compare yet..
< morcos>
Luke-Jr: the big problem is priority is very difficult to calculate
< Luke-Jr>
? no
< morcos>
If there is a consensus that its an important metric to keep, then I think we should #6357 which would make it much faster to calculate the correct priority in the mining code
< morcos>
however it'll still be impossible to keep a sort based on it (i think)
< morcos>
it changes!
< Luke-Jr>
you need to lookup the inputs anyway
< morcos>
not anymore
< morcos>
but even if you do, the biggest problem i see with the old code, is that you have to look up the inputs for ALL the txs in your mempool
< morcos>
not just the ones you're putting in a block
< Luke-Jr>
hmm
< morcos>
and if you're going to do a priority portion of the block, you have to keep doing that
< Luke-Jr>
resorting once per block seems reasonable imo?
< morcos>
although maybe the dynamic priority calculation would fix that, i haven't looked at it in a while.. and maybe it could be made even easier now with the concept of mempool children
< morcos>
Luke-Jr: you're right, that would be the way to improve this code if priority isn't going away
< Luke-Jr>
morcos: I still plan to redo all this btw :p
< morcos>
keep an index (either part of the multi-index or separate) of all the priorities sorted, and only update it once per block
< morcos>
yeah me too! :)
< Luke-Jr>
so the mempool is a list of block templates
< morcos>
but i was hoping we might be able to get something simple'ish done for 0.12, which would make GBT run a lot faster.
< gmaxwell>
morcos: the input look up problem exists for fees too. :(
< morcos>
gmaxwell: they are already stored in CTxMemPoolEntries
< morcos>
and now i addded sigops to that too
< gmaxwell>
Oh I see right fees can be cached but priority cannot.
< Luke-Jr>
right
< Luke-Jr>
priority is our best metric right now I think, so I wouldnt want to lose ikt even temporarily
< morcos>
best metric for what? why do you think its better than fees?
< gmaxwell>
Luke-Jr: I don't really agree there. Priority works fine for you and I, I don't think he serves most users all that well.
< Luke-Jr>
morcos: spammers are happy to pay fees
< morcos>
Luke-Jr: yeah i actually agree its a pretty good anti-spam mechanism
< morcos>
but thats not how we use it now!
< Luke-Jr>
we do both now
< Luke-Jr>
gmaxwell: imo thats why it isnt *exclusively* priority
< gmaxwell>
Luke-Jr: point was spam goes through currently.
< morcos>
Luke-Jr: there is also the problem of incentives
< Luke-Jr>
gmaxwell: not via priority…?
< morcos>
how do you make miners prefer priority vs fee
< Luke-Jr>
morcos: you dont
< phantomcircuit>
wumpus, i haven't forgotten about getting you a copy of a corrupted datadir btw
< Luke-Jr>
longterm fees are the only realistic metric
< morcos>
Luke-Jr: so you view priority as like an HOV lane. at least some txs will sneak past even if the spam is causing congestion on most of the block
< Luke-Jr>
but for now we want to try to keep fees low
< gmaxwell>
morcos: and it does have that effect.
< Luke-Jr>
morcos: basically
< Luke-Jr>
also a nice fallback
< morcos>
gmaxwell: if we care about preserving that, why don't we just redefine priority, to be your priority at the time the tx was accepted
< morcos>
then it can be cached and its easy to reason about and who cares if different nodes/miners calculate it differently
< Luke-Jr>
as long as we have priority, every tx can get confirmed *eventually*
< gmaxwell>
morcos: we could expect then it'll turn into dead weight in the mempool.
< morcos>
oh sorry, thats not what i mean
< morcos>
t
< morcos>
i meant the priority only depends on your inputs that were confirmed at the time you were accpeted
< morcos>
so its still a bit complicated, but way less than currently
< phantomcircuit>
a better question is, why would miners use priority?
< gmaxwell>
I think that would be fine. Then it only needs to update by some ratio of the size, I guess.
< Luke-Jr>
that may work good enough as a temporary thing
< Luke-Jr>
phantomcircuit: already had that minidiscussion scroll up
< morcos>
gmaxwell: but yeah, i mean even easier, lets just make it not age once its in your mempool. if you guessed wrong and it doesn't get confirmed soonish, then you resubmit after it expires (currently 72 hours)
< Luke-Jr>
morcos: maybe have a post-block resort for new confirmed inputs optionally enabled in conf file
< Luke-Jr>
just in case it turns out bad
< Luke-Jr>
re solrt I mean
< Luke-Jr>
re sort sorry
< gmaxwell>
morcos: thats a little obnoxious, in that if it doesn't make it in the first block then immediately anything new added has an advantage. Maybe it okay?
< morcos>
gmaxwell: yeah i think its a tradeoff. the annoying thing however is i'm not sure you can combine it into one score very easily and still have it serve the purpose you want it to serve
< Luke-Jr>
afk
< gmaxwell>
I supposed if we cared more we could have a background task that just goes and recosts them from time to time.,
< gmaxwell>
presumably we're doing some kind of linear scan for the expiration? (I haven't kept up with the latest changes)
< morcos>
gmaxwell: expiration? oh no, there is an entry time index as well.
< phantomcircuit>
gmaxwell, im seeing ~200ms on average for Connect total on my server for the last two months
< gmaxwell>
morcos: so... perhaps another way to handle priority is to maintain a seperate very small mempool for it.
< gmaxwell>
so then the cost of having to update and resort the 'whole mempool' is not very lage.
< gmaxwell>
er large*
< phantomcircuit>
gmaxwell, why not just mark transaction sin the mempool as dirty when there's a new block and update the priority in a background thread?
< sipa>
I'm probably wasting my time, but I'm writing a more (space) efficient std::vector drop-in replacement
< sipa>
a vector has an average overhead of 40 bytes currently...
< gmaxwell>
sipa: probably; ... maybe a more useful thing to do is to find things using vectors for small amounts of data and make them not use vectors. :P
< gmaxwell>
sipa: question; I can't figure out why we would need to cache failing signatures at all: rationale: constructing a new novel failing signature is free.
< sipa>
gmaxwell: the idea is to use a union to either store elements directly, or store the size + pointer of a dynamic array
< sipa>
gmaxwell: we cache failing transactions, not signatures
< sipa>
to prevent downloading and verifying them over and over again
< gmaxwell>
that was just a brainfart, for some reason I thought the sigcache type included a validity flag.
< gmaxwell>
of course it doesn't.
< gmaxwell>
sipa: I've wondered before why the normal std::vector didn't special case small numbers of objects by making one of the pointers null and storing the objects internally.
< gmaxwell>
sipa: even if you superoptimize the vector, there is still malloc overhead and fragmentation and the pointer to the vector object in its parent object which can all be eliminated in cases where the vector can be avoided.
< sipa>
gmaxwell: well for example almost every CScript (which is a std::vector<unsigned char>) in the utxo cache and mempool stores at least 25 bytes
< gmaxwell>
Every vector<char> is probably suspect to begin with. :P
< sipa>
i can make a vector that is exactly 32 bytes (4 bytes size + 28 bytes data, or 4 bytes size + 4 bytes alloced size + pointer to actual data)
< gmaxwell>
It might be interesting to instrument std::vector so we can get a report of vectors in the codebase that have a constant number of objects in them.
< sipa>
so it's 32B for up to 28 bytes of data, or 32B + N for N > 28 bytes of data
< sipa>
instead of vector's 40B + N for everything
< gmaxwell>
sipa: or you could make the parent object able to store up to 32 bytes internally, and only create a vector if there is more than that. (I suppose it could use a union with the char[32] to store the pointer to the vector when there is one).
< sipa>
gmaxwell: that's what i'm doing
< sipa>
when i say "vector" i mean parent type; i think you're interpreting that word as "malloced data"
< gmaxwell>
Got it.
< gmaxwell>
why 28? why not 31? you don't need a 4 byte length for only 31 bytes of range.
< sipa>
it complicates things a bit if you can't share the size for one with the other
< sipa>
but yes, indeed
< gmaxwell>
ah I see, actually that also lets you avoid a flag.. since you can switch the behavior on size.
< sipa>
i thought about that first, but that's unfortunately not the case
< sipa>
as vector guarantees that a resize that shrinks doesn't invalidate pointers
< sipa>
so dropping from over the threshold to below the threshold cannot switch to in-object storage
< sipa>
so i use 1 bit of the 4-byte size to determine what type of storage to use
< gmaxwell>
what I was thinking about when I said changing the datastructure was just flattening. e.g. one dynamically sized txout object that contains everything in it directly, and no pointers at all. It's not like we ever modify any of this except setting flags.
< gmaxwell>
so having any pointers in it under any condition, is just waste and overhead.
< gmaxwell>
but it means that there needs to be smart accessors for it.