< glaksmono> anyone is around?
< sipa> glaksmono: sure
< bitcoin-git> [bitcoin] leiyong1413 opened pull request #13206: Zerotoone (master...zerotoone) https://github.com/bitcoin/bitcoin/pull/13206
< bitcoin-git> [bitcoin] fanquake closed pull request #13206: Zerotoone (master...zerotoone) https://github.com/bitcoin/bitcoin/pull/13206
< kallewoof> promag: Do you agree with the conclusions Nicolas and I made in #12634 regarding 0/1?
< gribble> https://github.com/bitcoin/bitcoin/issues/12634 | [refactor] Make TransactionWithinChainLimit more flexible by kallewoof · Pull Request #12634 · bitcoin/bitcoin · GitHub
< jonasschnelli> What exactly does the undocumented -forcecompactdb do?
< jonasschnelli> or say, what is the effect of leveldb's pdb->CompactRange(&slKey1, &slKey2);?
< fanquake> jonasschnelli #10526
< gribble> https://github.com/bitcoin/bitcoin/issues/10526 | Force on-the-fly compaction during pertxout upgrade by sipa · Pull Request #10526 · bitcoin/bitcoin · GitHub
< fanquake> & #10985
< kallewoof> jonasschnelli: The docs seem to indicate it clears up some space: https://godoc.org/github.com/syndtr/goleveldb/leveldb#DB.CompactRange
< jonasschnelli> fanquake: Thanks for the research!
< gribble> https://github.com/bitcoin/bitcoin/issues/10985 | Add undocumented -forcecompactdb to force LevelDB compactions by sipa · Pull Request #10985 · bitcoin/bitcoin · GitHub
< jonasschnelli> kallewoof: indeed. Thanks
< jonasschnelli> When I execute "generate 10" on a regtest node, it will only inv the most recent block to the connected peer
< jonasschnelli> Is this also expected on mainnet?
< jonasschnelli> Why only the last block? Concurrency issue?
< promag> kallewoof: I'll take a look and I'll get back to you later ok?
< kallewoof> promag: No rush!
< bitcoin-git> [bitcoin] kallewoof opened pull request #13208: wallet rpc: constraints in send* methods (master...feature-sendtoaddress-constraints) https://github.com/bitcoin/bitcoin/pull/13208
< fanquake> kallewoof looks like Travis is failing early on the CHECK DOC
< bitcoin-git> [bitcoin] glaksmono opened pull request #13209: [WIP] Refactoring platform-specific code in util.h/util.cpp (master...bitcoin-12697) https://github.com/bitcoin/bitcoin/pull/13209
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/196c5a947a07...f3e747ee775f
< bitcoin-git> bitcoin/master 09c6699 Suhas Daftuar: [qa] Handle disconnect_node race...
< bitcoin-git> bitcoin/master f3e747e MarcoFalke: Merge #13201: [qa] Handle disconnect_node race...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #13201: [qa] Handle disconnect_node race (master...2018-05-rpc-disconnect-race) https://github.com/bitcoin/bitcoin/pull/13201
< promag> achow101: hi, you have a bunch of nits in #13112: `try {` instead of `try\n` and missing space in `fprintf(stderr,"Error...`
< gribble> https://github.com/bitcoin/bitcoin/issues/13112 | Throw an error for unknown args by achow101 · Pull Request #13112 · bitcoin/bitcoin · GitHub
< Lauda> Is gettxoutsetinfo supposed to take a long time to process?
< wumpus> Lauda: yes
< wumpus> Lauda: should be in the help message
< Lauda> Thanks!
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/f3e747ee775f...1c582503507b
< bitcoin-git> bitcoin/master 12d1b77 lmanners: [tests] Fixed intermittent failure in p2p_sendheaders.py....
< bitcoin-git> bitcoin/master 1c58250 MarcoFalke: Merge #13192: [tests] Fixed intermittent failure in p2p_sendheaders.py....
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #13192: [tests] Fixed intermittent failure in p2p_sendheaders.py. (master...p2p_sendheaders) https://github.com/bitcoin/bitcoin/pull/13192
< bitcoin-git> [bitcoin] jbampton opened pull request #13210: Trivial: Remove trailing whitespace from Python files (master...remove-whitespace) https://github.com/bitcoin/bitcoin/pull/13210
< bitcoin-git> [bitcoin] laanwj opened pull request #13211: Use a semaphore or pipe for shutdown notification (master...2018_05_shutdown_notification) https://github.com/bitcoin/bitcoin/pull/13211
< bitcoin-git> [bitcoin] laanwj closed pull request #13186: Use a semaphore to trigger shutdown procedures (master...shutdown-cv) https://github.com/bitcoin/bitcoin/pull/13186
< bitcoin-git> [bitcoin] lmanners opened pull request #13212: Net: Fixed a race condition when disabling the network. (master...setnetworkactive) https://github.com/bitcoin/bitcoin/pull/13212
< Odin> anybody has a cfw for antminer s5 or s7?
< wumpus> no (but this is probably not the right channel to ask)
< MarcoFalke> Meeting in 3 min?
< wumpus> yes
< MarcoFalke> Proposed topic: Cache witness hash in CTransaction #13011
< gribble> https://github.com/bitcoin/bitcoin/issues/13011 | Cache witness hash in CTransaction by MarcoFalke · Pull Request #13011 · bitcoin/bitcoin · GitHub
< wumpus> #startmeeting
< lightningbot> Meeting started Thu May 10 19:00:27 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< promag> hi
< jonasschnelli> hi
< wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr btcdrak sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator
< provoostenator> Hi
< cfields> hi
< wumpus> proposed topics?
< sipa> ohai
< jimpo> hi
< jcorgan> hey folks
< wumpus> hello
< jimpo> topic proposal: review coordination
< wumpus> ok thanks,let's start with the usual
< wumpus> #topic High priority for review
< achow101> hi
< wumpus> #10740 #13097 #12979 #12560 #10757
< gribble> https://github.com/bitcoin/bitcoin/issues/10740 | [wallet] `loadwallet` RPC - load wallet at runtime by jnewbery · Pull Request #10740 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/13097 | ui: Support wallets loaded dynamically by promag · Pull Request #13097 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/12979 | Split validationinterface into parallel validation/mempool interfaces by TheBlueMatt · Pull Request #12979 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/10757 | RPC: Introduce getblockstats to plot things by jtimon · Pull Request #10757 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/12560 | [wallet] Upgrade path for non-HD wallets to HD by achow101 · Pull Request #12560 · bitcoin/bitcoin · GitHub
< jamesob> hi
< morcos> i'm back
< wumpus> welcome back!
< sipa> hi back, i'm pieter
< morcos> oh man...
< luke-jr> XD
< BlueMatt> mr back?
< wumpus> 10740 should be pretty close to mergeable (but getting unicorns right now)
< jimpo> Can I request #12254 for hi pri?
< gribble> https://github.com/bitcoin/bitcoin/issues/12254 | BIP 158: Compact Block Filters for Light Clients by jimpo · Pull Request #12254 · bitcoin/bitcoin · GitHub
< BlueMatt> god if this unicorns thing persists we're gonna have to move off github
< BlueMatt> (actually)
< wumpus> jimpo: ye
< jimpo> The GitHub load timeouts seem to be location specific maybe -- have you tried loading over VPN from different places?
< jimpo> Like VPN near GitHub servers?
< promag> yeah imo 10740 is good to go
< BlueMatt> mostly it works if you refresh enough or are logged out, but neither of those is a solution when the refresh count is about 10
< wumpus> no, I haven't tried using a vpn, when I tried tor it didn't help though
< BlueMatt> but we cant use a platform where half the contributors cant load prs
< jimpo> I left some late comments on 10740 yesterday. Biggest one is around the memory leak fix.
< wumpus> usually refreshing helps but sometimes it doesn't for very large PRs with lots of comments
< wumpus> agree github is pretty much useless this way
< promag> jimpo: +1, beside existing comments
< jamesob> everyone knows the emulate-mobile/incognito workaround, right?
< jamesob> (if you don't: use chrome developer tools to emulate a mobile device and problematic PRs should load)
< wumpus> jamesob: thanks
< wumpus> #topic Cache witness hash in CTransaction #13011 (MarcoFalke)
< gribble> https://github.com/bitcoin/bitcoin/issues/13011 | Cache witness hash in CTransaction by MarcoFalke · Pull Request #13011 · bitcoin/bitcoin · GitHub
< MarcoFalke> Background: The witness hash is used for all loose transactions, so caching it would speed up validation (e.g. ATMP and compact block relay). Also, the witness hash is equal to the "normal hash" for transactions without a witness, so there is overhead for rescan/reindex is currently minimal (since there are not many transactions with witness). The gains of caching the witness hash dwarf any overhead during rescan/reindex, imo. And of course, we
< MarcoFalke> can just rework rescan in a future pr.
< MarcoFalke> The code changes are trivial, so at least that shouldn't be an issue
< jamesob> would be nice if that paragraph was in the PR description
< sipa> Downside is that it makes the transactions larger, and hardcodes some validation sprcific logic into the transaction data structure (which for example also affects serving blocks from disks etc)
< BlueMatt> upside is we rectify a significant performance regression
< sipa> So my view is that we should separate the transactions-for-validation tyoe and simple mutable transactions
< BlueMatt> and obviously all pre-segwit-activation blocks will not be served any slower
< MarcoFalke> sipa: I think those can easily be fixed in future prs (I have one open for the blocks serving from disks, and wumpus as well)
< sipa> MarcoFalke: i'm aware, not objecting to amything
< BlueMatt> personally, I find it really unacceptable that we have this huge performance regression and arent taking it as something to be fixed asap
< MarcoFalke> sipa: I know, just posting for reference
< sipa> just pointing out that we don't want to do this work everywhere
< sipa> so concept ack, if we commit to separating those types
< wumpus> agree with sipa
< BlueMatt> yes, imo we should merge the per as-is now, and then when we look towards MarcoFalke's next pr splitting out the types (which is gonna be a lot more work) we will probably pull out both hashes then anyway
< sipa> there are other reasons for separating those types as well, btw
< * BlueMatt> would kinda recommending backporting the pr as-is, though
< MarcoFalke> For reference, the "separating those types" is hidden in #13098
< wumpus> let's not make the code a mess to rush ahead
< gribble> https://github.com/bitcoin/bitcoin/issues/13098 | Skip tx-rehashing on historic blocks by MarcoFalke · Pull Request #13098 · bitcoin/bitcoin · GitHub
< BlueMatt> yes, I mean just fixing the txid hashing would be hugely nice
< sipa> such as using more efficient memory representation of scripts without individial mallocs etc
< kanzure> hi.
< wumpus> yes
< cfields> topic suggestion: ^^ one big malloc
< cfields> (when this one's done)
< BlueMatt> wumpus: if anything it makes the code simpler cause the witness and non-witness code is mirrored
< MarcoFalke> Should I put #13011 in hi priority and for backport?
< gribble> https://github.com/bitcoin/bitcoin/issues/13011 | Cache witness hash in CTransaction by MarcoFalke · Pull Request #13011 · bitcoin/bitcoin · GitHub
< wumpus> BlueMatt: I just don't like confounding all kinds of aspects on the CTransaction object through, it's used in too many places
< BlueMatt> good thing miners dont pay much attention, or they wouldnt be mining segwit txn
< BlueMatt> wumpus: agreed, but point being the txid and wtxid code being symmetric isnt that much of a "mess"
< wumpus> BlueMatt: right...
< BlueMatt> and obviously agree we should be looking into rescan with a non-txid/non-wtxid type
< sipa> fait
< sipa> fair
< BlueMatt> but I have a feeling that is gonna be a lot of work
< BlueMatt> or at least take a while review-wise
< wumpus> MarcoFalke: ok added to high priority
< MarcoFalke> wumpus: thx
< MarcoFalke> I will update the OP, as requested by jamesob
< wumpus> not sure about the backport, I think it makes sense to focus on master for performance optimizations
< MarcoFalke> I don't care about the backport, but BlueMatt was asking for it
< BlueMatt> it slows down compact block relay, too, which is mostly why it sucks
< wumpus> unless it's really easy and low risk to backport it
< BlueMatt> otherwise I might care less
< cfields> agree with wumpus. I'm sure the changes are straightforward, but that's still risky for backport
< BlueMatt> the code diff is ~trivial
< morcos> i'm against the backport, but i'm often against backports. i think they don't get enough review, so they need to be really justified to do them
< wumpus> but normally I'd prefer not to backport things that are not bugfixes, or required to support bugfixes
< MarcoFalke> So let's leave it for now only in master, seems to be the conclusion
< BlueMatt> I do like having more reason for miners to upgrade to 0.16.1
< wumpus> MarcoFalke: yes
< MarcoFalke> Alright, other topics?
< BlueMatt> cfields: had one
< wumpus> #topic one big malloc (cfields)
< sipa> i like working on that, after #13062
< cfields> sipa and I have discussed this briefly, I thought it might be helpful to have a quick discussion here
< gribble> https://github.com/bitcoin/bitcoin/issues/13062 | Make script interpreter independent from storage type CScript by sipa · Pull Request #13062 · bitcoin/bitcoin · GitHub
< cfields> because there are a few different approaches
< BlueMatt> yea, I think thats the One Big motivation for 13062
< wumpus> one big alloc for the entire process?
< cfields> sipa: is your plan to move to Spans inside of CTransaction?
< cfields> with a pool for the script data tacked onto the block somewhere?
< sipa> maybe, there are a lot of possibilities
< BlueMatt> without it 13062 makes less sense than just haveing a ScriptSig and ScriptPubKey types
< cfields> wumpus: one malloc for script data per-block
< cfields> or so
< sipa> but 13062 is a prerequisite
< wumpus> 2cf
< wumpus> cfields: ok!
< sipa> (and more; there are a lot.of things that at CScript specific that shouldn't)
< cfields> well another option is std::allocator magic, without having to switch to Span
< wumpus> yes happy to see "span" hapening
< wumpus> noooo
< BlueMatt> heh, that would be a lot less code for similar results........
< wumpus> no magic
< BlueMatt> I mean you're gonna have similar levels of magic (with more layer violations) to deserialize a whole block into one pool
< wumpus> yes, but much less easy to understand code
< cfields> wumpus: i can't argue with that, it looks like voodoo
< sipa> damn cool voodoo.
< sipa> but voodoo.
< wumpus> c++ is already too much voodoo
< BlueMatt> true, fuck voodoo
< sipa> let's switch to BASIC
< * jonasschnelli> ...
< BlueMatt> though absolutely NACK 13062 unless there is demonstrated branch that is realisticly-mergeable that uses it
< cfields> anyway, it got me wondering if it'd be worthwhile to change the p2p format to be more agreeable with allocations
< sipa> BlueMatt: :(
< cfields> (next time we're changing something that is, not for this alone)
< sipa> BlueMatt: i completely disagree :)
< * BlueMatt> isnt generally a huge fan of making everything a span, its just a ton more complexity
< jonasschnelli> cfields: can you make an example for the p2p allocation issue?
< BlueMatt> and with C++ very easy to fuck up references and have them break
< sipa> not everything will become a span?
< sipa> spans are only used temporarily
< cfields> BlueMatt: how so? Seems absolutely necessary to me if we're ever going to untangle our subsystems
< jonasschnelli> cfields: you mean things like inv size comes before the acctual inv data?
< wumpus> I am a fan of making everything a span, at least for the inner processing, obviously not to keep hold of the data
< BlueMatt> sure, of course, but I see no reason to be moving the script interpreter to a generic Span thing vs using a CScript type unless there is demonstrated use
< sipa> exactly
< cfields> jonasschnelli: right
< sipa> it's abstracting representation from processing
< BlueMatt> why? if its just operating on a CScript, it should just operate on a CScript
< wumpus> I've always found that wedding CScript to a specific backing storage type was a bad idea
< jonasschnelli> Stumbled over this today.
< BlueMatt> I mean you can template it if you want :p
< * sipa> jumps off bridge
< wumpus> it implies lots of extra allocations and copying
< cfields> BlueMatt: if something is just dumb bytes, why not represent it that way?
< BlueMatt> sipa: that was obviously a joke
< sipa> BlueMatt: so was my jumping :)
< BlueMatt> its not just dumb bytes
< luke-jr> wumpus: enough to actually be an issue?
< sipa> a script is just dumb bytes
< sipa> any sequence of bytes is a valid scriptPubKey at least
< wumpus> right - it's simply a span of bytes
< BlueMatt> yea, I mean ok, sure, dont disagree in principal, but I /really/ hate taking references to things in C++
< sipa> ok
< cfields> also, I love the idea of a MultiSpan for batched operations
< BlueMatt> oops, something realloc'd and now we're executing uninitialized memory
< sipa> BlueMatt: c++ already uses references for ~everything
< BlueMatt> kinda, at least you're taking an iterator and its clear whats going on, introduce a new type and we start passing them around in the script executor and.....
< wumpus> cfields: scatter gather lists?
< BlueMatt> when its Execute(script, script) you know the thing is fine cause you're giving it a reference to the byte storage
< cfields> wumpus: right.
< BlueMatt> not an object that holds a refernece to it that was created sometime in the past
< sipa> and now it will be Execute(Span(script), Span(script))
< BlueMatt> cfields: can we not?
< sipa> same thing.
< cfields> BlueMatt: hmm?
< BlueMatt> no, it'll be push_to_background_thread(Span(script), Span(script))
< sipa> maybe
< BlueMatt> and more layers makes it less easy to see exactly whats going on
< wumpus> you can still copy of spans are the API
< wumpus> it doesn't restrict you in that
< wumpus> it just gives the *option* to use a direct memory buffer
< BlueMatt> hmm?
< sipa> you'd pass script down everything
< wumpus> just copy a vector and make a span to it
< cfields> fwiw, it'd be possible to add an aliasing shared_ptr into Span (or a different SafeSpan maybe) which would keep its owner from deallocating
< sipa> and only when invoking the execution logic you create a span
< BlueMatt> yes, I get it, and I like the option...when we have a user
< cfields> that seems wrong though.
< sipa> anyway, i'd like to start by making scriotSig a different type than scriptPubKey
< BlueMatt> I mean I'm fine with a refactor that doesnt change the exposed interface as step 1
< wumpus> I really don't see why not to do it, but anyhow
< sipa> there are a few more changes to do before that is possible
< BlueMatt> and have a wrapper in executor.cpp that just calls EvalScript(Span(arg1), Span(arg2))
< sipa> as the scriot executiin is not the only thing we do with scripts
< Odin> anybody has a cfw for antminer s5 or s7?
< BlueMatt> also, it'd be nice to have a type check on ScriptSig/ScriptPUbKey types in the public interface :p
< sipa> Odin: not here, go away
< wumpus> Odin: I've replied to this before, go away
< cfields> BlueMatt: type check?
< Odin> where I can find that, what room you suggest?
< jonasschnelli> Odin: #bitcoin
< BlueMatt> cfields: well if we make scriptpubkey + scriptsig different types, you cant pass them in the wrong order to EvalScript :p
< cfields> ...I'm pretty sure you'd know quickly enough
< BlueMatt> the libbitcoinconsensus wrapper in rust was the wrong way around for like 3 months, and had users :p
< BlueMatt> (turns out that mostly just results in it always returning true)
< sipa> "users"
< sipa> anyway, no reason to discuss this further i think
< cfields> completely thread/memory safe though :p
< sipa> there will be reasons that type/execution separation will be useful for
< cfields> agreed, thanks. Just wanted a bit of general discussion around it.
< sipa> (prevector for scriotSig is reaaally bad)
< wumpus> #topic review coordination (jimpo)
< jimpo> so this might just be clarifying what hi pri is
< jimpo> my understanding is it's blockers on longer term things
< jimpo> and mostly more established contributors
< wumpus> that's the intent, yes
< jimpo> I think there's space for another list of things that have been concept ack'ed for people to review
< sipa> bitcoinacks.com ? :)
< jimpo> so that no everyone is reviewing different stuff and there's some actual coordination
< jimpo> Yes, bitcoinacks.com is great
< jimpo> I'm curious if people think there should be more of a process around this
< wumpus> the current list is already not working well, I don't really want to add another project
< wumpus> yes bitcoinacks is great
< sipa> but yes, i agree - having a better overview on what is concept acks (anf similarly, encouraging people to concept ack/nack quickly) would be good
< BlueMatt> apparently it doesnt distinguish between nacks and acks
< sipa> ha.
< jonasschnelli> pf.
< wumpus> lol that's an interesting bug
< sipa> "nack" "nack" "nack" "merged!"
< jonasschnelli> who maintains bitcoinacks.com?
< BlueMatt> pierre, apparently
< jonasschnelli> pierre r.?
< jimpo> yeah
< BlueMatt> yes
< jimpo> wumpus: I have thoughts, but why do you say the current list isn't working well?
< jonasschnelli> Lets try to get some feature in. I think an additional layer over github would really help
< BlueMatt> jimpo: it generates very little actual review
< jonasschnelli> jimpo: people do not review it "with high priority"
< jonasschnelli> I think this is also because of the nature of OSS
< wumpus> jimpo: this comes up every meeting, so I don't particularly like to discuss it, but it doesn't attract very much review. Also because the things that end up there tend to be large, complex, hard to review changes.
< jimpo> so does that mean 1) people don't want to review 2) people don't want to review large/hard changes 3) the list isn't actually hi pri?
< wumpus> yes, it's the nature of OSS, people can review or not review whatever they want
< BlueMatt> also reviewing small things is easier, and we've had an influx of small prs over the past 6+ months
< jonasschnelli> People want to review it,.. but maybe have other priorities?
< jimpo> I realize this comes up a lot, but that's because it's a big problem IMO
< wumpus> jonasschnelli: they might just not understand the change well enough
< jimpo> the solution might be just reducing the number of large changes in the hi pri list and prioritizing smaller ones that are more likely to get merged
< promag> jimpo: 4) some require deep understanding 5) usually UI stuff has fewer reviews
< BlueMatt> its not clear there's a solution, the problem is motivation and time to review big changes, and more lists doesnt help with that
< jonasschnelli> wumpus: and that...
< BlueMatt> jimpo: even weeks with fewer entries dont get more review
< jimpo> What if it was a list of 1?
< wumpus> maybe more entries is better -> not everyone is able to review the same things!
< jimpo> wumpus: yeah, true...
< jimpo> My main point is that if everyone is reviewing different stuff because there's no coordination, then reviews go to waste
< wumpus> jimpo: then what? and who decides anyway? the point is now that everyone can have a PR on there that's blocking them, that should foster cooporation
< MarcoFalke> Just as a side note: I try to respect the high priority pull requests when I merge things and avoid merging if they create a merge conflict with one of them
< jimpo> How do other people decide what to review if it's not on the hi pri list?
< wumpus> jimpo: you could ask someone to review your PR and you'd review theirs
< wumpus> depending on what they find interesting or they see as their part of the code
< sipa> jimpo: i generally sort by recently modified
< MarcoFalke> Agree with wumpus, currently the best way to get review is to find contributors in your "area" and trade review with them
< MarcoFalke> And coordinate with them what should go in in what order
< jonasschnelli> jimpo: also, every contributor has its own "roadmap". The high-prio list was usually "one PR per contributor" to "share the roadmap"
< promag> what MarcoFalke said, git blame and ping the guys here
< luke-jr> jimpo: personally, I just browse the PR list
< achow101> I don't do review :/
< wumpus> I tend to pay most attention to bugfixes, RPC changes, GUI and networking things
< promag> achow101: its' great
< jimpo> OK, I don't have much more to say on the topic other than that I think it would be more helpful if there were more clarity around who needs to review what and what should get review.
< wumpus> everything on the PR list needs review
< wumpus> otherwise it should be closed
< wumpus> or merged
< jonasschnelli> jimpo: what do you mean with *needs to review" ?
< jimpo> Personally, I'd just be interested in reviewing whatever needs review, so I try to go through the hi pri list.
< jimpo> Like, if there's a PR and it won't be merged until at least n of this set of loose owners reviews it.
< jimpo> then, eventually they need to weigh in
< wumpus> the point of the PR list is to keep track of what needs review, it's crazy that there's so much on there now, but we can't really help it
< promag> jimpo: IMO you should try to see all PR's
< luke-jr> right, all open PRs need review
< jonasschnelli> Yes. Even a full time reviewer like promag could not bring down the open PR #
< luke-jr> otherwise they wouldn't be open
< wumpus> and as said, it helps to ping people that edited the code before you, they probaly know the code
< BlueMatt> jimpo: I agree, the million-papercut-pr review approach doesnt get us anywhere as a project
< BlueMatt> reviewing things on the highprio list does, at least to me
< jimpo> if the entire PR list is valid and there's limited time for reviews and we are not willing to close stuff quickly, then there's just a fundamental problem
< BlueMatt> I generally try to get through the high prio list once a week, though often fail
< wumpus> a lot of open source projects don't have PR lists at all, for example the freedesktop projects have a mailing list where patches are posted, and the poster of the patch has the responsibility to cc: people that are relevant for review
< wumpus> the 'dump everyone on a list' approach of github doesn't scale that well
< promag> the PR's just sit there waiting for feedback.. sometimes a concept ACK can help push it forward
< jonasschnelli> at least the have no unicorns. :/
< jimpo> well, I'm basically proposing a ranking
< promag> otherwise people keep submitting PR's
< jonasschnelli> Not sure if a "global" ranking is possible.
< promag> who ranks?
< wumpus> jimpo: if you have specific proposals on what to close, feel free to let me know
< jonasschnelli> Because that would assume we have central planing
< promag> ^
< jimpo> # of concept acks is a ranking
< jimpo> which bitcoinacks.com is helpful for
< promag> but if you only see hp then thats biased..
< jonasschnelli> Could one say a concept ACK of sipa has the same "value" as one from John Doe?
< MarcoFalke> That is up to you to decide.
< wumpus> jonasschnelli: people longer in the projects are seen to hae more expertise
< MarcoFalke> Assign a ranking to each Concept ACK :p
< wumpus> if 1000 sybil accounts show up and ack something, we'd obviously not fall for that
< BlueMatt> whatever, I dont think we're making any more progress this week than we have the last 10 times we've discussed this
< jonasschnelli> But I kinda understand jimpo. We should extend bitcoinacks and make it flexible in terms of "getting the ranked list".
< wumpus> I agree, this is too meta of a topic, it's not constructive
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu May 10 20:00:24 2018 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< promag> I've seen some projects where PR's just automatically close after some inactivity.. then the author can reopen
< jonasschnelli> Yes. We do that manually...
< jonasschnelli> But should probably do that more often
< wumpus> again, if there's anything you think that should be closed, let me know and I'll look at it
< promag> we have 267 open PR's..
< wumpus> I don't have the time nor energy to keep on top of the PR list, if that was even humanly possible
< promag> and counting
< BlueMatt> wumpus: I dont think it has been for at least 6 months :/
< luke-jr> maybe auto-close after a week of needing rebase?
< wumpus> BlueMatt: right
< MarcoFalke> Is there a way to block people from creating pull requests if they have too many outstanding ones?
< luke-jr> problem is GitHub doesn't let authors reopen..
< MarcoFalke> I guess that'd be helpful
< promag> luke-jr: oh
< luke-jr> MarcoFalke: "you can't work because others aren't"? :P
< MarcoFalke> And then a note saying "Please trade review"
< promag> but that's also unfair, I think that closing should happen because of nacks
< skeees> RCO - review coin?
< luke-jr> skeees: I was thinking about joking that..
< jonasschnelli> heh... do an ICO, quick!
< luke-jr> it might be helpful if users had an easy way to put bounties on review of PRs they like
< wumpus> promag: ideally, yes, though if no one is interested in a change, it'd make sense to close it too
< wumpus> promag: I usually close my own PRs after a while if they don't get review or discussion, but many other people are not doing that
< luke-jr> of course, then we might get the problem of false reviews to collect it :/
< promag> wumpus: yeah I also want to close some of mines
< bitcoin-git> [bitcoin] promag closed pull request #11515: Assert cs_main is held when retrieving node state (master...201710-node-state-guard) https://github.com/bitcoin/bitcoin/pull/11515
< MarcoFalke> luke-jr: Then you stop trading reviews with that person ;)
< luke-jr> as soon as I get a chance, I'm hoping to go through mine and rebase/close as appropriate
< luke-jr> MarcoFalke: how do you even detect it?
< wumpus> I don't think we have to be so afraid of fake reviews by serious devs
< promag> wumpus: I still think an explicit ack/nack is better than "timeout"
< wumpus> promag: I also think so, but it's not always what you can hope for
< luke-jr> wumpus: no, just fake reviews from newbie devs after users put a bounty on it
< wumpus> promag: in open source it's sometimes necessary to be somewhat pushy with changes, promote them, and if still no one is interested well that's it, at some point it makes no sense to keep working on it
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #12935: Add ProcessOrphans (move-only) (master...Mf1804-ProcessOrphans) https://github.com/bitcoin/bitcoin/pull/12935
< promag> wumpus: yeah I usually beg for merge right? :P
< wumpus> promag: it's easier to get attention for changes that are end-user visible
< wumpus> e.g. someone really wants a certain RPC call and is willing to test it
< wumpus> optimizations also tend to attract review quickly
< luke-jr> I wonder if it would be helpful or harmful to encourage non-devs to test stuff more
< wumpus> unless the code changes are very complex and big
< wumpus> luke-jr: we usually do that, for GUI changes
< BlueMatt> I mean part of my goal with high-prio was "everyone gets to make some progress on one of their pr's once a week", but it seems no one else wanted to use it that way, and that's fine, but unless people have new suggestiotns, its not worth discussing these things anymore, I think
< BlueMatt> maybe we should have a rule against them at meeting
< wumpus> but GUI changes are, everything considered, already easy to get merged
< promag> wumpus: for instance #12578
< gribble> https://github.com/bitcoin/bitcoin/issues/12578 | Add transaction record type Fee by promag · Pull Request #12578 · bitcoin/bitcoin · GitHub
< promag> should be merged or closed?
< wumpus> (test changes are also, usually, either quickly merged or quickly rejected)
< wumpus> promag: for that it makes a lot of sense to let some non-dev users test
< wumpus> promag: whether the new transaction list format is desirable/useful
< wumpus> promag: maybe jonasschnelli can do a build there
< promag> some non-dev users test <- rarely seen right?
< wumpus> BlueMatt: I agree
< wumpus> promag: well try to find someone that is interested in your change!
< MarcoFalke> Right, GUI changes also need testing from GUI users.
< luke-jr> promag: well, lots of PRs get into Knots, so it could be a matter of getting those users to report on their issues or lack thereof
< wumpus> promag: I"m sure you can find people to at least discuss about whether this change is desirable or not?
< wumpus> promag: it's not some obscure checkbox that moves somewhere else, but the layout of the transaction list, people will care about this
< promag> wumpus: well I don't really care about that, I just think it makes sense but I also can close it..
< wumpus> I mean you could ask on reddit, or twitter, or whereever GUI users are, I think the problem on github is that no one really has much opinion on the GUI as long as it keeps working :-)
< MarcoFalke> On firefox the mobile mode is on CTRL+SHIFT+M
< MarcoFalke> You can set a custom device
< MarcoFalke> Or just modify the UA
< wumpus> promag: I've concept ACK'ed it FWIW
< luke-jr> wumpus: I use and have an opinion that it's dumb to have 2 lines for every tx :P
< wumpus> luke-jr: you did concept ACK it, maybe retract that then?
< luke-jr> Well, it makes sense for sendmanys
< wumpus> wouldn't expanding the fee only for sendmanys be inconsistent?
< promag> luke-jr: you think the fee should be "associated" to the destinatino?
< luke-jr> promag: not really
< wumpus> I mean it might seem silly but there's certainly some precedent in billing to have separate rows for fees
< luke-jr> Maybe a checkbox to show/hide fees or something would make sense
< promag> right, what I think could work well is to aggregate the fee outside the table
< luke-jr> or even just an extra column
< wumpus> then again I don't care so deeply I really want to argue either for or against this
< promag> luke-jr: with extra column and multisend, which row you put the fee?
< luke-jr> (for tax purposes, I found I have to use JSON-RPC to get useful data)
< wumpus> would vote against adding extra columns
< promag> anyway, a clear case of lack of feedback
< luke-jr> I suppose bank statements do show fees separately - they're just uncommon in general
< promag> luke-jr: right
< promag> but my point is consistency with listransactions
< promag> amounts are different
< luke-jr> listtransactions doesn't have separate rows for fees
< promag> right, but it doesn't sum amount and fee
< Odin> what it's the best pool for btc, not slush, prohashing or antpool
< promag> bye Odin
< luke-jr> if we want to imitate listtransactions, that would suggest an extra column (which wumpus doesn't like)
< luke-jr> wumpus: what if it was hidden by default?
< wumpus> there are already so many columns, I think adding more would be against user friendly UI design, then again I don't have a dog in that fight
< wumpus> sure, you could do optional columns
< luke-jr> how about an option? "Show fees: Not at all; As an extra column; Included in amount"
< luke-jr> hopefully it will become obvious which of the latter two is preferred with time
< wumpus> making everything an option because we just can't decide :)
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/1c582503507b...cb9bbf77726a
< bitcoin-git> bitcoin/master 5d53661 John Newbery: [tests] Remove 'account' API from wallet functional tests...
< bitcoin-git> bitcoin/master cb9bbf7 MarcoFalke: Merge #13075: [tests] Remove 'account' API from wallet functional tests...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #13075: [tests] Remove 'account' API from wallet functional tests (master...remove_functional_tests_accounts) https://github.com/bitcoin/bitcoin/pull/13075