19:03:58 <wumpus> #startmeeting
19:03:58 <lightningbot> Meeting started Thu Apr 16 19:03:58 2020 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:03:58 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:04:01 <jonasschnelli> hi
19:04:03 <sipa> hi
19:04:06 <achow101> hi
19:04:08 <amiti> hi
19:04:10 <meshcollider> hi
19:04:13 <jonatack> hi
19:04:22 <cfields> hi
19:04:28 <kanzure> hi
19:04:34 <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 amiti fjahr
19:04:36 <wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55
19:04:39 <dongcarl> hi
19:04:51 <elichai2> Hi
19:04:53 <jb55> hi
19:05:14 <wumpus> one proposed topic: experimental libmultiprocess, next steps for multiprocess in general (MarcoFalke)
19:05:22 <MarcoFalke> hi
19:05:30 <luke-jr> wumpus: I proposed one last week.. which really should get addressed before 0.20
19:05:40 <phantomcircuit> hi
19:05:44 <jkczyz> hi
19:05:51 <fjahr> hi
19:05:56 <cfields> I know fanquake really wants to be part of the multiprocess conversation but he can't make the meeting today.
19:05:57 <hebasto> hi
19:06:19 <cfields> maybe we can introduce the issue here and pick it up on the PR?
19:07:31 <wumpus> luke-jr: can you be more specific? the last topic suggestion from you that I see is the PPA URL and we've spent half a meeting on that
19:07:43 <wumpus> #topic High priority for review
19:07:54 <luke-jr> wumpus: right now, the wallet implementation is still broken, and if we don't address it before 0.20, it complicates backward compatibility
19:08:05 <sipa> can i haz #185512
19:08:06 <gribble> https://github.com/bitcoin/bitcoin/issues/185512 | HTTP Error 404: Not Found
19:08:07 <jonasschnelli> may I add #18242 to the hp list?
19:08:08 <sipa> can i haz #18512
19:08:08 <midnight> hi!
19:08:11 <gribble> https://github.com/bitcoin/bitcoin/issues/18242 | Add BIP324 encrypted p2p transport de-/serializer (only used in tests) by jonasschnelli · Pull Request #18242 · bitcoin/bitcoin · GitHub
19:08:14 <gribble> https://github.com/bitcoin/bitcoin/issues/18512 | Improve asmap checks and add sanity check by sipa · Pull Request #18512 · bitcoin/bitcoin · GitHub
19:08:21 <luke-jr> re #18192, #18550, #18572, and #18608
19:08:24 <gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
19:08:25 <gribble> https://github.com/bitcoin/bitcoin/issues/18550 | Store destdata for change in separate key for backward compatibility by luke-jr · Pull Request #18550 · bitcoin/bitcoin · GitHub
19:08:26 <gribble> https://github.com/bitcoin/bitcoin/issues/18572 | Wallet: Accept "changedata" db key as an alias to "destdata" by luke-jr · Pull Request #18572 · bitcoin/bitcoin · GitHub
19:08:27 <gribble> https://github.com/bitcoin/bitcoin/issues/18608 | refactor: Remove CAddressBookData::destdata by ryanofsky · Pull Request #18608 · bitcoin/bitcoin · GitHub
19:08:37 <wumpus> uhmm slow down a bit please
19:08:44 <jonasschnelli> ;-)
19:09:11 <luke-jr> (my list is not for high-prio-for-review topic)
19:10:53 <achow101> #18598 for 0.20 tag and backport (or merge now)
19:10:55 <gribble> https://github.com/bitcoin/bitcoin/issues/18598 | gitian: Add missing automake package to gitian-win-signer.yml by achow101 · Pull Request #18598 · bitcoin/bitcoin · GitHub
19:10:56 <wumpus> jonasschnelli: sipa  added
19:11:03 <jonasschnelli> thanks
19:11:46 <MarcoFalke> achow101: Assigned milestone. I think this will be dealt with in rc2
19:12:13 <wumpus> yes, if it's required for windows signing on some environments it definitely needs to go into the next rc
19:12:24 <wumpus> I didn't need it for LXC fwiw
19:12:34 <jonasschnelli> me2
19:12:37 <MarcoFalke> happens only on docker/podman
19:12:42 <hebasto> yes, lxc does need it
19:12:58 <achow101> yeah, seems to be a virutaliation specific issue
19:13:12 <achow101> just wanted to make sure no one forgot about it
19:13:29 <hebasto> achow101: mind specify it in pr description?
19:13:32 <wumpus> yes, good point
19:13:51 <jonatack> i think i ran into it when win gitian signing with docker
19:14:17 <achow101> hebasto: done
19:14:19 <wumpus> makes sense
19:15:46 <wumpus> luke-jr: I think there have been some disagreements about your PRs, meshcollider was concerned about #18550 for example
19:15:47 <gribble> https://github.com/bitcoin/bitcoin/issues/18550 | Store destdata for change in separate key for backward compatibility by luke-jr · Pull Request #18550 · bitcoin/bitcoin · GitHub
19:16:11 <wumpus> it's probably too late in the release cycle to do changes like that
19:16:14 <luke-jr> wumpus: are we changing to that topic now? I can wait a bit
19:16:32 <luke-jr> it's a bugfix
19:17:24 <MarcoFalke> What bug is it fixing? You didn't include any regression tests
19:17:33 <meshcollider> ryanofsky should really be here for this discussion too
19:17:41 <ryanofsky> here
19:17:46 <luke-jr> MarcoFalke: I don't see how to make tests for this kind of thing
19:18:14 <ryanofsky> My summary of the destdata issue is https://github.com/bitcoin/bitcoin/pull/18550#issuecomment-612672886
19:18:16 <wumpus> so is it a regression in 0.20?
19:18:41 <MarcoFalke> Is this a bug that users can observe or is it only a developer issue?
19:19:09 <luke-jr> it's a wallet format issue
19:19:21 <luke-jr> if we don't fix it now, it complicates backward compatibility when we do fix it later
19:19:26 <wumpus> yes, if it's hard to write a test for and it's not a GUI issue that usually means it's very hard to observe
19:19:40 <luke-jr> it's hard to write a test, because the breaking is backward compatibility
19:19:49 <luke-jr> so we'd need a way to run an old version
19:20:06 <MarcoFalke> the python tests can do that
19:20:08 <meshcollider> It's not a new issue, the actual issue itself was fixed in #18192
19:20:10 <gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
19:20:11 <luke-jr> "destdata" was added in a manner that it only supported non-change
19:20:20 <ryanofsky> Luke, I'm not sure what the exact issue you are concerned about, your PR changes behavior in lots of ways, most of them bad
19:20:26 <luke-jr> but now we suddenly need to support it for change too
19:20:28 <wumpus> destdata has been in there since 2012 or so isn't it
19:20:37 <luke-jr> the problem remaining is that the fix in #18192 breaks compatibility
19:20:40 <gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
19:20:53 <sipa> luke-jr: can you give an exact scenario for reproducing the issue?
19:20:55 <luke-jr> wumpus: yes, 0.9
19:21:14 <luke-jr> sipa: set a destdata on change, then use the wallet in an older version
19:21:18 <sipa> assuming no further changes related to the topic go into 0.20
19:21:25 <wumpus> why does it now suddenly need to be changed before 0.20
19:21:31 <sipa> what does a user do to cause "set a destdata on change" ?
19:21:33 <ryanofsky> luke-jr, that is a pre-existing bug in 0.19
19:21:54 <luke-jr> sipa: currently, it is possible only be using an avoid-reuse wallet
19:21:55 <ryanofsky> it already sets destdata on change and then starts treating it as nonchange
19:21:58 <luke-jr> only by*
19:22:07 <achow101> luke-jr: IMO it isn't worth trying to fix this display bug with users who upgrade then downgrade
19:22:22 <MarcoFalke> I'd say that anything that is not a regression can wait for 0.20.1 or 0.21.0
19:22:24 <luke-jr> achow101: it will come back when downgrading 0.21 to 0.20
19:22:27 <ryanofsky> it'd be easy to backport a trivial fix for that issue in the 0.19 branch, but this PR only makes the bug and introduces other bugs
19:22:27 <achow101> because it's already an issue for those users using the old software, this isn't a regression
19:22:34 <luke-jr> unless we specifically exclude "used" from the fix in 0.21
19:22:38 <ryanofsky> *masks the bug
19:22:49 <sipa> what is the impact? is it just the change address showing up in the address book, or more?
19:23:06 <luke-jr> sipa: just that, I think
19:23:18 <luke-jr> (which can be serious IMO)\
19:23:23 <achow101> sipa: it may have an effect on coin selection
19:23:33 <achow101> in particular trusted change vs untrusted non-change
19:23:38 <luke-jr> achow101: well, if it's spent…
19:24:01 <sipa> my understanding is that it does not affect IsChange, unless a user also goes to set an explicit label on one of those (now shown) change addresses?
19:24:09 <wumpus> who uses the address book for receiving addresses anyhow
19:24:10 <luke-jr> sipa: it does
19:24:20 <luke-jr> sipa: when the change is used, it gets a "" label
19:24:27 <sipa> i see
19:24:29 <luke-jr> wumpus: uh, everyone?
19:25:00 <sipa> i certainly don't, but i don't think that's an important question; if we have an address book, it should work
19:25:02 <wumpus> luke-jr: I must admit I haven't used the address book *at all* for years, but last time I did it was to check an address I sent to
19:25:09 <luke-jr> sipa: this gets worse if we add any new features using destdata - for example, marking which destination are actually used to provide warnings on reuse
19:25:10 <ryanofsky> this is right but it's a preexisting bug, 0.19 already marks addresses used and has the same bug
19:25:17 <achow101> CAddressBook effects more than just the address book IIRC
19:25:27 <luke-jr> wumpus: how do you even receive without using it now?
19:25:35 <sipa> it affecting IsChange is more concerning to me
19:25:47 <wumpus> luke-jr: create new receiving address in the receive tab, give it out
19:25:54 <luke-jr> wumpus: that uses the address book..
19:26:09 <wumpus> no, it doesn't, the 'payment request' table is separate
19:26:18 <sipa> ok, this is a semantics discussion; this is not productive
19:26:28 <MarcoFalke> we should just remove the wallet and gui. Doesn't that solve the problem?
19:26:34 <luke-jr> -.-
19:26:54 <luke-jr> ryanofsky was proposing removing destdata entirely, but that breaks compatibility in new ways, and loses a big feature IMO
19:27:08 <ryanofsky> no, that is not my proposal
19:27:21 <ryanofsky> my proposal is a refactoring that does not change behavior in any way
19:27:36 <ryanofsky> and is meant to prevent bugs like this in the future, but it's basically orthogonal to what we are talking about
19:27:39 <achow101> I think the question is whether we will try to fix this in the future in a way that allows downgrading to 0.19
19:27:51 <ryanofsky> i described the bug https://github.com/bitcoin/bitcoin/pull/18550#issuecomment-612672886
19:27:51 <luke-jr> achow101: to 0.20*
19:27:54 <sipa> so am i correct that this triggers only if a user on 0.19 has avoid-reuse on, and then uses 0.19 or 0.20?
19:27:57 <ryanofsky> and have 3 solutions for dealing with it
19:27:58 <wumpus> destdata was mainly introduced to store payment request data and such, if it's no longer needed it should be removed
19:27:59 <wumpus> but not for 0.20
19:28:07 <ryanofsky> my PR is separate and compatible with all 3 approaches
19:28:21 <sipa> or also when they use 0.20 and then downgrades in certain cases?
19:28:27 <luke-jr> sipa: it's unfixable for already-released 0.19; my hope is to fix it so downgrading to 0.20 works
19:28:40 <luke-jr> wumpus: it's still needed
19:28:59 <luke-jr> destdata is nice because you don't need to upgrade wallets to get features using new metadata
19:29:07 <achow101> to be clear, is the issue *only* with downgrading?
19:29:22 <luke-jr> achow101: yes, which is something we've always tried to support for wallets
19:29:23 <tryphe> address book should be removed outright imo, metadeta that can be modified in a locked wallet is kind of silly
19:29:30 <achow101> If we didn't care about dowgrading at all, this would not be an issue?
19:29:34 <ryanofsky> the issue is not only with downgrading, but the fix only deals with downgrading, and is only partial
19:29:58 <ryanofsky> and it introduces other bug of avoding-reuse feature in 0.19 being broken
19:30:13 <bitcoin-git> [13bitcoin] 15jnewbery opened pull request #18675: tests: Don't initialize PrecomputedTransactionData in txvalidationcache tests (06master...062020-04-precomputedtransactiondata-txvalidationcache) 02https://github.com/bitcoin/bitcoin/pull/18675
19:30:27 <achow101> ryanofsky: what's your pr?
19:30:39 <sipa> can someone give me an exact user scenario that results in a user observing something incorrect, where they start with 0.20.0rc1 as we have now (and then do thing, or downgrade, or ...)
19:31:23 <ryanofsky> again my pr is orthogonal, refactoring only #18608, comptaible with any dataformat we chose now or in the future
19:31:23 <wumpus> tryphe: I think that's unrelated; wallet encryption in bitcoin core only protects private keys, nothing more. Labels and such are not part of that either and should definitely be kept.
19:31:24 <gribble> https://github.com/bitcoin/bitcoin/issues/18608 | refactor: Remove CAddressBookData::destdata by ryanofsky · Pull Request #18608 · bitcoin/bitcoin · GitHub
19:32:07 <luke-jr> sipa: create avoid-reuse wallet; send a transaction that has change; spend that change; suddenly that change is no longer change
19:32:08 <ryanofsky> the scenario is just a change address gets used, and 0.19 software treats all addresses marked as used as nonchange
19:32:21 <ryanofsky> this is a pre-existing scenario that has been around since 0.19
19:32:29 <ryanofsky> and the only way to fix it reliably is with backports
19:32:30 <luke-jr> sipa: the last step is only in 0.19
19:32:34 <luke-jr> (the result)
19:32:53 <luke-jr> sipa: but if we fix this in 0.21, 0.20 would no longer see the "used" flag unless we special-case it
19:33:06 <sipa> luke-jr: that very much depends on how it is fixed
19:33:14 <ryanofsky> luke's pr is a partial fix for the the issue because it stops marking addresses as used in a way 0.19 understands, and marks them used in a different way it is blind to
19:33:17 <sipa> my concern right now is behavior that is observable with 0.20
19:33:28 <ryanofsky> there are 0 bugs observable with 0.20
19:33:29 <luke-jr> I don't see a way to fix it later, without special-casing 0.20 support
19:34:03 <wumpus> tbh if it's only a minor backwards compatibility issue that is only apparent in downgrading, I wouldn't want to do things substantially different because of it
19:34:06 <tryphe> wumpus, mostly agree, although allowing modification of metadata of someone's wallet while in cold storage that they might assume stays constant opens up a lot of doors for bad actors
19:34:09 <achow101> How about we revert the "fix" in 0.20 and try again for 0.21? It seems like ~no one has complained about this bug in 0.19
19:34:26 <ryanofsky> the fix in 0.20 is correct and causes no backwards compatiability issues
19:34:29 <meshcollider> I don't think that's a good idea, the fix is still an improvement
19:34:29 <sipa> luke-jr: if after that scenario (create avoid-reuse in 0.20; create change in 0.20; spend change in 0.20; downgrade to 0.19; do things), the user upgrades again to 0.20, is the issue resolved, or might it persist?
19:34:39 <luke-jr> achow101: leaving it as-is now is strictly less bad than reverting I think
19:34:43 <sipa> tryphe: please stay on topic
19:34:44 <wumpus> tryphe: if that's your concern, encrypt the entire wallet file or store it on an encrypted file system, it's not something bitcoin core's wallet encyrption solves
19:34:46 <ryanofsky> the fix causes no issues whatsoever
19:35:07 <ryanofsky> i mean the fix that's already been merged causes no issues, the new fix proposed is a different story
19:35:14 <luke-jr> sipa: the issue remains resolved in 0.20, provided the user doesn't see it in 0.19 and set a label or something
19:35:29 <sipa> luke-jr: thank you
19:35:57 <luke-jr> (in fact, I think 0.19 making the broken state itself, would also appear correct in 0.20)
19:36:07 <wumpus> okay
19:36:08 <sipa> that's good to hear as well
19:36:16 <wumpus> yes, that's good to hear
19:36:54 <sipa> i think we should document the issue in the release notes, with the point that if the issue appears, upgrading (again) to 0.20 will fix things unless a user manually set a label on an errorneously-shown address in 0.19
19:37:03 <luke-jr> my biggest "end result" concern, is that I don't think users should need to -upgradewallet to get address reuse warnings when we finally merge that ; but that's a few steps into the future
19:37:29 <sipa> for 0.21, we can discuss solutions - whether those involved -upgradewallet or special-casing the 0.20 behavior
19:37:30 <achow101> luke-jr: with what is currently merged, why is changing the fix necessary?
19:37:55 <achow101> as in why is your proposed 0.21 fix necessary
19:38:04 <luke-jr> achow101: it's one thing to break compatibility with <0.19 only for avoid-reuse wallets>, another entirely to break compatibiltiy with <normal wallets going back forever>
19:38:25 <luke-jr> achow101: since "used" destdata is only in avoid-reuse, we have the former situation right now
19:38:35 <luke-jr> but as soon as we add destdata for change on normal wallets, we get the latter
19:39:02 <achow101> are we adding destdata for change on normal wallets?
19:39:12 <luke-jr> achow101: that's my plan for address reuse warnings
19:39:23 <ryanofsky> achow101, yes, we've been doing that since kallewoof implemented avoidreuse
19:39:32 <wumpus> just a reminder that we have to reserve some time for MarcoFalke's topic as well
19:39:39 <jonatack> i agree with sipa's proposals (and with meshcollider and ryanofsky that it's better to keep fix in luke-jr's merged pr)
19:39:41 <luke-jr> that's how it can avoid needing a -walletupgrade
19:39:53 <achow101> ryanofsky: that's only for avoid reuse
19:40:00 <achow101> luke-jr: i see, so what if we don't do that?
19:40:06 <achow101> and just don't change it
19:40:18 <luke-jr> achow101: I don't understand what you mean
19:40:32 <luke-jr> if we don't add address reuse warnigns, we don't have address reuse warnings..
19:40:46 <achow101> why do we have to add destdata on change for normal wallets
19:40:55 <luke-jr> to mark the change address as used
19:41:01 <achow101> if the wallet doesn't signal avoidreuse, then there's no need?
19:41:16 <luke-jr> address reuse warnings are for all wallets
19:41:36 <sipa> yeah, address reuse warnings != avoidreuse behavior
19:41:46 <achow101> ok i see
19:41:48 <sipa> but can't that be done with a different db record instead?
19:41:50 <achow101> there's the context I'm missing :)
19:41:58 <luke-jr> sipa: that's a wallet format change, and needs -upgradewallet
19:41:59 <ryanofsky> sipa, yes
19:42:11 <sipa> luke-jr: why? old wallets would just ignore the record
19:42:24 <ryanofsky> from https://github.com/bitcoin/bitcoin/pull/18550#issuecomment-612672886
19:42:31 <ryanofsky> option 1 is keep using "destdata" record
19:42:40 <ryanofsky> option 2 is switch to "changedata" record
19:42:45 <sipa> also, can't the usage data be computed at runtime from other wallet data?
19:42:45 <ryanofsky> option 3 is "useddata" record
19:43:00 <luke-jr> sipa: historical best practice?
19:43:07 <sipa> ?
19:43:16 <luke-jr> sipa: maybe in this case (I haven't looked into that option yet, but I plan to), but this will probably come up again either way
19:43:23 <meshcollider> Using a different record would be fine, only a minor code complication
19:43:33 <luke-jr> sipa: we've always tried to not change wallet format outside of an explicit -upgradewallet
19:43:34 <meshcollider> I dont think it would come up again luke-jr
19:43:49 <luke-jr> meshcollider: tax metadata for example
19:43:53 <sipa> luke-jr: it sounds to me like no wallet format change is needed *at all* for this functionality
19:44:01 <meshcollider> Again use a different record, not destdata
19:44:12 <luke-jr> meshcollider: what different record?
19:44:43 <sipa> i think this is taking us too far
19:44:51 <luke-jr> meshcollider: adding a new one is a wallet format change..
19:45:06 <sipa> so be it?
19:45:09 <achow101> luke-jr: we've added/changed records before without needing upgradewallet
19:45:16 <wumpus> please wrap up this topic
19:45:19 <meshcollider> In a backwards compatible way if it's new
19:45:26 <sipa> if it's a record old wallets can just ignore, no need for upgradewallet
19:45:28 <luke-jr> achow101: we have?
19:45:29 <sipa> otherwise, use it
19:45:34 <luke-jr> ok then
19:45:42 <meshcollider> Yes the topic is done, the PRs are closed as noone else concept ACKs the change
19:45:48 <achow101> luke-jr: see UpgradeKeyMetadata
19:45:53 <wumpus> #topic experimental libmultiprocess, next steps for multiprocess in general (MarcoFalke)
19:45:55 <MarcoFalke> Background is that libmultiprocess has been added to ./depends , and libmultiprocess compile and link flags have been added to our makefile. Everything was opt-in but after merge discussion concluded that it was too early to do this step, so the change has been reverted. I (and ryanofsky) would like to hear and discuss everyone's concerns about libmultiprocess as part of the meeting for brainstorming.
19:46:09 <MarcoFalke> cc cfields
19:46:16 <cfields> ^^
19:46:22 <MarcoFalke> cc fanquake (probably sleeping)
19:46:34 <sipa> my impression is that we should only merge changes for this if the plan is that eventually this will end up in release binaries
19:46:52 <sipa> it's not clear to me if that's the case
19:46:57 <cfields> fanquake listed a bunch of really good questions, and ryanofsky has given good answers to on #18588.
19:46:58 <gribble> https://github.com/bitcoin/bitcoin/issues/18588 | Revert "Merge #16367: Multiprocess build support" by MarcoFalke · Pull Request #18588 · bitcoin/bitcoin · GitHub
19:46:59 <MarcoFalke> sipa: Eventually that was the goal (at least as I understood it)
19:47:12 <ryanofsky> that is the goal as far as i know
19:47:14 <MarcoFalke> sipa: Though, there was no timeline for it
19:47:22 <cfields> sipa: yes, that was exactly my concern as well. The path forward isn't clear enough to be merging it in in-parts, imo.
19:47:23 <wumpus> sipa: yes, I think that's obvious
19:48:05 <sipa> i haven't paid that much attention to the multiprocess project as i don't really care about it (and kind of dread pulling in extra dependencies like capnproto), but i assumed that it was something lots of people wanted - which is fine
19:48:15 <wumpus> though it's probably ok to have it experimental for a while before it ends up in release binaries
19:48:33 <ryanofsky> maybe relevant: it builds new binaries
19:48:34 <sipa> wumpus: sure
19:49:06 <wumpus> I'm not really sure I like importing capnproto either, just now we got rid of google serialization thing
19:49:14 <ryanofsky> so a theoretical release could include existing binaries, alongside new multiprocess binaries that let you start and stop node/gui/wallet separately and connect each other
19:49:35 <wumpus> then again it's probably better than hand-rolling everything
19:49:50 <MarcoFalke> The multiprocess and capnproto will stay opt-in unless it is decided to change it that
19:50:06 <luke-jr> ryanofsky: can the same binary do single-process and multiprocess?
19:50:23 <MarcoFalke> luke-jr: No, they are two binaries
19:50:30 <luke-jr> I mean hypothetically
19:50:42 <wumpus> ryanofsky: I'm not sure I like that solution, couldn't we emulate the old way using the new binaries without shipping everything twice?
19:50:47 <ryanofsky> yes, I just didn't add the option
19:50:56 <ryanofsky> there is lots of flexibility here
19:51:09 <wumpus> with the static builds we do that's expensive
19:51:15 <ryanofsky> I just built separate binaries because i meant I had to run ./configure less often
19:51:38 <ryanofsky> If we want unified binaries with a runtime options, that's easy too
19:51:52 <wumpus> ah yes like busybox
19:52:02 <luke-jr> eg, bitcoin-qt gets what we have now; bitcoin-qt -process=foo gets just the foo part in MP mode
19:52:34 <wumpus> that kinda makes sense
19:53:29 <ryanofsky> these are really just packaging questions, my question is how to make progress on getting code reviewed
19:53:53 <wumpus> well, 'just packaging questions' are kind of important to do this, I think
19:53:55 <luke-jr> review club? :P
19:54:12 <wumpus> if you split up things people are going to ask how to re-bundle them :)
19:54:28 <ryanofsky> i mean, packaging questions are the things you would be deciding on last, and maybe changing with trial and error
19:55:00 <cfields> ryanofsky: some of those packaging questions come down to language choices though, those things need to be decided on much earlier.
19:55:02 <ryanofsky> 99% of the code stays the same regardless
19:55:11 <wumpus> in any case if you want concept ACK on multiprocess, concept ACK, I think it's a good thing in the longer run
19:55:27 <ryanofsky> cfields ?
19:55:50 <wumpus> security-wise process isloation is a good start as well
19:56:21 <cfields> ryanofsky: I'm curious to know, for example, what the downside of using the c++11 version libmultiprocess you've mentioned?
19:56:26 <jonasschnelli> looking forward to wireguard-tunnel the GUI to a remote node.
19:56:29 <cfields> does that mean dragging boost back in?
19:57:01 <wumpus> so maybe the packaging details is the only thing we can disagree on :)
19:57:01 <jonasschnelli> (but I guess that needs much more)
19:57:01 <ryanofsky> cfields, no downside, i can revert the vasild pr and replace std::optional with boost::optional again
19:57:33 <wumpus> wait, why?
19:57:58 <MarcoFalke> In about 6 months we'll have C++17
19:57:59 <ryanofsky> jonasschnelli, not too much, you can kind of do it with my existing branch if you use socat (existing branch only create UNIX socket)
19:57:59 <cfields> wumpus: I'm just trying to understand our options.
19:58:02 <wumpus> how can we be using std::optional in the first place we haven''t switched to C++17 yet right?
19:58:20 <sipa> wumpus: libmultiprocess is using std::optional, but we're not using libmultiprocess yet
19:58:33 <ryanofsky> wumpus, we are not, libmultiprocess used it internally because vasild submitted a pr to get rid of boost, and i thought it was nice
19:58:51 <wumpus> I think the idea was to have 0.21 still c++11 compatible then go full c++17 for 0.22
19:58:52 <MarcoFalke> wumpus: libmultiprocess is compiled with c++14 flags in depends
19:58:55 <sipa> if it's just std::optional or boost::optional... that sounds like something we can just decide whenever it's merged
19:58:58 <wumpus> c++14??!
19:59:04 <wumpus> nooo
19:59:23 <MarcoFalke> s/is/was/
19:59:26 <luke-jr> do we need to fork upstream to get C++11 support?
19:59:27 <sipa> if multiprocess integration only lands with 0.22, we'd be fine
19:59:38 <ryanofsky> it works fine now...
19:59:42 <sipa> otherwise, it seems it can easily be turned into c++11 compatible
19:59:43 <MarcoFalke> sipa: Yes, that was my thinking
19:59:46 <wumpus> I doubt that has to depend on using boost::optional or std::optional they're kind of the same right?
19:59:51 <luke-jr> ryanofsky: with a non-C++14 compiler?
19:59:54 <wumpus> could use a wrapper or something like we do for fs.h
20:00:07 <wumpus> but we intentionally skipped over C++14, we're not going to do that now
20:00:14 <achow101> don't we have an Optional wrapper already?
20:00:18 <luke-jr> yes
20:00:23 <wumpus> achow101: lol yes we do
20:00:30 <ryanofsky> i'm not sure what's not supposed to work here
20:01:19 <wumpus> in any case I agree these are all small details?
20:01:22 <MarcoFalke> I think boost::optional vs std::optional should be the least of our concerns in regard to multiprocess. This is just a sylistic issue
20:01:28 <MarcoFalke> wumpus: jup
20:01:30 <jonatack> ryanofsky: why not add the PR to blockers?
20:01:32 <wumpus> let's go forward with multiprocess imo
20:01:50 <wumpus> my biggest gripe is really the serialization lib dependency not boost
20:02:13 <wumpus> everyone wants to get rid of boost but also not get rid of boost it's not going to happen any time soon
20:02:16 <ryanofsky> jonatack, #16367 has been in high priority list, and was even merged (then got reverted)
20:02:18 <gribble> https://github.com/bitcoin/bitcoin/issues/16367 | Multiprocess build support by ryanofsky · Pull Request #16367 · bitcoin/bitcoin · GitHub
20:02:49 <MarcoFalke> So I think people don't want a half-merged multiprocess? In which case we should focus on the main pr
20:03:04 <wumpus> we should wrap up the meeting btw
20:03:11 <wumpus> sorry for only leaving so little time for this
20:03:12 <cfields> MarcoFalke: I didn't want a half-decided multiprocess. If we've decided to move forward on defaulting it, I have no issue with the merge.
20:03:23 <luke-jr> it would be nice if there was a standard interface involved here, but lacking that, why not just use serialization.h?
20:03:24 <ryanofsky> wumpus, it's a valid concern. as much as possible I tried to make framework agnostic to serializtion and allow substituting in other implementations later
20:03:41 <jb55> wumpus: I've also ran into build issues with changing capnproto versions. is there a plan to rework that branch without capnproto?
20:03:46 <luke-jr> cfields: why do we need to default it?
20:04:00 <cfields> luke-jr: ship the binaries by default, sorry.
20:04:06 <luke-jr> ah
20:04:12 <wumpus> jb55: so the thing is, I think, that our own serialization framework is not powerful enough
20:04:17 <sipa> for the wallet<->node communication my big hope was that the protocol would be Bitcoin P2P, so that it can be substituted with whatever on either side... but it doesn't seem anyone's working on that
20:04:28 <jonatack> ryanofsky: yes; a re-opened PR when it's ready (since you mentioned review)
20:04:58 <wumpus> jb55: also using consensus serialization for other things might not be the best idea, so people have used something else
20:05:02 <ryanofsky> sipa, we're maybe getting closer that that with ariard's prs
20:05:08 <sipa> ryanofsky: okay
20:05:11 <wumpus> but yes what particular library to use, no idea
20:05:17 <ryanofsky> he's stripping down the node/wallet interface so it's something more akin to p2p
20:05:27 <MarcoFalke> sipa: Wouldn't that be dangerous to accidentally connect to a malicious remote p2p node?
20:05:28 <wumpus> ryanofsky: oh that's nice!
20:05:28 <jb55> what pr is that
20:05:48 <luke-jr> just to be clear: we're not planning on making the interfaces backward/forward compatible, right?
20:05:49 <sipa> MarcoFalke: well you'd have SPV security - which may be even desirable
20:06:08 <sipa> ryanofsky: (i didn't mean that as a "you shouldn't do what you're doing" - they're orthogonal ways of separating things; a P2P-protocol based one is just what i'm more interested in)
20:06:13 <ryanofsky> jb55, i think he only has locking PRs open now that make node/wallet more asynhrnous, but he has branches to do things like store block info in wallet i believe
20:06:41 <wumpus> but for an experimental thing it might be OK to rely on capnproto
20:06:46 <wumpus> I don't want to stand in the way of that
20:06:52 <ryanofsky> sipa, yes, I know that, that's the way I look at it too
20:07:11 <wumpus> we've often enough delayed things before some 'perfect solutoin' would appear which then would never happen
20:07:16 <sipa> agreed
20:07:21 <jb55> so experimental with plans to replace it eventually?
20:07:28 <wumpus> yess
20:07:34 <jb55> I'm ok with that
20:07:45 <sipa> ryanofsky: what specifically does capnproto offer that's hard to do in our own serialization.h?
20:07:48 <cfields> sgtm
20:07:56 <sipa> or is it just avoiding lots of boilerplate code
20:08:05 <sipa> (which may be a valid reason too)
20:08:11 <wumpus> boilerplate I think
20:08:19 <wumpus> capnproto autogenerates a lot
20:08:19 <ryanofsky> sipa, it's a whole io framework, more than just serialization
20:08:42 <ryanofsky> it's higher level even than things like grpc and thrift, it gives you object handles and supports callbacks
20:09:25 <wumpus> right, it's a real RPC mechanism, not just serialization
20:09:30 <sipa> i see
20:09:42 <wumpus> it goes further than protobuf in that regard
20:09:45 <ryanofsky> and i've found it just very nice to work with, but I've tried to make everything around it as flexible as possible so it could be swapped with something else
20:10:00 <luke-jr> so might be more apt to compare it to bitcoind's RPC server..
20:10:03 <wumpus> also it doesn't need to be compatible between differnt verisons
20:10:12 <wumpus> as it's only used for internal communication
20:10:16 <jb55> if its in in-tree as an experimental maybe we can get more people working on it... status has been ambiguous for so long
20:10:20 <wumpus> between components of the same release
20:10:30 <wumpus> luke-jr: nononono it's *NOT* an official API
20:10:40 <ryanofsky> wumpus, yes that is true, though capnproto does have same nice versioning / compatibility features as protobufs
20:10:44 <luke-jr> wumpus: I mean technically comparable, not supported the same
20:10:51 <wumpus> luke-jr: ok :)
20:11:10 <wumpus> agreed in that case
20:11:10 <luke-jr> wumpus: I too would be against a stable interface for this right now :P
20:11:20 <luke-jr> (unless we literally made it use bitcoind RPC maybe)
20:11:29 <wumpus> yes, maybe in the future
20:11:34 <wumpus> ok, let's wrap up the meeting
20:11:34 <cfields> Thanks for the discussion :)
20:11:38 <wumpus> thank you too
20:11:39 <ryanofsky> yes, the interfaces is just the headers in src/interfaces/, changes very often
20:11:39 <sipa> it's certainly hard to agree on a stable interface 2 major releases before we plan to have it available :p
20:11:49 <wumpus> sipa: lol exactly
20:12:09 <wumpus> #endmeeting