< sipa>
kallewoof: rev files are written at activation time, which is not the same time as the blocks are written
< sipa>
(not sure that explains what you're seeing, though0
< kallewoof>
sipa: we need to flush them afterwards then. maybe always flush after each write, if it is not the current one
< kallewoof>
sipa: i am seeing 2 separate bugs. 1 bug is that we over-allocate by (file size) amount, and 2nd is that we are not flushing afterwards, leaving some of the rev files in the overblown-size state.
< sipa>
hmm, we don't mark block index entries dirty when the corresponding block is activated/deactivated?
< kallewoof>
sipa: we only ever flush a undo file when we close its block file.
< kallewoof>
sipa: "Leaving block X" is when we flush both blk and rev. Never again after that
< sipa>
ah, that seems wrong
< sipa>
i'll have a look
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #17891: scripted-diff: Replace CCriticalSection with RecursiveMutex (master...2001-c2-CC) https://github.com/bitcoin/bitcoin/pull/17891
< kallewoof>
sipa: ok. I'm looking too
< kallewoof>
sipa: I think part of the issue is that blocks are stored to disk and there is a lag between that and the block being connected (probably due to sig checks / utxo etc), and since undo data is written then (at activation time as you said), the system will move on to the next block file but the activation will still be working against the previous one.
< kallewoof>
I saw the first encounter of 'write to an older rev file' right after closing block file 0, which said it had 'heights=0...119975', but right before that line there was an UpdateTip new best for height 119963, and my code broke when it was about to find undo pos for height 119965.
< kallewoof>
s/broke/hit my breakpoint/g
< kallewoof>
looks like a decent fix here is to flush the blk/undo when the activation occurs (as well, probably, to cover both cases).
< kallewoof>
i mean, when the activation goes from 'X' to 'X+1' in terms of the block file #.
< sipa>
kallewoof: iirc we only have one set of "dirty" files which are synced at flush time, which covers both blk and rev files
< sipa>
it seems we'll need to split that up
< kallewoof>
sipa: there is a setDirtyBlockIndex which is then chucked at a txdb.cpp method which writes a bunch. It's not immediately clear to me if those are the rev/blk or the index stuff.
< kallewoof>
the only place we flush (truncate to actual used size) the blk/rev is in that one place in validation.cpp. there is another place which calls but with finalize=false which doesn't do that.
< sipa>
kallewoof: ah right
< kallewoof>
Contrary to what the block file tostring() would have you believe, it seems that blocks are not actually in a strict range inside the blk files. I see block 119776 in blk1 and 119777 in blk0.
< goatpig_>
hasnt been the case since header first
< kallewoof>
I think the straightforward solution is to determine the last block in the old rev file beforehand and to have the write for that trigger the flush. But it's 3 am now. /waves
< bitcoin-git>
[bitcoin] kallewoof opened pull request #17892: bug-fix: re-flush undo files after they have been re-opened (master...200108-reflush-undo) https://github.com/bitcoin/bitcoin/pull/17892
< bitcoin-git>
[bitcoin] IPGlider closed pull request #17884: utils: Use fallback for macOS in AllocateFileRange (master...workaround_apfs_allocation_problem) https://github.com/bitcoin/bitcoin/pull/17884
< sipa>
e er5~
< sipa>
oops
< jeremyrubin>
Does anyone have any trouble with univalue encoding VARR types?
< jeremyrubin>
I'm working on an RPC and I'm getting: vector::_M_range_check: __n (which is 1) >= this->size() (which is 1)
< jeremyrubin>
(sidebar: the NONFATAL stuff makes debugging these sorts of problems harder, we should add flags to re-enable panic)
< sipa>
it's probably good to make NONFATAL failures turn into assertion failures in debug builds
< ryanofsky>
Oh I see how that would happen, it should not be a problem in the current PR.
< hebasto>
going to submit a pr to fix test
< ryanofsky>
in the commit you mentioned, value is a pointer to a string, and it was printing the pointer address instead of the string on: LogPrintf("Warning: parsed potentially confusing double-negative -%s=%s\n", key, value);
< hebasto>
correct
< ryanofsky>
oh I see, test could be fixed to more robust and not accept trailing characters after 0
< captjakk>
does bitcoind not support the newer 54 char onion addresses for tor?
< sipa>
no
< sipa>
needs bip 155 in order to represent those
< captjakk>
what's the timeline on 155, and is it something that could benefit from engineering contributions right now, or is it still being debated what the best approach is to get it in?
< sipa>
i don't know if anyone is working on it right now
< sipa>
i don't think there are any known problems with it
< captjakk>
kk, so if I submit a patch implementing it, are you optimistic it would be merged assuming no implementation problems?
< captjakk>
I'd imagine since this isn't a consensus issue, the vuln surface is substantially smaller