19:01:18 #startmeeting 19:01:18 Meeting started Thu Mar 30 19:01:18 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot. 19:01:18 Useful Commands: #action #agreed #help #info #idea #link #topic. 19:01:41 hi 19:01:46 here 19:01:56 BlueMatt: it shouldn't matter for the AbortNode() case though. Just make the conditional wait 200 milliseconds or so 19:02:03 topics? 19:02:24 doesn't matter if signals are slightly delayed 19:02:30 wumpus: the wait is already 200ms 19:02:30 yes, topics? 19:02:34 talk about abortnode 19:02:36 thats a reasonable topic 19:02:38 BlueMatt: yes, but make it a wait on a conditional 19:02:44 mmm, fair 19:02:56 yes, for sigterm its reasonable to wait a few 100 ms 19:03:03 for AbortNode it'll wake it immediately 19:03:06 right. 19:04:15 in other news, so we got 1/6 "Review Priority" PRs merged this week, that's ok, but we can do better :) 19:04:45 oh, nvm, got 2/6 19:04:49 FYI: https://github.com/bitcoin/bitcoin/projects/8 19:05:26 BlueMatt: 2/6. 19:05:28 removed the two that have been merged 19:05:28 I don't think anyone commented in mine, at least I got review in others 19:05:49 I forgot about the project tracker thing, but reviewed some of them anyways. 19:06:32 doesn't matter, we'll only add pulls that were mentioned to have priority in the meeting to that project tracker, so if you pay attention at the meeting you don't need it :) 19:07:31 anyhow, everyone go review them 19:08:21 topic: 0.14.1? 19:08:29 [13bitcoin] 15sipa opened pull request #10126: Compensate for memory peak at flush time (06master...06peak_compensation) 02https://github.com/bitcoin/bitcoin/pull/10126 19:08:29 Yes, please. 19:08:33 topic: slow tests? 19:08:37 #topic 0.14.1 19:09:02 already? 19:09:25 yes. lots of nice fixes to ship. Minor releases are minor. 19:09:39 we have 11 merged PRs marked 0.14.1 19:09:59 and three open pulls https://github.com/bitcoin/bitcoin/pulls?q=is%3Aopen+is%3Apr+milestone%3A0.14.1 19:10:32 sipa: only one still has the "needs backport" tag, I think MarcoFalke ported the rest 19:10:44 I think when we get those three in and those and the recent ones backported, we should be goof for a 0.14.1. 19:11:11 good too. 19:11:41 agreed 19:12:45 ok, next topic I guess 19:12:48 #topic slow tests 19:13:11 I made an overview here: https://github.com/bitcoin/bitcoin/issues/10026 of the slowest unit tests 19:13:15 A lot of it is my fault it seems. https://github.com/bitcoin/bitcoin/pull/10099 is ready to go 19:13:27 some of those have already been worked on or even PRs open to make them faster 19:13:36 jnewbery: if you're around, if 10106 doesn't move forward in a few hours, I recommend you create a new pr, cherry picking that one fix, with your new tests and the fix for the other function. (just because the PR wasn't opened by a regular so they may not be responsive or may not know how to use github well enough to pull your changes.) 19:14:12 gmaxwell: I'll do that this afternoon 19:14:43 yes, tend to agree, they may not know how to cherry-pick them. I could cherry-pick them into his branch but meh 19:14:53 wumpus: Yes, dropping GetRand() gave a 4* speedup. 19:15:02 jnewbery: and thanks for the awesome response. 19:15:08 np 19:15:37 MarcoFalke_lab: that'll at least move it down a bit in the top 20 19:15:42 The cuckoocache tests require a bit more discussion to decrease the parameters; I can pick something arbitrary 19:16:27 The checkqueue tests should also speed up a lot with #9938, but I'm preparing some tweaks to those 19:16:28 https://github.com/bitcoin/bitcoin/issues/9938 | Lock-Free CheckQueue by JeremyRubin · Pull Request #9938 · bitcoin/bitcoin · GitHub 19:16:46 re tests: Has anyone given any thought to #8469 , obviously this pull request sacrafices speed for exhaustiveness 19:16:48 https://github.com/bitcoin/bitcoin/issues/8469 | [POC] Introducing property based testing to Core by Christewart · Pull Request #8469 · bitcoin/bitcoin · GitHub 19:17:02 If anyone has high-core machines, they should try benching how that works 19:17:02 jeremyrubin: alternatively we could have an -extended mode for the unit tests, like we have for the functional tests, to do extra-thorough tests that shouldn't run every time 19:17:13 jeremyrubin: If the parameters matter, you could provide a switch to test against quick_run and an expensive one 19:17:20 yes ^^ 19:17:27 I think we should have an extended test, and make running it part of the release process. 19:17:42 Agree 19:17:45 but for the standard 'make check' there should be a guideline of max ~1 second per test case 19:17:59 Everyone agrees! :) 19:18:07 I don't care if the tests take an hour to run if it's only a mandatory once in release thing. 19:18:09 sgtm 19:18:11 no, 2 seconds! 19:18:18 gmaxwell: yes or once per day or so on master 19:18:35 (which is failing currently) 19:18:49 jonasschnelli: what is failing? 19:18:49 yes, travis is broken again, but there's a pull to fix that 19:18:52 note to self: gitian should run whatever extended tests it can 19:18:55 oh travis 19:18:58 gmaxwell: https://travis-ci.org/bitcoin/bitcoin/builds 19:19:13 Jup, will merge the travis fix tonight when I have access to my keys. (If no one beats me to it) 19:19:31 does anyone have the # perhaps? 19:19:58 i think #10114? 19:19:59 Though, we should be cautious with putting too much into the cron jobs. The extended test rely on the cache being up to date 19:19:59 https://github.com/bitcoin/bitcoin/issues/10114 | An error has occurred and has been logged. Please contact this bot's administrator for more information. 19:20:10 Otherwise they would break the 49 minute limit on traivs 19:20:11 there's also #10072 19:20:12 https://github.com/bitcoin/bitcoin/issues/10072 | Remove sources of unreliablility in extended functional tests by jnewbery · Pull Request #10072 · bitcoin/bitcoin · GitHub 19:20:39 but yes 114 was the one I meant 19:20:40 We should probably contemplate having some seperate process against master that does continual gitian builds or something. 19:21:10 jonasschnelli: Is doing that, gmaxwell 19:21:22 oh good. 19:21:25 ignore me. 19:21:29 gmaxwell: https://bitcoin.jonasschnelli.ch 19:21:32 yes, jonasschnelli has a build server with a very nice web UI 19:21:43 jonasschnelli: Are you running the tests? 19:21:44 (it's currently by manual trigger) 19:21:50 no.. just gitian 19:21:55 somehow I missed this. cool. 19:22:08 travis already runs the tests, this is to get executables for testing 19:23:31 travis is only broken now because we've set it to run the extended tests once per day, so we're currently flushing out all the extended tests that have always failed on travis. I think once #10114 and #10072 are merged the daily runs should succeed reliably 19:23:32 https://github.com/bitcoin/bitcoin/issues/10114 | An error has occurred and has been logged. Please contact this bot's administrator for more information. 19:23:33 https://github.com/bitcoin/bitcoin/issues/10072 | Remove sources of unreliablility in extended functional tests by jnewbery · Pull Request #10072 · bitcoin/bitcoin · GitHub 19:24:08 thanks jnewbery for the info (and the fixes) 19:25:56 [13bitcoin] 15sdaftuar opened pull request #10127: [0.14 backport] Mining: Prevent slowdown in CreateNewBlock on large mempools (060.14...062017-03-backport-cnb-optimizations) 02https://github.com/bitcoin/bitcoin/pull/10127 19:26:48 ok, any other topics? 19:27:41 [13bitcoin] 15laanwj pushed 3 new commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/cde9b1a864c1...edc62c959ab3 19:27:42 13bitcoin/06master 146426716 15John Newbery: Add send_await_disconnect() method to p2p-compactblocks.py... 19:27:42 13bitcoin/06master 146a18bb9 15John Newbery: [tests] sync_with_ping should assert that ping hasn't timed out... 19:27:43 13bitcoin/06master 14edc62c9 15Wladimir J. van der Laan: Merge #10114: [tests] sync_with_ping should assert that ping hasn't timed out... 19:27:49 I should make notes for topics... 19:27:57 oh, new prs 19:28:02 for review priority 19:28:12 looks like jonasschnelli already added his 19:28:16 [13bitcoin] 15laanwj closed pull request #10114: [tests] sync_with_ping should assert that ping hasn't timed out (06master...06improve_sync_with_ping) 02https://github.com/bitcoin/bitcoin/pull/10114 19:28:21 anyone else have something to add (aside from 0.14.1-tagged things) 19:28:22 fine with me, but don't forget the current bunch :p 19:28:32 yes please review 0.14.1 tagged things 19:28:35 although those should be easy 19:28:36 BlueMatt: Yes. The bumpfee refactor (to get a QT fee bump function) 19:28:53 0.14.1 has priority, but i'd like to see #9792 in at some point to further the get-rid-of-openssl thing :) 19:29:01 wumpus: there is a "For backport" column there... 19:29:02 https://github.com/bitcoin/bitcoin/issues/9792 | FastRandomContext improvements and switch to ChaCha20 by sipa · Pull Request #9792 · bitcoin/bitcoin · GitHub 19:29:08 but I think we should be able to do 0.14.1 tomorrow so to have something to review for the rest of the week :D 19:29:22 BlueMatt: yes good point 19:29:53 ok, so morcos and sdaftuar get to propose new ones since they dont have ones up, anyone else want to propose ones? 19:29:55 oh someone opened a PR to do something with debug log excludes, and it adds more insane string processing in the debugging critical path. Would anyone mind if I brought back the PR I shelved to make debug categories use an enum? 19:30:17 gmaxwell: wfm, the pr was jnewbery's 19:30:20 gmaxwell: ack enum debug categories 19:30:40 gmaxwell: not sure 19:30:43 #10123 19:30:44 https://github.com/bitcoin/bitcoin/issues/10123 | Allow debug logs to be excluded from specified component by jnewbery · Pull Request #10123 · bitcoin/bitcoin · GitHub 19:30:48 (the PR was just shelved because I opened it right before a release, and it is a conflict-the-universe PR) 19:30:59 gmaxwell: well no I guess it makes sense 19:31:00 gmaxwell: go for it now, I'd say 19:31:05 yes, do it 19:31:07 i'd like to get this DisconnectTip improvement wrapped up (#9208) but i need some help on resolving this annoying issue BlueMatt raised 19:31:08 https://github.com/bitcoin/bitcoin/issues/9208 | Improve DisconnectTip performance by sdaftuar · Pull Request #9208 · bitcoin/bitcoin · GitHub 19:31:08 gmaxwell: +1. 19:31:14 Yes. ACK 19:31:23 along the same lines, I'd like to do something similar for net messages 19:31:23 then you can also map the enum values to strings and automate the help message generation 19:31:26 #9424 19:31:27 https://github.com/bitcoin/bitcoin/issues/9424 | Change LogAcceptCategory to use uint32_t rather than sets of strings. by gmaxwell · Pull Request #9424 · bitcoin/bitcoin · GitHub 19:31:30 sdaftuar: want to bring it up now/ 19:31:38 gmaxwell: +1. I'll happily review that one 19:31:38 ? 19:31:48 cfields: yes, should be fairly easy to do, I already changed the syntax for net messages to be enum-like at some point 19:31:49 since #8855 didn't got new review nor re-review I think just leave that (just remind to potential reviewers that #8994 continues it, in case you "don't see the point") 19:31:51 https://github.com/bitcoin/bitcoin/issues/8855 | Use a proper factory for creating chainparams by jtimon · Pull Request #8855 · bitcoin/bitcoin · GitHub 19:31:51 https://github.com/bitcoin/bitcoin/issues/8994 | Testchains: Introduce custom chain whose constructor... by jtimon · Pull Request #8994 · bitcoin/bitcoin · GitHub 19:32:15 cfields: went as far as I could without making it an actual enum :) 19:32:17 cfields: I looked at adding a perfect hash for net messages... but didn't know if that would be a way you'd want to go. :) 19:32:29 new for review this week is 9792 and 9208 19:32:35 wumpus: yes, it sure looks like an enum :) 19:32:37 gmaxwell: just make sure all debug category names have different length 19:33:12 sipa: well we don't need to do any runtime lookup of category names at all.. so no need to do anything performant there. 19:33:27 at least give them consecutive values so a bit field can be used to represent the set 19:33:33 intead of a per-thread map :-( 19:33:40 gmaxwell: i had considered making it an array. But I really don't care, as long as it's switchable and a compile-time constant 19:33:41 wumpus: that is what PR 9424 does. 19:33:52 gmaxwell: ok, great 19:33:53 wumpus: it assigns them each to a bit. 19:34:22 the current code that assigns it a thread-local variable is really strange 19:34:27 [13bitcoin] 15JeremyRubin opened pull request #10128: Speed Up CuckooCache tests (06master...06cuckoo-tests-faster) 02https://github.com/bitcoin/bitcoin/pull/10128 19:34:30 (and now that I think about it, std::array probably isn't switchable anyway) 19:34:39 yes. horrors from beyond time. 19:34:54 topic suggestion: dealing with abortnode / ConnectTip / DisconnectTip failures 19:35:05 in any case, if people support-- 9424 is hard to rebase because it touches the whole codebase. 19:35:30 cfields: in some modern languages it's possible to switch on compound types and arrays and strings, but no, not c++XX before XX=50 or so :) 19:35:33 but I think it also makes jnewbery's exclude trivial. (in fact: no runtime impact...) 19:35:50 gmaxwell: yes, it's how it should be done 19:36:09 in haskell it's pretty much the only way you can inspect a compound type at all :p 19:36:14 gmaxwell: agree 19:36:22 sipa: indeed 19:36:41 ok, sdaftuar's topic? 19:36:46 cfields: my intution for that would be using gperf to map the messages to integers. then switching on them... but perhaps overkill. 19:36:53 #topic dealing with abortnode / ConnectTip / DisconnectTip failures 19:37:05 context: #9208 19:37:06 https://github.com/bitcoin/bitcoin/issues/9208 | Improve DisconnectTip performance by sdaftuar · Pull Request #9208 · bitcoin/bitcoin · GitHub 19:37:09 sdaftuar: i saw i was pinged in the PR, but haven't read it 19:37:13 so this issue might be hard to grasp without reviewing #9208 19:37:15 https://github.com/bitcoin/bitcoin/issues/9208 | Improve DisconnectTip performance by sdaftuar · Pull Request #9208 · bitcoin/bitcoin · GitHub 19:37:29 gmaxwell: will that work for unknown messages too? 19:37:40 but basically matt brought up that we have some edge cases in our code if ConnectTip or DisconnectTip return false 19:37:54 gmaxwell: does a perfect hash handle collisions? I don't remember 19:38:07 wumpus: perfect hash means no collisions :) 19:38:13 cfields: for known values 19:38:34 wouldn't that not be backwards compatible? 19:38:36 wumpus: gperf can check, it's an option. 19:38:44 cfields: but what if 'moo' maps to the same 32-bit value as 'block' 19:38:50 someone sends you a new type of message that hashes to something else 19:39:05 scary 19:39:06 sdaftuar: hmm, i'll review the PR 19:39:10 the last issue i'm trying to resolve is if ConnectTip fails (but the block is not invalid), then we should be in an AbortNode() state. what consistency are we aiming for in this situation? 19:39:14 wumpus: ah, true 19:39:16 wumpus: making it never have collisions for unknows is just an extra comparison with the correct value. 19:39:35 wumpus: which IIRC the gperf emitted code will do, if enabled. 19:39:47 (actually, looks like it's the default) 19:39:47 lol, ok, so lets pick a topic instead of two 19:40:16 sdaftuar: we should be able to shut down and come back up with the underlying issue resolved and continue. 19:40:26 yes, sorry, I was just curous about gperf 19:40:30 sdaftuar: I don't care if we lose a bunch of recent blocks. 19:40:32 sdaftuar: it's fine if we lose progress in that case, but if at all avoidable, the on disk state should not be corrupted 19:40:44 anyway, so I further observed that shutdown upon AbortNode can take up to 200ms (see recent PR), which is somewhat frightening, given that mempool and chainstate may not be consistent which we assume in many places :/ 19:40:46 gmaxwell: sipa: what should we do with the mempool? 19:41:03 sdaftuar: who cares? 19:41:08 so we keep running for a while normally and possibly have incorrect assumptions 19:41:11 I don't care. 19:41:16 just drop it 19:41:29 sdaftuar: at restart we'll try to reimport what is on disk 19:41:36 and re-run all necessary consistency checks 19:41:51 so mempool.dat doesn't need to be consistent with the chainstate 19:41:59 and wallet? 19:42:01 i think the concern matt was raising was if before shutdown, it's possible for eg an RPC call to get incorrect results 19:42:04 or the wallet 19:42:08 yes, no tight coupling, good design 19:42:19 wumpus: point is we *have* tight coupling 19:42:22 wallet doesn't need to be in sync either, though it should not be ahead 19:42:24 sdaftuar: ugh 19:42:28 and continue running after realizing something is corrupted 19:42:35 well abortnode should be instant-ish. 19:42:42 BlueMatt: so you're fixing that, what is there to discuss? 19:42:46 a bit behind (as long as it's not further than the prune distance) is ok, it will catch up in next start 19:42:49 gmaxwell: i mean the new pr is an improvement, but its not a fix 19:43:02 its not like you cant have something pending cs_main when coming out of Disconnect/ConnectTip 19:43:06 but if the wallet is ahead of the chain it will trigger a rescan IIRC 19:43:10 so perhaps abortnode needs to Abort()? 19:43:18 gmaxwell: thats pretty much the question 19:43:21 thats super shit, though, of course 19:43:24 and not faffabout witht politely shutting off threads. 19:43:24 I don't think so 19:43:39 i inadvertently introduced an assert failure in one of these situations. maybe that's a feature not a flaw! :) 19:43:44 Why is it shit? we should be durable across a power off, which is worse than aborting. 19:43:45 the point of abortnode is to be able to show a GUI dialog to the user 19:43:55 ah 19:43:57 if you abort() the user will never know to check the debug.log 19:44:07 wumpus: well we can show a gui dialog and abort() prior to unlocking cs_main 19:44:09 :P 19:44:15 have a graceful shutdown falg 19:44:19 *flag 19:44:24 wumpus: but it's not unlikely that we can't show a gui... if we can't allocate (likely cause). 19:44:27 which would effectively be sufficient 19:44:29 I always thought that AbortNode was for errors that could tolerate a graceful shutdown 19:44:35 the really terrible errors we already assert() or abort() on 19:44:36 and if we're shutting down gracefully, write to a file "was graceful" 19:44:41 gmaxwell: AbortNode() is usually disk corruption 19:44:45 On next start, show dialogue first thing? 19:44:46 please don't make it routine to abort() for everything 19:44:53 only for things that really warrant it 19:45:00 not crash on every single error 19:45:02 or too little disk space 19:45:02 or too little disk space, not quite teh same as corruption 19:45:03 that's just a mess 19:45:21 okay, sure. 19:45:24 for some problems it's fine to try to clean up normally 19:45:31 but we still need to exit with a message 19:45:34 that's what abortnode is for 19:45:56 if BlueMatt can make it work faster that's great, but don't silently kill the program on every error 19:46:18 wumpus: how about every other error? 19:46:26 :P 19:46:33 gmaxwell: hehe 19:46:59 do #9208 and #9725 interact? 19:47:01 https://github.com/bitcoin/bitcoin/issues/9208 | Improve DisconnectTip performance by sdaftuar · Pull Request #9208 · bitcoin/bitcoin · GitHub 19:47:03 https://github.com/bitcoin/bitcoin/issues/9725 | CValidationInterface Cleanups by TheBlueMatt · Pull Request #9725 · bitcoin/bitcoin · GitHub 19:47:07 Y'all so worried about the dressings on the coffin. "It's already dead!" But sure. Sorry I was only thinking of OOM, it's just a recent subject. 19:47:19 sipa: they dont, afaik, aside from there now being two structs that can be merged 19:47:22 I think deleting a file on graceful shutdown should work 19:47:42 And then starting when that file is present shows the dialouge rather than starting 19:47:46 gmaxwell: yea, AbortNode isnt used for thigns like OOM and total death 19:47:49 disk space also does it 19:47:55 but it can also be db corruption 19:48:13 This means you don't do anything at all on the Abort, but requires the user to restart their node to see the message 19:48:15 so maybe the solution is AbortNode gets renamed to ShutdownSoon() and use make sure disk corruption is something different? 19:48:31 BlueMatt: yes, makes sense to me 19:48:33 BlueMatt: or add a flag 19:48:38 I haven't really thought this all the way through, but doesn't it seem that as we move more and more towards things happening in other threads, that you'd rather let those threads finish what they are doing 19:48:45 BlueMatt: you could also harden things up against your current concer by having the rpc stuff always check the shutdown flag right after taking cs_main (whenever it does)? 19:48:47 If you're committing wallet changes for instance 19:48:59 BlueMatt: and if it finds out that its on its way down, just returning at that point. 19:49:06 in the long run morcos we should move toward looser coupling of things 19:49:09 I agree 19:49:13 gmaxwell: yea, I'm worried that if we start down that path we have to check it everywhere 19:49:22 eg wallet may make decisions based on some corrupted mempool state 19:49:27 Let's say you just got some new address from the wallet, and the wallet hasn't committed that state yet and then you Abort() 19:49:31 (I havent thought all the way through all the potential issues here, just a potential concern) 19:50:19 morcos: luckily we have the keypool to handle that specific contingency 19:50:24 Every thread could have their own unique ungraceful-close file that it should delete (via RAII) on clean exit. Starting with any present would show error. Uncoupled! 19:50:40 morcos: well at least the worst you get there is replay (due to keypool/hdwallets).. though replay can be pretty bad. 19:51:11 pretty bad but well at least they will never lose live keys 19:51:47 jeremyrubin: I have a feeling we can be more agressive on the super-strange issues, afaiu this stuff is pretty much hit just with out-of-disk everything else is rare enough..... 19:52:36 maybe we should just have some std::atomic shits_on_fire_yo; which when set prevents RPCs etc 19:52:46 jeremyrubin: that's not what I mean with uncoupling, what I mean is that if one thread messes up it doesn't need to take the others with it because it can operate more-or-less independently 19:52:58 sipa: well shutdown can be more or less that. 19:53:14 that can even be set from a signal handler 19:53:34 20<BlueMatt>30 so maybe the solution is AbortNode gets renamed to ShutdownSoon() and use make sure disk corruption is something different? 19:53:53 sipa: what do you do with an in progress RPC? 19:54:04 in ShutdownSoon cases (eg out of disk) we're ok to continue and just shut down, i think 19:54:08 jeremyrubin: we can only do so much 19:54:08 you can't generally break off an in-progress RPC 19:54:11 in other cases, we can just assert(false) 19:54:13 although some will pay attention to fShutdown 19:54:15 because they're super rare anyway 19:54:21 wumpus: ^ yes, O 19:54:21 but really, what probably should be done is having the rpcs being able to handle being told "No, you can't have access to [whatever chain state you wanted] right now." then these error conditions that could leave them unstable... could set them. 19:54:26 and if your disk is corrupted we probably shouldnt be flushing wallet and chainstate as a part of shutdown anyway 19:54:47 gmaxwell: yea, but thats a huge rewrite for cases that should be super rare 19:55:11 Or we take a whole different approach to failure messages, .e.g. wrap bitcoin-qt in another program that when qt exists it can go through the logs and give you a useful error. (though this doesn't answer morcos' wallet flush, but really that should be in another process.) 19:55:26 BlueMatt: I think there should be a clear separation between (A) I/O issues such as out of disk spae, which just happen regularly (B) rare implementation issues such as internal consistency errors 19:55:27 iirc rpc has a IsRPCShuttingDown() or so, but only a few things (gbt only, maybe?) checks it 19:55:28 wumpus: * yes, I'm aware that isn't quite what you meant, just making noise about having a graceful shutdown file because I think it's a reasonable way to mark if a node shut down dirty 19:55:44 (A) normal errors should just give the user an error as AbortNode does now and shut down properly 19:56:02 (B) internal state corruption errors could break off the process immediately 19:56:02 It's obnoxious that we can't preallocate resources such that we can guarentee that we never take a failure at an inoppturtune time. 19:56:14 having to slather the code in error handling to deal with that is not ideal. 19:56:41 gmaxwell: you can preallocate lots of things? 19:56:44 wumpus: "I had to abort processing a block" is a weak kind of internal state corruption. It leaves assumed invariants violated. 19:56:57 jeremyrubin: in particular we can't make leveldb's disk usage constant. 19:56:58 wumpus: yes, thats my proposal 19:57:12 wumpus: but, specifically, B would include things like chainstate-on-disk-corruption 19:57:17 which it already partially does, just not completely 19:57:26 having code to handle normal errors is perfectly normal and all code has that, I agree that paranoid memory/disk corruption errors tend not to be possible to handle in a sane way 19:57:33 BlueMatt: yes 19:57:48 ok, soooo, acks on: so maybe the solution is AbortNode gets renamed to ShutdownSoon() and use make sure disk corruption is something different? 19:58:07 BlueMatt: as I said beforer, yes, that or add a flag/criticality level 19:58:11 s/use make sure/make sure/ 19:58:15 BlueMatt: maybe if you paste it again 19:58:15 sure, that too 19:58:23 jeremyrubin: ok, ok, soooo, acks on: so maybe the solution is AbortNode gets renamed to ShutdownSoon() and use make sure disk corruption is something different? 19:58:39 wumpus: for example, if we run out of space while flushing our view of the blockchain will be corrupt, and information about the blockchain from rpcs will be wrong. In a world where error handling is free, every code path that gets access to the blockchain data would be able to gracefully handle being told it's just not available. But I think that would be an unreasonable and dangerous amount 19:58:45 of complexity. :( 19:58:50 wumpus: I missed that statement, but, yes 19:59:00 BlueMatt: that sounds okay to me. but I don't know that diskfull is really a shutdown soon though we really do need to give it a nice message. :( 19:59:03 gmaxwell: note that the out-of-disk error happens *before* the disk is entirely full 19:59:04 BlueMatt: sgtm 19:59:12 gmaxwell: there's a threshold 19:59:22 gmaxwell: so that should already be mitigated 19:59:26 wumpus: yes, but it's not atomic. you can't check and then successfully allocate space. 19:59:38 no, it's not perfect, but it works pretty well 19:59:43 I've never had corruption on full disk 20:00:33 also leveldb write failing shouldn't generally be fatal 20:00:39 on my desktop, which runs with debug=1 it almost always gets checked at the start of the flush. It doesn't corrupt things on disk, but as matt points out the rpc would return somewhat incorrect results during that time. 20:00:39 it just means the last transaction is not committed 20:01:00 it seems it's time to abort the meeting 20:01:06 #endmeeting