< wumpus>
though otoh it did get a lot of review, and it's an optional thing disabled by default
< jonasschnelli>
Yes. I agree. Just it looks like there is no clear conceptual ACK on serving filters after BIP157 (don't get me wrong, I'm all for serving BIP157).
< jonasschnelli>
I just think 6 days was to short for a reasonable discussion
< wumpus>
but it's sure very fast given that we're kind of in slow times
< fanquake>
🏎️
< fanquake>
One thing that's not getting faster is how long it takes to run all 160 tests of the functional test suite
< wumpus>
luke-jr: look, I disagree, personally I'd prefer not to include to autogenerated cruft at all, and there's more people that don't, so I don't see it as a regression. Spliting up the PR makes sure there is at least progress on parts that we do agree on. IF that isn't a positive thing I honestly don't know.
< jonasschnelli>
🐌🔑
< jonatack>
jonasschnelli: the conceptual discussion is ongoing (at least for me) on bip157, but the changes in that PR didn't seem dangerous on their own after reviewing them, and reversible before the next release
< jonasschnelli>
jonatack: I agree. Though an progressive merge behaviour is usually not the mode the project has worked in the past
< wumpus>
I think maybe we shouldn't include TODOs in the code at all and open a github issue instead, this is easier to manage
< jonatack>
jonasschnelli: i can understand that. there is also an opportunity cost in review resources.
< jonasschnelli>
jonatack: Yes. Agree. I just think leaving it open for a while would not have had an big impact on resources.
< jonasschnelli>
(have more feedback, improve quality and get a conceptual agreement)
< fanquake>
wumpus: Could do. Although looks like most are in validation & net processing, so might not be great as "good first issues"
< wumpus>
fanquake: they don't strictly need to be "good first issues", usually it's just a way to mark technical debt I guess :)
< jonatack>
jonasschnelli: can't disagree with that. and at some point the conceptual issuens need to be addressed. belcher's ML post about it today is interesting.
< wumpus>
it's fine to have them if they're actually TODOs but such opinions tend to change over time, while they languish in the source code
< wumpus>
(e.g. it was an opinion of one developer that it was a TODO at the time the code was originally written)
< fanquake>
heh. There are definitely opinionated TODOs
< fanquake>
"// TODO: Handle this much more gracefully (10 DoS points is super arbitrary)"
< wumpus>
I think that's an okay one, it could be written as "This could be handlded much more gracefully" without losing it's meaning, it's not like the one in the tests a fragment "todo: def base58 decoding" *why*
< jnewbery>
jonasschnelli: 18877 is the first 3 commits from 16442, which has been open for almost a year. It had 7 ACKs + 2 implicit ACKs from jimpo and me + 6 concept ACKs. How can one year and 15 [concept] ACKs possibly be considered too short or not enough discussion?
< jonasschnelli>
jnewbery: good point. The PR description was not refering to 16442. I missed that. I just think we need to make sure everyone has been heard and ideally agrees on the concept.
< jonasschnelli>
Dont get me wrong. I’m totally pro BIP157.
< wumpus>
psa: please do not add anything to the 0.20.0 milestone anymore unless it's a critical bugfix for a regression, at this pace we're noever going to be able to do rc2 let alone a release
< wumpus>
everything else (if it needs to go into 0.20 at all) can go into 0.20.1
< instagibbs>
unless it's a weird concept or lots of code I see no harm in the base58_decode PR :shrug:
< instagibbs>
agree in general TODOs need to be motivated
< wumpus>
no harm but also no gain unless you see a use case?
< wumpus>
we shouldn't just randomly add functions, it needs to be motivated
< instagibbs>
i kind of disagree, unless you think there's just no useful test to write with it
< wumpus>
that's not the point
< instagibbs>
(maybe not!)
< wumpus>
it can be added when a test needs it
< wumpus>
the library is wholly to support our own use case, there shouldn't be anything 'just in case' in there
< wumpus>
I mean, otherwise I'm sure in half a year or less someone will go through it and helpfully remove unused functions and we're round
< wumpus>
in any case if *you* need it for a test it's good to add it
< instagibbs>
tell him to add a test then? :)
< instagibbs>
I did
< wumpus>
a test for bitcoind that uses it, to be clear, not just a test for the function itself :)
< instagibbs>
anyways, if I ever need it I'll just try to remember the future-ly closed PR and cherry-pick
< wumpus>
fair enough
< ryanofsky>
i definitely think #18965 is useful in its current form, even without a round trip test, though a round trip test would be useful
< ryanofsky>
having a decode function could have saved me going that that rabbit hole
< theStack>
given that base58_decoded is needed, for me it seems kind of nonsensical to reinvent the wheel and manually code it out again; that's as absurd as if someone would implemented sha256 by hand in the test suite
< theStack>
of course base58 is not part of the python standard library, but i'd either use a tested widespread library, or (if we
< theStack>
don't want dependencies) copy it from that library
< jnewbery>
jonasschnelli: ah, you're right. I should have linked back to the full PR in 18877. The last comment before merge lists out all the places that people have [concept] ACKed.
< promag>
MarcoFalke: 18964 is not for 0.20 is it?
< jnewbery>
I don't think it's unreasonable to add a decode function if the encode function is there. At least that means we can test the encoder with round-trips.
< jnewbery>
(but agree that we should remove TODOs if they're of only marginal benefit to the project. The issue system seems like a more appropriate place for those)
< bitcoin-git>
bitcoin/master da16f95 Hennadii Stepanov: gui: Do not translate InitWarning messages in debug.log
< bitcoin-git>
[bitcoin] jonasschnelli merged pull request #18922: gui: Do not translate InitWarning messages in debug.log (master...200509-initwarn) https://github.com/bitcoin/bitcoin/pull/18922
< MarcoFalke>
[23:19] <luke-jr> would be nice if Travis sent an email to the PR creator when it failed :x
< MarcoFalke>
You can enable travis in your own repo to get emails
< MarcoFalke>
Btw, I have a lot of good first issues to work on. If someone is out there that wants them, please contact me. If I don't hear back, I will put them on the main repo maybe next week.
< shesek>
how do conflicting transactions fit into the most recent sorting of listttransactions? do they get sorted as the most recent like the negative confirmations number might imply? or at their original height? also, are there any guarantees for the ordering of unconfirmed transactions among themselves?