< kallewoof> Signet review beg -- no more changes planned, probably RFM: https://github.com/bitcoin/bitcoin/pull/18267
< phantomcircuit> sipa, 563,289,128
< sipa> phantomcircuit: that's unpossible
< sipa> txouts?
< phantomcircuit> wait did i count transactions
< sipa> i count 566823026 transactions
< phantomcircuit> damn it
< phantomcircuit> sipa, yeha i counted short at 99.4% done
< phantomcircuit> lets try that again
< phantomcircuit> 2020-09-10T04:49:27Z FlushStateToDisk: write coins cache to disk (72011604 coins, 9823747kB) started
< phantomcircuit> ok lets try that in 10-20 minutes
< phantomcircuit> only took 411.70s
< sipa> i hope my numbers are accurated, i have a script that maintains a database with some per-block statistics
< sipa> so i just added all the numbers in it
< phantomcircuit> sipa, 1,506,642,276
< sipa> that's more like it
< phantomcircuit> sipa, yeah so this guy has no hope https://github.com/bitcoin/bitcoin/issues/19921
< sipa> phantomcircuit: it's not O(m*n)
< sipa> IsMine is O(log n) in the number of keys you have, i think
< phantomcircuit> sipa, there's a mapWallet.count, IsMine, IsFromMe, all of which will run if the transaction doesn't involve your wallet
< sipa> phantomcircuit: yes, but they run once per transaction, not per transactions*keycount
< sipa> only the lookups in the keystore have a time that scales with the number of keys, but it's not linear
< phantomcircuit> sipa, right so it's O(m log n) for m transactions and n keys
< phantomcircuit> or something close to that
< phantomcircuit> transaction outputs not transactions
< sipa> yes
< sipa> O(outputs+inputs)
< sipa> O((outputs+inputs)*log(keys))
< achow101> it'll just take a long time
< achow101> probably a bit faster if mapKeys was unordered map instead of map
< sipa> yeah
< sipa> and way faster if it was a map of scriptPubKeys ;)
< achow101> I suppose he could try #16910
< gribble> https://github.com/bitcoin/bitcoin/issues/16910 | wallet: reduce loading time by using unordered maps by achow101 · Pull Request #16910 · bitcoin/bitcoin · GitHub
< achow101> changes a bunch of things to unordered_map/unordered_set
< hebasto> who is interested in today's meeting topic suggested by jnewbery mind considering the overview gist https://gist.github.com/hebasto/072ad3a9370641b035a36d08607a3d34
< bitcoin-git> [bitcoin] sipa opened pull request #19931: Change CSipHasher's count variable to uint8_t (master...202009_siphash_sillyness) https://github.com/bitcoin/bitcoin/pull/19931
< bitcoin-git> [bitcoin] hebasto closed pull request #19926: gui: Add Tor icon (master...200909-tor) https://github.com/bitcoin/bitcoin/pull/19926
< jonasschnelli> #proposedmeetingtopic review the experience of 3 month bitcoin-core/gui repository
< bitcoin-git> [bitcoin] Saibato opened pull request #19933: wallet: bugfix; if datadir has a trailing '/' listwalletdir would strip lead char of walletname (master...wallet-fix-missing-chars-boost-1.47) https://github.com/bitcoin/bitcoin/pull/19933
< bitcoin-git> [bitcoin] laanwj pushed 4 commits to master: https://github.com/bitcoin/bitcoin/compare/564e1ab0f3dc...a47e5964861d
< bitcoin-git> bitcoin/master 2ac8bf9 Pieter Wuille: Implement keccak-f[1600] and SHA3-256
< bitcoin-git> bitcoin/master 3f01ddb Pieter Wuille: Add SHA3 benchmark
< bitcoin-git> bitcoin/master ab654c7 Pieter Wuille: Unroll Keccak-f implementation
< bitcoin-git> [bitcoin] laanwj merged pull request #19841: Implement Keccak and SHA3_256 (master...202008_sha3) https://github.com/bitcoin/bitcoin/pull/19841
< bitcoin-git> [bitcoin] practicalswift opened pull request #19934: tests: Add fuzzing harness for Keccak and SHA3_256 (master...fuzzers-keccak-and-sha3_256) https://github.com/bitcoin/bitcoin/pull/19934
< bitcoin-git> [bitcoin] achow101 opened pull request #19935: Move SaltedHashers to separate file and add some new ones (master...mv-saltedhash) https://github.com/bitcoin/bitcoin/pull/19935
< robertnnms> Hi,I need to do advanced testing on testnet, but I would need between 10-15 BTC, could someone send me to the following address: tb1qs25dr4e8k29rwclxvnxedfdtprh5e89jtp5wleMany thanks
< dongcarl> Anyone know what the `UnloadBlockIndex` call here does in the context of initialization? https://github.com/bitcoin/bitcoin/blob/master/src/init.cpp#L1562
< wumpus> dongcarl: it's in a retry loop, I think it's there in case a previous block index load only succeeds partially?
< sipa> yep
< dongcarl> I see, that makes sense...
< phantomcircuit> achow101, the only real solution for rescanning with a huge number of keys is to use the block filter indexes
< phantomcircuit> it takes absolutely ages otherwise
< phantomcircuit> sipa, lol he closed the issue, what a child
< phantomcircuit> still wondering how he ended up with 150m keys
< sipa> i assume he generated them using a dictionary
< jonasschnelli> a cracker!
< jonasschnelli> Why he is not using a full indexer like electrs or so is unclear to me.
< wumpus> if it's a criminal even more reason to not give them ideas
< jonasschnelli> maybe he lost some of his mnemonic words...
< jonasschnelli> also... i would rather scan agains the UTXO set in that case (and not against all blocks)
< yanmaani> It's not a very competent attack that's for suer
< wumpus> #startmeeting
< lightningbot> Meeting started Thu Sep 10 19:00:25 2020 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
< kanzure> hi
< achow101> hi
< jonasschnelli> hi
< hebasto> hi
< fjahr> 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 moneyball kvaciral ariard digi_james
< wumpus> amiti fjahr jeremyrubin lightlike emilengler jonatack hebasto jb55 elichai2
< wumpus> two proposed meeting topics for today
< vasild> hi
< wumpus> strategies for removing recursive locking in the mempool (jnewbery), review the experience of 3 month bitcoin-core/gui repository (jonasschnelli)
< meshcollider> hi
< wumpus> any last minute topic proposals?
< jonatack> hi
< wumpus> #topic High priority for review
< wumpus> https://github.com/bitcoin/bitcoin/projects/8 11 blockers, 1 bugfix, 2 chasins concept ACK
< jonatack> #19845?
< gribble> https://github.com/bitcoin/bitcoin/issues/19845 | net: CNetAddr: add support to (un)serialize as ADDRv2 by vasild · Pull Request #19845 · bitcoin/bitcoin · GitHub
< meshcollider> Actually 10 now :) one was merged and hadn't been removed
< jnewbery> hi
< wumpus> jonatack: added
< meshcollider> #16378 pls
< gribble> https://github.com/bitcoin/bitcoin/issues/16378 | The ultimate send RPC by Sjors · Pull Request #16378 · bitcoin/bitcoin · GitHub
< achow101> #19077 please
< gribble> https://github.com/bitcoin/bitcoin/issues/19077 | wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101 · Pull Request #19077 · bitcoin/bitcoin · GitHub
< promag> #19033 for hp, ty
< gribble> https://github.com/bitcoin/bitcoin/issues/19033 | http: Release work queue after event base finish by promag · Pull Request #19033 · bitcoin/bitcoin · GitHub
< phantomcircuit> hi
< * luke-jr> pokes gribble
< wumpus> meshcollider, achow101 added
< wumpus> promag: already on there, right?
< jeremyrubin> i think so
< promag> indeed, I guess I have to name it "http: blocklist work queue..."
< wumpus> unless someone else just added it, but I vaguely remember it was already there
< wumpus> #topic Strategies for removing recursive locking in the mempool (jnewbery)
< jnewbery> Thanks wumpus
< jnewbery> summary: RecusiveMutex is generally considered bad, and avoided if possible. It'd be nice to change the mempool's mutex from recursive to non-recursive
< wumpus> I think getting rid of the recursive locks is good, though, it seems changes are becoming increasingly risky
< wumpus> and involved
< jnewbery> There are different ways to do this, and hebasto would like some input on which way is preferred
< wumpus> we had all the low-hanging fruit I guess
< jnewbery> wumpus: i don't think so. I think there's a lot more low-hanging fruit (although cs_mempool isn't very low hanging)
< hebasto> tracking issue for all recursive mutexes #19303
< gribble> https://github.com/bitcoin/bitcoin/issues/19303 | Replace all of the RecursiveMutex instances with the Mutex ones · Issue #19303 · bitcoin/bitcoin · GitHub
< jeremyrubin> Probably makes more sense to have the locking external to the functions
< jeremyrubin> More general to e.g., calling a function in a for loop
< jnewbery> Two possible ways are making a lot more of the mempool functions require the lock, and make locking external to the mempool
< wumpus> "... two functions could be used - one that locks and another that does not..." yes, that's the common solution
< jnewbery> and the other way is having two versions of the function - one which requires the lock and one which takes the lock
< jnewbery> wumpus: right
< wumpus> moving locking external has its own risks
< hebasto> what risk?
< jeremyrubin> wumpus: if we use the clang safety annotations it's lesser, right?
< wumpus> having separate private "without lock" avoids that
< jnewbery> wumpus: I agree. Best to keep locking internal wherever possible
< vasild> "the locking mechanism used by a thread-safe class is part of its internal implementation" (from https://gist.github.com/hebasto/072ad3a9370641b035a36d08607a3d34#gistcomment-3448023)
< * luke-jr> stabs ISP
< sipa> ideally, locks are internal
< wumpus> hebasto: well, it goes against, the principle of encapsulation, knowledge of how to remove locks moves to outside the code
< wumpus> hebasto: at the least it makes reasoning more difficult
< hebasto> wumpus: agree
< sipa> if a lock is internal to a class, and the class never invokes a callback passed down from elsewhere while holding that lock, it even guarantees no deadlocks
< wumpus> lock annotations are not a replacement for that
< jnewbery> if locks can't always be internal, then making two versions of the public interface function makes it explicit where the function is called with the lock held and without.
< sipa> jnewbery: that's what CAddrMan does
< sipa> but i don't see how one conflicts with the other
< jeremyrubin> What about a type-safe (using newtype) pass lock guard in as arugment?
< jeremyrubin> That would have both safety properties
< sipa> jeremyrubin: it works, but imho it still reeks of bad design
< jeremyrubin> as there would only be one way to get a satisfying lock
< wumpus> I don't really like that, I hope that can be avoided
< promag> and I'd like to discuss for() { LOCK(cs); } VS { LOCK() for() {} }
< hebasto> how to name this two functions? some kind of suffix to distinguish them?
< jnewbery> sipa: I'm not saying they do. Ideally, all locks are internal. If they can't be and sometimes need to be held by the caller, make it explicit where that is by using Mutex and having two versions of public functions where needed.
< aj> promag: with or without performance benchmarks?
< wumpus> promag: well that depends on the granularity of the work, so locking overhad versus the loop body, and whether it's undesirable to hold the lock for long
< sipa> promag: grabbing an uncontended lock is ~100 cpu cycles
< jeremyrubin> hebasto: I'd do a type tag argument maybe; like atmoic primitives.
< jeremyrubin> but maybe that's overkill
< sipa> jnewbery: sorry, i misread what you suggested
< aj> jeremyrubin: void foo(int x); void foo(int x, LockAlreadyHeld h); foo(3, LockAlreadyHeld()); // ?
< wumpus> I'm starting to understand Marcofalke's point now--this is a lot of work, conflicts with other changes, while we don't have a direct problem with RecursiveMutex
< aj> jeremyrubin: LockAlreadyHeld(cs_main); ?
< jeremyrubin> aj: Uh that can work yeah
< vasild> promag: I would say if the current code does one of that, we better have some proof that changing it is for the better.
< sdaftuar> wumpus: that is my gut reaction as well
< promag> aj: in simple cases not really
< sdaftuar> it would be nice to motivate this change with some new behavior we wish to implement, that woudl benefit from it?
< wumpus> if it helps to have a non-recursive mutex for future mempool changes it makes sense to do it, of course, but this seems a lot of design work for a pure refactor
< wumpus> sdaftuar: +1
< jeremyrubin> overall, same. It works as is.
< luke-jr> I suspect it actually might make future changes harder :x
< sipa> luke-jr: depends on how it's done, i think
< jeremyrubin> I think that's the motivation; if future changes are leaning more on the recursive mutexing we don't want to make them that way
< sipa> really usually cleaning up locking means cleaning up the boundary between interfaces
< sipa> and if that's a side effect, it may definitely make things easier
< promag> sipa: I'm taking about cs_main
< jnewbery> sipa: +1
< wumpus> sipa: yes, if it can make that clearer it's a good thing
< promag> wumpus: agree
< jnewbery> cs_main is something else entirely
< sipa> just doing a dumb "whatever necessary to avoid recursive locking" is unlikely to be beneficial
< wumpus> agree
< promag> vasild: agree
< sipa> but it's probably hard to speak very generically here
< jnewbery> I think adding a lock-not-held version of all public functions which may or may not hold a lock is a pretty trivial change
< promag> i'm meh with that approach
< jeremyrubin> It's kinda annoying for other patches if you have to change 2 func signatures
< wumpus> jnewbery: at least that can be done virtually mechanically which makes it easier to review
< jnewbery> I think changing the function to assert the lock is held and then adding locks outside the class _isn't_ a good change
< vasild> funcUnprotected() { do stuff; }; func() { LOCK(); funcUnprotected(); }
< sipa> jnewbery: that does not seem crazy, but it's easier to judge with code
< wumpus> jnewbery: +1
< sdaftuar> i'm a bit confused -- would some of those functions not be used?
< promag> jnewbery: IMO it's a good change if it comes with a follow up refactor
< luke-jr> I don't see what's wrong with func() that locks if necessary, but also works inside an external lock :x
< jnewbery> sdaftuar: you'd only add new functions where it may or may not already hold the lock
< jeremyrubin> luke-jr: I think that's basically a recursive lock
< sipa> luke-jr: it's a very abstract objection, but to me it is: code should be written to work inside our outside the private parts of a class that need locking
< vasild> luke-jr: that's like re-implementing recursive mutex
< jeremyrubin> the difference being maybe you can assert if you know you have lock already or not
< luke-jr> sipa: this does..?
< sipa> luke-jr: it's just hard to reason about the interface if you have code that works in both
< wumpus> hand rolling a recursive-ish mutex sounds even worse than simply using one
< sipa> please no
< nehan> the argument in https://gist.github.com/hebasto/072ad3a9370641b035a36d08607a3d34 for why RecursiveMutex is bad indicates why it's bad to use RecursiveMutex in the first place (it is a symptom of an underlying problem). i do not see how it explains the benefit of removing RecursiveMutex without addressing the underlying problem.
< sipa> nehan: +1
< nehan> which it sounds like what this dual-function thing is doing
< sipa> nehan: i think the dual function makes the interface layer explicit
< nehan> so the benefit of it is making a small (perhaps important) step towards making the interface more clear?
< luke-jr> my point wasn't to reinvent RM, but that RM has a use :p
< jnewbery> and by making it explicit, brings to attention what the underlying problem may be
< sipa> nehan: right, it makes it obvious where the problem is
< sipa> it doesn't fix it
< wumpus> nehan: yes, the general arguments against recursive mutex described there definitely make sense
< sdaftuar> so i think the right next step would be to precisely define what the interface ought ot be
< sdaftuar> and then we can work towards that
< jeremyrubin> It sounds like it might be worth to prepare this PR and not merge it
< jeremyrubin> Because if it indentifies what the problem is, then we can see if a fix can be built on it
< sdaftuar> refactoring without a design in mind seems bad to me
< jnewbery> sometimes it's not a problem to have a function that can be called with or without a lock. Sometimes you want to carry out multiple operations on an object atomically. That's not a problem, but it's always better to be explicit about what you're doing
< bitcoin-git> [bitcoin] instagibbs opened pull request #19936: Test: batch rpc with params (master...batch_param) https://github.com/bitcoin/bitcoin/pull/19936
< promag> sipa: WITH_LOCK(..., ...) is already an indicator
< jnewbery> hebasto: did you have anything else that you wanted input on?
< vasild> I think we can/should make mempool.cs mutex private
< jnewbery> vasild: that's a lot of work
< jnewbery> and might not even be desirable
< hebasto> jnewbery: it seems all discussed. what is consensus?
< promag> vasild: lots of refactors right?
< vasild> IMO amount of work would be comparable to making it non-recursive
< jnewbery> vasild: not even close, I don't think. There are lots of places where we make multiple calls to the mempool atomically. Those would all need to be changed into higher-level interfaces to the mempool
< wumpus> vasild: at least that'd remove the problem of public functions needing the lock held
< wumpus> but yes, it's also much more work
< vasild> ok, my assessment could be wrong
< jeremyrubin> wild idea: mempool via message passing only?
< jnewbery> vasild: eg all of this would need to become a single call to the mempool: https://github.com/bitcoin/bitcoin/blob/564e1ab0f3dc573bd3ea60a80f6649c361243df9/src/rpc/blockchain.cpp#L1406-L1421
< jeremyrubin> Might be good as there are some proposals to make the mempool a separate entity
< jeremyrubin> and message passing based would cleanly separate internal details
< hebasto> vasild: that is possible
< vasild> jnewbery: yes, I looked into that, it would be one call that returns a struct holding all the data, not a big deal
< jonatack> i like sdaftuar's suggestion that it be motivated by new behavior or a clear idea of the desired design/interface
< jnewbery> vasild: maybe. It just feels like a much bigger change. If you had a branch that moved the lock to be totally internal, I'd be very interested to see it
< vasild> mempool.AtomicallyGiveMeYourStats(&pool_stats); ;-)
< vasild> jnewbery: no, I don't have
< aj> time for the gui repo topic?
< jeremyrubin> vasild: a mempool interior thread/workqueue with condvar for wake on return could implement that API :p
< wumpus> I guess it would be good to see an example of the various proposals and maybe re-discuss next week
< hebasto> wumpus: +1
< wumpus> if this is all worth it at all
< wumpus> maybe we're creaeting a problem where there's none
< wumpus> #topic Review the experience of 3 month bitcoin-core/gui repository (jonasschnelli)
< sdaftuar> i would reiterate that a design document for how the mempool would ideally interact with the rest of our software would be very helpful for evaulating any proposals
< jonasschnelli> I'd like to collect feedback on how well the separation of the GUI repository has been received.
< jonasschnelli> my feedback: it's seems still unclear how we merge things back to the main repository and if that has been documented.
< wumpus> yes, what's the plan for merging back the GUI changes?
< jonasschnelli> on top, the mix of PRs on both sides feels confusing to me.
< jnewbery> I think Marco has a script that merges them into both repositories simultaneously, no?
< jonasschnelli> Overall,... i would have prefered to split of the repository with a split off through a clean interface (process seperation in some ways)
< wumpus> I think it's nice to have GUI design discussions etc in the other repo
< jonasschnelli> jnewbery: could be. I don't know.
< jonasschnelli> Yes. I also like the split of the discussions and some GUI only PRs.
< jonasschnelli> I just don't know what burden we have when merging things back
< wumpus> it's sufficiently distinct from what happens in the main repo that that makes sense
< jonasschnelli> Also,... things that touch both "sides": still unclear how to handle that
< wumpus> I don't know either, looks like Marco Falke is missing for this discussion
< jonasschnelli> looks like
< promag> wumpus: most people just have to go to both repos.. it was a good change for those that want stay away from gui stuff
< jonasschnelli> However we go further, I think it would be nice to document the process more clear
< jnewbery> they seem to be pretty synchronized. The GUI repository is just one merge commit behind bitcoin/bitcoin
< jonasschnelli> Also unclear to me how we handle merge conflicts if we merge node stuff into the GUI repository
< jeremyrubin> jonasschnelli: +1 on process isolation being important for making the separation clearer
< luke-jr> did anyone ever get in touch with GitHub about the PR issues?
< jnewbery> jonasschnelli: they're the same commits
< jnewbery> there's no conflicts because it's always fast-forwards
< wumpus> jeremyrubin: IIRC there's a PR that does process isolation
< jonasschnelli> jnewbery: Right. I tihink its a non-issue as long as they stay in sync. Which I guess is a manual script
< jeremyrubin> wumpus: ryanofsky's right?
< michaelfolkson> I think the major concern is if review decreases with it being separate. I think people on the GUI repo need to regularly prod (cc) GUI reviewers who spend most of their time on the main repo.
< wumpus> ryanofsky is working on that #19461
< gribble> https://github.com/bitcoin/bitcoin/issues/19461 | multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky · Pull Request #19461 · bitcoin/bitcoin · GitHub
< wumpus> yes
< jonasschnelli> michaelfolkson: maybe only an initial issue. I think there are now also contributors stepping forward _because_ its GUI only
< hebasto> ^ and designers
< wumpus> jonasschnelli: exactly, though there's some overlap, it's also meant to get a different group of people involved
< michaelfolkson> But mainly designers right jonasschnelli? It needs normal Core reviewers too
< michaelfolkson> I think for design related review it is a material success so far
< wumpus> ideally we want more GUI contributors, improving the GUI, they don't necessarily need to get involved with the rest of the code
< jonasschnelli> michaelfolkson: Yes. Thats correct.
< jonasschnelli> It's a different complexity level (mostly). Also the risks are different.
< vasild> I think a separate GUI repository only makes sense as long as both repositories contain different chunks of software. Right now both contain the same, e.g. the gui repo contains bitcoind.cpp, net_processing.cpp, etc. Would it be possible to make only src/qt in the gui repo (and then make it a git submodule in the main one)?
< wumpus> a different kind of complexity to navigate at least
< jeremyrubin> wumpus: is there a framework for moving forward ryanofsky's work? last I checked it was still nack on capnproto?
< wumpus> I definitely found out I can't do it, I can't handle the subjectivity and bikeshedding involved :)
< jonasschnelli> vasild: this is a valid point
< jonasschnelli> wumpus : that. yes.
< wumpus> vasild: maybe, though it can't be built independently anymore then, it's harder for contributors
< hebasto> bikeshedding is a work for designers, no?
< yanmaani> Can't you refactor it to make the GUI an RPC client?
< vasild> hm, right, src/qt can't be build separately
< yanmaani> (and then optionally have it spawn a bitcoind)
< vasild> make make it compilable, libsrcqt.so :)
< jonasschnelli> yanmaani: I would also like to see that
< yanmaani> Then the GUI could indeed be moved to a separate repo
< jonasschnelli> but I guess we are drifting off-topic
< yanmaani> and you could have other people making other GUIs in other frameworks
< wumpus> yanmaani: that's what the multiprocess PR does, basically
< sipa> people tried that in 2012 or so, but it's terrible as RPC is query-response based, not asynchronous
< sipa> but the multiprocess work does this the right way
< jonasschnelli> sipa: we could use the long polling to make it faster though
< yanmaani> sipa: is RPC single-threaded?
< wumpus> yanmaani: (except it doesn't use JSON-RPC but its own RPC mechanism)
< promag> yanmaani: no
< wumpus> in any case ryanofsky's work already exists
< sipa> yanmaani: it would be more useful if you'd comment after you've spent some time looking at the code
< yanmaani> ok :)
< wumpus> no need to brainstorm the 2012 idea of doing it again now
< provoostenator> I suggest waiting a while before opening that can of worms though :-)
< jnewbery> yanmaani: even more useful if you review/test ryanofsky's work!
< jonasschnelli> I think I suggest then that we pimp up the documentation on how things are managed between the two repositories, avoid questions, etc.
< wumpus> jonasschnelli: +1
< jeremyrubin> wumpus: if some set of maintainers can establish a bit more of a framework on the validity of ryanofsky's approach i would spend some cycles on it. but AFAIU some contribs are nack on the approach so I'm not sure how likely it will proceed if that remains the case or what can be done to un-nack
< jonasschnelli> Let's hope MarcoFalke has time and willingness to do this
< wumpus> just documenting things would make sense
< wumpus> jeremyrubin: who is completely against it?
< jonasschnelli> people deserve to know what happens with PRs they write on the GUI repo
< jeremyrubin> gmax iirc?
< jeremyrubin> iirc he was against capnproto at all and wanted a custom IPC lib
< hebasto> +1 for documenting
< wumpus> for using capnproto for the GUI? I don't think so, he hates using it for internet protocols, but for internal communication between processes it doesn't matter too much
< sipa> no reason to speculate here
< sipa> afaik ryanofsky said that capnproto could easily be swapped out for something else if needed
< jonasschnelli> just the glue
< provoostenator> Indeed
< wumpus> I'd really dislike rolling our own IPC mechanism tbh
< jeremyrubin> do we already depend on it?
< jeremyrubin> It's in depends :)
< wumpus> there are so many one already, not everything needs to be invented here
< provoostenator> We already merged the build stuff for it.
< jonasschnelli> I'm still in favour or RPC
< provoostenator> But it's an opt-in config flag
< wumpus> sipa: yes, it should be easy to swap out
< wumpus> he designed it so that the specific mechanism used is abstracted
< jnewbery> jonasschnelli: I think it's even easier than I said. Marco just merges the gui PR branch into the bitcoin/bitcoin repo
< wumpus> better just review the code...
< jonasschnelli> RPC can also work async. But yes. Not the topic now.
< wumpus> jonasschnelli: I mean we can do something wildly different but *only* if someone is willing to do the work
< jnewbery> and then I guess there's a bot or something that is syncing bitcoin-core/gui to the latest master
< jonatack> there is a good recent writeup by ryanofsky, one month ago, here: https://bitcoincore.reviews/19160
< wumpus> I see no reason to second-guess ryanofsky now after all this time
< jonasschnelli> wumpus: that's a point.
< jeremyrubin> I have no issue with capnproto I think it's fine :)
< provoostenator> Last time I dug into it I found the approach pretty sane.
< wumpus> at least it's a step forward
< jeremyrubin> I spent a bunch of time reviewing it in '17 and think it's a good approach
< wumpus> when RPC one day converges on what we need for the GUI, then we can switch to that
< jonasschnelli> Haven't looked into it. But separating the GUI from the node via the internet/wireguard would be a requirement,.. right?
< wumpus> I think it's *very important* to take a step by step approach with things
< provoostenator> Conversely, when the IPC interface simplifies and needs fewer locks...
< wumpus> if not, we'll never go anywhere, this kind of thing has been proposed since at least 2012
< jonasschnelli> indeed
< wumpus> because everyone wants something else
< jonasschnelli> the power of open source
< wumpus> and then someone does it and peopel want yet something else
< jeremyrubin> I think that's all I'm asking. If ryanofsky's pr is reasonably reviewed
< wumpus> so, anyhow, if you're interested in process separation, please review ryanofsky's PRs
< jeremyrubin> there is no 'hard reason' it can't be merged
< jeremyrubin> which is great news for anyone willing to spend review cycles on it, it derisks that time spent
< wumpus> dont' come up with new wild ideas that would have to start from scratch :)
< sdaftuar> jeremyrubin: i would hope not, my understanding is that ryanofsky has been making progress towards this goal for quite some time now
< wumpus> of course after reviewing the PR you could give more targeted suggestions
< wumpus> oh, it's time
< wumpus> #endmeeting
< lightningbot> Meeting ended Thu Sep 10 20:01:42 2020 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4)
< jonasschnelli> \o
< vasild> o/
< jonatack> psa, the signet PR seems to be in the final stretch if anyone wants to have a look
< wumpus> yay!
< jonatack> \o
< michaelfolkson> Final stretch as in we can start a barrage of follow up smaller signet PRs
< wumpus> but FWIW I had the idea that the capnproto approach for IPC communication was widely concept-ACKed a long time ago, that's why the build system part was merged
< jeremyrubin> I hadn't tracked that development :)
< luke-jr> I don't see why we need two RPC interfaces ;)
< wumpus> it's not a RPC interface it's internal interface only
< wumpus> it will not be documented for external clients so it has much more flexibility
< wumpus> it will also not be compatible between versions
< luke-jr> no matter how we use it, it's an RPC interface..
< jeremyrubin> I reviewed the process isolation stuff relatively early on, but it wasn't clear that it was going to move forward and didn't think i could do anything to move the needle on making an IPC interface available.
< luke-jr> we could just as well add an undocumented and unstable JSON-RPC interface
< jeremyrubin> now that I know it is, I'll re-review :)
< wumpus> there's a very different set of requirements for it anyhow
< wumpus> but as said maybe they can converge in the future
< jeremyrubin> luke-jr: the difference is that you can fork a process and explicitly set up the IPC channel
< wumpus> the idea is to allow for some experimentation here
< jeremyrubin> so that the connection is internal to one parent process
< jeremyrubin> You could maybe do this for RPC, but the point of IPC is to allow deeper integration & work with e.g. shared memory.
< luke-jr> so it won't be possible to run the GUI on another machine? :/
< wumpus> it's simply a pipe, it could send every protocol, but in any case, capnproto is ok for now imo
< sipa> jeremyrubin: first thing i hear about shared memory
< wumpus> I doubt there are any plans about shared memory
< wumpus> if gmaxwell is queasy about capnproto I'm sure he'll go insane when you mention shared memory :-)
< wumpus> it's also really not needed for this
< yanmaani> It's two different approaches. With well-defined RPC and all that, you can run it on separate machines, and you can have other projects writing GUIs. With shared memory and more "usual" IPC, you're more tightly coupled. So you can iterate faster, but you lose some of the benefits of keeping -gui separate
< yanmaani> if it always has to be the same version, for instance
< wumpus> yes
< luke-jr> fwiw someone mentioned to me the other day a desire to revive wxBitcoin - better separation helps that too
< jeremyrubin> iirc capnproto had some shared memory stuff but i may be wrong
< yanmaani> If you do the separation very cleanly you could have hundreds of random bozos making GUIs. Whether this is great or horrifying is up to your imagination.
< wumpus> revive wxbitcoin?!? whyy
< sipa> lol
< sipa> has 3.9 been released already?
< yanmaani> It was I. I don't like Qt.
< wumpus> sigh, I don't even want to know, I want to have 0 to do with the GUI anymore
< sipa> eh, 2.9 - apparently yes, they're at 3.1.4
< wumpus> sipa: progressss
< sipa> yanmaani: use RPC :p
< yanmaani> well, with these changes, you can just jettison the unloved components of the codebase
< wumpus> I just dislike all the bikeshedding, everyone wants something else, I think "I don't like library X" is an awful reason
< yanmaani> Or I can procrastinate on it and wait for these changes to be made, and use whatever you eventually bikeshed on.
< wumpus> imo ideally the GUI design should really be made by someone that gets GUIs at a psychological level and designs something that is usable by end-users, not by developers bikeshedding over libraries and widget types
< yanmaani> imo you should have two groups
< yanmaani> one group of designers who know design who design the psychologically optimal GUI in photoshop
< wumpus> developes come with ever sillier concerns
< yanmaani> one group of developers who bikeshed over widget types
< sipa> yanmaani: will you pay them all?
< yanmaani> (and implement the mockup)
< wumpus> it's interaction design, you can't really do that in photoshop
< sipa> UX is far more than graphical design
< wumpus> ^^ very much this
< yanmaani> I actually like the way the current GUI looks, and the fact that Electrum has converged on something extremely similar to Core is reassuring
< phantomcircuit> what's missing from the current gui is mostly debug related things, but im guessing the intersection between people who need debug stuff and people who won't use the cli is pretty small
< sipa> i haven't used the GUI in years
< luke-jr> ♥ current GUI
< yanmaani> ^
< sipa> then what's the problem with Qt?
< wumpus> phantomcircuit: what debug information are you missing?
< * luke-jr> ♥ Qt too
< achow101> the current gui kinda sucks with multiwallet
< achow101> it's really easy to accidentally use the wrong wallet
< luke-jr> admittedly, I don't use multiwallet much yet
< luke-jr> too much in the habit of closing one and opening another
< wumpus> I only use multiwallet for watch-only things
< gwillen> I mostly use multiwallet for manual testing tbh, but it's fantastic for that
< achow101> I run multiple wallets watching the same thing lol
< achow101> tbf, one is legacy, another descriptor, and the last sqlite descriptor
< wumpus> hehe
< phantomcircuit> wumpus, "is this transaction in the mempool", "how big is the mempool" ie things that aren't actually relevant to most users
< wumpus> phantomcircuit: ah yes, mempool information is a bit sparse
< achow101> phantomcircuit: how big is the mempool is reported
< sipa> i'd like a debug tool with a secp256k1 DL solver
< luke-jr> phantomcircuit: Knots has some mempool graph stuff jonasschnelli wrote years ago
< sipa> would come in really.handy
< wumpus> did we never merge the mempool graph stuff?
< wumpus> I thought that was pretty nice
< luke-jr> wumpus: I have rebased versions of an older version of the PR
< jonasschnelli> I have picked this up recently
< luke-jr> jonasschnelli changed the RPC side of it all around, but never updated the GUI for it
< luke-jr> ah, nice
< wumpus> that's the kind of things I like about the GUI tbh, debug and data visualization
< sipa> indeed
< jonasschnelli> Made it more like Jochen Hoenick ones
< jonasschnelli> With few ranges
< jonasschnelli> I’ll PR that soon.
< wumpus> jonasschnelli: awesome!
< wumpus> I think it would be very nice if users can see those things locally, without having to go to a site
< jeremyrubin> [9/10/20 13:28] <sipa> i'd like a debug tool with a secp256k1 DL solver
< jeremyrubin> I'd think you'd see this before a good QT GUI debugger
< jeremyrubin> The only question is if you can make a good trapdoor function out of bugs in QT
< jeremyrubin> BIP QT-root
< wumpus> a DL solver would definitely be useful :')
< wumpus> at least if it doesn't hold up the GUI loop for a million billion years
< sipa> haha
< promag> <sipa> i haven't used the GUI in years
< promag> because GUI uses sipa
< jonasschnelli> for the ones interested in the mempool size/fee chart: https://github.com/bitcoin/bitcoin/compare/master...jonasschnelli:2020/03/mempool_graph?expand=1
< jonasschnelli> (it's WIP)
< jonasschnelli> It's more or less a clone of https://jochen-hoenicke.de/queue/#0,24h
< jonasschnelli> (but obviously internal and QT native)
< gribble> https://github.com/bitcoin/bitcoin/issues/0 | HTTP Error 404: Not Found
< phantomcircuit> sipa, btw going from 1000 to 100k keys only added 50% to the runtime of a rescan
< sipa> phantomcircuit: wait until you hit enough keys that the mapKeys structure doesn't fit in RAM anymore
< sipa> ;)
< luke-jr> could always use a bloom filter for the wallet keys, to speed it up ;)
< phantomcircuit> sipa, that would be a very very large number of keys
< sipa> phantomcircuit: you just have too much RAM
< sipa> ;)
< sipa> phantomcircuit: more seriously, i think the actual impact of number of keys on rescan speed isn't so much the O(log n) scaling, but whether or not the mapKeys fits in various levels of CPU cache or RAM
< phantomcircuit> sipa, indeed
< wumpus> jonasschnelli: that's great!
< phantomcircuit> luke-jr, the gcs filters that 'neutrino' use are what i was using, but to actually use them requires changing violating some assumptions the current logic has (namely that every block derives the filter key from the block hash)
< luke-jr> phantomcircuit: I meant the other direction
< phantomcircuit> luke-jr, ?
< phantomcircuit> oh put the wallet keys into a filter
< luke-jr> yes
< luke-jr> if you do it right, you might even be able to do a union of the neutrino filters with the wallet filter..
< phantomcircuit> iirc most of the time for the current rescan is actually deserializing the block and verifying the merkle root
< achow101> sipa: interestingly on my branch where i've changed ~all of the wallet maps and sets to unordered_map/set, the rescan actually takes 10x longer. On both newly created legacy and descriptor wallets
< luke-jr> you don't need to verify the merkle root
< achow101> seems like siphash is a lot slower than one map comparison?
< sipa> achow101: interesting
< achow101> actually, siphash probably isn't being used there, the hashers for CKeyID and CScriptID just use uint's GetUint64
< phantomcircuit> luke-jr, ok but it does verify the merkle tree because you're loading the block from disk
< yanmaani> would PRs be accepted where you replace std::unordered_map with something else for CCoinsViewCache, or is that not worth looking into?
< sipa> if it's measurably faster, possibly
< wumpus> jonasschnelli: will try it out a bit tomorrow
< phantomcircuit> luke-jr, anyways if you setup the gcs filter index things using a local static key then you can calculate the hashes that you need to check against and save them, which makes the rescan basically just finding the union of two sets
< luke-jr> phantomcircuit: but make both sets a filter
< phantomcircuit> luke-jr, i guess you could take the gcs set and make a bloom filter on top of that?
< wumpus> besides being demostratively faster, it also depends on 'something else', e.g. probably not if it introduces any new dependencies on boost, or anything else external
< phantomcircuit> iono i suspect that would actually be slower than just doing the giant union of the sets calculation
< wumpus> also as it's a consensus critical data structure it's something to be very careful with
< luke-jr> bdb!
< * luke-jr> hides
< jb55> achow101: agreed on multiwallet gui. this is something I still use cli for over qt. would be great to have an "all wallets" transaction view.
< sipa> jb55: if you have the same key/address/descriptor imported in multiple simultaneously-loaded wallets, an "all wallets" overview may be confusing
< * sipa> predicts people creating 1M wallets with the same 21 BTC UTXO on it, and screenshottijg their 21M BTC combined balance
< achow101> sipa: it feels like importing the same thing into multiple wallets might not be something people tend to do
< luke-jr> sipa: then don't use it? :P
< jb55> sipa: maybe they shouldn't do that :P
< sipa> jb55: seems achow101 just did!
< achow101> sipa: I suspect that you won't be able to load 1M wallets
< sipa> achow101: software should be optimized for all use cases!
< jb55> 150m keys should be instant imo
< bitcoin-git> [bitcoin] ajtowns opened pull request #19937: signet mining utility (master...202009-signet-generate) https://github.com/bitcoin/bitcoin/pull/19937