< bitcoin-git> [bitcoin] ajtowns opened pull request #18017: txmempool: split epoch logic into class (master...202001-epoch) https://github.com/bitcoin/bitcoin/pull/18017
< bitcoin-git> [bitcoin] fanquake opened pull request #18018: tests: reset fIsBareMultisigStd after bare-multisig tests (master...fix_p2sh_tests_failure) https://github.com/bitcoin/bitcoin/pull/18018
< bitcoin-git> [bitcoin] Bushstar closed pull request #18012: GBT segwit rule in RPC error msg missing single quotes (master...patch-5) https://github.com/bitcoin/bitcoin/pull/18012
< bitcoin-git> [bitcoin] fanquake pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/fe48ac8580ae...1326092e6cef
< 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] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/1326092e6cef...e061b8dc8fba
< bitcoin-git> bitcoin/master e80317b Bushstar: refactor: Remove redundant conditional
< bitcoin-git> bitcoin/master e061b8d fanquake: Merge #17971: refactor: Remove redundant conditional
< bitcoin-git> [bitcoin] fanquake merged pull request #17971: refactor: Remove redundant conditional (master...remove-redundant-conditional) https://github.com/bitcoin/bitcoin/pull/17971
< fanquake> Thanks fjahr
< fjahr> fanquake: sure :)
< bitcoin-git> [bitcoin] fanquake pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/e061b8dc8fba...c434282d2cb8
< 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] laanwj pushed 5 commits to master: https://github.com/bitcoin/bitcoin/compare/c434282d2cb8...01fc5891fb57
< 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
< bitcoin-git> [bitcoin] laanwj closed pull request #17514: util: Make logging noexcept (master...2019_11_logging_noexcept) https://github.com/bitcoin/bitcoin/pull/17514
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/01fc5891fb57...3b5b27673414
< bitcoin-git> bitcoin/master b6c3e84 Fabian Jahr: doc: Improve fuzzing docs for macOS users
< bitcoin-git> bitcoin/master 3b5b276 MarcoFalke: Merge #17942: doc: Improve fuzzing docs for macOS users
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #17942: doc: Improve fuzzing docs for macOS users (master...fuzzing_docs) https://github.com/bitcoin/bitcoin/pull/17942
< 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
< gribble> https://github.com/bitcoin/bitcoin/issues/17994 | validation: flush undo files after last block write by kallewoof . Pull Request #17994 . bitcoin/bitcoin . GitHub
< bitcoin-git> [bitcoin] laanwj pushed 3 commits to master: https://github.com/bitcoin/bitcoin/compare/3b5b27673414...c1607b5df487
< bitcoin-git> bitcoin/master ca34c5c Pieter Wuille: Add FORMATTER_METHODS, similar to SERIALIZE_METHODS, but for formatters
< bitcoin-git> bitcoin/master 4de934b Pieter Wuille: Convert compression.h to new serialization framework
< bitcoin-git> bitcoin/master c1607b5 Wladimir J. van der Laan: Merge #17957: Serialization improvements step 3 (compression.h)
< bitcoin-git> [bitcoin] laanwj merged pull request #17957: Serialization improvements step 3 (compression.h) (master...202001_noncastserial_3) https://github.com/bitcoin/bitcoin/pull/17957
< bitcoin-git> [bitcoin] sdaftuar closed pull request #16401: Add package acceptance logic to mempool (master...2019-07-package-relay) https://github.com/bitcoin/bitcoin/pull/16401
< jeremyrubin> Anyone opposed to adding a reference to review #15465 in the style guide?
< gribble> https://github.com/bitcoin/bitcoin/issues/15465 | Code style PRs after v0.18 branch split . Issue #15465 . 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? :-) )
< jeremyrubin> yeah
< bitcoin-git> [bitcoin] sipa opened pull request #18021: Serialization improvements step 4 (undo.h) (master...202001_noncastserial_4) https://github.com/bitcoin/bitcoin/pull/18021
< 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"
< jonatack> gwillen: been working on guidelines since last Spring https://jonatack.github.io/articles/how-to-review-pull-requests-in-bitcoin-core
< 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] meshcollider pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/c1607b5df487...aabec94541e2
< 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
< bitcoin-git> [bitcoin] sipa opened pull request #18023: Some asmap improvements (master...202001_asmap_nits) https://github.com/bitcoin/bitcoin/pull/18023
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/aabec94541e2...638239de7502
< bitcoin-git> bitcoin/master fa1a46e MarcoFalke: build: Fix appveyor test_bitcoin build of *.raw
< bitcoin-git> bitcoin/master 638239d MarcoFalke: Merge #18022: test: Fix appveyor test_bitcoin build of *.raw
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #18022: test: Fix appveyor test_bitcoin build of *.raw (master...2001-winTestRaw) https://github.com/bitcoin/bitcoin/pull/18022