19:00:51 <wumpus> #startmeeting
19:00:51 <lightningbot> Meeting started Thu Oct 24 19:00:51 2019 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:51 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:58 <fanquake> Hi
19:00:59 <jonatack> hi
19:01:01 <sipa> hi
19:01:08 <jeremyrubin> hi
19:01:15 <MarcoFalke> I'd like to add #16975 and remove my current pull request from high prio
19:01:17 <gribble> https://github.com/bitcoin/bitcoin/issues/16975 | test: Show debug log on unit test failure by MarcoFalke · Pull Request #16975 · bitcoin/bitcoin · GitHub
19:01:32 <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 moneyball kvaciral
19:01:47 <amiti> hi
19:01:48 <moneyball> hi
19:01:48 <wumpus> #topic High priority for review
19:01:51 <kanzure> hi
19:01:57 <achow101> hi
19:02:00 <dongcarl> ih
19:02:06 <ariard> hi
19:02:51 <wumpus> MarcoFalke: done
19:03:07 <jamesob> hi
19:03:25 <MarcoFalke> thx
19:03:44 <MarcoFalke> https://github.com/bitcoin/bitcoin/projects/8
19:03:45 <fjahr> hi
19:03:45 <fanquake> I'll propose #17165 of mine, as that's now in a fairly reviewable state.
19:03:48 <gribble> https://github.com/bitcoin/bitcoin/issues/17165 | Remove BIP70 support by fanquake · Pull Request #17165 · bitcoin/bitcoin · GitHub
19:04:02 <provoostenator> hi
19:04:09 <jamesob> can I request we add #16442 to high prio?
19:04:13 <gribble> https://github.com/bitcoin/bitcoin/issues/16442 | Serve BIP 157 compact filters by jimpo · Pull Request #16442 · bitcoin/bitcoin · GitHub
19:04:25 <provoostenator> +1 for 16422
19:04:43 <wumpus> fanquake: added
19:05:01 <MarcoFalke> fanquake: Needs (trivial) rebase ;)
19:05:23 <wumpus> jamesob: provoostenator  also added
19:05:29 <jamesob> thanks!
19:05:39 <fanquake> MarcoFalke: I feel like thats at least the 3rd time I've had to rebase recently for that same file :o
19:05:47 <wumpus> which file ?
19:05:48 <provoostenator> Suggested topic BIP157 if we have time...
19:05:55 <fanquake> ci/test/00_setup_env_mac_functional.sh
19:06:18 <wumpus> oh, well, mac functional tests are gone now, you shouldn't have to rebase anymore for that
19:07:06 <instagibbs> hi
19:07:14 <wumpus> I think we have plenty of time, no topics have been suggested for today; though I think we need to discuss 0.19.0rc2 as well
19:07:28 <jtimon> can we add #17037 to chasing concept ack?
19:07:30 <gribble> https://github.com/bitcoin/bitcoin/issues/17037 | Testschains: Many regtests with different genesis and default datadir by jtimon · Pull Request #17037 · bitcoin/bitcoin · GitHub
19:07:41 <wumpus> #topic BIP157 (provoostenator)
19:08:01 <provoostenator> I found some issues while testing against Lnd / Btcd
19:08:18 <provoostenator> cc roasbeef
19:08:20 <wumpus> jtimon: added
19:08:22 <digi_james> hi
19:08:27 <jtimon> thanks
19:08:38 <instagibbs> provoostenator, testing what against, 0.19?
19:09:00 <provoostenator> btcd uses a max getcfilters of 1000
19:09:19 <provoostenator> Where the BIP uses 100
19:09:34 <jeremyrubin> suggested topic: mempool limits
19:09:37 <provoostenator> So the #16442 will disconnect from those
19:09:39 <gribble> https://github.com/bitcoin/bitcoin/issues/16442 | Serve BIP 157 compact filters by jimpo · Pull Request #16442 · bitcoin/bitcoin · GitHub
19:10:12 <provoostenator> I believe the rationale for 100 was to get those messages to about 2 MB
19:10:22 <provoostenator> Bigger means fewer round dtrips for mobile.
19:10:33 <provoostenator> I don't know if there's a downside to bigger...
19:10:42 <provoostenator> We don't send these things unsollicited
19:11:20 <MarcoFalke> That sounds like a bug in either btcd or the bip? Maybe the mailing list is a better place to discuss?
19:11:21 <provoostenator> Converesy, we don't have a rate limiter for this in the PR. Lnd, when "misconfigured" will happily fetch gigabytes per minute...
19:11:50 <provoostenator> Yeah, mailinglist makes sense regardless, but was hoping to find opinions here first.
19:12:09 <sipa> provoostenator: how does the misconfiguration manifest?
19:12:15 <sipa> is it fetching the same block over and over?
19:12:35 <provoostenator> sipa: when it checks lightning channel gossip, it refetches old filters all the time
19:12:41 <provoostenator> That's an Lnd bug imo
19:12:57 <provoostenator> But someone can do this intentionally too
19:13:30 <sipa> of course
19:13:33 <provoostenator> Do we have any rate limiting on block fetching and such?
19:14:09 <sipa> not afaik
19:14:30 <provoostenator> Ok, I guess in that case there's not much precedent to add it for filters.
19:14:35 <wumpus> no, there's no rate limiting on block fetching
19:15:05 <wumpus> it's only limited by the I/O speeds, disk and network
19:15:33 <MarcoFalke> or by -maxuploadtarget
19:16:13 <wumpus> the extra DoS vector with bloom filters is that it allowed to do a DoS on the disk without actually having to receive the data over the network, but, it's easy to saturate bandwidth
19:16:23 <wumpus> yes, there's that
19:16:40 <jnewbery> Can we add #15934 to high priority? It's blocking three other PRs which add quite nice functionality (#15935, #15936, #15937)
19:16:43 <gribble> https://github.com/bitcoin/bitcoin/issues/15934 | Merge settings one place instead of five places by ryanofsky · Pull Request #15934 · bitcoin/bitcoin · GitHub
19:16:45 <gribble> https://github.com/bitcoin/bitcoin/issues/15935 | WIP: Add /settings.json persistent settings storage by ryanofsky · Pull Request #15935 · bitcoin/bitcoin · GitHub
19:16:46 <gribble> https://github.com/bitcoin/bitcoin/issues/15936 | WIP: Unify bitcoin-qt and bitcoind persistent settings by ryanofsky · Pull Request #15936 · bitcoin/bitcoin · GitHub
19:16:48 <gribble> https://github.com/bitcoin/bitcoin/issues/15937 | WIP: Add loadwallet and createwallet load_on_startup options by ryanofsky · Pull Request #15937 · bitcoin/bitcoin · GitHub
19:16:53 <sipa> BIP157 doesn't have the same problem as the I/O required is proportional to what is sent over the network
19:16:59 <wumpus> right
19:17:25 <wumpus> jnewbery: sure, though I think with 10 blockers in high prio we're kind of pushing it
19:17:26 <jamesob> +1 on 15934
19:17:35 <jamesob> (but agree the list is getting long)
19:17:43 <provoostenator> Ok, so any thoughts on the maximum size of filter messages we send (ignoring the BIP)?
19:17:49 <jnewbery> wumpus: how about if I promise to review some of the other ones? :)
19:18:04 <instagibbs> wumpus, people have different interests in subtopics, i dont think "long" hurts more than too many type collsions
19:18:17 <wumpus> jnewbery: great!
19:18:50 <wumpus> instagibbs: 10 is fine
19:18:57 <instagibbs> :)
19:19:38 <jeremyrubin> I've been making fine progress on the things that depend on #16766, so am OK with either removing from high priority while it gets more review or else I think it's basically mergeable now.
19:19:41 <gribble> https://github.com/bitcoin/bitcoin/issues/16766 | wallet: Make IsTrusted scan parents recursively by JeremyRubin · Pull Request #16766 · bitcoin/bitcoin · GitHub
19:20:11 <wumpus> #topic 0.19.0rc2
19:20:41 <fanquake> https://github.com/bitcoin/bitcoin/milestones/0.19.0
19:20:42 <wumpus> there have been quite a few things merged since rc1, and some time has passed, I think it is time to tag rc2?
19:20:59 <fanquake> I agree. I think I backported most/all of the bug fixes
19:21:22 <wumpus> #17120 should make it in probably
19:21:24 <gribble> https://github.com/bitcoin/bitcoin/issues/17120 | gui: Fix start timer from non QThread by promag · Pull Request #17120 · bitcoin/bitcoin · GitHub
19:22:03 <fanquake> I'd sort of lost whats been happening in there. Also nother GUI only issue.
19:22:15 <provoostenator> That definately needs to be in an rc.
19:22:17 <wumpus> it's an actual serious bug, which can result in crashes
19:22:29 <MarcoFalke> so #17112 is not going to get fixed?
19:22:30 <gribble> https://github.com/bitcoin/bitcoin/issues/17112 | v0.19.0rc1 GUI repeatedly not responding · Issue #17112 · bitcoin/bitcoin · GitHub
19:22:53 <provoostenator> MarcoFalke: #1712 fixes that
19:22:54 <gribble> https://github.com/bitcoin/bitcoin/issues/1712 | Qt: possible bug related to immature balance? · Issue #1712 · bitcoin/bitcoin · GitHub
19:22:58 <wumpus> (creating qt objects like timers outside the GUI thread should be considered *really* carefully)
19:22:58 <fanquake> I opened the original issue that that is fixing. The crashes only occur, or at least the ones I saw, when you run with FATAL_WARNINGS
19:23:05 <fanquake> Which turns warnings into crashes
19:23:12 <MarcoFalke> provoostenator: Does it?
19:23:25 <provoostenator> MarcoFalke: I meant #17120
19:23:27 <gribble> https://github.com/bitcoin/bitcoin/issues/17120 | gui: Fix start timer from non QThread by promag · Pull Request #17120 · bitcoin/bitcoin · GitHub
19:23:35 <wumpus> remember, qt is essentially single-threaded
19:23:43 <wumpus> at least the GUI part
19:24:04 <fanquake> We are still talking about fixing this right #16296 ?
19:24:05 <gribble> https://github.com/bitcoin/bitcoin/issues/16296 | gui: crash with loadwallet & QT_FATAL_WARNINGS · Issue #16296 · bitcoin/bitcoin · GitHub
19:24:25 <wumpus> I'm talking about the fix in #17120
19:24:27 <gribble> https://github.com/bitcoin/bitcoin/issues/17120 | gui: Fix start timer from non QThread by promag · Pull Request #17120 · bitcoin/bitcoin · GitHub
19:24:39 <MarcoFalke> provoostenator: I thought that #17135 fixes it, but that isn't tagged for backport
19:24:41 <gribble> https://github.com/bitcoin/bitcoin/issues/17135 | gui: Make polling in ClientModel asynchronous by promag · Pull Request #17135 · bitcoin/bitcoin · GitHub
19:25:03 <wumpus> MarcoFalke: we're not sure that that fixes it, and it's too risky to merge between RCs imo
19:25:07 <fanquake> Right, 17120 will close 16296
19:25:17 <provoostenator> MarcoFalke: for the freeze UI problem there were two seperate solutions, I only tested 17120, which fixes it
19:25:33 <MarcoFalke> Ah nice
19:26:19 <fanquake> So should 17120 be high-prio, and once it's merged we tag an rc2 ?
19:26:32 <fanquake> Or do we have other rc blockers?
19:26:46 <wumpus> sgtm
19:27:12 <promag> provoostenator: wat?
19:27:20 <wumpus> #17035, though tagged 0.19.0 is definitely not a blocker imo
19:27:23 <gribble> https://github.com/bitcoin/bitcoin/issues/17035 | qt: Fix text display when state of prune button is changed by emilengler · Pull Request #17035 · bitcoin/bitcoin · GitHub
19:27:35 <wumpus> it's also nowhere near ready
19:27:40 <MarcoFalke> provoostenator: If that is the case, the pull should mention it somewhere
19:27:42 <promag> 17120 fixes UI freeze?
19:28:00 <MarcoFalke> yeah, I am doubtful as well
19:28:02 <emilengler> wumpus: The current text is a bit misleading IMO
19:28:13 <emilengler> Same with storage etc.
19:28:16 <MarcoFalke> emilengler: Is it a regression?
19:28:18 <wumpus> emilengler: yes, it is, I don't disagree
19:28:23 <MarcoFalke> If not, it can go in 0.19.1
19:28:40 <provoostenator> Oh wait, #17133 fixes those, argh
19:28:42 <gribble> https://github.com/bitcoin/bitcoin/issues/17133 | 0.19: gui: Fix start timer from non QThread by promag · Pull Request #17133 · bitcoin/bitcoin · GitHub
19:28:54 <promag> IMO both 17120 and 17135 should go to RC
19:29:05 <sipa> #17135
19:29:08 <gribble> https://github.com/bitcoin/bitcoin/issues/17135 | gui: Make polling in ClientModel asynchronous by promag · Pull Request #17135 · bitcoin/bitcoin · GitHub
19:29:41 <provoostenator> What sipa says, that's the one I tested. Indeed that needs to go in the rc too
19:30:07 <wumpus> I still think it's too much of a change to go in a rc, but ok...
19:30:41 <sipa> (to be clear i don't have a strong opinion on the issue; i was just trying to quickly check what 17135 was)
19:30:51 <promag> wumpus: what changes if you only merge after rc?
19:31:06 <wumpus> promag: it can be in master for a while
19:31:08 <MarcoFalke> I think the changes are straightforward (moving polling to a new thread)
19:31:17 <wumpus> so this creates a thread per wallet?
19:31:18 <MarcoFalke> What could possibly go wrong?
19:31:25 <wumpus> yes, what could possibly go wrong...
19:31:39 <promag> wumpus: no, one thread only
19:31:46 <sipa> last week we discussed reverting the change that exacerbated the issue; i assume that's considered too complicated?
19:32:02 <promag> ClientModel is singleton I think?
19:32:09 <wumpus> clientmodel is
19:32:15 <MarcoFalke> sipa: I think a lot more can go wrong when we remove all the lock annotations in validation/mempool
19:32:20 <promag> sipa: not a clean revert by far
19:32:21 <MarcoFalke> and restore the 0.18.0 mempool locks
19:32:28 <fanquake> sipa: At least in my opinion, reverting a mempool related bug fix to "fix" the gui doesn't seem like the way to go.
19:32:29 <sipa> MarcoFalke: that's fair
19:32:40 <MarcoFalke> agree with fanquake
19:32:42 <promag> too many lock annotations and other refactors were merged
19:33:05 <wumpus> yes, the revert is a mess
19:33:08 <sipa> ok
19:33:36 <MarcoFalke> With the gui fix the worst that could happen is that the polling in the new thread just does not work at all?
19:33:37 <promag> well I guess its ok too have a UI freezing in a RC
19:33:46 <wumpus> a lot can go wrong with qt and threads
19:34:11 <MarcoFalke> I don't know a lot about qt, so I should probably shut up
19:34:28 <promag> In this particular case I think it's fine - threading with loading wallets etc was more tricky
19:34:36 <wumpus> like, if you update the GUI from any thread but the GUI thread, you risk a race/crash
19:34:54 <MarcoFalke> crash doesn't sound too nice
19:35:21 <wumpus> it's worse than a temporary hang anyhow
19:36:33 <wumpus> anyhow, I think what 17135 does is correct
19:36:46 <wumpus> it only emits signals from the thread right?
19:36:52 <promag> right
19:37:52 <wumpus> wait, no, it's not correct
19:38:00 <promag> acquires the locks -> reads -> enqueues signal events to gui event loop -> repeat
19:38:07 <wumpus> you move pollTimer to the thread then in the destructor, delete it in the main thread
19:38:23 <MarcoFalke> Is there anything we need to do about this macOS crap?
19:38:23 <wumpus> no I'm not 100% sure about this
19:38:25 <MarcoFalke> #16387
19:38:26 <gribble> https://github.com/bitcoin/bitcoin/issues/16387 | macOS Catalina · Issue #16387 · bitcoin/bitcoin · GitHub
19:38:40 <promag> wumpus: that's fine the thread is alread stopped
19:38:47 <wumpus> promag: I don't think that makes it ok
19:38:55 <provoostenator> So macOs requires ./configure CFLAGS="-fno-stack-check"
19:38:59 <fanquake> MarcoFalke: I have not upgraded to 10.15, so someone else will have to comment
19:39:15 <wumpus> e.g. the timer affects the local event loop of the thread
19:39:15 <provoostenator> For secp256k1 tests to pass
19:39:22 <wumpus> deleting it somewhere else might mess with the main event loop
19:39:24 <provoostenator> No idea if that's a sane config flag.
19:39:33 <promag> but I stop and join the thread
19:39:40 <wumpus> I know
19:39:56 <promag> so the timer's event-loop is no longer running
19:40:08 <wumpus> but things need to be deleted inthe thread that owns them
19:40:39 <promag> yes, if the event loop is running
19:40:42 <wumpus> no, always
19:40:55 <promag> ref?
19:41:20 <promag> I can change to deleteLater(); quit(); wait() if you prefer
19:41:29 <wumpus> I'd rather have that you find for sure that this is safe
19:41:36 <promag> wumpus: deal
19:41:58 <wumpus> we've had some horrible crashes due to things like this w/ the debug console thread
19:42:20 <wumpus> it takes some very careful steps there to delete everything in the thread that owns it
19:42:42 <wumpus> provoostenator: what does no-stack-check do?
19:43:17 <provoostenator> No idea, elichai2 found this "fix" in https://github.com/bitcoin-core/secp256k1/issues/674
19:43:22 <wumpus> it doesn't disable any hardening features does it?
19:44:36 <elichai2> provoostenator: I hope this bug will be fixed before the stable release
19:44:46 <wumpus> can we find out what code makes this necessary? is it a bug on our end?
19:44:50 <elichai2> wumpus: sounds like a weird story https://stackoverflow.com/questions/10712972/what-is-the-use-of-fno-stack-protector
19:45:26 <elichai2> wait it Catalina stable already?
19:45:38 <fjahr> elichai2: I thought this was a compiler bug!?
19:45:40 <fjahr> yes
19:45:45 <wumpus> yes stack protector is what protects against buffer overflows on the stack
19:45:53 <MarcoFalke> why can't apple fix their crap?
19:46:00 <provoostenator> Catalina is released yes, they even did a few security patches...
19:46:02 <elichai2> fjahr: sounds like a compiler bug. https://forums.developer.apple.com/thread/121887 https://trac.ffmpeg.org/ticket/8073
19:46:19 <elichai2> but I don't have a mac to try and dive deep into this
19:46:19 <wumpus> we're definitely not going to disable that by default, if people want to use such a work-around they're on their own
19:46:32 <elichai2> wumpus: +1
19:46:57 <provoostenator> The gitian / rc binaries work fine, so I indeed wouldn't change anything there.
19:47:01 <elichai2> My comment was more as a step in debugging this :) I really don't know the consequences of actually using this
19:47:26 <elichai2> the bug is in AVX assembly *produced by the compiler* (i.e. secp has no avx)
19:48:21 <wumpus> ok, nothing for us to do there then
19:48:41 <elichai2> isn't it possible to just compile clang on Mac OS and use all of the llvm ecosystem instead of xcode?
19:48:52 <elichai2> but yeah, off topic
19:49:25 <wumpus> any other topics?
19:50:16 <wumpus> #endmeeting