< bitcoin-git>
[bitcoin] naumenkogs opened pull request #18991: Cache responses to GETADDR to prevent topology leaks (master...2020-05-addr-response-caching) https://github.com/bitcoin/bitcoin/pull/18991
< bitcoin-git>
[bitcoin] 10xcryptodev opened pull request #18992: qt: add sign and verify message support to AddressBookPage (master...202005-sign-message-ui) https://github.com/bitcoin/bitcoin/pull/18992
< bitcoin-git>
[bitcoin] 10xcryptodev opened pull request #18993: qt: increase console command max lenght (master...202005-increase-console-command) https://github.com/bitcoin/bitcoin/pull/18993
< tryphe>
does a tACK imply a concept ack? or it is simply a confirmation that the commit works, regardless of conceptual opinion (suppose you agree with the concept but don't know whether to agree with the approach taken or not)?
< yevaud>
my reading is that you wouldn't test something you don't conceptually agree with.
< tryphe>
makes sense
< yevaud>
that may not be correct, but I'm not sure how to understand it otherwise, as you've pointed out.
< tryphe>
my feeling was that maybe you'd test it just to test it because it's a good concept, without being able to weigh the tradeoffs of what's possible with another implementation of the same concept
< tryphe>
and maybe you'd concept ack it if you had gone through implementations of that concept in your head
< tryphe>
(or maybe that would be something different like implementation ack)
< tryphe>
and maybe code review is more of an implementation ack
< bitcoin-git>
[bitcoin] practicalswift opened pull request #18994: tests: Add fuzzing harnesses for functions in script/ (master...fuzzers-script-slash) https://github.com/bitcoin/bitcoin/pull/18994
< sipa>
tryphe: sometimes people comment "code review ack" to be specific about the fact that the code looks good, but they don't have much of an opinion on the concept
< tryphe>
sipa, ahh, that makes sense then, i guess i had assumed those code review acks were concept acks in a sense, but indeed it's nice that code review can complete without putting concept ack pressure on the reviewers
< tryphe>
on another note: shouldn't there be a github tag called "needs testing", or are testers in high supply and usually not necessary??
< tryphe>
err, multiple question marks not intended, sorry
< tryphe>
usually not necessary/usually not necessary enough to need a tag*
< tryphe>
or is that what "review club" is?
< jonatack>
tryphe: i think the review club is to help contributors learn about the review process and participate in it
< meshcollider>
In my opinion, code review ACK is just the more descriptive version of utACK, and that any form of ACK (including tACK) is implicitly a concept ACK unless stated otherwise
< jonatack>
meshcollider: this is how i have been using it as well. the more specific version had not occurred to me, but it strikes me as more useful. if i'm not the only one who read it this way then it may be useful to disambiguate it.
< michaelfolkson>
I thought the whole motivation for the change in wording last year was to make it more standardized and less convoluted.
< jonatack>
michaelfolkson: this is what i meant above with "(though that somewhat newer guideline isn't necessarily followed)"
< michaelfolkson>
Well they should be right? What is the point of guidelines if no one follows them?
< jonatack>
michaelfolkson: we're discussing what is seen in practice... "code review ack" is used frequently
< michaelfolkson>
If we want to get BitcoinACKs back up and other tools/analytics we need people to understand the guidelines and follow those guidelines
< jonatack>
michaelfolkson: this is open source...
< michaelfolkson>
Ok let's ditch the guidelines then
< michaelfolkson>
I don't think guidance docs like the one you put together should be encouraging things that aren't in the guidelines
< jonatack>
i update it often to reflect actual practice, it's not intended to be a copy of contributing.md. you are free to write your own document. this is open source.
< michaelfolkson>
That makes things really confusing if two different docs tell you to do two different things
< michaelfolkson>
Perhaps ditch the style guidelines too if this is open source. I don't mean to be flippant but "this is open source so feel free not to follow guidelines" seems bizarre to me
< jonatack>
observing and discussing actual practice is different, in my view, from proposing to ditch guidelines, which can still be helpful
< jonatack>
i'm not really interested in debating that
< michaelfolkson>
As long you appreciate downsides. You can't one day talk about why BitcoinACKs doesn't effectively work and the next day encourage flexibility around the review wordings people use
< michaelfolkson>
I don't know. I don't want to discuss it either but I am more confused after this conversation than I was before it started. Let's leave it
< jonatack>
i'm doing neither of those things, i think? bitcoinacks could be updated occasionally when practices evolve. i'm not advocating about current practices, simply looking to understand them
< tryphe>
another question about review though. is there a general need for tested ACKs other than from reviewers? i guess what i mean is, is testing and reviewing seen as more mutually exclusive, where everyone testing a PR is seen beneficial, beyond reviewer testing?
< instagibbs>
tryphe, code reviewing and testing are two separate things generally
< instagibbs>
for instance some PRs I just don't have time/interest/knowledge to review the code well, but I know how to test how users are expected to use it, try to trip it up, etc. Both are valuable.
< sipa>
i'd say that the need for manual testing goes down the more testable (through unit or functional or fuzz tests) it is
< sipa>
and the latter is certainly preferable
< sipa>
so the distinction between "ack, tested" and "ack, didn't test anything myself but the included tests look sufficient"
< sipa>
is not always so big
< tryphe>
michaelfolkson, yep, i took a look, but i wasn't sure if tACKS were inherently concept ACKs or not, or if both should be done
< tryphe>
i guess maybe that should be added? it seems like newer people might get thrown off by that
< michaelfolkson>
tryphe: I think generally if you are taking the time to review the code you are a Concept ACK or you think the Concept ACK has consensus. Otherwise why review the code?
< michaelfolkson>
tryphe: But you can Concept NACK and ACK a commit in the same comment if you are in that situation
< sipa>
indeed
< sipa>
but you may be indifferent about the concept
< tryphe>
michaelfolkson, suppose you are feeling neutral about the concept, reviewed the code, but want to leave the concept ack to other people to decide
< tryphe>
yeah what sipa said
< sipa>
"Code review ACK <commitid>, unsure about the concept"
< sipa>
You're always welcome to elaborate more on your opinion.
< michaelfolkson>
The guideline is ACK <commitid> rather than Code review ACK <commitid>
< michaelfolkson>
Obviously you can write whatever you want after ACK <commitid> to explain in greater detail exactly what you have reviewed
< michaelfolkson>
That's the current guideline (as I can make out)
< tryphe>
that makes sense, thanks
< sipa>
for example other useful things to add are "verified move only" if the PR includes move-only commits, and things like "thought hard about how the change X could break Y but didn't find any"
< tryphe>
i had found myself going through a bunch of historical commits to try and find a theme but it seems like everyone has their own preference of style, i think that might deter newer folks from jumping right in, unsure about what they've seen vs. what they interpret the guidelines as
< tryphe>
i mean, just as an observation
< sipa>
yeah, that's certainly possible
< michaelfolkson>
Maybe. I am happy to be one of those annoying people who badger people to follow the guidelines ;)
< michaelfolkson>
We certainly need those richer comments that sipa lays out too. Hopefully we can get best of both worlds
< tryphe>
not nitpicking the guidelines or anything, i just wish it was easier to parse i guess, but can't really suggest any improvments
< michaelfolkson>
It is good feedback. I think the guidelines are good. Just need to try to get people to follow them I think
< jonatack>
when in doubt adding context doesn't hurt; the shortcuts seem popular because they are just handy
< tryphe>
i see, that makes sense
< tryphe>
i guess my concern is, to the casual observer or someone who might be a potential tester, seeing the shortcut acks might throw them through a loop about the whole process
< tryphe>
even if they had read the guidelines after, i mean
< sipa>
i think from a new contributor it's even more advisable to be verbose in review comments
< michaelfolkson>
Makes sense. A long term contributor ACKing a commit without any comment has value. But if a new contributor does the same it is unclear what exactly they have reviewed and if they have truly understood the change
< sipa>
exactly
< tryphe>
ahh yeah
< tryphe>
as someone who has looked over a lot of PRs but never commented much, i always feel like verbose conversation was sort of unwelcome and would clog up the comment feed with chatter instead of acks, but i guess that's completely wrong
< sipa>
i think the most disrupting thing to a PR is getting a multitude of low level/nits/code style comments, while it's very unclear if a PR is desirable as a concept
< michaelfolkson>
Personally I have been quite conservative on when I comment. John Newbery has encouraged me to comment more on PRs having participated in a lot of PR review clubs.
< michaelfolkson>
It is a balance though I think. Often I am trying to understand the conceptual change at a time when it clearly already has consensus on the Concept ACK and the discussion has moved onto the code review
< michaelfolkson>
Definitely do a lot of PR review clubs tryphe and it will start to become clearer where you can add value
< tryphe>
thanks. i think i might do that!
< tryphe>
thanks. i think i might do that!
< tryphe>
oops
< tryphe>
when getting into bitcoin PR review i feel like there's this -almost- unscalable mountain in front of you; semi-scalable in terms of "things that i can actually review without much historical bitcoin knowlegde" (NATPMP for example, that was easy, but almost straightforward enough that it requires no comments), but also unscalable parts in terms of other PRs where the motivation was so ingenius that trying to review it almost seems like a
< tryphe>
fallacy (maybe i understand the motivation conceptually but trying to constructively add to conversation is not possible)
< tryphe>
i guess the almost unscable part becomes scalable over time with much more PRs reviewed though, but to me that's the biggest detterent to newcomers
< tryphe>
hard to constructively comment on low hanging fruit (because it's easy), but overwhelmed when trying to push a boulder, so to speak
< tryphe>
i guess it seems like there's a lack of middle ground, just a huge disparity between simple changes and huge ones. observationally speaking, anyways.
< michaelfolkson>
Yeah I think that is fair tryphe. It is not easy. But ACKing the more basic PRs and attending the PR review club to understand the more complex ones is the way to go
< tryphe>
michaelfolkson, thanks for that, i'll definitely give it a go. is the review club new, btw? it seems like it wasn't around even a few years ago, or maybe i'm just ignorant
< sipa>
tryphe: it just celebrated its 1 year anniversary
< jonatack>
tryphe: jnewbery launched it began a year ago. it's sort of like an online version of the chaincode labs seminars.
< tryphe>
that's pretty cool
< tryphe>
there's such a large following now it almost seems like it existed longer
< michaelfolkson>
jonatack: Looks good apart from code review ACK. As I said before the guideline is ACK <commitid> and then comment e.g. Reviewed the code.
< michaelfolkson>
We have no hope of people following guidelines if guidelines aren't clear and inconsistent between documents
< tryphe>
existed/had to exist*
< jonatack>
michaelfolkson: thanks for reviewing, will look at it tomorrow with fresh eyes
< michaelfolkson>
At this point your doc is the most useful doc out there. Important to get it right in my view
< jonatack>
👍
< tryphe>
what do you guys think of the PR-comment reaction emojis on github? is it generally useless to thumbs up the first message in a conversation, but okay to thumbs up a comment that you agree with but don't care to comment on?
< tryphe>
or is it better to just reply?
< tryphe>
i was just curious if anyone used the emojis as a metric or if it's just purely visual in terms of review
< tryphe>
ie. maybe sometimes emojis would be preferred, or maybe it's just seen as a dumb idea
< tryphe>
in some ways i think it's hard to parse if someone gave an edited comment a thumbs up, because it's hard to know which version of the comment they approved of, but otoh replying to a full copy of a message every time seems spammy
< jonatack>
tryphe: on github projects with a small number of collaborators i think they are fun; on more widely-watched ones like bitcoin core i tend to not use them to avoid it turning into a social media or slack feed. using an emoji inside a comment, sure. worked more ideas into the doc from all three of you, thanks!
< tryphe>
lol, good point :)
< gleb>
tryphe: I like emojis and use them for different purposes. But at the same time I don't assume that everybody looks at them. So if I really need to say that I agree, I'd rather make an explicit message.
< michaelfolkson>
tryphe: Emojis definitely aren't used as a formal metric. I find them useful to see whether people are enthusiastic or not about a comment without them needing to add unnecessary comments
< michaelfolkson>
tryphe: Agree with gleb. If you have something important to say it needs a comment. If you don't have much to say but like/dislike a comment some people (e.g. gleb and me) take notice ;)
< jonatack>
hm, i do use the heart emoji to say thanks to people sometimes for reviewing
< tryphe>
ahh, good point. yeah i think in some sense emojis are somewhat redundant other than a simple "i agree but no comment needed"
< michaelfolkson>
I only realized recently when Marco commented that too many unnecessary comments is a problem. Makes reviewers life more difficult because GitHub starts hiding important review comments
< michaelfolkson>
So that makes emojis useful. You don't want lots of "Yes I agree with the above comment" statements
< sipa>
Unfortunately, emoji are also mostly anonymous.
< michaelfolkson>
No you can hover over it and see who posted the emoji?
< sipa>
sure
< sipa>
that's why i say mostly
< sipa>
but it's not clear at a first glance who liked/agreed/disliked
< sipa>
and when a comment gets brigaded by random people because someone linked it on social media, it becomes entirely worthless
< michaelfolkson>
Yup. Definitely flawed but generally maintains some limited value in most cases
< tryphe>
yeah, after a certain threshold it just becomes "alice, bob, jane, and X more reacted"
< tryphe>
i think in the future if you had a historical archive of PRs the "X more reacted" might pollute things a little bit, because you don't get to see all participants
< tryphe>
and maybe some reviewers would get washed out by mass emoji-ers
< bitcoin-git>
[bitcoin] 10xcryptodev opened pull request #19001: qt: bugfix unsupported QLocale languages (master...202005-bugfix-qlocale) https://github.com/bitcoin/bitcoin/pull/19001
< shesek>
why does `gettransaction` only list the fee for "send" transactions? is doing this for incoming transactions problematic for some reason, or just not implemented yet?
< sipa>
yes, your wallet does not know the input value of non-wallet tx inputs
< shesek>
I meant specifically for `gettransaction`, which is for wallet transactions only
< sipa>
yes, but the inputs of a transaction that pays you are not yours
< shesek>
bitcoin-qt does seem to somehow get it
< sipa>
so your wallet does not know about them
< shesek>
it does know them at validation time though, couldn't this get persisted alongside the other wallet tx information?
< sipa>
it could :)
< sipa>
but conceptually it doesn't really make sense; gettransaction shows the effect of a tx on your wallet
< sipa>
you're not the one paying the fee
< sipa>
but sure, it could be persisted
< shesek>
its useful for incoming payments to know how likely they are to get confirmed. its something that a wallet gui should probably be displaying
< sipa>
yeah, that makes sense
< shesek>
does bitcoin-qt not show the fee of confirmed incoming transactions (ie where the spent prevouts are no longer in the utxo set)?
< shesek>
I haven't used the gui in ages >_<
< shesek>
how about including the "fee" in the `{get,list}transaction` rpcs for unconfirmed transactions only? the fee is really only useful for them, and it could be deducted by a pruning node
< sipa>
i think it's cleaner to persist the fee in the wallet
< sipa>
otherwise the wallet needs access to the utxo set
< shesek>
how does this work for outgoing transactions? persisted in the wallet?
< sipa>
for outgoing transactions (at least when they're not coinjoins), you have the inputs anyway
< shesek>
ah, so it wouldn't report a fee if any of the inputs are not from the wallet? this should probably get documented
< shesek>
maybe something like "This is negative and only available for the 'send' category of transactions when all inputs are owned by the wallet."
< sipa>
if it's persisted in the wallet it would work for every transaction
< shesek>
well, sign me up as someone eagerly waiting for this :)
< sipa>
the best way to make it happen is to implement it :)