--- Log opened Wed Dec 22 00:00:08 2021
01:10 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
01:11 -!- grettke [~grettke@184.62.226.206] has quit [Client Quit]
01:26 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has joined #bitcoin-core-pr-reviews
01:30 -!- raj [~raj@2602:ffb6:4:e396:f816:3eff:fe47:ca2a] has quit [Killed (NickServ (GHOST command used by Guest2756!uid72176@user/raj))]
01:30 -!- raj_ [~raj@167.182.81.172.lunanode-rdns.com] has joined #bitcoin-core-pr-reviews
01:58 -!- Talkless [~Talkless@mail.dargis.net] has joined #bitcoin-core-pr-reviews
03:15 -!- Common [~Common@user/common] has quit [Quit: Leaving]
03:57 -!- Common [~Common@096-033-221-075.res.spectrum.com] has joined #bitcoin-core-pr-reviews
03:59 -!- Common [~Common@096-033-221-075.res.spectrum.com] has quit [Changing host]
03:59 -!- Common [~Common@user/common] has joined #bitcoin-core-pr-reviews
05:44 < michaelfolkson> Don't think I am getting any debug logs that are IPC specific :/
05:45 < michaelfolkson> I think they would all be labeled IPC or at least bitcoin-node
05:47 < michaelfolkson> Hmm
05:51 < michaelfolkson> I did configure with both --enable-debug and --enable-multiprocess, I am assuming that is fine
06:23 < michaelfolkson> D'oh was running bitcoind and assuming multiprocess would run automatically rather than running bitcoin-node
06:23 < michaelfolkson> Working now
06:52 < michaelfolkson> I was expecting to have to run bitcoin-wallet to use the wallet but running bitcoin-node spawns a bitcoin-wallet process automatically (makes sense I guess initially to assume you'll want the wallet process running and on the same machine)
08:11 < ryanofsky> michaelfolkson, good catch. I should maybe make it an error to pass -debug=ipc command line argument to an executable that doesn't support IPC
08:12 < ryanofsky> All, review club https://bitcoincore.reviews/10102 starts in about an hour (if I understand time zones correctly)
08:20 < michaelfolkson> ryanofsky: Yes that would have flagged what I was doing wrong
08:22 < michaelfolkson> I'm assuming all the normal debug categories work normally when running multiprocess
08:23 < michaelfolkson> Obviously debug=1 has additional IPC logging when running multiprocess
08:24 < michaelfolkson> But yeah -debug=ipc could give an error when not actually running multiprocess
08:24 < ryanofsky> Yes in #10102 debug output is the only externally visibible difference between bitcoind and bitcoin-node
08:24 < ryanofsky> In later pr's actual user features are added
08:31 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
08:32 -!- grettke [~grettke@184.62.226.206] has quit [Read error: Connection reset by peer]
08:33 < michaelfolkson> Do you need to configure with --enable-debug to be able to use -debug=ipc (or -debug=1) with multiprocess? I'm assuming you do... 
08:34 < sipa> No, they're orthogonal.
08:35 < sipa> --enable-debug creates unoptimized binaries
08:36 < sipa> which are more usable when you're trying to run a debugger on them
08:36 < sipa> it's unrelated to debug output
08:39 < michaelfolkson> In theory it is possible to create a more optimized binary for multiprocess that doesn't need to present detailed debug logs? But it doesn't exist yet/may never?
08:39 < sipa> In theory you can write code to do anything.
08:40 < sipa> As I said, these options are orthogonal. Debug output is controlled at runtime using -debug=X flags.
08:41 -!- erik1 [~erik@181-191-0-203.uplinkx.com.br] has joined #bitcoin-core-pr-reviews
08:41 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Remote host closed the connection]
08:49 < ryanofsky> michaelfolkson, -ipc=debug does add a lot of debug output, but it only comes from a handful of logprint statements, so a binary built without support for -debug=ipc wouldn't actually be much smaller
08:49 < lightlike> michaelfolkson: you choose --enable-debug when you want to attach a debugger such as gdb. nothing to do with logging
08:53 < lightlike> oops, that was said before already...
08:54 < michaelfolkson> lightlike: Ahh ok --enable-debug only does a subset of what I thought it did then :) That's helpful (it was said already but this made it clearer to me)
08:55 < michaelfolkson> I was just blindly using --enable-debug with configure thinking it gave me more detailed debug logs. But you get them anyway
08:55 < michaelfolkson> (if you specify you want them at runtime)
08:56 < sipa> It only means you get them slower :p
08:59 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has joined #bitcoin-core-pr-reviews
09:00 < ryanofsky> #startmeeting
09:00 < ryanofsky> Hi!
09:00 < lightlike> hi
09:00 < ryanofsky> Today going to review Multiprocess bitcoin https://bitcoincore.reviews/10102
09:00 < b10c> hi
09:00 < michaelfolkson> hi
09:00 < svav> Hi
09:01 < ryanofsky> Hi and first question is the usual who's here and did you did you review the PR? (Concept ACK, approach ACK, tested ACK, or NACK)
09:02 < lightlike> concept ACK, looked at it but not too much in-depth
09:02 < b10c> I haven't managed to look closer, but concept ACK
09:02 < michaelfolkson> Yeah all the ACKs from me from what I've done so far
09:02 < ryanofsky> Also wondering if there were parts of the description or implementation that could be explained better? Things that were confusing?
09:03 < svav> I read the notes
09:03 -!- OliverOffing [~OliverOff@189.1.174.49] has joined #bitcoin-core-pr-reviews
09:04 < michaelfolkson> I guess this particular PR has been open for years and has hundreds of comments that are hard to read due to GitHub
09:05 < ryanofsky> Yeah, but if people mostly think purpose is clear and top description is clear I'm happy
09:05 < svav> One thing I would say is interfaces::WalletClient  and interface::Wallet are very similarly named, so you don't get any hint of what is different about them.
09:06 < ryanofsky> Right WalletClient is the interface the bitcoin-node uses to control bitcoin-wallet, Walelt is the interface bitcoin-gui uses to control bitcoin-wallet
09:06 < michaelfolkson> Just wasn't clear to me initially whether this was a PR covering the whole project or a prototype or whether it was just one PR in a series
09:07 < michaelfolkson> It is the latter but in that case I don't know how it has been open for years in that case :)
09:07 < ryanofsky> I wonder what a better name for WalletClient might be. Really it only exists for backwards compatability so bitcoin-node will continue to load wallets by default. More ideally bitcoin-wallet would just connect to node, I think
09:08 < ryanofsky> It's not the whole project, or a prototype. It's base cross-process functionality that features can be built on later
09:09 < svav> ryanofsky: I guess what I am saying is from the names, you can't tell that one is for use by the Node, and the other for use by the GUI
09:09 < michaelfolkson> But it needed all the previous PRs in the project to be merged first right? https://github.com/bitcoin/bitcoin/projects/10
09:09 < svav> ryanofsky: So, it might be something that could create confusion
09:09 < ryanofsky> svav, do you think interfaces::Wallet is not a good name for the interface used to control a Wallet? It seems to me interfaces::Wallet is a good name, but only WalletClient is the bad name
09:10 < ryanofsky> michaelfolkson, that's right it has a lot of dependencies that were merged previously
09:11 < ryanofsky> Q2 is What are some disadvantages of separating bitcoin node, wallet, and GUI into separate processes? What are some benefits?
09:11 < michaelfolkson> For what its worth I think the names are fine as well they are well documented (ideally both in the code and in the separate multiprocess doc)
09:12 < michaelfolkson> *as long as they are
09:13 -!- jb55 [~jb55@user/jb55] has joined #bitcoin-core-pr-reviews
09:13 < ryanofsky> That's good, always looking for ideas of better names and things that are missing from docs
09:14 < svav> Well, as a suggestion, you could have interfaces::WalletForNode and interface::WalletForGUI so they are differentiated. I'm just saying at the moment, they both seem very similar.
09:14 < lightlike> i'd say one disadvantage is that it's slower if things get serialized at one process, sent over a socket, deserialized by the another process
09:15 < svav> One disadvantage of separation might be there is more code to maintain.
09:15 < ryanofsky> Thanks svav! Yes ForXXX would be possible convention to use for interface names
09:15 -!- azor [~azor@186-90-35-210.genericrev.cantv.net] has joined #bitcoin-core-pr-reviews
09:15 < michaelfolkson> +1
09:15 < ryanofsky> lightlike, yes it adds more code, more dependencies, strictly worse performance
09:16 < michaelfolkson> Lots of work too :)
09:16 < ryanofsky> Well work is in the benefits column for me
09:16 < svav> The advantages as said in the notes are greater resilience, as node fault will not bring down wallet and vice versa.
09:17 < lightlike> advantages: there could be alternative implementations for parts, such as GUI or wallet; separating code like this makes it easier to be sure that parts that should be independent really are
09:17 < ryanofsky> Yeah main thing I like about it is it forces codebase to be more modular, and can let you run a node continuosly and connect wallets and gui instances to it as needed
09:18 < ryanofsky> Q3 Did you try building with `--enable-multiprocess` ? And did you try running with `-debug=1` to view IPC logging?
09:18 < ryanofsky> (and feel free to keep going on Q2 if more to say there)
09:19 < lightlike> yes, building and running on regtest worked fine for me.
09:19 < michaelfolkson> Q3 - Yes I ran on regtest and signet
09:20 < michaelfolkson> I wasn't sure what I'd see. Like should the wallet only communicate the node when the wallet wants to make a transaction or wants to know its balance
09:20 < michaelfolkson> But there was so many log messages it was also hard to see specific logs when you made a specific action
09:21 < michaelfolkson> I think Sjors said that in a comment too
09:21 < ryanofsky> michaelfolkson, yeah, there is a lot of crosstalk between the processes, especially when you use the GUI
09:22 < ryanofsky> It just shows all the places where gui, node, and wallet are notifying or polling each other
09:23 < ryanofsky> michaelfolkson, I wonder if this is unexpected, or you see it as a problem
09:23 < ryanofsky> To me purpose of IPC logging is just to debug IPC, but if you want to use it for other things then it could be a problem
09:24 < michaelfolkson> I guess in an idealized world there would be less cross talk and they would only talk when absolutely necessary or when they need something. But in reality we are dealing with Satoshi's inheritance so I'm not sure what the idealized state would be
09:25 < michaelfolkson> A oversimplification would be the wallet only hears from the node say every block? Or if the wallet wants to make a transaction or hasn't figured out the UTXOs it controls
09:26 < ryanofsky> michaelfolkson, very much agree. I think removing the most egregious polling is probably low hanging fruit for cleanup prs (independent of anything in 10102), and would also be good for improving gui performance
09:27 < ryanofsky> michaelfolkson, well the wallet also needs to know about every transaction added to the mempool to see if it's relevant
09:28 < michaelfolkson> ryanofsky: Right for zero conf transactions
09:29 < ryanofsky> Exactly, yes
09:29 < michaelfolkson> And the node has to ask the wallet if it is interested in every transaction as the node doesn't know any prior keys, addresses. I guess that's a lot of chatter in itself
09:30 < lightlike> are the objects that are exchanged displayed in Cap'n'Protos serialization format? I see lots of hex code in my logs.
09:31 < ryanofsky> Any object that can be sent in bitcoin's native serialization format is serialized that way, and displayed as hex
09:31 < ryanofsky> UniValue objects are sent as JSON. A few objects that don't have bitcoin serialize methods are serialized as capnproto structs
09:32 < ryanofsky> A good example of the last case is NodeStats https://github.com/ryanofsky/bitcoin/blob/pr/ipc.168/src/ipc/capnp/node.capnp#L139
09:34 < ryanofsky> Q4 is semirelated
09:34 < ryanofsky> Q4 Why is it important that Cap'n Proto types and libraries are only accessed in the `src/ipc/capnp/` directory, and not in any other public header files?
09:35 < michaelfolkson> Don't know... so it doesn't impact non-multiprocess users?
09:36 < lightlike> 1) its better to keep it in one place so we can easily add alternatives to capnproto, 2) to be able to still build bitcoind without having capnproto installed
09:37 < ryanofsky> Yes to both. Goal is to make it possible to run without multiprocess code, and make the dependency confined and easier to replace if needed
09:38 < ryanofsky> Q5 What is the difference between node and wallet running on different processes vs different threads? Why must they communicate via an IPC framework instead of calling each other's functions normally?
09:38 < lightlike> are there plans of moving towards multi-process only in the long term?
09:40 < michaelfolkson> That would require https://github.com/chaincodelabs/libmultiprocess in the Core repo right?
09:40 < ryanofsky> What would advantages / disadvantages be?
09:41 < michaelfolkson> So threads run in shared memory space whereas processes run in separate memory spaces https://stackoverflow.com/questions/200469/what-is-the-difference-between-a-process-and-a-thread
09:41 < ryanofsky> michaelfolkson, that's probably true, but I think kind of a project management question more than a technical one
09:42 < lightlike> Maybe it reduces code complexity not to have to maintain code for both options? But I'm not sure about specifics.
09:42 < ryanofsky> lightlike, yeah I think that's the way I see it
09:42 < michaelfolkson> If there are trade-offs you kinda need to support both
09:42 < ryanofsky> No reason to require multiprocess unless you are really going to separate code into different projects
09:43 < lightlike> But I guess if the loss of performance is significant, it's not really an option anyway?
09:43 < ryanofsky> Right I'm also assuming supporting multiprocess + single process with everything in one repo is not more work than just supporting multiprocess
09:44 < michaelfolkson> For the security benefits of separation you want separate processes rather than separate threads
09:44 < svav> If you have multi-process, why do you have to keep single process?
09:44 < lightlike> did you try IBD with multiprocess and compare the performance?
09:45 < ryanofsky> lightlike, that would be a good experiment. I think there should be no change in performance really unless the wallet is somehow slowing down IBD
09:46 < michaelfolkson> Have to add Core multiprocess to Jameson Lopp's node performance reviews :)
09:46 < ryanofsky> svav, you don't have to keep single process, but dropping support for single process removes very little code
09:46 < michaelfolkson> https://blog.lopp.net/2020-bitcoin-node-performance-tests/
09:47 < ryanofsky> 10102 almost strictly adds code and changes very little existing code
09:48 < michaelfolkson> svav: If users like the trade-offs offered by single process rather than multiprocess you shouldn't take it away (assuming the trade-offs are significant)
09:48 < ryanofsky> In general on performance, I think most of the loss is caused by unnecessary cross talk, not necessary overhead
09:49 < lightlike> if not IBD, are there any specific things/user actions where you would expect a notable performance loss?
09:50 < ryanofsky> lightlike, that's a good question. I guess I'd expect performance loss in random weird places
09:50 < ryanofsky> Especially the GUI does a lot of weird things querying a lot of unnecessary information from the wallet
09:51 < ryanofsky> I wouldn't be surprised if IBD was slow, or if some other random thing was slow
09:52 < ryanofsky> Q6 is a code question
09:52 < ryanofsky> Q6 Where are the `spawnProcess` calls added in the "Make bitcoin-gui spawn..." and "Make bitcoin-node spawn..." commits? What type of interface pointer is requested from each of the spawned processes?
09:53 < ryanofsky> https://github.com/bitcoin/bitcoin/pull/10102/commits/11df1f8701f4f693b0c2fbdc0000649a625ec150
09:53 < ryanofsky> https://github.com/bitcoin/bitcoin/pull/10102/commits/188de5680348dfb6993d4f8c0a43437eb1436ffd
09:54 < lightlike> wallet/init.cpp the wallet is spawned when it didn't already exist
09:54 < ryanofsky> lightlike, yes exactly
09:54 < lightlike> I think the requested pointer is std::unique_ptr<interfaces::Init>
09:55 < ryanofsky> Yes!
09:55 < lightlike> will that wallet init code try something if we compile without a wallet?
09:55 < ryanofsky> If you are running `bitcoind` executable `auto wallet_client = node.init->makeWalletClient(*node.chain);` is not null, so no need to call spawnProcess
09:56 < ryanofsky> lightlike, if compiled with --disable-wallet, this code isn't compiled at all, and src/dummywallet.cpp is used instead
09:56 < lightlike> ah, thanks!
09:56 < michaelfolkson> So --enable-multiprocess and --disable-wallet configure options
09:57 < ryanofsky> If you are running `bitcoin-node` `wallet_client` is null, so call to spawnProcess is made instead
09:58 < ryanofsky> The idea is bitcoind has wallet code linked in, doesn't need to spawn anything. bitcoin-node doesn't have wallet code linked in so needs to spawn a bitcoin-wallet process
09:58 < ryanofsky> Regardless of which happens, the node code calls wallet code with an interfaces::WalletClient pointer
09:59 < ryanofsky> But in one case the calls do IPC communicate, in the other case IPC is skipped and everything happen locally
10:00 < lightlike> That makes sense!
10:01 < ryanofsky> Hours up but feel free to ask me any questions. I learned I need to actually do something about the WalletClient name (this complaint has been made before!)
10:01 < lightlike> thanks ryanofsky, that was really interesting!
10:01 < ryanofsky> Thanks you for putting in work to understand this and bringing up great points!
10:02 < svav> Thanks ryanofsky and all
10:03 < ryanofsky> Maybe WalletLoader would be a good name instead of WalletClient...
10:03 < michaelfolkson> One final question. Could you re-introduce non security critical shared state between the wallet and node just for say addresses that the wallet is tracking? To reduce the chatter?
10:04 < michaelfolkson> I get it is going back in the opposite direction (!) but just wondering if there is a grey area that is optimal
10:04 < ryanofsky> michaelfolkson, yes that would be possible and it could reduce amount of traffic substantially
10:05 < lightlike> michaelfolkson: you mean as an alternative way enabled only for non-multiprocess builds? wouldn't otherwise the multiprocess build be impossible?
10:05 < ryanofsky> lightlike, probably you would only do this for multiprocess builds and leave single process case alone
10:06 < michaelfolkson> Would a multiprocess build with some limited shared state between multiple processes be impossible?
10:06 < ryanofsky> wallet code would tell node code what addresses it cares about
10:07 < lightlike> isn't one idea of the mulitprocess build that it could also apply in situations where sharing state in other ways would be physically impossible (like different processes running on different computers)?
10:07 < michaelfolkson> The shared state could be stored on both machines?!
10:07 < ryanofsky> Easiest way to implement shared state would be to have it reside inside the bitcoin-node process. Of course it would be technically possible to have it somewhere else too though
10:08 < ryanofsky> lightlike, I think I'm just talking about "shared state" at a high level, like what information each process knows about. Not shared memory literally
10:09 < lightlike> oh ok, understood, i thought about shared memory.
10:10 < ryanofsky> Yeah, I would avoid using shared memory / shared files by default unless there was some big advantage to using that because it adds many limitations
10:11 -!- svav [~svav@82-69-86-143.dsl.in-addr.zen.co.uk] has quit [Quit: Connection closed]
10:13 < michaelfolkson> The wallet can't do anything without the node so it is not like the wallet process can continue doing useful stuff if the node goes down. So the wallet giving the node a bit of extra information that isn't strictly node information could make sense
10:13 -!- azor [~azor@186-90-35-210.genericrev.cantv.net] has quit [Quit: Connection closed]
10:14 < michaelfolkson> Anyway, really interesting. Many thanks ryanofsky (for all your work on this)
10:14 < ryanofsky> Thank you!
10:15 < michaelfolkson> There's some interesting process rearchitecture going on in c-lightning too. I'm also struggling to understand that :)
10:15 < michaelfolkson> https://btctranscripts.com/c-lightning/2021-11-29-developer-call/#rearchitecting-c-lightning-daemons
10:18 -!- chunkblob [~chunkblob@cpe-184-153-102-43.nyc.res.rr.com] has joined #bitcoin-core-pr-reviews
10:19 < ryanofsky> Thanks for pointers, interesting to know about ibd and clightning stuff
10:19 < michaelfolkson> Although that is daemons vs processes
10:19  * michaelfolkson looks up definitions
10:20 < sipa> a daemon is a background process
10:20 < sipa> the "d" in bitcoind stands for daemon
10:21 < michaelfolkson> Right but the foreground, background thing isn't clear to me
10:21 < sipa> like: you don't see it running
10:21 < sipa> you start it, it keeps running in the background
10:21 < sipa> but it doesn't show a GUI, or occupy your terminal
10:23 < lightlike> a bit weird though that bitcoind needs an additional "-daemon" argument in order to actually be a daemon :)
10:23 < sipa> True!
10:23 < michaelfolkson> Ha
10:40 -!- OliverOffing [~OliverOff@189.1.174.49] has quit [Quit: Connection closed]
10:48 < ryanofsky> FWIW, made pr to rename WalletClient https://github.com/bitcoin/bitcoin/pull/23842
11:39 -!- akjsd [~akjsd@p5dc33eba.dip0.t-ipconnect.de] has joined #bitcoin-core-pr-reviews
11:40 -!- akjsd [~akjsd@p5dc33eba.dip0.t-ipconnect.de] has quit [Client Quit]
12:08 -!- lukedashjr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
12:10 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Ping timeout: 240 seconds]
12:11 -!- lukedashjr is now known as luke-jr
12:20 -!- chunkblob [~chunkblob@cpe-184-153-102-43.nyc.res.rr.com] has quit [Quit: Connection closed]
12:34 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
12:44 -!- lukedashjr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
12:46 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Ping timeout: 256 seconds]
12:46 -!- lukedashjr is now known as luke-jr
13:04 -!- ___nick___ [~quassel@cpc68286-cdif17-2-0-cust533.5-1.cable.virginm.net] has quit [Ping timeout: 260 seconds]
13:35 -!- lukedashjr [~luke-jr@user/luke-jr] has joined #bitcoin-core-pr-reviews
13:37 -!- luke-jr [~luke-jr@user/luke-jr] has quit [Ping timeout: 240 seconds]
13:38 -!- lukedashjr is now known as luke-jr
13:56 -!- Talkless [~Talkless@mail.dargis.net] has quit [Quit: Konversation terminated!]
15:20 -!- grettke [~grettke@184.62.226.206] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
15:36 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
15:45 -!- grettke [~grettke@184.62.226.206] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
15:55 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
16:06 -!- grettke [~grettke@184.62.226.206] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
16:16 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
16:30 -!- grettke [~grettke@184.62.226.206] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
16:36 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
16:50 -!- grettke [~grettke@184.62.226.206] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…]
17:59 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
18:03 -!- grettke [~grettke@184.62.226.206] has quit [Client Quit]
18:07 -!- grettke [~grettke@184.62.226.206] has joined #bitcoin-core-pr-reviews
18:07 -!- grettke [~grettke@184.62.226.206] has quit [Read error: Connection reset by peer]
--- Log closed Thu Dec 23 00:00:09 2021