< fanquake> MarcoFalke: Is there something wrong with the bot. It keeps tagging and commenting on closed PRs. Some of them more than once.
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #16386: depends: disable unused Qt features (master...slim_qt_597) https://github.com/bitcoin/bitcoin/pull/16386
< jonasschnelli> elichai2: re https://github.com/drone/drone: what I wanted is a lightweight solution. I'm not a fan of docker and I think especially for a CI its not ideal (but I may be wrong).
< wumpus> i'm really confused about #16386
< gribble> https://github.com/bitcoin/bitcoin/issues/16386 | depends: disable unused Qt features by fanquake · Pull Request #16386 · bitcoin/bitcoin · GitHub
< wumpus> i remember merging it yesterday but it shows that MarcoFalke did so
< jonasschnelli> wumpus: where did it show that MarcoFalke did?
< jonasschnelli> "where does"
< jonasschnelli> I only see Drahts "needs rebase" comments after the merge in the timeline
< bitcoin-git> [bitcoin] jonatack opened pull request #16467: rpc: sendrawtransaction help privacy note (master...sendrawtransaction-privacy-note) https://github.com/bitcoin/bitcoin/pull/16467
< jonatack> is it just me, or did one of the last dozen commits on master break building locally on debian?
< jonatack> will try to isolate which one
< jonatack> ah nvm it's building again
< jonasschnelli> jonatack: works on my end
< jonatack> jonasschnelli: thank you
< fanquake> wumpus: yes that confused me as well. It’d be good to get an explanation as to why/how the MarcoFalke merge message showed up in this channel.
< fanquake> Obviously you’d merged it first quite a while earlier, so I don’t understand how a merge message from a different user, on an already closed & merged PR has ended up posted here.
< cfields> jonasschnelli: sorry for missing the meeting yesterday. That looks great!
< cfields> I'm excited to catch up and play around.
< wumpus> fanquake: i think the github API was messed up for tha tPR, also causing the bot to misbehave
< wumpus> this is the logging from the bot: https://0bin.net/paste/PeFZGajcOPfMmXV0#Dea0H42UViZiyCF86Pb2YspJma47s9NycgKX4FSspKS it got a pull request closed event, which is strange so long after the actual merge, but anyhow, let's hope it was a fluke
< jonasschnelli> cfields: if you want to work on a better per-package dependency cache... I'm all in.
< cfields> jonasschnelli: sure, happy to lend a hand. What do you have in mind?
< cfields> jonasschnelli: fyi, the package names are deterministic and shouldn't ever collide. So the easiest option is to simply never delete them. Then, if it exists when a client requests it, it's guaranteed to be what they wanted.
< jonasschnelli> for the CI, I guess it would be ideal to just cache all possible versions of packages to avoid rebuilding depends all the times when a package change in master
< jonasschnelli> although I might have a lack of understanding how the cache works
< cfields> jonasschnelli: see ^^, then :)
< jonasschnelli> I see
< cfields> jonasschnelli: we could just add a NO_CLEANUP option, so that depends doesn't delete as it goes. It would just leave old copies behind.
< cfields> Hmm, maybe I already did that? I certainly intended to at one point.
< cfields> I'll PR if not.
< jonasschnelli> thanks
< cfields> jonasschnelli: does that fix the problem you're talking about? I'm only pretty sure we're on the same page :)
< jonasschnelli> my head is almost exploding.. I need to dive into that once I have more time. I'll let you know it I see potential for further optimization
< cfields> Haha, no worries. I'll PR the option and we'll see if it ends up helping. I think it will.
< phantomcircuit> there's a memory cliff for io, at 1GB im seeing 1000 tps reading the utxo set
< phantomcircuit> but at 4GB there's almost zero
< phantomcircuit> has anybody looked at the number of leveldb reads vs dbcache size?
< cfields> jonasschnelli: I just added the option to not delete previous builds, and it works as intended, but it occurs to me that a malicious PR could simply disable the option.
< cfields> So I'm afraid it will actually need to be handled on the builder's side.
< cfields> jonasschnelli: basically, you want a filesystem where "rm -rf" of a certain directory is a no op.
< cfields> jonasschnelli: hmm, I think a chmod/chown after the build should do the trick.
< jonasschnelli> in the CI, a PR can only affect the PRs branch cache...
< jonasschnelli> and I guess one could do way more dangerous stuff
< jonasschnelli> at least bitcoinbuilds.org could exploited on PR level quite hard...
< jonasschnelli> I need to add some protection at some point
< jonasschnelli> if it builds a new branch (PR) for the first time, it copies over master depends built, builds depends and caches under the new branch
< jonasschnelli> next time the same PR gets built, it uses that cache
< jonasschnelli> master is unaffected
< cfields> jonasschnelli: mmm, that's similar to what Travis does. It's rather non-ideal for taking advantage of depends, though :(
< cfields> jonasschnelli: let's discuss when you're less fried. I'll go ahead and push a branch with a few changes that I think would be helpful.
< jonasschnelli> thanks... its 37C here.
< sipa> jonasschnelli: jump in a lake
< fanquake> Or find a beach 🏄‍♂️
< jonasschnelli> there is no lake where I live... but the rhein... will jup in soon though!
< jonasschnelli> jump
< sipa> fanquake: i challenge you to find switzerland's coastline :)
< jonasschnelli> heh lol
< fanquake> sipa: hah yes, I guess the more viable cooling option is heading for the top of a very large mountain.
< provoostenator> What does GetDepthInMainChain do? And what is CMerkleTx? In context of #15713 where this is removed from SubmitMemoryPoolAndRelay (prev: RelayWalletTransaction).
< gribble> https://github.com/bitcoin/bitcoin/issues/15713 | refactor: Replace chain relayTransactions/submitMemoryPool by higher method by ariard · Pull Request #15713 · bitcoin/bitcoin · GitHub
< cfields> jonasschnelli: Now that I've worked through it locally, I think there are a few easy changes to make on the builder side, no need to change depends. Ping me whenever you'd like to discuss.
< bitcoin-git> [bitcoin] promag opened pull request #16468: Exclude depends/Makefile in .gitignore (master...2019-07-gitignore) https://github.com/bitcoin/bitcoin/pull/16468
< promag> elichai2: re drone, I've used it, it's cool and all, but IIRC lacks support for parallel builds and caching
< elichai2> promag: caching isn't as easy as travis but you can set it up and parallel builds is definitely supported (though you need to set a env var for it)
< jonasschnelli> elichai2: can you tell docker to only use limited resources? Similar to a VM with X amount of cores and Y amount of ram?
< jonasschnelli> cfields; sounds good. Interested to hear...
< elichai2> jonasschnelli: it seems so: https://docs.docker.com/config/containers/resource_constraints/ (altough if you have a dedicated machine for that I don't see why you would want that)
< jonasschnelli> elichai2: Good to know. I'm not an expert,... but I though limiting each build (that runs in parallel) by VM's memory and CPU may make sense for sanity reasons...
< cfields> The option is unsafe for public use, but illustrates the idea for testing.
< jonasschnelli> cfields: thanks. I see.
< jonasschnelli> How "more unsafe" is that than allowing to run arbitrary python/shell-scripts or even binaries in the build VM?
< cfields> jonasschnelli: it's unsafe in that when using that, you'd want a cache shared among all builders. Problem is that if any build removes the whole cache, that would remove the cache for all builders.
< cfields> So in reality you'd likely want to handle it with permissions changes before/after the builds.
< cfields> Or symliks, or mount options. I assume there are plenty of clever ways to handle it safely.
< jonasschnelli> cfields: is it possible somehow to make it "add only"?
< cfields> jonasschnelli: right, that's the goal. But it has to be done on the builder. Any changes to depends could be subverted by a malicious PR.
< jonasschnelli> (then eventually delete oldest package [or package with minimal hits] when cache size limit reached)
< cfields> Yup, exactly.
< jonasschnelli> I see
< bitcoin-git> [bitcoin] MarcoFalke pushed 8 commits to master: https://github.com/bitcoin/bitcoin/compare/a54a12046e98...dbf4f3f86a8f
< bitcoin-git> bitcoin/master ab28e31 Andrew Chow: Change ImportScriptPubKeys' internal to apply_label
< bitcoin-git> bitcoin/master fae7a5b Andrew Chow: Log when an import is being skipped because we already have it
< bitcoin-git> bitcoin/master c6a8274 Andrew Chow: Have importprivkey use CWallet's ImportPrivKeys, ImportScripts, and Import...
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #16301: Use CWallet::Import* functions in all import* RPCs (master...imports-use-cwallet-funcs) https://github.com/bitcoin/bitcoin/pull/16301
< MarcoFalke> > [09:49] <fanquake> wumpus: yes that confused me as well. It’d be good to get an explanation as to why/how the MarcoFalke merge message showed up in this channel.
< MarcoFalke> Just a GitHub bug
< MarcoFalke> I am glad we don't use GitHub for merges, considering how often they corrupted metadata or the git tree in the last couple of weeks
< MarcoFalke> people should start signing their ACKs *stares at everyone*
< achow101> but that takes so much more work
< promag> (hides)
< elichai2> achow101: should DescriptorImpl keep doing all the work for the deriving classes even when there's special work that needs to be done only for a specific desc? (i.e. I don't think you would want all of the taproot hashing/branching logic in `DescriptorImpl::ExpandHelper`, isn't it better that inheriting classes can override it? )
< achow101> probably, ask sipa, he wrote it
< elichai2> (like they override `IsSolvable` and `MakeScripts`, altough `MakeScripts` is a bad example because that's what `ExpandHelper` uses)
< sipa> expandhelper does all the work
< sipa> makescripts is the per-descriptor logic
< elichai2> I thought it would be better if the taproot descriptor kept the tree and expanded everything but i see that in the other cases you do all the work in `DescriptorImpl` and `ExpandHelper`, I'm just afraid that this function will blow up.
< elichai2> (so I guess `DescriptorImpl` will keep a unique_ptr to the tree and treat it like m_script_arg)
< sipa> elichai2: i guess the taproot descriptor impl should work pretty much like the p2sh/p2wsh ones?
< sipa> ah, i see
< elichai2> yeah, they do all the logic in the parent.
< sipa> instead of m_script_arg you need something to store a tree
< elichai2> (parent = DescriptorImpl)
< elichai2> yes
< sipa> you can also just put that structurr in the tapprot specific one, i think
< elichai2> until now I implemented it in the taproot descriptor itself which inherits from DescriptorImpl
< sipa> if that's easier
< sipa> it's a bit of a weird design, iknow
< elichai2> but that I can't call MakeScripts on the leafs and I can't override ExpandHelper (without changing anything)
< elichai2> *but then
< elichai2> so either I make them public/virtual or I keep everything in the DescriptorImpl as it already is
< sipa> you could turn m_script_arg into a vector
< sipa> but you'd still need some place to storr tree structure information
< elichai2> yeah... and the base class can't access the deriving class data members. so I guess i'll keep everything in DescriptorImpl
< sipa> we may need to refactor stuff at some point
< elichai2> aren't you changing some of this logic with the miniscript code?
< sipa> not every descriptor will fit in the structure imposed by the current DescriptotImpl
< sipa> yeah, i was about to mention that
< elichai2> (waiting eagerly to see your changes :) )
< elichai2> will be a hard rebase but i'll get over it haha
< bitcoin-git> [bitcoin] hebasto opened pull request #16469: refactoring: Use direct list initialization for Arg struct (master...20190726-remove-arg-ctor) https://github.com/bitcoin/bitcoin/pull/16469
< bitcoin-git> [bitcoin] MarcoFalke opened pull request #16470: test: Fail early on disconnect in mininode.wait_for_* (master...1907-testMininodeNoWaitOnDisconnect) https://github.com/bitcoin/bitcoin/pull/16470
< bitcoin-git> [bitcoin] jnewbery opened pull request #16471: [mempool] log correct messages when CPFP fails (master...2019-07-fix-CalculateMempoolAncestors-logging) https://github.com/bitcoin/bitcoin/pull/16471
< fanquake> MarcoFalke: Thanks