<vasild>
sipa: in case you remember, that is from 2013, not too long ago. To my understanding nBind should be present in both expressions
aleggg has joined #bitcoin-core-dev
kevkevin has joined #bitcoin-core-dev
<bitcoin-git>
[bitcoin] sipa opened pull request #29757: feefrac: avoid explicitly computing diagram; compare based on chunks (master...202403_implicit_diagram) https://github.com/bitcoin/bitcoin/pull/29757
pablomartin has joined #bitcoin-core-dev
<sipa>
vasild: i have no memory of that code
<vasild>
;-)
<vasild>
ok, maybe I will PR some simplification of that
<_aj_>
vasild: fd_max - nbind -- so you can have an fd for each interface you're listening on would be my guess?
<vasild>
yes, for example -bind=1.2.3.4:1234 -bind=[2001::234]:2837 -bind=... would need 3 file descriptors for bind/listening
<vasild>
it is properly accounted for on line 990 in the latest code but is missing from line 993 and I wonder why, I think it should be present on line 993 as well
<darosior>
a story of a bug in one part "Not sure i understand this code. I will refactor it to make it simpler." :p
<vasild>
darosior: been there done that :) or also "s/refactor/delete/" ;_)
<_aj_>
vasild: it's mentioned in line 981, so seems like it should appear in 993 as well to me too, fwiw
<vasild>
to be honest this nMaxConnections tweaking around those lines gives me a headache, it could be that I misunderstand it. darosior _aj_ sipa I will poke you for review if I gather enough bravery to simplify it (aka break it)
<darosior>
is this the best use of everyone's time? Did you find a bug related to this or are you just trying to make it "look nicer"?
<_aj_>
vasild: i think the main value is (a) to give an error in the logs if your system can't support the connections you want, and (b) to ensure enough fds are reserved for manual addnode connections
Guyver2 has left #bitcoin-core-dev [Closing Window]
<vasild>
darosior: I think this piece is complicated enough to warrant a simplification. If I have troubles understanding it then others may as well. The code is read many times by many people, so that multiples. Also, I am now adding a new metric to that, similar to nBind and if there is a bug in the existent code, then should I do as in nBind, repeat the bug in the new code or not? This arised in
<darosior>
sure, just asking cause you wanted me to review.
<vasild>
ok, just "poke", feel free to ignore any review request. And I still have not opened any PR for that, maybe I will put it somewhere lower on my TODO and will never get to it...
pablomartin has quit [Ping timeout: 264 seconds]
BrandonOdiwuor has quit [Ping timeout: 250 seconds]
<sdaftuar>
Hi -- I found a memory accounting bug in the draft PR this week, which I think I've figured out and have a fix for, but haven't yet pushed. I'm verifying my fix now on my historical data and will push once I'm done.
<sdaftuar>
Also, the draft PR is now badly in need of a rebase, which I probably won't get to in the next week -- I'm prioritizing trying to do research on historical data with the branch that I have.
<sdaftuar>
My hope is to put together some kind of summary of the changes to transaction acceptance that would have occurred with the new mempool, based on last year's transaction data. (This includes redoing the preliminary performance benchmarks that I'd previously done which need to be re-done with the above-mentioned bugfix in place.)
<sdaftuar>
So I'm planning to tackle the rebase and re-working of the draft PR after that research is complete.
<sdaftuar>
Excitingly (for me), today I discovered over 1000 examples of RBFs that took place in January 2023 which were accepted by the old mempool logic but would have been rejected by the new feerate diagram check, which I'm in the process of exploring further.
<sdaftuar>
If anyone has any questions that I can answer, please let me know!
<sdaftuar>
(that's it for me)
<glozow>
cool! (the RBFs)
<achow101>
Is there anything that can be reviewed right now?
<kanzure>
hi
luke-jr_ is now known as luke-jr
<sdaftuar>
achow101: well, i'm going to start asking people for help with the wallet and mini_miner changes soon. probably not a lot of useful review to be done now on the implementation itself, but i think reviewing the test changes could be helpful--
<sdaftuar>
really i think it would be helpful if anyone is interested in writing better/more comprehensive tests which i could take in the PR
<sdaftuar>
but that can also come later, of course
<vasild>
"last year's transaction data" - do you take that from the blockchain or did you run some recorder that saves each tx that makes it to your mempool. E.g. if a transaction was accepted in the mempool but never mined, would it be in that historical data you tested with?
<sdaftuar>
vasild: i have a data logging system that records every transaction and block that my node sees, and a patch set that allows me to play that data back through our validation logic
<vasild>
cool :)
<achow101>
Hasn't been much activity in the last week so the thing to review is still #26606. Also been reviewing #28574, hoping to get that in soon
<dergoegge>
i have a pr open to discourage the use of recursion (and to make it obvious when we introduce it) using the "misc-no-recursion" clang-tidy plugin #29690
<dergoegge>
if anyone has an issue with that please comment on the PR :)
<vasild>
Is recursion frowned upon? :-O
<luke-jr>
there's some arguments made, but I think the point here is to make it explicit
<luke-jr>
contrasted with (eg) calling the same function with a different signature
<dergoegge>
there is nuance to that question, i'd frown upon it in our repo but it's mostly about making it explicit
<achow101>
The suggestion is that if a function is recursive, just mark it as such, rather than avoiding it entirely?
<achow101>
iirc it's used quite a bit in wallet related things (descriptors, signing, etc.)
<lightlike>
Is this an actual problem? Are there too many (or any) PRs that attempt to introduce recursion?
<luke-jr>
lightlike: introducing recursion shouldn't be forbidden.. that's a different thing from making it explicit
<fjahr>
I think it’s not going to be doing much for us because it’s not like inexperienced developers include recursion in their PRs by accident very often. But since this also won’t affect ~99% all PRs anyway, I won’t stand in the way if enough others see value in this.
<sipa>
there have been some cases of recursive functions in the codebase that could be triggered to cause stack overfloe
<sipa>
(including ones introduced by me)
<luke-jr>
I guess the real question is, would this have helped catch those? ;)
<sipa>
i doubt it
<sipa>
but it's hard to say, having the explicit marker in the codebase permanently may cause some people to occasionslly just look over all of them
<achow101>
I suppose if you have opinions, go comment on the PR?
<luke-jr>
is it the function that gets marked, or the call?
<sipa>
ok
<luke-jr>
achow101: k
<dergoegge>
👍
<vasild>
In my experience some problems are recursive in nature and the recursive solution is neat an tidy and easy to follow and review and thus unlikely to have bugs. Iterative solutions for those on the other hand could be messy and easier to get wrong. I don't think a blanket ban on recursion makes sense in any project. Explicitly flagging a function "this is recursive" to avoid accidental recursion
<achow101>
#topic writeups and disclosure of historical bugs reported but were never published (darosior)
<vasild>
is another thing. +1 on that.
<sipa>
luke-jr: in recursive functions, those two are the same :)
<darosior>
So last time a group of us met in person, Niklas and i volunteered to make a few writeups about historical vulnerabilities which were reported to the project. So we did that. Thanks to fanquake for sharing with us the old reports.
<luke-jr>
sipa: not really, the call site may be a page or two away
<darosior>
We only have two though, looks like there were not that many in the end.
<sipa>
luke-jr: the function gets marked
<darosior>
We think we could write a blog post at bitcoincore.org about them, and maybe announce them on the ML. Does anyone has any feedback / opinion / objection to us doing this? (remember we are talking about very old vulnerabilities which were just never disclosed)
<achow101>
darosior: I think putting them on bitcoincore.org is fine
<sdaftuar>
seems like a good idea to me, thanks for doing this!
<instagibbs>
+1
<luke-jr>
darosior: there's definitely more than 2?
<darosior>
luke-jr: we can only put so much pressure on fanquake to spit historical reports
<b10c>
+1 on publishing
<darosior>
cool, also maybe as a teaser: we want to animate a discussion at the next about how to handle disclosures moving forward
<achow101>
we can talk offline if there are any more to write
<darosior>
s/at the next/at the next in-person meeting/
<sipa>
yeah i think i can help with more old ones too
<achow101>
yes, I think bringing up this discussion at the next coredev would be good
<darosior>
sipa: cool, will reach out
<luke-jr>
k
<vasild>
What's the point in publishing those? To get people running vulnerable versions to upgrade?
<dergoegge>
vasild: yes and to give credit to the people that find the bugs
<darosior>
I don't think anybody runs the affected versions anymore
<achow101>
vasild: transparency, and it's also generally useful to have old vulns published so people can talk about them
<sdaftuar>
vasild: i think it can also be instructive to contributors to learn from the past
<sipa>
and also for ourselves, i think it's good to have a culture of keeping track of tjedr
<sipa>
*these
<darosior>
and there is also maybe a point to communicate to the larger Bitcoin community that this kind of thing does happen
<vasild>
Good! I did not think about those
<kanzure>
is there any existing disclosure content on bitcoincore.org? i don't want to see a "policy"
<luke-jr>
darosior: there's at least a few trying to encourage downgrading to 0.12 :/
<achow101>
kanzure: pretty sure there's a few older things
<sdaftuar>
kanzure: isnt' the inflation bug on there?
<harding>
kanzure: year, duplicate inputs bug is there
<achow101>
they're buried deep in the blog section
<luke-jr>
not-time-sensitive stuff has historically been published elsewhere tho
<luke-jr>
maybe we can add a page of links to the menus somewhere so they're easy to find
<kanzure>
there needs to be room to maneuver with respect to responsible disclosure and the unique nature of each incident. historical should be fine to document, but i wouldn't want it to be misinterpreted as an expectation on any specific timeline.
<achow101>
luke-jr: it does make sense to have one place with/linking to all of them though
<achow101>
kanzure: I don't think we're going to be setting a policy with hard deadlines
<darosior>
kanzure: yes this topic was only about historical reports. Disclosure of more recent reports will be discussed at the next in-person meeting.
<sipa>
kanzure: agreed, but i also think the number of bugs that require unusual treatment i a tiny fraction only
<kanzure>
sipa: glad to hear that.
<darosior>
Alright i think that's all we had to share, dergoegge anything to add?
<sipa>
i assume darosior and dergoegge are talking about the few very serious ones we've had, and for those, we need ad-hoc proceddes
Guest42 has quit [Quit: Client closed]
<dergoegge>
darosior: no :)
<fjahr>
some old fork based altcoins could be affected still right? Not saying that means we shouldn't do it, just a thought. Maybe you could grep for the affected lines in github?
Guest42 has joined #bitcoin-core-dev
<darosior>
fjahr: it's a decade old, i don't think anything remotely sane still running would be on these versions anymore
<achow101>
darosior: there are plenty of insane people :p
<b10c>
fjahr: if they haven't cherry-picked commits in 10 years, they won't start now?
<kanzure>
fjahr: yes this has been discussed on the mailing list in the past as an issue with responsible disclosure (do i personally owe any responsibility to disclose to altcoins that forked and didn't provide developer resources back? what about their users...?) etc
<luke-jr>
b10c: maybe they have, just didn't know about the relevant ones?
<luke-jr>
kanzure: even if they had provided resources back, they wouldn't necessarily know
<fjahr>
b10c: yeah, still a heads up a few days before publishing would be nice, but no strong feelings either way...
<kanzure>
(and there are also security issues with disclosing to altcoins: many of these projects are acting as adversarial entities! why would you disclose weapons or vulnerabilities or whatever?)
<luke-jr>
there's only a small few legit altcoins tho, and I don't see why we should care about the scams
<achow101>
as usual, I think it needs to be evaluated on a case by case basis
<luke-jr>
the legit ones are likely already rebased anyway
<darosior>
fjahr: for recent disclosure i agree out of courtesy we could be reaching to a few of the main altcoins a few days before publishing. For historical ones, meh
<kanzure>
sure, sure. this is an old topic, and not much has changed.
<achow101>
anything else to discuss?
<dergoegge>
i think i'd be irresponsible not to give them a heads up but only after we've decided it is safe to publish anyway
<achow101>
#endmeeting
lbia has quit [Quit: lbia]
cguida has quit [Ping timeout: 255 seconds]
luke-jr has quit [Read error: Connection reset by peer]
Guest42 has quit [Quit: Client closed]
luke-jr has joined #bitcoin-core-dev
pablomartin has joined #bitcoin-core-dev
pablomartin has quit [Read error: Connection reset by peer]
pablomartin4btc has joined #bitcoin-core-dev
preimage has joined #bitcoin-core-dev
bugs_ has joined #bitcoin-core-dev
puchka has quit [Ping timeout: 264 seconds]
luke-jr has quit [Ping timeout: 252 seconds]
brunoerg has quit [Remote host closed the connection]