eyedeekay
I've got the various refactors in scoped branches right now, some are close to ready to look at, 2 are not
eyedeekay
I'll look into what we can get from backing it out and redoing it
dr|z3d
eyedeekay: no, not proposing to erase the history.
dr|z3d
just propose to use git revert on the relevant commits so the history persists.
dr|z3d
I think the concerns and issues zzz raised in his various bug reports are serious enough to warrant a rethink and revert while they're addressed. Too many possible issues to be confident it's not going to break in unforeseen ways.
dr|z3d
I also think that having this at the _beginning_ of the dev cycle and not the end, for such a big chunk of patches, is not only sane, but should be mandatory.
obscuratus
eyedeekay: I've been running the i2p.i2p.2.4.0-segmented-netdb-safer-interfaces branch on my testing network. It's been running fine. No signs of regressions.
obscuratus
I was scared when I saw it's size, but good job on that. It looks good.
dr|z3d
Needs more testing, I don't think it's ready for 2.4.0
obscuratus
There's no need to even bother with 2.4.0 without it. If we have to delay 2.4.0 for testing, then I think we'll just have to do it.
dr|z3d
I had my suspicions when I had to swap out lookupRouterInfoLocally to (RouterInfo) lookupWithoutValidation to prevent it from spewing cocncurrency and stackoverflow errors, and with zzz's comments, I'm even less confident.
orignal
yes, NTCP2 is fine
orignal
only SSU2 has this issue
orignal
so, what's your opinion about possible solution?
eyedeekay
obscuratus it's a big one but all it's really doing is lopping off anything unused or redundant
eyedeekay
And writing better javadoc for what remains
obscuratus
It looks like it addresses the API issue about as neatly as we could have hoped for, right?
eyedeekay
There were definitely too many ways to access it and letting the FNDS guess the subdb probably encouraged misusing it and made it harder to understand
eyedeekay
Needs a bit more work but it's a lot closer
dr|z3d
obscuratus: if 2.4.0 needs to be put back 3 months so the segmented netdb stuff can get refactored, sensibly merged, tested and validated, so be it.
dr|z3d
pushing all the changes in a single commit should never have happened.
zzz
eyedeekay, the only reason to back it out is if you want to do a release without it
zzz
orignal, there's many possible solutions, I can't evaluate it in detail until you write it up
zzz
dr|z3d, you've stared at the code more than anybody as part of your merging, any contributions you can make on the git tickets would be helpful
zzz
there's large parts I haven't even eyeballed yet
dr|z3d
zzz: If I'm honest, I'd say I don't have enough of an idea what's going in the code to provide useful commentary at this point. It arrived like a tsunami.
dr|z3d
I spent 1/2 a day or so just trying to troubleshoot areas where it broke + in ways that weren't obvious to fix, spent another day or so failing to identify why geoip lookups were broken post merge, and then just moved on to something else that was less stressful.
dr|z3d
And that was after the 1/2 day required to manually merge 50 files.
dr|z3d
obscuratus: re the deprecated! debug logging, I was seeing those all the time, eyedeekay suggested they were harmless, so I disabled them. The actual message itself is opaque enough to only be of use to the person that included them.
dr|z3d
And I'd politefully disagree regarding backing out the code, the other reason to do that, aside from wanting to do a release early, is to scope the commits in stages so it's more obvious what the scope of each commit is, with extended commit notes to indicate what's changed and the purpose of the commit.
dr|z3d
As I said to eyedeekay at the time, the size of the commit was bad enough, but the inclusion of patches that were unrelated to the main purpose of the commit, such as renaming banlistForever to banlistHard, various changes to the stats collection, just made the commit even more difficult to grok.
zzz
yeah I understand it's tough to figure out but it's gotta be all hands on deck to pull this together
zzz
what is planned or already in for the release besides this netdb thing?
eyedeekay
Some tweaks to the blocklist/banlist, congestion caps, tweaks to the rate-limit on lookups, compared to the netDb changes it's all very minor
zzz
but rate limit lookups was just disabled? or just the banning?
dr|z3d
just the banning I think, at least in canon. i2pgit.org/i2p-hackers/i2p.i2p/-/commit/dc68fdc0a012866ee940916461568462f996f20c
eyedeekay
Yeah we only stopped banning, we still added the burst limit
obscuratus
dr|z3d: Re: "The actual message itself is opaque enough to only be of use to the person that included them." Lol, that's fair pushback. You're more-or-less correct about that. I put those in there to mark a spot in the code that needs review for iteration over the netDbs. Most of them still mark a spot that needs review.
obscuratus
I was going to work on that this weekend, but I ran into a leak that's screaming for attention first.
dr|z3d
:)
zzz
added a bunch more tix, most not as serious as the last batch
zzz
I don't know how we got here, but I see a lot of evidence of code-thrashing and WIP that wasn't cleaned up, and a real lack of review and testing and linting
zzz
I suggest you review your processes and standards that got you to within a week of release
zzz
dr|z3d, on a router w/ ipv6, would you please check your logs for any of these errors? git.idk.i2p/i2p-hackers/i2p.i2p/-/issues/415
zzz
I'm still carrying a bunch of 6-month-old WIP so maybe it's just me...
orignal
zzz it two word
orignal
we should send a block with Bob's ident hash
orignal
either Alice or Bob
orignal
and verify
zzz
orignal, ok, but need a writeup with the problem/attack, followed by solution and alternatives
zzz
you want me to agree 100% before you write it down?
zzz
eyedeekay told me in June it was a "good idea" and you agreed to write it up
zzz
so what do you want from me?