< 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 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
< 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).
< 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.
< 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...
< 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)
< 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