< bitcoin-git> [bitcoin] achow101 opened pull request #19700: wallet: Replace -zapwallettxes with wallet tool command (master...zapwallettxes-wallettool) https://github.com/bitcoin/bitcoin/pull/19700
< luke-jr> I thought that was already open O.o
< achow101> luke-jr: I've opened 3 prs, one for each option
< luke-jr> aha
< luke-jr> where can I learn about this crazy(?) new RPC implementation syntax, and why it's better than the older more readable version?
< bitcoin-git> [bitcoin] fanquake pushed 20 commits to master: https://github.com/bitcoin/bitcoin/compare/cb1ee1551cf3...ce3bdd0ed1bb
< bitcoin-git> bitcoin/master 3f1b714 Amiti Uttarwar: scripted-diff: Rename OneShot to AddrFetch
< bitcoin-git> bitcoin/master 26304b4 Amiti Uttarwar: [net/refactor] Introduce an enum to distinguish type of connection
< bitcoin-git> bitcoin/master 1521c47 Amiti Uttarwar: [net/refactor] Add manual connections to ConnectionType enum
< bitcoin-git> [bitcoin] fanquake merged pull request #19316: [net] Cleanup logic around connection types (master...2020-06-conn-refactor) https://github.com/bitcoin/bitcoin/pull/19316
< jeremyrubin> luke-jr: eh? example
< achow101> you mean the RPCHelpMan stuff?
< luke-jr> yes
< achow101> luke-jr: it's better because it aligns all the help text correctly
< achow101> the syntax is just matching brackets correctly
< luke-jr> achow101: I don't mean RPCHelpMan itself, I mean the new change of moving code inside it
< jeremyrubin> it looks like it has for some time now
< jeremyrubin> i don't see any changes from what it's been for a while
< achow101> luke-jr: oh, I see, 19550 is don't something funky
< achow101> *doing
< jeremyrubin> line 638?
< achow101> I assume so
< luke-jr> yeah
< achow101> did someone suggest him to do that? I haven't seen that syntax before
< achow101> luke-jr: I guess #19528 explains it
< gribble> https://github.com/bitcoin/bitcoin/issues/19528 | rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) by MarcoFalke · Pull Request #19528 · bitcoin/bitcoin · GitHub
< jeremyrubin> seems like a good change
< achow101> this'll be one of those things that you just copy and paste
< luke-jr> I don't see the explanation
< achow101> " However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the CRPCCommands and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors)."
< luke-jr> what does that have to do with this new RPC method definition stuff?
< jeremyrubin> it checks the args and then makes the call
< achow101> it makes the function return the RPCHelpMan which can then report the arg names
< luke-jr> it did that before this..?
< achow101> then the actual RPC stuff is a function within the RPCHelpMan
< jeremyrubin> but it required redundant and potentially inconsistent info
< luke-jr> this seems like a rather ugly way to accomplish it :/
< luke-jr> probably better to get rid of the function/return, and just declare it as a static variable?
< achow101> it's just a lambda
< luke-jr> #proposedmeetingtopic Can we recreate bitcoin-core/gui so GitHub will let us do PRs from the same <user>/bitcoin forks instead of making a new remote for everyone?
< kallewoof> Signet pull request has been updated with ajtowns suggestions, and the network is up and running now. Please review! https://github.com/bitcoin/bitcoin/pull/18267
< jonatack> review beg for #19455 (which arguably should have been in the last release with -generate to make multiple bitcoin command line tutorials work again for users), as requested by ajonas and MarcoFalke. It's a quick review with complete test coverage.
< gribble> https://github.com/bitcoin/bitcoin/issues/19455 | rpc generate: print useful help and error message by jonatack · Pull Request #19455 · bitcoin/bitcoin · GitHub
< jonatack> elichai2: "<elichai2> I don't work on anything p2p related right now, but I do plan to review a bunch of stuff, so if there'll be a high priority for review out of this it would be great" --> high prority suggestion: #19628
< gribble> https://github.com/bitcoin/bitcoin/issues/19628 | net: change CNetAddr::ip to have flexible size by vasild · Pull Request #19628 · bitcoin/bitcoin · GitHub
< jonatack> as part of #19031
< gribble> https://github.com/bitcoin/bitcoin/issues/19031 | Implement ADDRv2 support (part of BIP155) by vasild · Pull Request #19031 · bitcoin/bitcoin · GitHub
< vasild> +1
< elichai2> jonatack: Thanks!
< jonatack> elichai2: 10/10 you'll like it, lots of low-level memcpy/memcmp mixed with some Span to overhaul the critical netaddress infra ;)
< elichai2> :D
< vasild> well, in my defence, I tried to split the patch, but everything seems to be linked together...
< jonatack> that is true
< vasild> it would be possible to remove GetByte() as a first patch, but that would be lots of mechanical changes, all retouched/redone in the second patch
< jonatack> while shilling PRs, i find having the cli -netinfo peer connections dashboard (e.g. <human-readable getpeerinfo>) reassuring as a sanity check for testing running nodes with the addrv2 changes
< vasild> e.g. GetByte(3) (this gets the first byte!) would become ip[sizeof(IPV4_IN_IPV6_PREFIX) + 0] (in about 100 places) and then all those will be changed to m_addr[0] in a second patch - I think that would be wasting reviewer's time
< jonatack> vasild: i think you'll have several acks soon so would not rework it now
< vasild> let it rain acks! :)
< ryanofsky> fanquake, can you give specific examples of where review recap comments have been misleading? I think they are essential for long-lived prs because we have no explicit standards for merging
< vasild> (but not before I address your suggestions)
< ryanofsky> so without review recap comments, there's basically no transparency about why a pr has or hasn't been merged
< ryanofsky> if a comment is misleading, you can call out the comment, not discourage comments in general
< fanquake> ryanofsky: Sure, I'll try dig up some examples. I wasn't trying to discourage commenting. My point is that Concept ACKs left at the opening of a PR, are not necessarily relevant if they are being "recapped" 2 months later, after the implementation has changed, or a different approach is being taken. Similarly, if someone ACK'd something 3 rebases and a few code changes ago, I'm not really looking at that as
< fanquake> a "tested ACK".
< fanquake> Do you think people are changing how they are reviewing based on these comments, or just wether or not they are going to review something in the first place?
< fanquake> I'd also like to avoid the idea that just because a PR has *had* a large number of ACKs throughout it's lifetime, it just ready to merge. Obviously this is all dependent on the change itself, and a bit hard to generalize. However as I mentioned above, ACKs are certainly not equal when it comes to more important parts of the code.
< fanquake> Hopefully this is something we'll only have to worry about less as more (frequent) contributors are onboarded etc etc
< ryanofsky> Sure I agree with all that, I just think it's mostly obvious. I plan to keep making review recap comments and want to encourage other people to make them
< ryanofsky> So if there are specific comments that are misleading, which includes outdated acks, or mischaracterized acks, let's just respond to and fix those comments
< bitcoin-git> [bitcoin] jnewbery opened pull request #19704: Net processing: move ProcessMessage() to PeerLogicValidation (master...2020-07-process-message-plv) https://github.com/bitcoin/bitcoin/pull/19704
< wumpus> yes, definitely call out comments that you think are misleading or ask for more information if you think someone is not clear enough in their review, I definitely agree "number of ACKs" is not the only measure, judging whether something is ready for merge is not *that* trivial
< wumpus> in general I pay attention to: number of ACKs, who has ACKed, are there tests that cover the functionality, are there any unaddressed comments, if it's a bugfix did the original reporter test it, is there anyone with principal problems with the concept or its implementation, etc etc…
< wumpus> it definitely also matters where the code change is, is it in one of the risky parts or only user facing, need different kinds of review
< wumpus> this is why we need actual maintainers and not a merge-bot :-)
< jnewbery> is there a make target that I can run before pushing that will maximize my chances of passing travis? I used to do `make check && ./test/functional/test_runner.py`, but that often doesn't catch stuff now because of linters and fuzzers.
< fanquake> You could append && ./test/lint/lint-all.sh (I think) for the linters
< fanquake> If you want to sanity check the fuzzers you'll have to recompile
< troygiorshev> We were trying to get away from that, right? in #19388?
< gribble> https://github.com/bitcoin/bitcoin/issues/19388 | Build fuzz tests by default · Issue #19388 · bitcoin/bitcoin · GitHub
< wumpus> troygiorshev: yes and no: the fuzz tests that would be always built are shims and not actually useful for fuzzing, it's just to make sure there's 100% compiler coverage
< troygiorshev> wumpus: you're right, thx
< bitcoin-git> [bitcoin] laanwj pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/ce3bdd0ed1bb...bd00d3b1f203
< bitcoin-git> bitcoin/master f26502e John Newbery: [addrman] Specify max addresses and pct when calling GetAddresses()
< bitcoin-git> bitcoin/master ae8051b John Newbery: [test] Test that getnodeaddresses() can return all known addresses
< bitcoin-git> bitcoin/master 37a480e John Newbery: [net] Add addpeeraddress RPC method
< bitcoin-git> [bitcoin] laanwj merged pull request #19658: [rpc] Allow RPC to fetch all addrman records and add records to addrman (master...2020-07-addrman-get) https://github.com/bitcoin/bitcoin/pull/19658
< bitcoin-git> [bitcoin] vasild opened pull request #19705: Shrink CAddress from 48 to 40 bytes on x64 (master...shrink_caddress) https://github.com/bitcoin/bitcoin/pull/19705
< bitcoin-git> [bitcoin] theStack opened pull request #19706: refactor: make EncodeBase58{Check} consume Spans (master...20200810-util-make-base58encode-consume-spans) https://github.com/bitcoin/bitcoin/pull/19706
< jamesob> fwiw going to take some time this weekend to get the next assumeutxo PR filed, which will be the (unused) process for snapshot activation. main bottleneck is that I've gotta write some decent unittests, which will take a few hours.
< bitcoin-git> [bitcoin] achow101 closed pull request #19653: wallet: Replace -zapwallettxes with zapwallettxes RPC (master...zapwallettxes-rpc) https://github.com/bitcoin/bitcoin/pull/19653
< willcl_ark> troygiorshev: I did make a bit of progress on #19388 but could not get the linking working correctly at the end. If you have any ideas on this, my branch is here: https://github.com/willcl-ark/bitcoin/tree/default_fuzz_tests
< gribble> https://github.com/bitcoin/bitcoin/issues/19388 | Build fuzz tests by default · Issue #19388 · bitcoin/bitcoin · GitHub
< troygiorshev> willcl_ark: thanks for the link. build stuff is definitely something I'm weak on, but if I spend a day figuring it out I'll give that a look!
< nehan> "< wumpus> this is why we need actual maintainers and not a merge-bot :-)" <-- wait wumpus isn't a bot?
< bitcoin-git> [bitcoin] laanwj pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/bd00d3b1f203...13c4635a3ecf
< bitcoin-git> bitcoin/master a51d0ad Fabian Jahr: rpc: Improve addnode remove command error message
< bitcoin-git> bitcoin/master 13c4635 Wladimir J. van der Laan: Merge #19696: rpc: Fix addnode remove command error
< bitcoin-git> [bitcoin] laanwj merged pull request #19696: rpc: Fix addnode remove command error (master...rpc_net_tests) https://github.com/bitcoin/bitcoin/pull/19696
< wumpus> nehan: shhh that's supposed to be secret
< michaelfolkson> If we were going to down the road of quantifying it, it would be a reputation score for each reviewer based on various metrics e.g. bugs found in past, code reviews that led to material changes in the code other than nits, number of PRs reviewed, understanding displayed in the past on the part of the codebase they are reviewing.
< michaelfolkson> Obivuosly we won't go down that road but highlights why simple ACK counts aren't sufficient
< michaelfolkson> The only downside to beginners posting comments about testing etc that I see is GitHub hides large swathes of comments (Marco has mentioned this before). I don't see any harm otherwise when the maintainers can just ignore them.
< michaelfolkson> Assuming we all understand that one ACK can be much more valuable than another ACK
< sipa> an "expand all" button on github pages would be really useful
< sipa> it's a pain to go through the history of big PRs as an author to see if you've addressed everything
< michaelfolkson> It must be a commonly requested feature
< wumpus> michaelfolkson: maybe, the thing is, as soon as you quantify it, people will try to game it
< michaelfolkson> It wasn't a serious suggestion. Just a thought exercise ;)
< michaelfolkson> In reality the discretion of maintainers is the best option of many worse options
< sipa> goodhart's law
< luke-jr> sipa: at least a few times, I didn't even realise I was missing the comments :/
< wumpus> michaelfolkson: but yes it's annoying that github hides comments, like at some point they made the website too heavy for some browsers then they started hiding comments to compensate for that, if it was just dumb HTML there'd be no problem
< sipa> i'm ok even with if hiding stuff by default
< wumpus> or even a forum-like "page 1 of N" interface would be better than this
< sipa> but clicking to expand everything can take ages
< wumpus> yes, when you expand everything it becomes really slow
< wumpus> there's probably 40MB of javascript running in the background
< * sipa> , remembering spendong 4 hours downloading several megabytes internet explorer when he first got internet..
< bitcoin-git> [bitcoin] MarcoFalke pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/13c4635a3ecf...038a04eb80a5
< bitcoin-git> bitcoin/master c71bdf9 Hennadii Stepanov: build, test: Add support for llvm-cov
< bitcoin-git> bitcoin/master 8ebc050 Hennadii Stepanov: build: Add missed fuzz_filtered.info to COVERAGE_INFO
< bitcoin-git> bitcoin/master 75f9659 Hennadii Stepanov: build: Add missed fuzz.coverage/ directory to .gitignore
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #19688: build: Add support for llvm-cov (master...200808-lcov) https://github.com/bitcoin/bitcoin/pull/19688
< fanquake> wumpus sipa: I've also been trying to make the point to GH https://0bin.net/paste/jzVybfePdQQQgBcs#Y2Vh1PlrS2WQBeT8TRJPJPfaOlIgcsR53q1iHk+AW2l
< fanquake> Obviously still not complaining loud enough