--- Log opened Wed Apr 19 00:00:37 2023
00:10 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
00:15 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 240 seconds]
00:21 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
00:26 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 256 seconds]
00:36 -!- b_101_ [~robert@216.144.236.70] has joined #bitcoin-core-pr-reviews
00:37 -!- b_101 [~robert@216.144.236.70] has quit [Ping timeout: 276 seconds]
00:43 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
00:48 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 248 seconds]
01:00 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
01:05 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 260 seconds]
01:07 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
01:11 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
01:15 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 265 seconds]
01:33 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
01:38 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 264 seconds]
02:06 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
02:11 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 264 seconds]
02:17 -!- brunoerg [~brunoerg@187.183.43.117] has joined #bitcoin-core-pr-reviews
02:22 -!- brunoerg [~brunoerg@187.183.43.117] has quit [Ping timeout: 265 seconds]
02:40 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
02:48 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 265 seconds]
03:01 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
03:06 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 248 seconds]
03:18 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
03:22 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 260 seconds]
03:34 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
03:39 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit [Ping timeout: 248 seconds]
03:44 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has joined #bitcoin-core-pr-reviews
04:02 -!- brunoerg [~brunoerg@2804:14c:3bfb:8a:cd41:9775:5fb5:3a00] has quit []
05:05 -!- pablomartin [~pablomart@185.169.233.210] has joined #bitcoin-core-pr-reviews
06:31 -!- Zenton [~user@user/zenton] has quit [Read error: Connection reset by peer]
06:31 -!- Zenton [~user@user/zenton] has joined #bitcoin-core-pr-reviews
06:46 -!- pablomartin [~pablomart@185.169.233.210] has quit [Ping timeout: 240 seconds]
06:47 -!- pablomartin [~pablomart@217.130.254.81] has joined #bitcoin-core-pr-reviews
07:35 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 240 seconds]
07:38 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
09:02 -!- turkycat [~turkycat@76.146.17.180] has joined #bitcoin-core-pr-reviews
09:21 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
09:36 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews
09:36 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has quit [Changing host]
09:36 -!- TheRec [~toto@user/therec] has joined #bitcoin-core-pr-reviews
09:47 -!- abubakar [~abubakars@102.91.4.86] has joined #bitcoin-core-pr-reviews
09:48 -!- DaveBeer [~DaveBeer@ip-62-245-124-60.bb.vodafone.cz] has joined #bitcoin-core-pr-reviews
09:57 -!- alex-wiederin [~alex-wied@5.151.70.133] has joined #bitcoin-core-pr-reviews
09:59 -!- esixce [~esixce@8.242.158.105] has joined #bitcoin-core-pr-reviews
10:00 < stickies-v> #startmeeting
10:00 < LarryRuane> hi
10:00 < pinheadmz> hi !
10:00 < alex-wiederin> hi
10:00 < DaveBeer> hi
10:00 < lightlike> hi
10:00 < pablomartin> hi all!
10:00 < turkycat> hi everyone
10:01 < stickies-v> welcome everyone! Today we're looking at #27039, authored by pinheadmz. The notes and questions are available on https://bitcoincore.reviews/27039
10:01 < stickies-v> anyone joining us for the first time today? even if you're just lurking, feel free to say hi!
10:03 < stickies-v> an old-timers only day it seems
10:03 < abubakar> hi
10:03 < stickies-v> who got the chance to review the PR or read the notes? (y/n)
10:03 < turkycat> y
10:03 < pablomartin> y
10:03 < abubakar> read the notes
10:03 < DaveBeer> y, read the notes
10:04 < LarryRuane> y
10:04 < alex-wiederin> read the notes
10:05 < stickies-v> the PR only modifies a few lines of business logic, almost all of the PR is tests, but I think touches on a lot of interesting concepts re how we store and index blocks
10:05 < stickies-v> how would you summarize this PR in your own words?
10:05 -!- Eppie [~Eppie@102.89.34.58] has joined #bitcoin-core-pr-reviews
10:06 < DaveBeer> minimize unnecessary disk writes
10:06 < turkycat> this PR makes it possible to share all but the latest block file amongst multiple clients, also might prevent potential data corruption by preventing many unnecessary writes
10:07 < stickies-v> DaveBeer - it doesn't actually change any writing behaviour, as far as I understand
10:07 < pinheadmz> turkycat youre right about being able to share the files, but its not really about disk writes !
10:07 < turkycat> ack
10:08 < pinheadmz> stickies-v is right. did anyone check out the related issue?
10:08 < LarryRuane> It improves performance by refraining from writing blocks to disk that are already present on disk (so are no-op writes).. but also +1 to @turkycat 's comment about corruption, hadn't thought of that
10:08 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has joined #bitcoin-core-pr-reviews
10:08 < stickies-v> pinheadmz: what's the issue with sharing block files prior to this PR? I'd think that's possible either way?
10:08 < pinheadmz> https://github.com/bitcoin/bitcoin/issues/2039
10:08 < pinheadmz> you can try this with your own node - set the blocks dir to readonly and then start with -reindex
10:09 < pinheadmz> that *should* be possible but bitcoin throws an error - not because its trying to WRITE a file but .... ??? anyone ?
10:09 < stickies-v> LarryRuane: prior to this PR, we weren't writing any blocks to disk again either - I don't think this behaviour is changed?
10:09 < pinheadmz> correct!
10:09 < turkycat> it should be possible to share, but the latest files (blk and rev) might change while being read by another client
10:10 < pinheadmz> the error message is in this comment: https://github.com/bitcoin/bitcoin/issues/2039#issuecomment-1101330894
10:11 < turkycat> or maybe the OS would fail to open the file for reading if it was opened for writing by bitcoin core
10:11 < stickies-v> turkycat: ah, I was understanding "sharing" as copying the files to another client, instead of multiple clients using the same file (which is the more obvious interpretation). my bad
10:11 < LarryRuane> oh i see, we're trying to open the file with read-write permission, but if the file is RO, that fails
10:11 < pinheadmz> turkycat yes!
10:11 < pinheadmz> the bug is we cant OPEN the file, because we are trying to open it with W flag
10:11 < pablomartin> stickies-v I was thinking the same... no, same location, locking issue...
10:11 < pinheadmz> LarryRuane yep
10:12 < stickies-v> LarryRuane: indeed, see https://github.com/bitcoin/bitcoin/blob/d908877c4774c2456eed09167a5f382758e4a8a6/src/flatfile.cpp#L83 where we're calling `Open()` without the `read_only` parameter which defaults to `false`)
10:13 < LarryRuane> oh i see, so the PR avoids calling the function (Flush) that is calling Open()
10:13 < stickies-v> yessir
10:13 < LarryRuane> (unless necessary of course)
10:14 < LarryRuane> I even reviewed this PR but had forgotten... they say memory is the first thing to go... I don't recall what's the second thing
10:14 < stickies-v> (I think the error message could've been clearer if `FlatFileSeq::Flush` logged "failed to open file in write mode" instead of just "failed to open file")
10:15 < LarryRuane> should this PR improve that error message, maybe?
10:15 < pinheadmz> It should also log "Unable to open file" from flatfile.cpp Open() I think
10:15 < turkycat> lol LarryRuane
10:16 < stickies-v> gonna start moving on to the next questions, but as always - feel free to keep the discussion on previous questions going!
10:16 < LarryRuane> I've noticed it's really hard to decide which functions should log failures and which should not!
10:17 < stickies-v> LarryRuane: I think that's where the `util::Result` class comes in handy, so we can more easily propagate detailed error messages all the way to where we decide if we want to log (which depends on the use case)
10:17 < LarryRuane> stickies-v: +1 ... there's a review club for that in case anyone here isn't aware
10:17 < stickies-v> After this PR, can the whole `blocks/` directory be made read-only? Why (not)?
10:18 -!- Eppie [~Eppie@102.89.34.58] has quit [Quit: Connection closed]
10:18 < LarryRuane> NO! because we still need to be able to write to the latest blocks file
10:18 < turkycat> ^
10:18 < DaveBeer> no it can't, client still needs to be able to create block files
10:18 < pablomartin> LarryRuane +1
10:18 < stickies-v> LarryRuane: correct! but... there's another reason
10:18 < LarryRuane> oh sorry, you were asking about the directory, @DaveBeer +1
10:18 < turkycat> we wouldn't be able to -reindex either
10:18 < stickies-v> turkycat: why?
10:19 < turkycat> since the index/ directory is a sub of blocks/
10:19 < pablomartin> blocks/index/
10:19 < LarryRuane> oh but that can have its own (write) permission enabled
10:19 < stickies-v> exactly! we store multiple kinds of data in the `blocks` directory, including the block index (which is stored in a leveldb db)
10:19 < stickies-v> LarryRuane: well yes but it's in the `blocks` directory. perhaps the question should have been clearer about including subdirectories too
10:20 < stickies-v> anyway, being able to write to the latest block file is reason enough
10:20 < LarryRuane> it probably would have been better if the actual blocks files were in a subdir one level lower .. and maybe the rev (undo) files also one level lower (in own subdir)
10:20 < LarryRuane> but too late now
10:21 < stickies-v> LarryRuane: what would that have improved?
10:21 < lightlike> another thing is that if we -reindex and there is some corruption at some point, we'd stop at that point and rebuild the rest of the blocks - for that, the blocks would need to be writeable again.
10:21 < turkycat> LarryRuane yea the chainstate/ folder is aso built while -reindex but it's a sibling of blocks/
10:21 < turkycat> lightlike good point
10:22 < LarryRuane> one thing I've done often to get the size of the blockchain is `mkdir tmp ; ln ~/.bitcoin/blocks/blk*dat tmp ; du -sh tmp ; rm -r tmp` ... all that wouldn't be needed if blk*.dat files were in a separate subdir
10:22 < stickies-v> lightlike: because we attempt to remove the corrupted data from the block files?
10:23 < stickies-v> In `FindBlockPos()`, what is the relevance of `fKnown` to this PR, conceptually? How does the behaviour of `FindBlockPos()` change when `fKnown==true` instead of `false`?
10:23 < LarryRuane> i think lightlike meant the directory needs to be writeable
10:23 < lightlike> stickies-v: I think if -reindex fails to continue at some point, we'd fall back to IBD, download the missing blocks from peers, and overwrite the existing higher block files
10:23 < turkycat> I guess I assume, to lightlike's comment, that we would delete all blk and revs from the corruption point onward when rebuilding
10:24 < lightlike> but not 100% sure about that...
10:24 < pinheadmz> lightlike im not sure thats an automatic thing either but makes sense
10:24 < turkycat> yea, interesting- this might be worth a test case
10:24 < LarryRuane> turkycat: I don't think anything ever automatically deletes blk and rev files
10:24 < LarryRuane> pinheadmz: i think that is automatic
10:25 < turkycat> so we would just insert over the corrupted data?
10:25 < pinheadmz> well yeah actually, bc reindex would return
10:25 < pinheadmz> then bitcoin would just... do bitcoin
10:25 < LarryRuane> no i think we just create new blk and rev files
10:25 < turkycat> ahh fair
10:25 < pinheadmz> yeah probably the old corrupted data stays put
10:25 < LarryRuane> one thing to be aware of is, it's okay for the blk files to have redundant blocks (multiple copies of the same block)
10:25 < LarryRuane> (it only wastes disk space)
10:26 < turkycat> yea I guess if our index points to the 'correct' one (or either if both are correct) it doesn't really matter
10:26 < LarryRuane> it's the first one we encounter
10:26 < LarryRuane> (when doing reindex)
10:26 < pinheadmz> huh, what happens if reindex encounters a duplocate block ?
10:27 < turkycat> right but if corrupted, I guess it would fail validation and find it later
10:27 < stickies-v> on https://github.com/bitcoin/bitcoin/blob/d26a71a94ac4ae1b1a091f4412d390afba69b2f8/src/node/blockstorage.cpp#L877-L896 I can't immediately see any logic that deletes blk and rev files when reindex fails, but may need deeper digging
10:27 < LarryRuane> pinheadmz: it just ignores it
10:27 < pinheadmz> it wouldnt over-write the first idex ?
10:27 < pinheadmz> ah
10:27 < LarryRuane> stickies-v: right, i don't think anything auto-deletes those files
10:27 < pinheadmz> only in pruning mode !
10:28 < turkycat> yea I made a bold assumption and was wrong
10:29 < stickies-v> just reposting the current question since we've had a lot of (good!) discussion after:
10:29 < stickies-v> In `FindBlockPos()`, what is the relevance of `fKnown` to this PR, conceptually? How does the behaviour of `FindBlockPos()` change when `fKnown==true` instead of `false`?
10:30 < LarryRuane> I think `fKnown` means the block already exists on disk .. maybe `fExists` would have been a better name?
10:30 < alex-wiederin> stickies-v `fKnown` param determines whether `FlushBlockFile` is called in `FindBlockPos()`. The call to flush block file has been moved to the condition of `!fKnown` (i.e. if the position is unknown), I believe this is where get the reduction in calls to write.
10:31 < turkycat> LarryRuane no harder problem in computer science than naming things
10:31 < LarryRuane> turkycat: +1
10:32 < LarryRuane> the block has a "known" location, meaning that it already exists on disk (so it's not a terrible name)
10:32 < turkycat> we also only allocate if the position is not known, after finding a good position for it and deciding if we should finalize the file after
10:32 < LarryRuane> `FindBlockPos()` flushes the block if the position isn't known
10:32 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Remote host closed the connection]
10:32 < pinheadmz> yeah it does a lot more than just find something!
10:32 < stickies-v> LarryRuane: alex-wiederin: yeah you're both correct! I'd say the relevance is that `fKnown` is going to always be true when reindexing, which is what this PR is targeting
10:33 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
10:33 < pinheadmz> lightlike has a WIP to clean up that part of the code, but for now its a sort of typical bitcoin core codebase thing
10:33  * pinheadmz grimace
10:33 < turkycat> or, as a clarification, the finalize is for the current file (if there is not sufficient space for the new block) before the allocate logic
10:33 < turkycat> none of that logic is necessary if we know where the file position is
10:34 < lightlike> yeah, i plan on opening a cleanup PR for that, but only after this PR is merged.
10:34 < LarryRuane> pinheadmz: agree needs cleanup, i think in general when there are boolean flags, something may not be designed in the cleanest way
10:34 < pinheadmz> heh yeah but someone sometime made a nice little PR that was easy to review at the time :-)
10:34 < stickies-v> a little bonus side question: what does finalizing a block or undo file mean? like - what happens, and why do we need this?
10:34 < LarryRuane> (i say "may not" because it could be okay... but often isn't)
10:35 < LarryRuane> i think finalizing means truncating the file to the minimum length (?)
10:35 < stickies-v> LarryRuane: yes, and why does that need to happen?
10:36 < turkycat> because we allocate in chunks that are likely larger than the actual space used
10:36 < LarryRuane> it's created with a somewhat larger than needed size (i think it's 16m unless test mode), and grows by larger amounts (probably also 16m), because it's slightly more efficient than growing it on demand
10:36 < turkycat> coming in with the assist for LarryRuane
10:37 < stickies-v> exactly! we pre-allocate space, then fill it up best we can, and once we're about to exceed size we trim off whatever we didn't use
10:37 < stickies-v> alright next question
10:37 < stickies-v> What happens if bitcoind is killed after when block(s) have been processed, but before `FlushBlockFile()` is called? Does this PR affect that behaviour?
10:37 < LarryRuane> makes sense because we know it's very likely that we'll be writing to the file again soon
10:38 < LarryRuane> "if bitcoind is killed" ... I'm not sure about this, but i think the block index isn't flushed until after the block file, so we re-do the block processing when we come back up
10:39 < turkycat> the buffered content won't be written to disk and we'll have to re-download the block
10:39 < alex-wiederin> Agree. Don't think PR changes in that sense
10:39 < LarryRuane> in general, when something on disk (A) refers to something else on disk (B), you always want to write out B first (and sync it) and THEN write A to disk
10:40 < stickies-v> (we preallocate in chunks of 16MiB for block files and 1MiB for undo files: https://github.com/bitcoin/bitcoin/blob/d26a71a94ac4ae1b1a091f4412d390afba69b2f8/src/node/blockstorage.cpp#L587-L592)
10:40 < LarryRuane> (filesystem code has many of those kinds of sequenced writes to try to be corruption-proof)
10:40 < LarryRuane> in a way, the block index and the block data comprise a kind of primitive filesystem
10:40 < lightlike> I think if bitcoind is killed during -reindex, it will always start the reindex from scratch (and not try to continue at the latest point)
10:41 < LarryRuane> lightlike: is that right, interesting, i thought it would continue (IF you don't specify `-reindex` again .. if you do then of course it does start over)
10:41 < pinheadmz> lightlike https://bitcoin.stackexchange.com/questions/32835/safely-interrupt-reindex
10:41 < pinheadmz> sipa says it continutes
10:42 < LarryRuane> i think many people specify `-reindex` on the restart, thinking it's needed ... but it's not
10:42 < lightlike> oh, interesting. will try that out
10:42 < LarryRuane> pinheadmz: good SE find
10:42 < pinheadmz> i knew this one, i must have looked it up before lol
10:43 < turkycat> yea for sure if I wasn't 100% sure it would resume I'd start it over on a relaunch just to be sure. this is good to know
10:44 < sipa> The reindexing process consists of two phases really, the first is rebuilding the block index, the second is wiping the utxo set and rebuilding is. If you interrupt during the second one, it'll definitely just continue where it left off, because that is just the normal "try to activate the best known block" logic, unrelated to reindexing.
10:44 < sipa> I'm not sure if progress is saved during the first block index rebuilding phase, but if past-me says so, it's probably true.
10:44 < pinheadmz> past-you is so smart
10:45 < pinheadmz> and is reindex-chainstate then just skipping right to that ssecond step ?
10:45 < sipa> Exactly. -reindex-chainstate is exactly equivalent to rm -rf ~/.bitcoin/chainstate
10:45 < LarryRuane> that second phase takes much longer in practice, BTW ... actually @sipa aren't there now 3 phases? first block headers (takes only a couple of minutes on most systems), then the two you mentioned?
10:45 < lightlike> sure - but what if it's aborted midway through the first part
10:45 < LarryRuane> and actually, block headers are now downloeaded twice
10:46 < sipa> @LarryRuane You'll know this better now than I do.
10:46 < stickies-v> I think this might be where we check if we need to continue reindexing: https://github.com/bitcoin/bitcoin/blob/d26a71a94ac4ae1b1a091f4412d390afba69b2f8/src/node/blockstorage.cpp#L364-L367
10:46 < sipa> @stickies-v Nice catch, indeed.
10:47 < stickies-v> In the `blockmanager_flush_block_file` unit test, why are we pausing before starting a new blockfile?
10:47 < stickies-v> (link: https://github.com/bitcoin-core-review-club/bitcoin/commit/470ef396b5498d8689802c359a216d5a3c4749a5#diff-d6d633592a40f5f3d8b03863e41547de8751b874c1d20f129a616b9dd719b999R170)
10:47 < LarryRuane> stickies-v: yes .. and IIUC, if for some reason that flag in the leveldb index weren't `true`, we'd IBD from that point?
10:47 < lightlike> LarryRuane: redownloading the block headers via p2p isn't part of -reindex as far as I know. -reindex works without any peers.
10:48 < LarryRuane> oh that's right
10:49 < LarryRuane> I think the node does get bothered a little during reindex by P2P messages, which you can disable with `--maxconnections=0` if you're doing reindex performance comparisons
10:49 < LarryRuane> (i'm not sure how much of a difference that makes tho)
10:50 < LarryRuane> "why are we pausing" to detect if the file was written (timestamp will be different if so)
10:51 < stickies-v> LarryRuane: but why do we need to pause to check for a different timestamp when these operations are sequential?
10:51 < pinheadmz> this was a really hard test to write, because "flushing" is something unpredictable the OS does and theres no great way to know if it happened or not
10:53 < stickies-v> pinheadmz: yeah, and i'm not sure if it's reliable now? i tried running the updated `feature_reindex.py` test with the changes to `blockstorage.cpp` reverted and the test keeps succeeding
10:53 < LarryRuane> stickies-v: I think it's because sequential operations can still happen within the same second (or whatever the file timestamp resolution is)
10:53 < pinheadmz> Thats correct LarryRuane on windows its 2 seconds!
10:53 < turkycat> so, I believe the answer is that the OS writing the data and updating the timestamp on the file is async and there is some delay, for which `pause` is set at what should be the max delay
10:53 < LarryRuane> i think i suggested on the PR to compute the file checksum instead of depending on timestamps, but that's more work, probably not worth it
10:53 < stickies-v> LarryRuane: yeah the latter part is what kinda surprised me - that there's such a huge variance in resolution of last modified timestamps
10:53 < stickies-v> see e.g. https://stackoverflow.com/a/31521204/734612 for an overview
10:54 < pinheadmz> stickies-v yeah the test is, hard. I added it by jonatack request and it doesnt really cover the change in the PR as much as it just sets a regression test that the PR DOESNT BREAK
10:54 < LarryRuane> stickies-v: madness :)
10:54 < stickies-v> turkycat: this is more of a filesystem than an OS thing, afaik (which is also one concern I have with the current approach)
10:54 < pablomartin> thankfuklly the intermittent failure of the test caught it
10:55 < turkycat> I found these pauses strange though, in their order. I made a comment on it and tbh still don't fully understand why we aren't 1) read timestamp 2) perform write 3) pause 4) read timestamp again 5) compare
10:55 < pinheadmz> write = update timestamp
10:55 < pinheadmz> so reading timestamp before writing isnt useful
10:55 < pinheadmz> then we want to know if we WROTE AGAIN
10:55 < pinheadmz> and the only way to know is, the timestamp has changed
10:56 < turkycat> right but reading it first gives you the comparison point, you want to make sure that the value didn't change
10:56 < pinheadmz> but if the second write happened so fast that time itself hasnt advanced, the test would false-positibe
10:56 -!- abubakar [~abubakars@102.91.4.86] has quit [Quit: Lost terminal]
10:56 < pinheadmz> actually we are doing setps (1,2,3,4,5) as yo umention
10:57 < turkycat> ok, fair, I'll consider that
10:57 < pinheadmz> but we do a write before step 1 as well
10:57 < pinheadmz> er sorry no sorry
10:57 < pinheadmz> yeah we need to pause between writes so that if a write happened, the time will definitely be updated
10:58 < turkycat> so, perhaps I misunderstand the filesystem update delay- but I guess my thought is that since we're writing on line 150 we should pause again before reading `last_write_time` again, for comparison?
10:58 < stickies-v> turkycat: it's not a lag thing, it's a resolution thing
10:59 < stickies-v> the last modified timestamp just doesn't store any more specific timestamps than that resolution
10:59 < pinheadmz> yeah imagine if timestamp resolution was one day
10:59 < turkycat> yea ok I think that was my fundamental misunderstanding. I assumed the fflush and fsync were async and you needed to check the timestamp after some period to be sure
10:59 < stickies-v> so on FAT filesystem, if you perform 10 operations within an (ideally timed) span of 2 seconds, you'd update the last modified timestamp 10 times, but they'd all have the same value
10:59 < pinheadmz> you could update your file all day long and keep checking the time, itd never change
11:00 < turkycat> got it, cheers guys
11:00 < pinheadmz> yeah so if you happen to run this test on windows, its extra slow
11:00 < pinheadmz> theres an ifdef that changes the pause length for linux
11:00 < stickies-v> with that said, we're out of time for this meeting so the remaining questions are left as an exercise to the reader - feel free to continue discussing here!
11:00 < LarryRuane> another reason to avoid windows haha
11:00 < pinheadmz> thanks everyone for taking a look!
11:00 < LarryRuane> thanks, @stickies-v and everyone else!
11:01 < stickies-v> thank you everyone for participating, a bit less code heavy but i hope the concepts were interesting
11:01 < LarryRuane> and @pinheadmz !!
11:01 < stickies-v> and thank you pinheadmz for authoring the PR and helping out!
11:01 < alex-wiederin> thanks stickies-v!
11:01 < stickies-v> #endmeeting
11:01 < pablomartin> very informative session, thanks all!
11:01 < turkycat> thanks everyone
11:01 < DaveBeer> thanks stickies-v and pinheadmz
11:01 < pinheadmz> hopefully review keeps up! but im finding that devs are pretty scared of touching blockstorage... like its so important or something
11:02 < pinheadmz> and as illustrated by jonatack's comment - blockstorage isnt well tested enough to get high confidence when modifying it
11:02 < LarryRuane> question 6, I think the answer is that we specify a location for block 3 here: https://github.com/bitcoin-core-review-club/bitcoin/blob/470ef396b5498d8689802c359a216d5a3c4749a5/src/test/blockmanager_tests.cpp#L150
11:02 < pinheadmz> LarryRuane correct
11:02 < pinheadmz> and we dont write if the block position is known
11:02 -!- DaveBeer [~DaveBeer@ip-62-245-124-60.bb.vodafone.cz] has quit [Quit: Connection closed]
11:02 < turkycat> pinheadmz this was actually a source for one of the critical issues in bitcoin past
11:02 < LarryRuane> so it doesn't actually write block 3 to disk .. that's why at line 159 we expect block 2
11:03 < pinheadmz> if we *did* -- the test would overwite block data and that would be a mess
11:03 < pinheadmz> turkycat oh ?
11:03 < turkycat> https://bitcoinmagazine.com/technical/bitcoin-network-shaken-by-blockchain-fork-1363144448
11:03 < turkycat> issue with level db caused hard fork
11:03 < pinheadmz> oh yeah, but not the flat file system
11:04 < turkycat> true
11:05 < turkycat> ok, I gotta head out. cheers everyone and nice work pinheadmz on this PR!
11:05 -!- esixce [~esixce@8.242.158.105] has quit [Quit: Connection closed]
11:09 -!- alex-wiederin [~alex-wied@5.151.70.133] has quit [Quit: Connection closed]
11:17 -!- pablomartin [~pablomart@217.130.254.81] has quit [Ping timeout: 255 seconds]
11:24 -!- b_101_ [~robert@216.144.236.70] has quit [Ping timeout: 240 seconds]
11:24 -!- pharonix71 [~pharonix7@user/pharonix71] has quit [Remote host closed the connection]
11:24 -!- pharonix71 [~pharonix7@user/pharonix71] has joined #bitcoin-core-pr-reviews
11:25 -!- b_101 [~robert@189.236.59.209] has joined #bitcoin-core-pr-reviews
11:30 -!- ___nick___ [~quassel@2a00:23c6:8d9f:f501:57ce:e03d:168c:8986] has joined #bitcoin-core-pr-reviews
11:51 -!- b_101_ [~robert@216.144.236.70] has joined #bitcoin-core-pr-reviews
11:54 -!- b_101 [~robert@189.236.59.209] has quit [Ping timeout: 255 seconds]
12:00 -!- ___nick___ [~quassel@2a00:23c6:8d9f:f501:57ce:e03d:168c:8986] has quit [Quit: https://quassel-irc.org - Chat comfortably. Anywhere.]
12:02 -!- ___nick___ [~quassel@2a00:23c6:8d9f:f501:57ce:e03d:168c:8986] has joined #bitcoin-core-pr-reviews
12:02 -!- ___nick___ [~quassel@2a00:23c6:8d9f:f501:57ce:e03d:168c:8986] has quit [Client Quit]
12:05 -!- ___nick___ [~quassel@2a00:23c6:8d9f:f501:57ce:e03d:168c:8986] has joined #bitcoin-core-pr-reviews
12:25 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
13:04 -!- ___nick___ [~quassel@2a00:23c6:8d9f:f501:57ce:e03d:168c:8986] has quit [Ping timeout: 240 seconds]
14:35 -!- b_101 [~robert@189.236.59.209] has joined #bitcoin-core-pr-reviews
14:36 -!- b_101_ [~robert@216.144.236.70] has quit [Ping timeout: 255 seconds]
15:07 -!- b_101 [~robert@189.236.59.209] has quit [Ping timeout: 252 seconds]
15:07 -!- b_101 [~robert@216.144.236.70] has joined #bitcoin-core-pr-reviews
16:12 -!- TheRec [~toto@user/therec] has quit [Ping timeout: 240 seconds]
16:19 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews
16:19 -!- TheRec [~toto@84-75-225-47.dclient.hispeed.ch] has quit [Changing host]
16:19 -!- TheRec [~toto@user/therec] has joined #bitcoin-core-pr-reviews
17:00 -!- TheRec [~toto@user/therec] has quit [Ping timeout: 250 seconds]
17:01 -!- TheRec_ [~toto@84-75-225-47.dclient.hispeed.ch] has joined #bitcoin-core-pr-reviews
17:01 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Remote host closed the connection]
17:01 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
17:08 -!- effexzi [uid474242@id-474242.ilkley.irccloud.com] has quit [Quit: Connection closed for inactivity]
17:40 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
17:41 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
17:59 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Read error: Connection reset by peer]
17:59 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
19:12 -!- jon_atack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
19:15 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 248 seconds]
20:04 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Remote host closed the connection]
20:04 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
21:10 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Remote host closed the connection]
21:13 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has joined #bitcoin-core-pr-reviews
21:30 -!- furszy [~furszy@user/furszy] has quit [Quit: ZNC - https://znc.in]
21:32 -!- furszy [~furszy@104.128.239.93] has joined #bitcoin-core-pr-reviews
21:37 -!- instagibbs [~instagibb@pool-100-15-126-231.washdc.fios.verizon.net] has quit [Quit: Ping timeout (120 seconds)]
21:38 -!- instagibbs [~instagibb@pool-100-15-126-231.washdc.fios.verizon.net] has joined #bitcoin-core-pr-reviews
23:14 -!- jonatack1 [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
23:16 -!- jon_atack [~jonatack@user/jonatack] has quit [Ping timeout: 240 seconds]
23:25 -!- jonatack2 [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
23:25 -!- jonatack1 [~jonatack@user/jonatack] has quit [Read error: Connection reset by peer]
23:29 -!- jonatack3 [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
23:32 -!- jonatack2 [~jonatack@user/jonatack] has quit [Ping timeout: 255 seconds]
23:41 -!- jonatack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
23:41 -!- jonatack3 [~jonatack@user/jonatack] has quit [Ping timeout: 252 seconds]
23:54 -!- jon_atack [~jonatack@user/jonatack] has joined #bitcoin-core-pr-reviews
23:55 -!- jonatack [~jonatack@user/jonatack] has quit [Ping timeout: 265 seconds]
23:56 -!- __gotcha [~Thunderbi@94.105.119.42.dyn.edpnet.net] has quit [Ping timeout: 240 seconds]
--- Log closed Thu Apr 20 00:00:38 2023