< 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?
< 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.
< 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
< 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?
< 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
< 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] 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.
< 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?
< 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..