19:00:17 <laanwj> #startmeeting 
19:00:17 <core-meetingbot> Meeting started Thu Apr  7 19:00:17 2022 UTC.  The chair is laanwj. Information about MeetBot at https://bitcoin.jonasschnelli.ch/ircmeetings.
19:00:18 <core-meetingbot> Available commands: action commands idea info link nick
19:00:38 <hebasto> hi
19:00:55 <laanwj> #bitcoin -core-dev Meeting: achow101 _aj_ amiti ariard BlueMatt cfields Chris_Stewart_5 darosior digi_james dongcarl elichai2 emilengler fanquake fjahr gleb glozow gmaxwell gwillen hebasto instagibbs jamesob jarolrod jb55 jeremyrubin jl2012 jnewbery jonasschnelli jonatack jtimon kallewoof kanzure kvaciral laanwj larryruane lightlike luke-jr maaku marcofalke meshcollider michagogo moneyball
19:00:57 <laanwj> morcos nehan NicolasDorier paveljanik petertodd phantomcircuit promag provoostenator ryanofsky sdaftuar sipa vasild
19:01:04 <jonatack>  hi
19:01:06 <fanquake> hi
19:01:07 <laanwj> welcome to the weekly general bitcoin-core-dev meeting
19:01:08 <sipa> hi
19:01:18 <cfields> hi
19:01:42 <laanwj> no meeting topics have been proposed in advance (you can propose meeting topics with #proposedmeetingtopic <topic> during any time of the week)
19:01:49 <laanwj> any last minute ones?
19:01:54 <jonatack> #proposedmeetingtopic approach regarding user behavior and config options deprecation (e.g. adjusted time)
19:02:18 <laanwj> ok!
19:02:34 <lightlike> hi
19:02:56 <laanwj> let's start with high priority for review as usual
19:03:01 <laanwj> #topic High priority for review
19:03:02 <core-meetingbot> topic: High priority for review
19:03:38 <laanwj> https://github.com/bitcoin/bitcoin/projects/8  10 blockers, 1 chasing concept ACK
19:03:58 <fanquake> I'll add #22380. Should probably get that in at some point
19:03:58 <laanwj> anything to add/remove, or that is on the list and almost ready for merge?
19:03:59 <gribble> https://github.com/bitcoin/bitcoin/issues/22380 | build: add and use C_STANDARD and CXX_STANDARD in depends by fanquake · Pull Request #22380 · bitcoin/bitcoin · GitHub
19:04:55 <laanwj> fanquake: ok
19:07:02 <laanwj> oh PSA: i've started testing BIP324 P2P v2 (#24545) on a node, let me know if you want to also test and connect
19:07:04 <gribble> https://github.com/bitcoin/bitcoin/issues/24545 | BIP324: Enable v2 P2P encrypted transport by dhruv · Pull Request #24545 · bitcoin/bitcoin · GitHub
19:07:17 <jonatack> laanwj: +1
19:07:19 <laanwj> (in private is fine, this is not a meeting topic)
19:07:22 <jonatack> great, i plan to
19:07:50 <laanwj> cool!
19:08:10 <lightlike> maybe #21726 could be close (after next rebase)?
19:08:13 <gribble> https://github.com/bitcoin/bitcoin/issues/21726 | Improve Indices on pruned nodes via prune blockers by fjahr · Pull Request #21726 · bitcoin/bitcoin · GitHub
19:08:42 <sipa> note that BIP324's spec is undergoing some changes still... the protocol will likely not be interoperable
19:09:25 <laanwj> lightlike: it does still have some unaddrssed comments, agree it's close though
19:10:06 <laanwj> sipa: right, that's good to make clear
19:11:52 <laanwj> #topic Approach regarding user behavior and config options deprecation (jonatack)
19:11:52 <core-meetingbot> topic: Approach regarding user behavior and config options deprecation (jonatack)
19:12:03 <jonatack> hi! i've prepared a dozen lines on this then propose we open up for discussion
19:12:11 <jonatack> there has been discussion recently on the topic of removing the adjusted time code
19:12:21 <jonatack> which has been been in bitcoin core since the first commit in the git log
19:12:33 <jonatack> there is some good refactoring that can be done
19:12:38 <jonatack> nevertheless, i'm somewhat uncomfortable with the approach i'm seeing
19:12:57 <jonatack> in that we may be making what are essentially product management decisions, to drop a feature and configuration option
19:13:21 <jonatack> without a deprecation period or optionality for users
19:13:52 <laanwj> i like the approach to first default the setting to 0
19:13:55 <jonatack> the idea seems to be based eagerness to get started removing it and on on speculation or hand-wavey logic about what users would do (or never do)
19:14:07 <jonatack> i worry that it's not necessarily a good basis for making these decisions, and in some contexts a flimsy one
19:14:16 <jonatack> istm users aren't necessarily logical, and if users can do something, then some likely will
19:14:23 <jonatack> i checked in with a couple of well-known Bitcoin PMs today and they aren't aware
19:14:24 <jonatack> of any Bitcoin Core user studies on this topic (which doesn't seem surprising,
19:14:24 <jonatack> and maybe they are busy in miami atm, but the answer was: "no idea")
19:14:26 <laanwj> my main problem with adjusted time is that it just doesn't do what it's supposed to do, it is a false promise, and unsafe
19:15:09 <sipa> i wouldn't consider time adjustment to be a user-visible feature
19:15:19 <jonatack> yes. though if we don't know if people depend on it, perhaps we can proceed on that basis
19:15:21 <fanquake> Yes. This isn't quite like deprecating a well-used RPC
19:15:27 <sipa> removing/changing it shouldn't affect user experience
19:15:28 <laanwj> how can people *depend* on it?
19:15:51 <jonatack> in conclusion, it seems like a deprecation process for configuration options would be of help here, and maybe save us discussion going forward.
19:15:54 <fanquake> If we want a period of deprecation, I'm also unsure how changing the default helps things? That isn't going to communicate anything to users who are actually setting it right?
19:16:21 <laanwj> i would agree for user visible wallet features and such
19:16:57 <sipa> RPC / REST / Wallet behavior / UI features / ... changes need a deprecation plan
19:17:28 <laanwj> yes
19:17:30 <sipa> but time adjustment is just an internal algorithm... if we believe that not doing it is better than what we have, we should get rid of it
19:17:45 <fanquake> and if the feature is broken, and that brokenness is leaking out into other parts of the codebase, it would just seem better to remove it.
19:18:33 <fanquake> for anyone following along the related PRs are #24606 and #24662
19:18:35 <gribble> https://github.com/bitcoin/bitcoin/issues/24606 | Change -maxtimeadjustment default from 70 minutes to 0 by jonatack · Pull Request #24606 · bitcoin/bitcoin · GitHub
19:18:36 <gribble> https://github.com/bitcoin/bitcoin/issues/24662 | addrman: Use system time instead of adjusted network time by MarcoFalke · Pull Request #24662 · bitcoin/bitcoin · GitHub
19:18:39 <jonatack> istm that starting by changing the default first, for a release or so, reduces  risk and provides optionality for us (i.e. code reversion) and for users  -- unless there is an urgent need for change, or unless we are certain that it no one uses or depends on it
19:18:49 <laanwj> there are some things we'd really want to be careful around, like anything that potentialy causes coins to be unspendable, or makes old wallets unusable
19:19:45 <sipa> if we're unsure that not doing adjustment is better than what we have, defaulting it to max 0 minutes at first makes it somewhat easier to reintroduce
19:20:01 <laanwj> yes
19:20:03 <sipa> but that's more about how certain we are it's an improvement, than about user expectations
19:22:35 <laanwj> in any case, i'm fine with first defaulting it to 0 for a release, then if no one reports problems due to that (however unlikely), really remove it a major release later
19:22:52 <fanquake> I think turning the option into a no-op, without making it somehow apparent to users who are currently using it, would surely also just undermine user expectations if anything?
19:23:12 <laanwj> no, if you remove it, specifying it should be an error
19:23:16 <laanwj> silently ignoring options is really bad
19:23:34 <fanquake> by no-op I mean changing the default to 0
19:23:56 <fanquake> then we'd have an option that is confusing / doesn't do anything for new users
19:23:58 <laanwj> yes, but you can mention it in the release notes
19:24:08 <fanquake> and for anyone already setting it, they just wont notice anyway
19:24:28 <fanquake> if anything you'd probably be better off with just the release note?
19:24:30 <laanwj> that if they rely on the feature (unlikely), they can specify the option for now
19:24:45 <laanwj> well if it causes problems they can report it as an issue?
19:24:50 <laanwj> then set the setting
19:25:03 <laanwj> it's easier to do without recompile
19:25:06 <sipa> if we want people to complain if they're intentionally setting the option, because they rely on it, we should remove the option to configure it first
19:25:29 <sipa> ... but I can't imagine that being the case, really
19:25:31 <laanwj> yea, ok...
19:25:43 <lightlike> I don't really see how changing it globally to 0 reduces risk, I'd think it's more risky. Removing it step by step makes reviewer think about the location it is used, and about possible consequences. And maybe there are completely different things to take into account in validation than in addrman.
19:25:43 <laanwj> maybe log a deprecation notice first
19:25:47 <_aj_> for this, it seems easier to fix your computer's time than to complain about it...
19:26:02 <sipa> +1 aj
19:26:17 <laanwj> yes, if you know you have problems with you rsystem time, it's better to fix it at the source
19:26:57 <jonatack> users may not behave logically or the way we think they do, though
19:27:05 <laanwj> lightlike: agree, in addrman it seems pointless in any case
19:27:27 <fanquake> there's not much we can do about a user who just refuses to fix their clock
19:27:38 <_aj_> (adding a gui warning if your peers are advertising a different time than you have could be useful, though; i thought that was mentioned in the PRs, can't remember if it went anywhere)
19:27:45 <laanwj> the clock can't be *that* far off anyway
19:27:47 <fanquake> they've probably got other non-bitcoin issues in that case too
19:27:55 <laanwj> as it won't compensate that far
19:27:58 <laanwj> right
19:28:02 <jonatack> lightlike: that seems orthogonal (reviewer approach)
19:28:13 <sipa> being even minutes off probably doesn't hurt much even
19:28:28 <sipa> and we don't correct more than 60 or 70 minutes iirc?
19:28:36 <laanwj> sipa: right
19:29:24 <laanwj> pretty sure there are many things more time sensitive than bitcoin, e.g. 2fa tokens
19:29:40 <sipa> indeed
19:29:48 <sipa> that was different in 2009
19:30:35 <laanwj> right, i don't think ntp was even default on most OSes back then
19:31:26 <sipa> ntp being unauthenticated is a problem, probably a bigger one than the issues enabled by bitcoin's time adjustment... but nothing we can do about
19:33:00 <laanwj> i mean if bitcoin's time adjustment was a serious contender for protecting against ntp attacks things would be different, but it is effectively unmaintained, never reasoned about code, with known unfixed issues for years
19:33:30 <laanwj> but i feel we already had this discussion very recently
19:34:49 <jonatack> yes, it's more the process that's in question i think
19:34:52 <laanwj> i get the idea this gets really blown out of proportion compared to the impact
19:35:21 <laanwj> there's probably a few more risky things we merge each week than this particular deprecation
19:35:31 <fanquake> Yea. Getting rid of this is hardly like getting rid of getinfo
19:35:37 <laanwj> right
19:36:10 <sipa> agree
19:37:40 <laanwj> jonatack: how would you propose to move forward with this, then
19:39:24 <laanwj> i mean the "set the default to 0" PR is yours so i guess that'd be the first step?
19:39:39 <laanwj> then what next release?
19:40:00 <jonatack> laanwj: when i was doing PM at L'Oreal a looong time ago (early 90s) it was hammered into me to never guess or assume what users do. I don't feel competent to say more than that here yet
19:41:05 <jonatack> and that's maybe not transferable to this context, but if we're not in a hurry and not sure, yeah
19:42:10 <jonatack> i could be wrong of course, and will learn from why
19:42:19 <laanwj> i guess that's good general advice, but to do development at all you need to make some assumptions, like in a way a program is a set of assumptions, if not, everyone would have to write their own for their specific context and situation
19:42:33 <_aj_> hmm, shouldn't the version messages listening nodes receive be able to give a good indication how out-of-sync random nodes tend to be?
19:43:04 <fanquake> my assumption is that basically everyone running a node, other than people in this channel, have likely never even heard of this option, let alone changed it in some meaningful way
19:43:33 <sipa> _aj_: the numbers may be skewed because those numbers themselves are subject to adjustment?
19:44:38 <_aj_> sipa: and doesn't look like they're logged after the buffer gets filled either
19:45:17 <sipa> jonatack: I think we shouldn't think of configuration options themselves as features; they may be part of one, but not on their own. And for configuration options, I like the general advice I once heard: don't add a means to configure if you can't explain or give advice about when someone should set it. For maxtimeadjustment my advice would be: set it to 0, because if you have a need to set it to anything else your system is broken.
19:45:33 <laanwj> in any case, that's not a deprecation plan
19:45:41 <_aj_> sipa: i guess non bitcoind nodes don't do adjustment, so messages from light clients might give an indication of how many systems are out of sync?
19:46:04 <laanwj> sipa: right
19:46:28 <sipa> _aj_: That'd only tell us how many light clients are out of sync, which may be very different from how often bitcoin core nodes are.
19:46:41 <sipa> (in positive or negative sense)
19:47:19 <laanwj> id say light clients would tend to be biased towards phones which have many ways of potential time synchronization
19:47:28 <jonatack> sipa: agree. but how to remove a config option once it's there?
19:47:53 <fanquake> my deprecation plan would just be to just remove the option, and all of the old, broken code, prior to the 24.x branch off. If anyone who was using it even notices, and then for some reason, can't seem to operate without it, they could remain on 23.x while we figure out a non-broken, actually reasoned about alternative, that we could explain to them how to use (if we even want to do something like that going forward).
19:48:03 <laanwj> jonatack: that should be considered on a case by case basis i think, like what and how much effect does it have
19:48:08 <jonatack> (this may come up again if we add sat/vB config options to replace BTC/kvB ones)
19:48:34 <sipa> jonatack: I think that's different; that's RPC, which is a user interface.
19:48:53 <sipa> There we need migration options, and be sensitive on people relying on it.
19:48:54 <lightlike> _aj_: the behavior used to be different for inbound/outbound (until #23695). For outbound connections, we always used the unadjusted time I think.
19:48:56 <gribble> https://github.com/bitcoin/bitcoin/issues/23695 | p2p: Always serialize local timestamp for version msg by MarcoFalke · Pull Request #23695 · bitcoin/bitcoin · GitHub
19:49:22 <laanwj> fanquake: right! we shouldn't forget people always have the option of staying on an older release until something is resolved
19:49:23 <sipa> lightlike: Oh, interesting.
19:49:52 <laanwj> it's not some kind of cloud service where people are forced to the newest version
19:50:48 <fanquake> Yes, and if it comes to it, and there's one stubborn user who just wont budge, I'll buy them a fancy new computer, with a working clock, and we remain forgetful of this code
19:50:58 <sipa> A deprecation plan for time adjustment may be (a) just removing it and see who complains (because of a well-reasoned assumption that nobody actually needs it). (b) removing it, but adding for 1 or 2 major releases a trigger if it's set that explains why it was removed, and what to do about it (c) using something like -deprecatedrpc where you inform bitcoind that you're working on getting rid of your dependency on it, but need it for a little while longer.
19:51:38 <sipa> But there is more at stake here than just removing the "be tolerant to bad clocks" feature - having it in the first place I think is a (albeit small) risk on itself to the network.
19:51:42 <_aj_> ("secure ntp" aka "nts" is a thing these days apparently)
19:52:34 <laanwj> _aj_: which is the place where this should be solved, by people who study it and are specialized in it, fixing internet time sync is out of scope for bitcoin
19:53:49 <laanwj> i tend toward either (1) default the option to 0 for 24.0, then remove it entirely in 25.0 or 2) just remove it entirely for 24.0, we'll see if people complain
19:54:15 <laanwj> with 'remove it' i mean removing both the option and the functionality
19:54:44 <sipa> _aj_: So, actually, we can observe how much nodes are off just fine, on inbound connections, because bitcoind on outbound seems to not be using adjusted time.
19:54:49 <fanquake> My vote is for 2), and the further refactoring / code improvements it will enable
19:54:59 <sipa> I'm fine with both (1) and (2).
19:55:05 <_aj_> sipa: need to update logging to actually log that info though, yeah
19:55:11 <laanwj> fanquake: is it blocking some other things?
19:56:36 <fanquake> my understanding is that the affected code overlaps with other addrman / time related refactors, migrating further towards chrono etc
19:56:57 <fanquake> unsure about in validation, I don't think there is a PR open for the removal there yet either
19:57:16 <laanwj> re: "it's been in there since the first git release", we really shouldn't be sentimental about code, it'll always be there in git history :)
19:57:49 <fanquake> in the worst case everything can always just be recovered from there too
19:57:58 <fanquake> there are some demons hiding in .git
19:58:00 <laanwj> right
19:58:22 <jonatack> there is indeed user folklore about it but agree about not being sentimental
19:58:44 <jonatack> the folklore can remain without the code
19:59:29 <laanwj> exactly!
20:00:01 <_aj_> okay, i've got some logging going, see if i catch any peers with a signfiicant delta
20:00:31 <laanwj> and with that, we should probably close the meeting
20:00:43 <sipa> it is indeed robot time
20:00:45 <jonatack> oops, yes
20:00:47 <laanwj> #endmeeting