< bitcoin-git> [bitcoin] benthecarman opened pull request #15185: docs: Spelling error fix on fuzzing.md (master...docs_fuzzing_spelling_mistake) https://github.com/bitcoin/bitcoin/pull/15185
< gkrizek> wumpus the problem is the 'encoding' arg to the subprocess.check_output function. That was introduced in 3.6. I don't really see a need for that parameter anyway, so I think it's safe to just remove it. I can open a PR for it
< wumpus> gkrizek: the encoding parameter is for operating systems such as FreeBSD which don't set the system locale to utf-8 by default, leaving it up to python to decide what the locale is. It'll then pick ASCII only which results in problems in some cases.
< wumpus> gkrizek: (for example when calling git, and any of the commit messages contains characters >=128)
< bitcoin-git> [bitcoin] fanquake opened pull request #15186: rpc: remove duplicate solvable field from getaddressinfo (master...duplicate-solvable-fields) https://github.com/bitcoin/bitcoin/pull/15186
< bitcoin-git> [bitcoin] Empact closed pull request #15133: [WIP] test: Extract BuildCrediting/SpendingTransaction to shared factories folder (master...factories) https://github.com/bitcoin/bitcoin/pull/15133
< promag> wumpus: pushKV should assert(key not exist)
< promag> it checks for duplicate but overwrites
< promag> I don't think that's our use case (from API point of view)
< hebasto> wumpus: hi, mind reviewing #14250?
< gribble> https://github.com/bitcoin/bitcoin/issues/14250 | qt: Remove redundant stopThread() and stopExecutor() signals by hebasto · Pull Request #14250 · bitcoin/bitcoin · GitHub
< wumpus> promag: from the point of view of our API that's true, though on the other hand there's nothing in JSON that disallows multiple values per key
< wumpus> hebasto: sure
< wumpus> promag: and checking that *efficiently* would involve adding a set to the Univalue type; not instead, but in addition to the vector because we want to preserve order as well
< promag> true, univalue is generic and that's fine. just think it could have more specialized mutations. for instance, pushKV should be setKV ?
< wumpus> (also asserting is really dangerous here; it can turn a mild asthetic issue into a crash)
< promag> wumpus: that should be fine, we don't use user keys
< wumpus> (say, REST returns some JSON structure and the code can be manipulated to add the same field twice, somehow, whoopsie instant DoS - a better solution would be to replace the value for the existing key)
< wumpus> that'd also be "javascript semantics" FWIW, in any case there's no need to overreact to this case
< promag> wumpus: look #14984 for instance
< gribble> https://github.com/bitcoin/bitcoin/issues/14984 | rpc: Speedup getrawmempool when verbose=true by promag · Pull Request #14984 · bitcoin/bitcoin · GitHub
< wumpus> if there's anything to worry that would be that this should have been caught at *review time* :)
< promag> I'm not arguing it shouldn't be caught at review time
< wumpus> I know, but I mean *if* this should trigger any kind of discussion it's that; the end result of a key appearing twice on the API is hardly a problem, the only worry (if this was, say, a silent merge issue) is that it could have been worse
< wumpus> *silent merge conflict*
< fanquake> wumpus how often do you use the mallocinfo mode for getmemoryinfo ?
< bitcoin-git> [bitcoin] laanwj pushed 3 new commits to master: https://github.com/bitcoin/bitcoin/compare/fcb6694a9945...7ee604487f54
< bitcoin-git> bitcoin/master 1c0e0a5 Hennadii Stepanov: Remove redundant stopThread() signal
< bitcoin-git> bitcoin/master 24313fb Hennadii Stepanov: Remove redundant stopExecutor() signal
< bitcoin-git> bitcoin/master 7ee6044 Wladimir J. van der Laan: Merge #14250: qt: Remove redundant stopThread() and stopExecutor() signals...
< bitcoin-git> [bitcoin] laanwj closed pull request #14250: qt: Remove redundant stopThread() and stopExecutor() signals (master...stopthread-signal) https://github.com/bitcoin/bitcoin/pull/14250
< wumpus> fanquake: not often, why?
< fanquake> wumpus just curious, don't see it mentioned often, and had just about forgotten it was a thing, but am writing RPC wrappers. Can't use it on macOS anyways.
< wumpus> it's mostly useful for developers when diagnosing some kinds of allocation behavior
< dongcarl> luke-jr: how does OpenRC work with env vars? How are they passed to the service?
< dongcarl> I can do the same for #12255, which means the user can override env vars in their systemd unit
< gribble> https://github.com/bitcoin/bitcoin/issues/12255 | Update bitcoin.service to conform to init.md by dongcarl · Pull Request #12255 · bitcoin/bitcoin · GitHub
< gkrizek> wumpus thanks for the explanation on the encoding arg. So is there a work around for 3.4? That arg wasn’t introduced until 3.6.
< wumpus> gkrizek: I don't know if there's a workaround for earlier versions :/
< wumpus> well, one way would be to read *bytes* from the pipe and convert it to utf-8 using an encoder
< wumpus> I think I did this in the github-merge script
< wumpus> can't wait until we can support 3.6 as minimum python version tbh, this keeps coming up
< gkrizek> Ha, I agree. Ok I’ll try to look around and find some other scripts to see how they do it
< wumpus> yes see contrib/devtools/github-merge.py
< bitcoin-git> [bitcoin] MarcoFalke pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/7ee604487f54...003a47f804b1
< bitcoin-git> bitcoin/master 31097b7 benthecarman: docs: Spelling error fix on fuzzing.md
< bitcoin-git> bitcoin/master 003a47f MarcoFalke: Merge #15185: docs: Spelling error fix on fuzzing.md...
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #15185: docs: Spelling error fix on fuzzing.md (master...docs_fuzzing_spelling_mistake) https://github.com/bitcoin/bitcoin/pull/15185
< fanquake> Looks like configuring with zmq on macOS is broken after brew bumped to 4.3.1 :(
< wumpus> hmm
< bitcoin-git> [bitcoin] practicalswift opened pull request #15187: fees: Complete the removal of fee-estimation file read code for old versions (master...fee-estimation) https://github.com/bitcoin/bitcoin/pull/15187
< wumpus> fanquake: so it might be a bug in their packaging of the new version, instead of the code itself?
< bitcoin-git> [bitcoin] MarcoFalke pushed 5 new commits to master: https://github.com/bitcoin/bitcoin/compare/003a47f804b1...12b30105fc59
< bitcoin-git> bitcoin/master 638e53b practicalswift: Pin shellcheck version to v0.6.0
< bitcoin-git> bitcoin/master 07a53dc practicalswift: Remove repeated suppression. Fix indentation.
< bitcoin-git> bitcoin/master 0b7196e practicalswift: Fix warnings introduced in shellcheck v0.6.0
< bitcoin-git> [bitcoin] MarcoFalke closed pull request #15166: qa: Pin shellcheck version (master...opt-out-of-new-shellcheck-warnings) https://github.com/bitcoin/bitcoin/pull/15166
< hebasto_> provoostenator: thanks
< jnewbery> promag: I've lost the thread a bit on where we are with #13100. Is there a path to getting all the load/unload/create wallet functionality into the GUI for v0.18?
< gribble> https://github.com/bitcoin/bitcoin/issues/13100 | gui: Add dynamic wallets support by promag · Pull Request #13100 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] rex4539 opened pull request #15188: Update zmq to 4.3.1 (master...update-zmq) https://github.com/bitcoin/bitcoin/pull/15188
< luke-jr> dongcarl: /etc/conf.d/<servicename> sets them, and they're in the environment for the init script
< dongcarl> I see... I think I'll make it easy to override for systemd as well then. Thanks!
< wumpus> ryanofsky: thanks for the extensive reviews on various PRs by the way
< promag> could #15101 be merged?
< gribble> https://github.com/bitcoin/bitcoin/issues/15101 | gui: Add WalletController by promag · Pull Request #15101 · bitcoin/bitcoin · GitHub
< wumpus> promag: it's quite a large chance to the gui, would be nice if jonasschnelli could at least take a look at it
< jonasschnelli> I'll take a closer look
< wumpus> jonasschnelli: thank you!
< luke-jr> hi
< jonasschnelli> hi
< gleb> hi
< promag> hi
< promag> jonasschnelli: ty
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Jan 17 19:01:06 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< achow101> hi
< sipa> hi, will have to run in 5-10 minutes
< jamesob> hi
< wumpus> #bitcoin-core-dev Meeting: wumpus sipa gmaxwell jonasschnelli morcos luke-jr sdaftuar jtimon cfields petertodd kanzure bluematt instagibbs phantomcircuit codeshark michagogo marcofalke paveljanik NicolasDorier jl2012 achow101 meshcollider jnewbery maaku fanquake promag provoostenator aj Chris_Stewart_5 dongcarl gwillen jamesob ken281221 ryanofsky gleb
< instagibbs> sup
< wumpus> any topics?
< wumpus> (nothing scheduled in moneyball's list)
< kanzure> hi.
< gleb> I would appreciate if we prioritize 14897 because a) I know someone stacks new changes on top of it and b) people on twitter are really exciting about replicating the topology inference through this vuln. on mainnet :)
< jonasschnelli> #14897
< gribble> https://github.com/bitcoin/bitcoin/issues/14897 | randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs · Pull Request #14897 · bitcoin/bitcoin · GitHub
< wumpus> #topic high priority for review
< meshcollider> hi
< wumpus> gleb: ok added
< gleb> wumpus: thanks!
< wumpus> anyone wants anything to be added and/or removed otherwise? 7 is quite a lot of things to have in the list, in any case
< wumpus> anything nearing ready for merge?
< sipa> i'll go through them soon
< luke-jr> it seems 14897 was rewritten to do some refactoring too
< sipa> #14897
< gribble> https://github.com/bitcoin/bitcoin/issues/14897 | randomize GETDATA(tx) request order and introduce bias toward outbound by naumenkogs · Pull Request #14897 · bitcoin/bitcoin · GitHub
< luke-jr> I would think it'd be nice to get a minimal implementation of the change reviewed and merged, and THEN do the refactoring
< luke-jr> that way the former can be backported easier
< wumpus> looks like meshcollider could take over jnewbery's branch in #14491 to make it pass travis again
< gribble> https://github.com/bitcoin/bitcoin/issues/14491 | Allow descriptor imports with importmulti by MeshCollider · Pull Request #14491 · bitcoin/bitcoin · GitHub
< meshcollider> Yep I'll do that shortly
< sipa> luke-jr: i'm not sure that's possible; do you see anything obvious that could be left out?
< sipa> or gleb ?
< luke-jr> sipa: well, the original PR was apparently 60 LOC changed, and it got revised by request?
< gleb> Half of the new LOC is a comment :)
< gleb> Most of the comments on the original work were about *significant* refactoring (move from net to net_processing), which I did
< luke-jr> you mean insignificant?
< wumpus> if you got those comments and implemented them I think it's unfair to complain about it now
< luke-jr> I'm not sure I have the original code to compare
< luke-jr> if it was just a code move, no worries
< luke-jr> gmaxwell's suggestion sounded more in depth than that though
< achow101> luke-jr: github shows the diffs between force pushes now
< luke-jr> achow101: how?
< luke-jr> ooh, neat
< wumpus> ok, any other topics?
< luke-jr> rebasing seems to break it though
< meshcollider> achow101: only in a nice way if it was a commit amendment, rebases are impossible to read
< gleb> luke-jr: Well, I first did what gmaxwell suggested (before other reviews), THEN received refactoring comments, and then we moved code. Not sure which step was wrong and what I should've done better :)
< bitcoin-git> [bitcoin] practicalswift closed pull request #15187: fees: Complete the removal of fee-estimation file read code for old versions (master...fee-estimation) https://github.com/bitcoin/bitcoin/pull/15187
< luke-jr> gleb: I'm not saying any of it was wrong, just that doing it in two separate steps/PRs would make it easier to backport ONLY the fix part
< wumpus> if people deem the refactor a necessary part of this, then that should be backported too
< sipa> mostly afk, but will check occasionally if someone pings me
< wumpus> but sure if it's possible to do a minimal fix for the 0.17 branch that might be less risky, if this is a risky refactor, but if it's move-only I don't think that's the case
< wumpus> for master this is fine anyhow
< wumpus> any other topics?
< wumpus> apparently not! that's a short meeting then
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Jan 17 19:21:25 2019 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< sipa> sorry, seems i killed the party :)
< wumpus> are people traveling or is something going on?
< jonasschnelli> The usual january-downer probably. :)
< wumpus> ahh :)
< sipa> i'm about to fly to boston for the mystery hunt
< jonasschnelli> promag: with 15101, does that mean we would no longer instantiate the WalletModels at startup? (ref: with getOrCreateModel())
< promag> WalletController construtor queries interfaces::Node for all current wallets and instantiates respective WalletModel's
< promag> jonasschnelli: ^
< jonasschnelli> promag: your pull makes first use of QMutexLocker? Would it be terrible wrong to use the core class synchronisation stuff?
< jonasschnelli> But its probably a non issue
< jonasschnelli> (to use QMutexLocker)
< promag> jonasschnelli: that's true
< promag> jonasschnelli: I can replace
< jonasschnelli> I'm not sure if it makes a difference... so I'll leave it up to you. Was just a thought
< jonasschnelli> But most places in the GUI code we just use our marcos (LOCK/LOCK2)
< jonasschnelli> *macros
< wumpus> if the lock is not shared with the core code, I think it's fine to use qt primitives
< wumpus> we also use qt's threading primitives in the qt code
< luke-jr> it may actually be safer to use Qt mutexes with Qt threading
< jonasschnelli> Yeah. I agree. I only thought: "what if we port the code to the Core layer"... but I guess that never happens.
< luke-jr> well, porting implies changing that then :p
< jonasschnelli> that *will* never happen
< jonasschnelli> expect we'r writing a curses-like terminal UI.
< jonasschnelli> I agree with luke-jr, wumpus . Lets keep using QMutexLocker with QThread
< promag> ok, I won't change :P
< wumpus> ok
< bitcoin-git> [bitcoin] practicalswift opened pull request #15189: validation: Add missing cs_nBlockSequenceId lock in UnloadBlockIndex(). Add missing locking annotation for nBlockSequenceId. (master...cs_nBlockSequenceId) https://github.com/bitcoin/bitcoin/pull/15189
< bitcoin-git> [bitcoin] LifeIsPizza opened pull request #15190: [Trivial] Update copyright comments to 2019 (master...copyright-2019) https://github.com/bitcoin/bitcoin/pull/15190
< luke-jr> ^ here we go..
< sipa> every year :p
< gwillen> can we like, spent a few bucks to get a legal opinion that says you don't have to have the year in every file for it to count? :-P
< gwillen> I know that many projects do not do this anymore
< gwillen> spend*
< bitcoin-git> [bitcoin] LifeIsPizza closed pull request #15190: [Trivial] Update copyright comments to 2019 (master...copyright-2019) https://github.com/bitcoin/bitcoin/pull/15190
< sipa> gwillen: IANAL but i believe those copyright statements per file (and much less the year indications) are almost certainly worthless, especially when attributing to "The Bitcoin Core developers", which is not a legal entity
< wumpus> it wouldn't prevent people from opening such PRs anyway
< wumpus> and just posting 'we do this at the end of the year' then closing isn't exactly that much work
< gwillen> sipa: I am in total agreement with you on this
< gwillen> right, but we do still actually do it, right?
< wumpus> yes, once per year with an automated script
< gwillen> ahh, *nods*
< gwillen> removing all the years would prevent the PRs and having to run the script, but if it's fully automated I guess it could be worse.
< wumpus> hah yes, that's true, removing the years would make it impossible for people to make PR changing them
< gwillen> also like, when I made a new file that was largely code copied from another file, I spent some time contemplating what exactly I should do for the copyright header
< bitcoin-git> [bitcoin] practicalswift opened pull request #15191: validation: Add missing cs_LastBlockFile locks in PruneAndFlush() and UnloadBlockIndex(). Add missing locking annotation for nLastBlockFile and fCheckForPruning. (master...cs_LastBlockFile) https://github.com/bitcoin/bitcoin/pull/15191
< gwillen> which would be fixed by having a generic invariant one.
< luke-jr> gwillen: copyright counts even if there's no copyright notice at all
< luke-jr> IANAL also btw
< gwillen> yeah, this is true. I was going to say that a written copyright notice still does something, but after quickly reading the wikipedia article on copyright notices to refresh my memory... it's not actually clear that they have any practical effect here
< [\\\]> According to copyright.gov, "Copyright notice is optional for works published on or after March 1, 1989, unpublished works, and foreign works; however, there are legal benefits for including notice on your work."
< gwillen> the strongest benefit that Wikipedia lists is that it prevents an infringer from claiming ignorance as a defense, which would reduce statutory penalties
< wumpus> also mind that this is an international project, not only US law counts
< gwillen> anyway I'm not advocating removing the notice
< luke-jr> arguably, we can't remove it entirely due to the MIT license and Satoshi's notices
< gwillen> just replacing it with some kind of simple fixed notice with a pointer to the COPYING file, without varying years or names other than "the Bitcoin developers" or what have you
< wumpus> IIRC the idea was to remove the years, not the entire notice
< gwillen> yeah, I would advocate for removing anything that changes between files
< luke-jr> fwiw, from me, ACK removing years
< bitcoin-git> [bitcoin] practicalswift closed pull request #15189: validation: Add missing lock in UnloadBlockIndex(). Add missing locking annotation for nBlockSequenceId. (master...cs_nBlockSequenceId) https://github.com/bitcoin/bitcoin/pull/15189
< wumpus> how does the "Needs gitian build" label work? I've added it to #15188, does this mean it will automatically pick it up?
< gribble> https://github.com/bitcoin/bitcoin/issues/15188 | Update zmq to 4.3.1 by rex4539 · Pull Request #15188 · bitcoin/bitcoin · GitHub
< luke-jr> wumpus: fwiw, it's not exactly pretty, but I haven't had any problem reports from https://github.com/bitcoinknots/bitcoin/commit/8261704f7fec4e3879f2b018fc27eb8834e698a6 so far
< bitcoin-git> [bitcoin] jonasschnelli pushed 2 new commits to master: https://github.com/bitcoin/bitcoin/compare/12b30105fc59...cd42553b1178
< bitcoin-git> bitcoin/master 7c572c4 Hennadii Stepanov: Add workaround for QProgressDialog bug on macOS...
< bitcoin-git> bitcoin/master cd42553 Jonas Schnelli: Merge #15040: qt: Add workaround for QProgressDialog bug on macOS...
< bitcoin-git> [bitcoin] jonasschnelli closed pull request #15040: qt: Add workaround for QProgressDialog bug on macOS (master...20181226-fix-macos-qprogressdialog) https://github.com/bitcoin/bitcoin/pull/15040
< jonasschnelli> This is eventually ready #14353 (maybe another review)?
< gribble> https://github.com/bitcoin/bitcoin/issues/14353 | REST: add blockhash call, fetch blockhash by height by jonasschnelli · Pull Request #14353 · bitcoin/bitcoin · GitHub
< bitcoin-git> [bitcoin] practicalswift opened pull request #15192: Add missing cs_main locks in ThreadImport(...)/Shutdown(...)/gettxoutsetinfo(...)/InitScriptExecutionCache() and benchmarks/tests. Add annotations. (master...validation-cs_main) https://github.com/bitcoin/bitcoin/pull/15192
< bitcoin-git> [bitcoin] practicalswift closed pull request #11652: Add missing locks: validation.cpp + related (master...init-and-validation-locks) https://github.com/bitcoin/bitcoin/pull/11652
< bitcoin-git> [bitcoin] sdaftuar opened pull request #15193: Default -whitelistforcelay to off (master...2019-01-forcerelayoff) https://github.com/bitcoin/bitcoin/pull/15193
< dongcarl> Any further review on https://github.com/bitcoin/bitcoin/pull/14605 ?
< bitcoin-git> [bitcoin] dongcarl opened pull request #15194: Add comment describing fDisconnect behavior (master...2019-01-add-fDisconnect-comment) https://github.com/bitcoin/bitcoin/pull/15194
< fanquake> wumpus looks like that might be the case
< fanquake> dongcarl congrats on the Chaincode hire
< dongcarl> Haha thank you thank you :-)