< wumpus>
I don't think that's useful, in itself, the people doing rebases probably perceive it as needing to, what would be more useful is to list concretely when to rebase and when not
< sipa>
right; but i think it's easy to enumerate the cases when you need to: the PR no longer applies cleanly (=github complains) or (very rarely) a fresh merge succeeds but is incorrect
< luke-jr>
wumpus: sure, I was just being brief on IRC :p
< luke-jr>
making sure others agreed before I went to the trouble to write up something
< wumpus>
ok, that makes sense
< sipa>
for a while i would rebase the whole PR any time i made any changs to it... which isn't very compatible with all review workflows
< sipa>
luke-jr: to be clear, by rebase you actually changing the base of the PR, not any invocation of the "git rebase" command
< luke-jr>
sipa: yes
< sipa>
in that case, agreed
< luke-jr>
I'll be clear that fixups and such are fine
< wumpus>
I usually rebase the PR for every change when the PR is new, to avoid 'messy' commits, but when there has been review, it's better to minimise rebases
< sipa>
right
< wumpus>
it really depends on the PR though, sometimes it is even requested by reviewers to split things into (atomic) commits
< luke-jr>
would be nice if more people would do daggy fixes too :x
< wumpus>
I think it's not very easy to enumerate in pracice
< sipa>
daggy?
< luke-jr>
sipa: bugfixes having a base of the commit introducing the bug, unless it makes problems
< sipa>
luke-jr: ugh, not a fan of that :)
< luke-jr>
no downsides, and it means it's a clean merge to all affected branches
< wumpus>
I'm not sure about that, and that's also too much to ask from most people I think regarding git skills
< luke-jr>
hmm
< luke-jr>
still, even if not in docs, would be nice to see people do it more ;)
< sipa>
luke-jr: i guess i haven't even noticed you doing that, which must mean i can't complain
< luke-jr>
usually the only thing noticable is my PR comment like "this is a clean merge to x, y, z" or such
< wumpus>
elichai2: if it needs to access properties or other methods of the class it should be a method, if it is a pure function the a function is better
< sipa>
elichai2: functions are more composable
< sipa>
say you want to have some function that operates on a CTransaction, if you add it as a method, you're forcing every user of CTransaction to depend on your new code
< sipa>
as a free function, implemented where you need it (unless it's very fundamental) can avoid annoying dependency cycles
< wumpus>
also a good point
< sipa>
(^ my personal opinion, which not everyone always agrees with)
< wumpus>
I agree "is it fundamental" is the right question there
< sipa>
right, i guess the differences in opinion aren't so much the principle, but what people consider fundamental
< bitcoin-git>
bitcoin/master 4b8f1e9 Gregory Sanders: IsUsedDestination shouldn't use key id as script id for ScriptHash
< bitcoin-git>
bitcoin/master 6dd59d2 Gregory Sanders: Don't allow implementers to think ScriptHash(Witness*()) results in nestin...
< bitcoin-git>
bitcoin/master f018d0c Wladimir J. van der Laan: Merge #17924: Bug: IsUsedDestination shouldn't use key id as script id for...
< bitcoin-git>
[bitcoin] laanwj merged pull request #17924: Bug: IsUsedDestination shouldn't use key id as script id for ScriptHash (master...reuse_regression) https://github.com/bitcoin/bitcoin/pull/17924
< bitcoin-git>
[bitcoin] hebasto opened pull request #17943: qt, refactor: Remove never used default parameter (master...20200116-message-parameter) https://github.com/bitcoin/bitcoin/pull/17943
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jan 16 19:00:20 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< jamesob>
can I request we add #17737? as an aside, assumeutxo is in pretty good shape - I think it's got pretty broad concept ACK and the prototype branch seems to be working well. if I can get some help with review and testing, we might be able to `loadtxoutset` by the end of the year... and then we can start thinking about (and whether or not it's worth it) to propagate utxo snapshots through the p2p network
< 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
< wumpus>
the latter two are to work around a macosx bug with preallocating files
< sipa>
ok, that doesn't look too daunting
< wumpus>
last week there seemed to be agreement that this was important for 0.19.1, I'm somewhat sceptical about working around an OS bug at our side, but they're not terrile backs
< jamesob>
I'll take a look at 17897
< wumpus>
jamesob: great to hear that assumeutxo is on the right track!
< instagibbs>
jamesob, is 15606 actually the "child" PR?
< instagibbs>
or mothership, I dunno
< jamesob>
instagibbs: yep
< jamesob>
more mothership than child
< jamesob>
but all the assumeutxo code is there
< wumpus>
added 17737
< instagibbs>
cool, will review
< wumpus>
please also review #16702, it seems to be pretty close to mergable
< gribble>
https://github.com/bitcoin/bitcoin/issues/16702 | p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs . Pull Request #16702 . bitcoin/bitcoin . GitHub
< jnewbery>
we're having our next coredev irl meeting soonish
< promag>
hi
< jnewbery>
I'm going to send around a few questions to active contributors before the meeting, and then present the summary at the coredev meeting
< kanzure>
regarding that meeting, i plan to once again collect topics (or maybe someone else will)- so send topic proposals that you'd like to see discussed or that you would like to discuss yourself
< jnewbery>
so expect that in your inbox soon. It shouldn't take too long to fill out
< jnewbery>
that's all I had. Feel free to message me if you have any questions or thoughts about that
< kanzure>
is this meeting related or bitcoin core contributor quality of life questions?
< wumpus>
ok, good to know
< jnewbery>
kanzure: a bit of both. Trying to find out what people's priorities are and if they have any suggestions about the Bitcoin Core process, and then we can talk about them at the meeting
< kanzure>
cool. makes sense.
< kanzure>
maybe add some topic-collection questions too so i can minimize my workload :) not required though!
< luke-jr>
hi
< luke-jr>
jnewbery: do you want those of us who won't make it to do the survey?
< jnewbery>
yes please! Looking for input from anyone who's active in the project.
< jnewbery>
kanzure: good idea. I'll add a topic-collection question and share results with you
< kanzure>
thank you
< wumpus>
thanks jnewbery
< wumpus>
any other topics for today?
< wumpus>
ok, have a good week then everyone, hope we can get some more of the high priority PRs merged
< wumpus>
#endmeeting
< lightningbot>
Meeting ended Thu Jan 16 19:20:40 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< kanzure>
anyone should feel free to pm/email me topic ideas for physical meeting. i was just thinking that something about fuzzing would be a fun topic....
< kanzure>
(or the mutation testing technique for that matter.)
< luke-jr>
jnewbery: btw, by soon just how soon do you mean?
< instagibbs>
oh, I missed the pr review club on fuzzing. :( i even did my homework
< jonatack>
wallet meeting tomorrow, yes?
< instagibbs>
jonatack, yep
< instagibbs>
MarcoFalke, Seems like no one figured out how to run `test_runner.py` for the fuzzer?
< instagibbs>
at least in pr club notes
< jonatack>
instagibbs: nope. still trying to catch the bad-txns-inputs-duplicate cve with that fuzz test PR and the check removed from CheckTransaction().
< instagibbs>
I ran it overnight and decided it wasn't getting enough runs per second to be worth it for me
< MarcoFalke>
instagibbs: The test_runner is mostly for travis
< MarcoFalke>
It is meant to run over existing seeds
< MarcoFalke>
Not really meant to create new inputs