bitcoin-git has left #bitcoin-core-dev [#bitcoin-core-dev]
jespada has joined #bitcoin-core-dev
SpellChecker has quit [Remote host closed the connection]
SpellChecker has joined #bitcoin-core-dev
sudoforge has quit [Ping timeout: 260 seconds]
Guest27 has joined #bitcoin-core-dev
Guest27 has quit [Client Quit]
sudoforge has joined #bitcoin-core-dev
AaronvanW has joined #bitcoin-core-dev
bitcoin-git has joined #bitcoin-core-dev
<bitcoin-git>
[bitcoin] KevinMusgrave opened pull request #24800: lint: convert lint-python-mutable-default-parameters.sh to Python (master...port-lint-script-to-python) https://github.com/bitcoin/bitcoin/pull/24800
bitcoin-git has left #bitcoin-core-dev [#bitcoin-core-dev]
jarthur has joined #bitcoin-core-dev
noonien2 has joined #bitcoin-core-dev
noonien has quit [Ping timeout: 246 seconds]
noonien2 is now known as noonien
agrosant has quit [Ping timeout: 272 seconds]
Talkless has joined #bitcoin-core-dev
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
gnaf has joined #bitcoin-core-dev
jonatack has joined #bitcoin-core-dev
bomb-on has joined #bitcoin-core-dev
Talkless has quit [Quit: Konversation terminated!]
Talkless has joined #bitcoin-core-dev
vysn has quit [Ping timeout: 268 seconds]
bitcoin-git has joined #bitcoin-core-dev
<bitcoin-git>
[bitcoin] Eunoia1729 opened pull request #24802: lint: convert format strings linter test to python (master...lint-format-strings-py) https://github.com/bitcoin/bitcoin/pull/24802
bitcoin-git has left #bitcoin-core-dev [#bitcoin-core-dev]
Kaizen_Kintsugi_ has quit [Remote host closed the connection]
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
Kaizen_K_ has joined #bitcoin-core-dev
Kaizen_Kintsugi_ has quit [Ping timeout: 256 seconds]
Kaizen_K_ has quit [Remote host closed the connection]
<laanwj>
welcome to the weekly general bitcoin-core-dev meeting
<sipa>
hi
<cfields>
hi
<laanwj>
no meeting topics have been proposed in advance (you can propose meeting topics with #proposedmeetingtopic <topic> during any time of the week)
<laanwj>
any last minute ones?
<jonatack>
#proposedmeetingtopic approach regarding user behavior and config options deprecation (e.g. adjusted time)
<laanwj>
ok!
<lightlike>
hi
<laanwj>
let's start with high priority for review as usual
<sipa>
note that BIP324's spec is undergoing some changes still... the protocol will likely not be interoperable
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
<laanwj>
lightlike: it does still have some unaddrssed comments, agree it's close though
nick_ has joined #bitcoin-core-dev
<laanwj>
sipa: right, that's good to make clear
mikolaj has joined #bitcoin-core-dev
<laanwj>
#topic Approach regarding user behavior and config options deprecation (jonatack)
<core-meetingbot>
topic: Approach regarding user behavior and config options deprecation (jonatack)
<jonatack>
hi! i've prepared a dozen lines on this then propose we open up for discussion
<jonatack>
there has been discussion recently on the topic of removing the adjusted time code
<jonatack>
which has been been in bitcoin core since the first commit in the git log
<jonatack>
there is some good refactoring that can be done
<jonatack>
nevertheless, i'm somewhat uncomfortable with the approach i'm seeing
<jonatack>
in that we may be making what are essentially product management decisions, to drop a feature and configuration option
<jonatack>
without a deprecation period or optionality for users
<laanwj>
i like the approach to first default the setting to 0
Kaizen_Kintsugi_ has quit [Remote host closed the connection]
<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)
<jonatack>
i worry that it's not necessarily a good basis for making these decisions, and in some contexts a flimsy one
<jonatack>
istm users aren't necessarily logical, and if users can do something, then some likely will
<jonatack>
i checked in with a couple of well-known Bitcoin PMs today and they aren't aware
<jonatack>
of any Bitcoin Core user studies on this topic (which doesn't seem surprising,
<jonatack>
and maybe they are busy in miami atm, but the answer was: "no idea")
<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
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
<sipa>
i wouldn't consider time adjustment to be a user-visible feature
<jonatack>
yes. though if we don't know if people depend on it, perhaps we can proceed on that basis
<fanquake>
Yes. This isn't quite like deprecating a well-used RPC
<sipa>
removing/changing it shouldn't affect user experience
<laanwj>
how can people *depend* on it?
<jonatack>
in conclusion, it seems like a deprecation process for configuration options would be of help here, and maybe save us discussion going forward.
<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?
<laanwj>
i would agree for user visible wallet features and such
Cory has quit [Ping timeout: 246 seconds]
<sipa>
RPC / REST / Wallet behavior / UI features / ... changes need a deprecation plan
<laanwj>
yes
<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
<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.
<fanquake>
for anyone following along the related PRs are #24606 and #24662
<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
<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
<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
<laanwj>
yes
<sipa>
but that's more about how certain we are it's an improvement, than about user expectations
<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
<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?
<laanwj>
no, if you remove it, specifying it should be an error
<laanwj>
silently ignoring options is really bad
<fanquake>
by no-op I mean changing the default to 0
<fanquake>
then we'd have an option that is confusing / doesn't do anything for new users
<laanwj>
yes, but you can mention it in the release notes
<fanquake>
and for anyone already setting it, they just wont notice anyway
<fanquake>
if anything you'd probably be better off with just the release note?
<laanwj>
that if they rely on the feature (unlikely), they can specify the option for now
Kaizen_Kintsugi_ has quit [Remote host closed the connection]
<laanwj>
well if it causes problems they can report it as an issue?
<laanwj>
then set the setting
<laanwj>
it's easier to do without recompile
<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
<sipa>
... but I can't imagine that being the case, really
<laanwj>
yea, ok...
<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.
<laanwj>
maybe log a deprecation notice first
<_aj_>
for this, it seems easier to fix your computer's time than to complain about it...
<sipa>
+1 aj
<laanwj>
yes, if you know you have problems with you rsystem time, it's better to fix it at the source
<jonatack>
users may not behave logically or the way we think they do, though
<laanwj>
lightlike: agree, in addrman it seems pointless in any case
<fanquake>
there's not much we can do about a user who just refuses to fix their clock
<_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)
<laanwj>
the clock can't be *that* far off anyway
<fanquake>
they've probably got other non-bitcoin issues in that case too
<laanwj>
as it won't compensate that far
<laanwj>
right
<jonatack>
lightlike: that seems orthogonal (reviewer approach)
<sipa>
being even minutes off probably doesn't hurt much even
<sipa>
and we don't correct more than 60 or 70 minutes iirc?
<laanwj>
sipa: right
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
<laanwj>
pretty sure there are many things more time sensitive than bitcoin, e.g. 2fa tokens
<sipa>
indeed
<sipa>
that was different in 2009
<laanwj>
right, i don't think ntp was even default on most OSes back then
mikolaj has quit [Quit: mikolaj]
<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
<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
<laanwj>
but i feel we already had this discussion very recently
<jonatack>
yes, it's more the process that's in question i think
<laanwj>
i get the idea this gets really blown out of proportion compared to the impact
<laanwj>
there's probably a few more risky things we merge each week than this particular deprecation
<fanquake>
Yea. Getting rid of this is hardly like getting rid of getinfo
<laanwj>
right
<sipa>
agree
Talkless has quit [Quit: Konversation terminated!]
Kaizen_Kintsugi_ has quit [Remote host closed the connection]
<laanwj>
jonatack: how would you propose to move forward with this, then
<laanwj>
i mean the "set the default to 0" PR is yours so i guess that'd be the first step?
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
<laanwj>
then what next release?
<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
<jonatack>
and that's maybe not transferable to this context, but if we're not in a hurry and not sure, yeah
<jonatack>
i could be wrong of course, and will learn from why
<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
<_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?
<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
<sipa>
_aj_: the numbers may be skewed because those numbers themselves are subject to adjustment?
<_aj_>
sipa: and doesn't look like they're logged after the buffer gets filled either
<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.
<laanwj>
in any case, that's not a deprecation plan
<_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?
<laanwj>
sipa: right
<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.
<sipa>
(in positive or negative sense)
<laanwj>
id say light clients would tend to be biased towards phones which have many ways of potential time synchronization
<jonatack>
sipa: agree. but how to remove a config option once it's there?
<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).
<laanwj>
jonatack: that should be considered on a case by case basis i think, like what and how much effect does it have
<jonatack>
(this may come up again if we add sat/vB config options to replace BTC/kvB ones)
<sipa>
jonatack: I think that's different; that's RPC, which is a user interface.
<sipa>
There we need migration options, and be sensitive on people relying on it.
<lightlike>
_aj_: the behavior used to be different for inbound/outbound (until #23695). For outbound connections, we always used the unadjusted time I think.
<laanwj>
fanquake: right! we shouldn't forget people always have the option of staying on an older release until something is resolved
<sipa>
lightlike: Oh, interesting.
<laanwj>
it's not some kind of cloud service where people are forced to the newest version
<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
<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.
<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.
<_aj_>
("secure ntp" aka "nts" is a thing these days apparently)
<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
<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
<laanwj>
with 'remove it' i mean removing both the option and the functionality
<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.
<fanquake>
My vote is for 2), and the further refactoring / code improvements it will enable
<sipa>
I'm fine with both (1) and (2).
<_aj_>
sipa: need to update logging to actually log that info though, yeah
<laanwj>
fanquake: is it blocking some other things?
<fanquake>
my understanding is that the affected code overlaps with other addrman / time related refactors, migrating further towards chrono etc
<fanquake>
unsure about in validation, I don't think there is a PR open for the removal there yet either
<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 :)
<fanquake>
in the worst case everything can always just be recovered from there too
<fanquake>
there are some demons hiding in .git
<laanwj>
right
<jonatack>
there is indeed user folklore about it but agree about not being sentimental
<jonatack>
the folklore can remain without the code
<laanwj>
exactly!
<_aj_>
okay, i've got some logging going, see if i catch any peers with a signfiicant delta
<laanwj>
and with that, we should probably close the meeting
<sipa>
it is indeed robot time
<jonatack>
oops, yes
<laanwj>
#endmeeting
<core-meetingbot>
Meeting ended Thu Apr 7 20:00:47 2022 UTC.
<sipa>
8 out of 253 inbound connections have timeoffset>0
<sipa>
2 have timeoffset>1
<sipa>
those two are 30s and 3788s off
<jonatack>
i've generally been -4 here per getnetworkinfo
Kaizen_Kintsugi_ has quit [Remote host closed the connection]
Guyver2 has quit [Quit: Going offline, see ya! (www.adiirc.com)]
<_aj_>
i have a 0.19.0.1 node that claims an offset of 1046 (17min) and a nodes.mom.market:0.2 (?) one that says -1795
<sipa>
kanzure: gnusha.org logging off this channels seems to have stopped
<sipa>
the 3788s off one is a /Satoshi:0.16.2/ node
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
<laanwj>
i also have two nodes.mom.market connections that have timeoffsets -1794 and -1795, i think they made a coding mistake where they send the local time instead of utc
bitcoin-git has joined #bitcoin-core-dev
<bitcoin-git>
[bitcoin] Eunoia1729 opened pull request #24803: linnt: convert submodule linter test to Python (master...lint-submodule-py) https://github.com/bitcoin/bitcoin/pull/24803
bitcoin-git has left #bitcoin-core-dev [#bitcoin-core-dev]
<laanwj>
though that's... half an hour, no, that wouldn't make sense
<lightlike>
can you geolocate the ip, if they are from some place on earth with a weird 30min time zone?
<jonatack>
yes ^^
brunoerg has quit [Remote host closed the connection]
Kaizen_Kintsugi_ has quit [Remote host closed the connection]
brunoerg has joined #bitcoin-core-dev
<laanwj>
it seems to be the same node, connected over ipv4 and ipv6, and it's in some datacenter in the Netherlands (UpCloud Ltd)
Kaizen_Kintsugi_ has joined #bitcoin-core-dev
<laanwj>
so no, no explanation
brunoerg has quit [Ping timeout: 240 seconds]
brunoerg has joined #bitcoin-core-dev
<laanwj>
sipa: you probably want to use timeoffset!=0 in your query, not >0
<sipa>
Oh, right.
<laanwj>
(no, jq doesn't have an abs function)
rex4539 has quit [Ping timeout: 240 seconds]
<sipa>
Sorry, my numbers above are totally off.
mudsip has joined #bitcoin-core-dev
brunoerg has quit [Ping timeout: 260 seconds]
brunoerg has joined #bitcoin-core-dev
gnaf has quit [Quit: Konversation terminated!]
brunoerg has quit [Ping timeout: 260 seconds]
AaronvanW has joined #bitcoin-core-dev
brunoerg has joined #bitcoin-core-dev
brunoerg has quit [Ping timeout: 248 seconds]
Kaizen_Kintsugi_ has quit [Read error: Connection reset by peer]