< bitcoin-git>
bitcoin/master f1ef7f0 Andrew Chow: Don't calculate tx fees for PSBTs with invalid money values
< bitcoin-git>
bitcoin/master deaa6dd Andrew Chow: psbt: check output index is within bounds before accessing
< bitcoin-git>
bitcoin/master 1326092 fanquake: Merge #17156: psbt: check that various indexes and amounts are within boun...
< bitcoin-git>
[bitcoin] fanquake merged pull request #17156: psbt: check that various indexes and amounts are within bounds (master...psbt-fuzz-fix) https://github.com/bitcoin/bitcoin/pull/17156
< bitcoin-git>
bitcoin/master b35567f fanquake: test: only declare a main() when fuzzing with AFL
< bitcoin-git>
bitcoin/master c434282 fanquake: Merge #18008: test: only declare a main() when fuzzing with AFL
< bitcoin-git>
[bitcoin] fanquake merged pull request #18008: test: only declare a main() when fuzzing with AFL (master...macos_libfuzzer_weak_main) https://github.com/bitcoin/bitcoin/pull/18008
< bitcoin-git>
bitcoin/master 8feb4e4 Gleb Naumenko: Add asmap utility which queries a mapping
< bitcoin-git>
bitcoin/master ec45646 Gleb Naumenko: Integrate ASN bucketing in Addrman and add tests
< bitcoin-git>
bitcoin/master e4658aa Gleb Naumenko: Return mapped AS in RPC call getpeerinfo
< bitcoin-git>
[bitcoin] laanwj merged pull request #16702: p2p: supplying and using asmap to improve IP bucketing in addrman (master...asn_buckets) https://github.com/bitcoin/bitcoin/pull/16702
< wumpus>
replaced #17892 with #17994 in high prio
< gribble>
https://github.com/bitcoin/bitcoin/issues/17892 | bug-fix: delay flushing undo files until after they are finalized by kallewoof . Pull Request #17892 . bitcoin/bitcoin . GitHub
< jonatack>
jeremyrubin: I learned a great deal from reading and taking notes on the discussion in that PR. Out of curiosity were there particular comments you are referring to?
< gwillen>
jeremyrubin: like, adding it as a reference / further explanation where the styleguide talks about when one should or should not make style changes?
< gwillen>
(and to encourage people not to argue about it without reading this first? :-) )
< gwillen>
jeremyrubin: I feel like after reading that thread, one thing that I would love to see would be guidelines for how to _review_ PRs
< jeremyrubin>
Certainly. I don't want to point at specific examples, as it's a bit more of a general issue with review presently.
< gwillen>
yeah, and I think it's absolutely not bitcoin-specific, I have had similar problems with code review processes in most contexts where I've had code review
< jeremyrubin>
But the way I think about style is it's a sub-goal. And as long as the style is flagrantly bad, focusing on the substance of a PR and correctness are priroties
< gwillen>
is not* I think you meant, but yeah
< jeremyrubin>
yes
< * jeremyrubin>
adds whitespace to the end of every line
< jeremyrubin>
I also think that unlike other projects perhaps we have a low trust environment, which means that re-review is particularly annoying
< gwillen>
I think a more-structured review process would be helpful, i.e. "currently we are in design review, next we will be in general code review, then after that is nitpick review"
< jeremyrubin>
Paired with a culture of preferring squashed branches it's kind of annoying because you trigger re-review for all prior reviewers
< jeremyrubin>
Which can then add weeks to the cycle of a PR
< gwillen>
one problem with any kind of more structured review is that, as you say it's a bit low-trust, and the longer your PR is open the more likely you are to draw a comment that says "please make huge changes", which has a good chance of killing your work
< sipa>
really? i've never felt that requests for squashing delay things
< gwillen>
well, if you require everybody to re-ack after squash because the commit ID changed, it seems like it would be surprising if it did not create dleay?
< sipa>
sure, but those acks are trivial
< sipa>
i mean, obviously it adds something... but i've never seen that being a problem
< jeremyrubin>
I think they're also just not worth it -- people are likely to be less dilligent in their re-review unless they are actually fetching the branch and diffiing
< sipa>
i don't think so
< sipa>
the majority of the work when reviewing a PR is understanding it
< gwillen>
I have indeed been fetching the branch and diffing the squash
< jeremyrubin>
Anyways the squashing is a minor issue sipa
< sipa>
a re-review can be much faster, even if you diligently read every line again, just because you already know what's going on
< sipa>
jeremyrubin: fair
< jeremyrubin>
I think that if it's a functional change, fixing a bug, yes, squash it
< gwillen>
just as a matter of policy I'm not comfortable giving an ack unless I'm confident I know exactly what changed since my last ack
< gwillen>
but also I tend not to be comfortable giving an ack without a pretty detailed understanding of the change, to the point where doing a re-review (without a diff-since-last-review) does feel like a significant burden
< jeremyrubin>
but for things like comments improvements or renaming variables for semantics it's not a great use of contributor time compared to a separate fix up
< gwillen>
is everybody aware that git will show you the diff across a force-push (usually) if you click the words 'force push'
< jeremyrubin>
click?
< sipa>
sure, comment improvements can totally be separate commits
< gwillen>
sorry, github, oops. I have become the thing I despise XD
< sipa>
git push --force will also show you the diff (in the form of commitid..commitid)
< jeremyrubin>
sipa: I think this is all we're expressing is a desire to better contextualize when that's OK to just be like "address this later"
< sipa>
jeremyrubin: i think those things are mostly up to the author really
< jeremyrubin>
Because pointing to a doc or something that says "in general, style changes are not worth a re-ack unless the author wants to" is good
< jeremyrubin>
I like jonatack's doc
< gwillen>
it's a little tricky because I'd say you want at least one person other than the contributor to carefully examine it enough to say "yes, this is indeed just a style change"
< jonatack>
gwillen: even if GitHub can show the diff, i reckon it's best to do git diffing locally after pulling the changes, moreso for final acks
< gwillen>
jonatack: I agree, but I discovered the other day that if you don't have all the previous branch heads after a force push, you can't easily get them to diff them
< jonatack>
jeremyrubin: thanks, looks like i need to add a section about re-acking and git diffing
< gwillen>
there's a trick but it's annoying
< jonatack>
gwillen: agreed, pulling prev branch heads seems par for the course
< gwillen>
well in particular, after a force push of a PR branch from X to Y, if you do not already have X locally, you can't easily get X in order to "git diff X Y"
< gwillen>
you have to have already done it (which I guess is a good habit to be in anyway, having reviewed it)
< sipa>
some reviewers prefer not rebasing unless necessary (even if you're rewriting commits)
< jeremyrubin>
it also might not be the worst to have it be a maintainer script that we maintain a master unsquashed and a master squashed branch -- where master squashed squashes all commits prefixed as a fixup@<hash> to fixup @hash
< jeremyrubin>
retract that idea
< jeremyrubin>
sounds like a nightmare
< sipa>
at some point it's a tradeoff between tangible benefits and process overhead
< jeremyrubin>
I think it's reasonable to say that style/whatever fixes don't need a rebase. But if there's a bug, which requires a fix, it actually *should* invalidate all acks, because they missed the bug.
< jeremyrubin>
Anyways, jonatack if you were to make a PR for the "How to Review in Core" guide I think it could be acceptable.
< jonatack>
jeremyrubin: I still update it frequently, but maybe when it settles down
< bitcoin-git>
bitcoin/master f41d589 Antoine Riard: Document better -keypool as a look-ahead safety mechanism
< bitcoin-git>
bitcoin/master aabec94 Samuel Dobson: Merge #17719: Document better -keypool as a look-ahead safety mechanism
< bitcoin-git>
[bitcoin] meshcollider merged pull request #17719: Document better -keypool as a look-ahead safety mechanism (master...2019-12-improve-keypool-doc) https://github.com/bitcoin/bitcoin/pull/17719
< elichai2>
<<jeremyrubin> > Anyways, jonatack if you were to make a PR for the "How to Review in Core" guide I think it could be acceptable. I'd definitely like to read it :)
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #18022: test: Fix appveyor test_bitcoin build of *.raw (master...2001-winTestRaw) https://github.com/bitcoin/bitcoin/pull/18022