vasild has quit [Remote host closed the connection]
vasild has joined #bitcoin-core-dev
cmirror has quit [Remote host closed the connection]
cmirror has joined #bitcoin-core-dev
sudoforge has quit [Quit: 404]
SpellChecker has quit [Remote host closed the connection]
SpellChecker has joined #bitcoin-core-dev
Guest34 has joined #bitcoin-core-dev
Guest34 has quit [Quit: Client closed]
dviola has joined #bitcoin-core-dev
SpellChecker has quit [Ping timeout: 255 seconds]
SpellChecker has joined #bitcoin-core-dev
NorrinRadd has quit [Ping timeout: 268 seconds]
salvatoshi has joined #bitcoin-core-dev
jarthur has quit [Quit: jarthur]
TheRec_ has joined #bitcoin-core-dev
TheRec has quit [Ping timeout: 246 seconds]
stratospher[m] has quit [Read error: Software caused connection abort]
stratospher[m] has joined #bitcoin-core-dev
takinbo has quit [Read error: Software caused connection abort]
takinbo has joined #bitcoin-core-dev
bitdex has quit [Remote host closed the connection]
bitdex has joined #bitcoin-core-dev
ziggie has joined #bitcoin-core-dev
andrewtoth_ has quit [Remote host closed the connection]
andrewtoth_ has joined #bitcoin-core-dev
bitdex has quit [Remote host closed the connection]
bitdex has joined #bitcoin-core-dev
andrewtoth_ has quit [Remote host closed the connection]
andrewtoth_ has joined #bitcoin-core-dev
AaronvanW has joined #bitcoin-core-dev
kexkey has quit [Ping timeout: 260 seconds]
kexkey has joined #bitcoin-core-dev
andrewtoth_ has quit [Remote host closed the connection]
andrewtoth_ has joined #bitcoin-core-dev
steve__ has joined #bitcoin-core-dev
<fanquake>
lightlike: thanks for investigating further. So it seems the changes in #26328 are probably overcomplicated, given it's basically impossible to hit the third state (and not something we should have to worry about in -netinfo)?
bitdex has quit [Remote host closed the connection]
<MacroFake>
Code was contributed by aureleoules :)
bitdex has joined #bitcoin-core-dev
Guyver2 has joined #bitcoin-core-dev
andrewtoth_ has quit [Remote host closed the connection]
Guyver2 has left #bitcoin-core-dev [#bitcoin-core-dev]
andrewtoth_ has joined #bitcoin-core-dev
<fanquake>
I guess this means we should also partially revert https://github.com/bitcoin-core/gui/pull/676, which contained the same "(not available while the peer connection is being set up)"
bitdex has quit [Remote host closed the connection]
bitdex has joined #bitcoin-core-dev
bitdex has quit [Ping timeout: 255 seconds]
<fanquake>
> and need to sign off to present at a conf. Up to you all.
<fanquake>
jonatack: When you're done presenting, please clarify exactly what is going to "break user space and production systems", and which specific change(s) need backporting.
<fanquake>
I'm assuming it's only minfeefilter (setting aside whether this should be handled downstream), in which case, only 5c03df1fce744fbc357e32f66f9a50b8e609c386 from 26457 would be a last-minute blocker?
bitdex has joined #bitcoin-core-dev
steve__ has quit [Quit: Leaving]
steve__ has joined #bitcoin-core-dev
NorrinRadd has joined #bitcoin-core-dev
<aureleoules>
thanks MacroFake for reviewing the code!
SpellChecker has quit [Remote host closed the connection]
bitdex has quit [Remote host closed the connection]
yanmaani3 has quit [Remote host closed the connection]
NorrinRadd has quit [Ping timeout: 260 seconds]
SpellChecker has joined #bitcoin-core-dev
NorrinRadd has joined #bitcoin-core-dev
bitdex has joined #bitcoin-core-dev
yanmaani3 has joined #bitcoin-core-dev
bitdex has quit [Ping timeout: 255 seconds]
bitdex has joined #bitcoin-core-dev
NorrinRadd has quit [Ping timeout: 268 seconds]
NorrinRadd has joined #bitcoin-core-dev
bitdex_ has joined #bitcoin-core-dev
Guest95 has joined #bitcoin-core-dev
bitdex has quit [Remote host closed the connection]
Guest5 has joined #bitcoin-core-dev
Guest95 has quit [Client Quit]
Guest5 has quit [Client Quit]
brunoerg has joined #bitcoin-core-dev
yanmaani3 has quit [Ping timeout: 255 seconds]
yanmaani3 has joined #bitcoin-core-dev
andrewtoth_ has quit [Remote host closed the connection]
bitdex_ has quit [Remote host closed the connection]
bitdex_ has joined #bitcoin-core-dev
bitdex_ has quit [Remote host closed the connection]
<bitcoin-git>
[bitcoin] MarcoFalke merged pull request #25112: util: Move error message formatting of NonFatalCheckError to cpp (master...2205-err-impl-🗡) https://github.com/bitcoin/bitcoin/pull/25112
SpellChecker has quit [Remote host closed the connection]
SpellChecker has joined #bitcoin-core-dev
andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
_andrewtoth_ has quit [Remote host closed the connection]
<bitcoin-git>
[bitcoin] willcl-ark opened pull request #26512: init: Evaluate sysperms before config file (master...2022_13371_sysperms) https://github.com/bitcoin/bitcoin/pull/26512
_andrewtoth_ has joined #bitcoin-core-dev
_andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
_andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
stevenroose has quit [Remote host closed the connection]
stevenroose has joined #bitcoin-core-dev
_andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
gnaf has joined #bitcoin-core-dev
Guyver2 has joined #bitcoin-core-dev
Guyver2_ has joined #bitcoin-core-dev
jarthur has joined #bitcoin-core-dev
_andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
bitdex_ has quit [Ping timeout: 255 seconds]
bitdex_ has joined #bitcoin-core-dev
_andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
Guyver2_ has left #bitcoin-core-dev [#bitcoin-core-dev]
_andrewtoth_ has quit [Remote host closed the connection]
ziggie has quit [Quit: Connection closed for inactivity]
test_ has joined #bitcoin-core-dev
_flood has quit [Ping timeout: 268 seconds]
bitdex_ has quit [Remote host closed the connection]
bitdex_ has joined #bitcoin-core-dev
test_ is now known as _flood
bitdex_ has quit [Ping timeout: 255 seconds]
bitdex_ has joined #bitcoin-core-dev
salvatoshi has quit [Quit: Leaving]
jonatack has joined #bitcoin-core-dev
bitdex_ has quit [Remote host closed the connection]
Guest10 has joined #bitcoin-core-dev
Guest10 has quit [Client Quit]
halosghost has joined #bitcoin-core-dev
bitdex_ has joined #bitcoin-core-dev
_andrewtoth_ has joined #bitcoin-core-dev
<bitcoin-git>
[bitcoin] aureleoules opened pull request #26513: Make static nLastFlush and nLastWrite Chainstate members (master...2022-11-remove-static-chainstate) https://github.com/bitcoin/bitcoin/pull/26513
AaronvanW has quit [Remote host closed the connection]
pablomartin has joined #bitcoin-core-dev
Lov3r_Of_Bitcoin has joined #bitcoin-core-dev
<jonatack>
lightlike: cf yesterday's discussion on the breaking getpeerinfo API, the v24 changes also broke our own code, see #25176
<jonatack>
after my last talk today, i'll reverify with 24rc4
<jonatack>
these weren't rare occurences, at least in my testing, they were frequent. note that i use a vpn, have tor/i2p/cjdns peers, and some addnode manual peers with colleagues on the other side of the world.
<fanquake>
> please clarify exactly what is going to "break user space and production systems", and which specific change(s) need backporting.
<lightlike>
jonatack: are you sure that the errors in #25176 happened during connection setup for a new peer - and not during disconnection of a peer? Because I can't see how this would be possible from looking at the code.
<jonatack>
lightlike: yes, i read your comments (thanks for looking!), and will reverify, but the manual testing methods above showed it happening on connection. i will recheck to be sure.
<jonatack>
fanquake: messages forwarded to me by the CashApp PM from two of their developers: "we have layers of bitcoin core nodes and use that endpoint [getpeerinfo] .. if we upgraded our production systems without Jon's patch, it would break our production systems and we'd get paged"
<jonatack>
another dev: "and related, we need to update our core nodes"
<jonatack>
first dev: "if you want to link the github issue/PR, I can comment directly and ack the proposed changes"
<fanquake>
Ok. So what is "johns patch" in this scenerio? I'm sure it doesn't include doc changes, rpc help changes, netinfo changes, qt changes etc
<fanquake>
What is the specific commit or change they are talking about
<fanquake>
From the investigation here, it's not clear what is broken, or how it's broken, or, aslo, why cashapp couldn't work around this on their side, even temporarily.
<jonatack>
i had similar feedback from a dev at Block a month or so ago regarding their deployment pipeline. the cashapp messages were a week ago
<dergoegge>
jonatack: why can't they upgrade their handling of the getpeerinfo call to support missing fields?
<dergoegge>
This would have all been much simpler if they (CashApp devs) would have just opened an issue explaining their exact problem tbh
<jonatack>
fanquake: the fields that became optional
<jonatack>
dergoegge: the Block dev contacted me after reading the discussion in my first proposal and found some of the reactions too intimidating to comment
<jonatack>
dergoegge: sure, everyone can patch it on their own, but that doesn't seem ideal
<fanquake>
jonatack: which fields are you talking about?
<fanquake>
My understanding is that only minfeefilter has changed in terms of being optional
<fanquake>
The presence of relaytxes is unchanged in 26457
<jonatack>
fanquake: and relaytxes, which i proposed to also patch but their was resistance. If we leave the change to make it optional in v24 (after 8 years of being always present, it would introduced in 2015), it would be good to mention that change in the release notes
<fanquake>
sorry, I'm misunderstanding, 26457 does nothing in regards to changing wether relaytxes is present on not. It's just moved to a different fStateStats conditional
<fanquake>
so, to clarify, the current PR, which just shifts relaytxes from one fStateStats conditional, to another, doesn't actually fix the cashapp issue in any case?
bitdex_ has quit [Ping timeout: 255 seconds]
<fanquake>
but does make minfeefilter non-optional, by giving it a value of 0 if there is no fstatestats
<fanquake>
however the current question / understanding is that it's only possible for there to be no fstatestats if you're racing a getpeerinfo call, and a peer disconnection
<fanquake>
and this can't happen during connection setup
___nick___ has joined #bitcoin-core-dev
<jonatack>
i doubt that, but if it were true then we could make relaytxes always present as before with no downside
<jonatack>
will check in a few hours, thanks
sipsorcery has joined #bitcoin-core-dev
darosior has quit [Read error: Connection reset by peer]
darosior has joined #bitcoin-core-dev
steve__ has quit [Quit: Leaving]
AaronvanW has quit [Ping timeout: 260 seconds]
_andrewtoth_ has quit [Remote host closed the connection]
_andrewtoth_ has joined #bitcoin-core-dev
darosior has quit [Ping timeout: 256 seconds]
Lov3r_Of_Bitcoin has quit [Quit: Connection closed]
darosior has joined #bitcoin-core-dev
Talkless has joined #bitcoin-core-dev
Guyver2 has quit [Quit: Going offline, see ya! (www.adiirc.com)]
PaperSword has quit [Read error: Connection reset by peer]
NorrinRadd has quit [Ping timeout: 240 seconds]
Guest7 has quit [Ping timeout: 260 seconds]
NorrinRadd has joined #bitcoin-core-dev
Guest7 has joined #bitcoin-core-dev
<luke-jr>
jonatack: during peer disconnection, we can't know what the correct value was, so how could it be present?
<luke-jr>
returning incorrect data is IMO worse than omitting it
Guest7 has quit [Quit: Client closed]
Zenton has quit [Read error: Connection reset by peer]
Zenton has joined #bitcoin-core-dev
<bitcoin-git>
[bitcoin] mzumsande opened pull request #26515: rpc: skip getpeerinfo for a peer without CNodeStateStats, make its fields non-optional (master...202211_getpeerinfo_allornothing) https://github.com/bitcoin/bitcoin/pull/26515
zeropoint has joined #bitcoin-core-dev
AaronvanW has joined #bitcoin-core-dev
<lightlike>
I opened 26515 as a more general proposal to deal with this long-term.
AaronvanW has quit [Ping timeout: 260 seconds]
vasild has quit [Ping timeout: 255 seconds]
theStack has quit [Ping timeout: 248 seconds]
<jonatack>
BlueMatt[m]: the comment you linked to refers to a doc update proposed two months ago and is not the regression in question. at the time, it was within the window, as was the backports pull.
bitdex has joined #bitcoin-core-dev
Zenton has quit [Ping timeout: 256 seconds]
<BlueMatt[m]>
jonatack: so, iiuc, the issue here is that one field (minfeefilter) was always present in 23 and prior releases, and now there's some (absurdly, apparently?) rare race condition where it may not be while a peer is disconnecting?
vasild has joined #bitcoin-core-dev
<BlueMatt[m]>
is that correct?
<jonatack>
BlueMatt[m]: two fields in getpeerinfo, relaytxes and minfeefilter. no, it's not rare. we had to patch our own code for the former in #25176
sipsorcery has quit [Remote host closed the connection]
<BlueMatt[m]>
(fwiw its not at all clear from the current pr, the previous pr, or any commit message - none of them describe the concrete issue here, just trying to make sure I get what's going on)
sipsorcery has joined #bitcoin-core-dev
<BlueMatt[m]>
okay, so 26457 does not re-add relaytxes, its still missing in the same set of cases as before?
<BlueMatt[m]>
is there some second pr to make it always present?
<BlueMatt[m]>
btw who is "we" here? You're saying in some deployment of yours you had an issue because of this? I assume it's because something was deserializing the response into a struct that expects the field to be present?
NorrinRadd has joined #bitcoin-core-dev
<jonatack>
and the original discussion at the start of #25923
<gribble>
https://github.com/bitcoin/bitcoin/issues/25923 | p2p: always provide CNodeStateStats and getpeerinfo/netinfo/gui updates by jonatack · Pull Request #25923 · bitcoin/bitcoin · GitHub
<BlueMatt[m]>
jonatack: right, I'm not really trying to understand what did or didnt happen, just trying to make sure I understand where we sit today.
<jonatack>
BlueMatt[m]: some out-of-date docs that don't cause a regression and two longstanding getpeerinfo fields (relaytxes since 2015 and mifeefilter since 2018 or 2019 iirc) that in v24 are no longer always present
<BlueMatt[m]>
(am I still correct that rpc changes are supposed to go through the announced-in-one-release-changed/removed-in-the-next two-step process thing?)
<BlueMatt[m]>
jonatack: right, okay, so then I'm a bit confused why a release is being held up for a non-fix? Irrespective of whether Marco was right to argue against it or not, I don't see why fixing the removal of one field, and not another, makes sense this late in rc?
<BlueMatt[m]>
like, if removal of a field is an issue (again, not taking a stance here), then fix both, if its not, then ship?
<BlueMatt[m]>
again irrespective of if it should have landed before now, it is where it is
<jonatack>
BlueMatt[m]: we had to patch -netinfo that doesn't null check fields that are always present. given the number of fields returned by getpeerinfo, and given feedback from some companies in the space, this needlessly breaks their software as well while being trivial on our end to fix once, rather than each of them patching it. in any case, this has been proposed for months now. it's not
<jonatack>
holding up the release. it could have been merged but was not. that is not in my control. however, we should also warn user space in the release notes if we leave it as is.
<BlueMatt[m]>
(fwiw I asked the cash app folks - they have no idea what the patch does or what the specific issue is, and dont know if it would actually cause them pain, they were only told that "if you use getpeerinfo this may break your stuff" and they said "yea, we use that")
<BlueMatt[m]>
hence why I'm here asking trying to understand exactly where things sit
<BlueMatt[m]>
what is "-netinfo" that you had to patch? You saw the missing fields in prod?
<jonatack>
BlueMatt[m]: see my irc comments earlier today for what was forwaded to me
<jonatack>
i don't have direct access to them
<BlueMatt[m]>
again, I'm not taking a position on if it should be merged or not, but trying to understand (a) if its a regression (sounds like yes), (b) how likely it is that you can even observe the fields being missing (you seem to say they can be and are in prod, others have said they basically cant be), and (c) if its an issue that merits last-minute fixes.
<jonatack>
BlueMatt[m]: re -netinfo, see the patch PR i linked to above
<BlueMatt[m]>
yes, I know, that was a game of telephone, that's why I went and asked ryan directly :)
<BlueMatt[m]>
he said he's happy to get on a call, if you'd like, but he's busy dealing with mempool growth today :)
<jonatack>
i'm free any time for a call
<jonatack>
i've just finished a bunch of stuff and will re-verify but the issues have been trivial to reproduce in at least three ways
<jonatack>
- observing our GUI peers details
<BlueMatt[m]>
okay, by "frequently" there in your comment about netinfo I'm a bit confused - fanquake seemed to indicate that in tests others had noted they weren't able to reproduce this at all unless they added some sleeps in the code, implying its an incredibly rare race?
<BlueMatt[m]>
oh, and, finally, it seems pretty obvious that if we think this is a major breaking issue worth backporting, we should make sure both fields are always present, even in a 24-only PR.
pablomartin_ has joined #bitcoin-core-dev
<jonatack>
BlueMatt[m]: I prefer making both present for v24 as it's more prudent and then figuring out the best path for the next releases
<BlueMatt[m]>
because obviously just fixing it for one field wont help.
<BlueMatt[m]>
is there a pr for that?
<jonatack>
agree
pablomartin has quit [Ping timeout: 260 seconds]
<jonatack>
#26109 did that before dropping the relaytxes change. i'll open one to re-propose and re-verify the recent feedback