19:00:05 <wumpus> #startmeeting
19:00:05 <lightningbot> Meeting started Thu Apr  9 19:00:05 2020 UTC.  The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
19:00:05 <lightningbot> Useful Commands: #action #agreed #help #info #idea #link #topic.
19:00:12 <sipa> hi
19:00:22 <elichai2> hi
19:00:44 <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:00:46 <wumpus> jeremyrubin lightlike emilengler jonatack hebasto jb55
19:00:51 <achow101> hi
19:00:52 <jonasschnelli> hi
19:00:52 <jonatack> hi
19:00:55 <hebasto> hi
19:00:58 <jkczyz> hi
19:01:05 <aj> hola
19:01:05 <MarcoFalke> hi
19:01:12 <sdaftuar> hi!
19:01:38 <MarcoFalke> (sorry, merge bot incoming in a few secs)
19:01:39 <cfields> hi
19:01:53 <amiti> hi
19:01:55 <wumpus> any proposed topics?
19:02:11 <jnewbery> hi
19:02:12 <MarcoFalke> wen release?
19:02:16 <wumpus> looks like there's one by achow101: deprecating signrawtx RPCs
19:02:28 <sipa> low priority topic if there's time: future of asmap?
19:02:31 <wumpus> MarcoFalke:depends on whether  #18553 is a blocker
19:02:32 <bitcoin-git> [13bitcoin] 15MarcoFalke pushed 6 commits to 06master: 02https://github.com/bitcoin/bitcoin/compare/081dcbde6623...405713d00fb4
19:02:32 <bitcoin-git> 13bitcoin/06master 145560845 15Pieter Wuille: Make a fuzzer-based copy of the prevector randomized test
19:02:33 <bitcoin-git> 13bitcoin/06master 14eda8309 15Pieter Wuille: Assert immediately rather than caching failure
19:02:33 <gribble> https://github.com/bitcoin/bitcoin/issues/18553 | Avoid non-trivial global constants in SHA-NI code by sipa · Pull Request #18553 · bitcoin/bitcoin · GitHub
19:02:33 <bitcoin-git> 13bitcoin/06master 14402ad5a 15Pieter Wuille: Only run sanity check once at the end
19:02:52 <bitcoin-git> [13bitcoin] 15MarcoFalke merged pull request #18529: Add fuzzer version of randomized prevector test (06master...06202004_prevector_fuzz) 02https://github.com/bitcoin/bitcoin/pull/18529
19:03:06 <achow101> rc1 soon?
19:03:11 <MarcoFalke> I wish rebroad could ACK it, since they reported the issue
19:03:16 <hebasto> why 18553 could be a blocker?
19:03:23 <wumpus> it's the only PR left that is tagged for 0.20
19:03:47 <wumpus> hebasto: because if your system doesn't support the instruction it'll just crash before main()
19:04:11 <MarcoFalke> Does anyone have that CPU to test?
19:04:26 <elichai2> MarcoFalke: I'm surprised this is the first time we're hearing about this issue. I would've thought everyone without SSE* support will get this error
19:04:27 <sipa> i'm not sure the problem will appear in every build (it may be compiler dependent)
19:04:39 <wumpus> no, bitcoind master runs fine even on my oldest pc, but it might depend on compiler too
19:04:41 <sipa> would i understand how our existing code is broken for systems that do not have sse4
19:04:58 <sipa> *but i understand
19:05:13 <wumpus> movaps is SSE2, right?
19:05:16 <promag> hi
19:05:36 <wumpus> if the init code contained *SSE4* oh sure we'd have noticed
19:05:51 <cfields> *sse2
19:05:54 <cfields> wumpus: right
19:06:08 <sipa> i suspect he is executing an sse4 instruction
19:06:10 <wumpus> almost(?) all amd64 processors support SSE2
19:06:14 <sipa> because his system has sse2
19:06:18 <wumpus> oh
19:06:32 <wumpus> okay in that case we don't know if the PR fixes theproblem at all
19:06:40 <sipa> it does
19:06:41 <wumpus> I suggest we just move on with the branch-off and rc1
19:06:58 <cfields> ah, I guess if we're targetting sse4 it's free to ignore the sse2 intrinsic. Annoying that those are only hints.
19:07:14 <sipa> wumpus: the sha256-shani module is compiled with sse4 on, so any code the compiler produces in that module is allowed to have sse4 instructions
19:07:24 <sipa> the fact that it has a global initializer is a bug regardless
19:07:30 <wumpus> sipa: yes, I agree
19:07:45 <wumpus> I'm the person who ACKed that PR, I think it's a good change
19:07:46 <elichai2> anyways the current code is somewhat broken
19:07:53 <elichai2> even if that's not his specific problem
19:08:01 <sipa> we don't know the exact conditions to reproduce it (which is hard, as it's compiler dependent), but i believe my PR is a bugfix independently of that
19:08:09 <elichai2> sipa: exactly
19:08:12 <wumpus> yes
19:08:29 <wumpus> it makes sense no matter what
19:08:46 <wumpus> even reduces code size a bit
19:09:03 <sipa> but if we want to make sure rebroad's issue is fixed in 0.20... we have no choice but to wait for him
19:09:07 <sipa> i think we can do that in rc1
19:09:13 <jarthur> If anyone needs time on a machine w/ hardware SHA-NI for profiling/memory sanitization, send me your SSH pubkey and I'll give you a VM
19:09:26 <sipa> i have one too
19:09:36 <wumpus> I'm not going to hold up rc1 on them testing it
19:09:39 <sipa> a machine without sse4 would be more useful :p
19:10:00 <luke-jr> (proposed topic: change destdata)
19:10:01 <sipa> there are very few x86_64 systems without sse4 i think
19:10:12 <luke-jr> although maybe better for wallet meeting tomorrow
19:10:21 <wumpus> true, even my old dev machine has "sse4a"
19:10:29 <luke-jr> (but does relate to 0.20)
19:12:41 <elichai2> jarthur: I would like to run some UB sanitizer on the patch, just because I'm a bit uncomfortable with C++'s alignment rules
19:13:05 <wumpus> in that case, let's do rc1 without it
19:13:11 <sipa> ack
19:13:32 <cfields> elichai2: if you're that uncomfortable, there's an intrinsic that doesn't require alignment.
19:13:46 <luke-jr> should probably get at least #18572 into 0.20
19:13:48 <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:13:56 <cfields> compiler might even auto-upgrade it?
19:14:11 <sipa> cfields: i don't think it can
19:14:14 <wumpus> the current solution has 0 overhead at least
19:14:18 <elichai2> cfields: I'm not saying it's UB/ID just that I don't know the rules well enough :) I assume the unaligned load is costlier
19:14:31 <sipa> elichai2: on most systems they have the same cost, but not all
19:14:33 <wumpus> it even generates the same code as before
19:15:14 <elichai2> sipa: I thought SIMD instructions do require aligned pointers. unlike regular loads/stores
19:15:22 <elichai2> (x86/x86_64)
19:15:23 <cfields> yeah, I'm not really understanding the concern. We use alignment tricks in sse4 as well.
19:15:26 <sipa> elichai2: correct
19:15:33 <luke-jr> depends on the CPU in my expereince
19:15:43 <wumpus> yes, alignas(__m128i) should just work
19:15:46 <sipa> elichai2: movdqa requires alignment, movdqu does not
19:16:11 <elichai2> Ok, if other people are confident in this than I'm ACK too
19:16:15 <luke-jr> for a long time at least, Mesa and glibc used ssse3 for memcpy-type stuff, even when unaligned - and it broke on (IIRC) Sandy Bridge
19:16:15 <sipa> but still on most systems they have the same cost; the distinction was made because on early CPUs they differed
19:17:28 <wumpus> we use alignas() in a few other places too
19:17:43 <wumpus> if the compier ignores it, we're screwed with regard to UB in either case
19:17:46 <sipa> yeah, i'm not worried about the alignment thing
19:17:59 <wumpus> (prevector comes to mind)
19:19:06 <elichai2> wasn't there a UB in a version of prevector?
19:19:43 <wumpus> yes, that was solved using the current explicit construction
19:19:52 <sipa> elichai2: the kind of technically-UB-by-the-C++-spec-but-not-on-any-real-platform one only, afaik
19:20:04 <wumpus> it wasn't alignas()'s fault
19:20:07 <elichai2> isn't that the worse kind? :P
19:20:28 <sipa> elichai2: i think bugs that affect production code are just *slightly* more serious
19:20:30 <elichai2> right it's a pragma thing
19:21:11 <elichai2> anyhow, we're talking about this too much :) I'm ACK if people feel confident, my lack of understanding shouldn't stop anything
19:21:16 <jeremyrubin> just noting that there is a pragma free version of prevector that could be written and has no UB; but it's a bigger refactor
19:21:27 <cfields> elichai2: nothing wrong with being paranoid.
19:21:33 <sipa> of course
19:21:50 <wumpus> #topic deprecating signrawtx RPCs (achow101)
19:22:11 <wumpus> jeremyrubin: we're not discussing refactoring prevector again :)
19:23:01 <achow101> so multisig signrawtransaction workflows don't work with descriptor wallets
19:23:20 <achow101> I was thinking that because we have psbt now, we should deprecate the signrawtx RPCs
19:23:35 <MarcoFalke> completely or only for multisig?
19:23:38 <jonasschnelli> can you quickly elaborate why it won't work with desc. wallets?
19:23:40 <wumpus> I'm not generally in favor of depracating signrawtx RPCs, many people use the raw transactions workflow
19:23:48 <achow101> it should be a longer deprecation cycle because it's so widely used
19:23:53 <wumpus> if we do it should be *very* well documented first
19:24:00 <sipa> i'm not so convinced here
19:24:00 <jonasschnelli> agree with wumpus
19:24:20 <sipa> i believe it's good to "nudge" people towards PSBT, but deprecation is probably too hard a hammer for that
19:24:28 <luke-jr> [19:23:38] <jonasschnelli> can you quickly elaborate why it won't work with desc. wallets?
19:24:29 <wumpus> like make a blog post how to *old thing* in *new way*
19:24:33 <achow101> jonasschnelli: because of the separation of watchonly things, currently we can't create a wallet that has both the multisig script and keys to sign for it
19:24:36 <wumpus> yes, I agree
19:24:51 <achow101> so doing a multisig becomes a half assed psbt workflow
19:25:09 <sipa> achow101: i don't understand why we'd want descriptor wallets to not support private keys for multisig
19:25:46 <achow101> sipa: the issue is with exporting the private keys to the multisig
19:25:59 <achow101> IIRC there was contention about exporting keys that used unhardened derivation
19:26:06 <jonasschnelli> are those technical or conceptual limitations?
19:26:17 <sipa> achow101: so use hardened derivation?
19:26:22 <achow101> jonasschnelli: conceptual and current implementation limitations
19:26:51 <sipa> i understand there may be UI issues on how to make this easy
19:27:20 <sipa> but say you have a private key, even generated manually or whatever... you should be able to import a descriptor for a multisig based on it, and have that private key in the same wallet
19:28:01 <achow101> sipa: sure
19:28:13 <sipa> and that would work fine with signraw*, right?
19:28:17 <achow101> (this is also partly in the context of making all of the RPC tests work)
19:28:31 <achow101> sipa: yes
19:29:35 <sipa> ok
19:30:01 <achow101> but conceptually, it feels like psbt should be a directly replacement to raw txs, so we should move to remove those eventually
19:30:31 <jonasschnelli> I could understand why the signraw commands could refuse to work for BIP44-ish descriptor wallets (due to the hardening violation),... though for manual privkey-ckd it should work
19:30:58 <sipa> jonasschnelli: once you have all the right things in a descriptor wallet, it doesn't matter - hardened or not
19:31:14 <sipa> (is my understanding)
19:31:58 <sipa> achow101: i think my preference would be to mark signraw* as in "maintenance mode" or so, where they don't receive new features (e.g. they wouldn't support taproot signing when that gets in)
19:32:10 <instagibbs> +1
19:32:29 <sipa> but i feel like deprecation is kind of ruthless
19:32:38 <achow101> I was thinking of an extra long deprecation cycle
19:32:40 <wumpus> yes
19:32:52 <instagibbs> imo I think the tooling is too widely used and PSBT still has an adoption curve to hit
19:32:58 <wumpus> agree
19:33:00 <achow101> like 2 releases with a note saying it's deprecated, but don't disable yet. then 2 releases with it hidden behind -deprecatedprc. then remove
19:33:02 <instagibbs> the tooling meaning *raw*
19:33:05 <wumpus> maybe in a few years bring this up again :)
19:34:08 * achow101 adds to 2022 calendar
19:34:33 <jonasschnelli> I can't completely follow why we should remove/deprecate it since in many use cases those commands work fine.
19:34:57 <jonasschnelli> (the accounting system had conceptual flaw in contrast)
19:35:02 <jonasschnelli> *flaws
19:35:08 <luke-jr> jonasschnelli: what flaws?
19:35:09 <wumpus> yes
19:35:20 <sipa> jonasschnelli: i think the reasoning is "it's hard to make it work nicely with descriptors, and there is a better system already... would be easy to just get rid of it"
19:35:24 <luke-jr> the accounting system worked fine AFAIK, just nobody cared to maintain it
19:35:30 <wumpus> accounting system discussion is off topic
19:35:34 <luke-jr> true
19:36:00 <luke-jr> couldn't signrawtx be reimplemented as a wrapper around PSBT?
19:36:01 <jonasschnelli> is the plan to only support descriptor wallets in the future?
19:36:02 <jonasschnelli> with some sort of migration
19:36:14 <sipa> luke-jr: yes (and probably should), but that wouldn't solve the problem
19:36:24 <achow101> my plan is to make descriptor wallets the default wallet type
19:36:27 <achow101> eventually
19:36:55 <jonasschnelli> this would be fine. As long as "legacy" wallets are still supported, signwith* commands shound't go away?
19:37:33 <sipa> the issue (as i understand it) is constructing a descriptor wallet that has all the same pieces of information as a current legacy wallet is unclear (where do the keys come from, how to import without reintroducing mixed wallets or watching the wrong kind of things...)
19:37:54 <achow101> sipa: yes
19:38:01 <sipa> i believe it would be nice to spend some time on actually solving that... because there is no technical reason why a descriptor wallet couldn't have that information
19:38:16 <jonasschnelli> exactly
19:38:35 <sipa> achow101: FWIW, i think your envisioned workflow (having two wallets, one with the multisig, and one with the private keys) is also pretty suboptimal
19:38:37 <jonasschnelli> we shouldn't enforce modes of use due to solvable technical imitations
19:38:42 <MarcoFalke> If the call doesn't work with descriptor wallets, it should be disabled for those wallets, not for legacy wallets as well.
19:38:46 <MarcoFalke> agree with jonasschnelli
19:38:53 <sipa> it isn't a technical limitation
19:39:02 <sipa> it's an unsolved UI question
19:39:12 <sipa> (where UI includes RPC and workflows)
19:42:03 <achow101> i suppose it is
19:42:57 <sipa> i don't think disabling signraw* for descriptor wallets would even solve the root of the issue - users would still need a workaround to do what they could before (having two wallets, and run PSBT RPCs on both)
19:43:08 <jonasschnelli> achow101: maybe you could write a short gist/paper about the issue for help us to understand it better?
19:43:32 <sipa> can i try in 3 lines?
19:43:36 <jonasschnelli> plz
19:43:49 <achow101> jonasschnelli: sure. I should write release notes for descriptor wallets anyways and there should be section on known limitations
19:44:53 <wumpus> +1
19:45:04 <sipa> currently you can construct a legacy wallet which has (1) a private key for one key and (2) watchonly records for multisigs involving that public key - this is crazy (because it means payments to the individual single key will be treated as incoming money, unable to separate it from the multisig funds), but it works great: you have the script information (the the multisig watchonly) and the private
19:45:10 <sipa> key for one of the keys in one wallet,...
19:45:13 <sipa> so it can do everything
19:46:58 <sipa> in descriptor wallets, you'd need to explicitly import a descriptor for the multisig, and then add a private key for one - you can't have started with a wallet that had that private key already as a single-key wallet (because then you reintroduce the mixing of singlekey/multisig funds), and you can't export that private key from another wallet (because we don't want export of non-hardened keys)....
19:47:04 <sipa> so where does it come from?
19:47:31 <sipa> i think the only question is a UX one around construction of such wallets
19:47:55 <sipa> </fin>
19:48:22 <jonasschnelli> I see. So one would need a watch-only-ms desc wallet and a single-key desc wallet for signing the PSBT (or whatever)
19:48:50 <achow101> yes
19:48:59 <achow101> and with both psbt and raw tx, the workflow is the same
19:48:59 <sipa> right, that would work - and the PSBT would carry the script information from the watch-only-ms wallet to the signing-key wallet
19:49:04 <instagibbs> user stories may help cover cases, I tend to only think about MY use case
19:49:18 <jonasschnelli> Which the signraw commands could construct in the background (assume providing all the infos)?
19:49:21 <luke-jr> aren't the multisig funds classified as watchonly?
19:49:53 <sipa> luke-jr: right, fair - that's the reason watchonly exists
19:49:56 <achow101> you go to the watch-only-ms wallet with a psbt or rawtx, it adds the scripts. then you go to the single-key wallet and sign it. it's the same workflow, but psbt is better suited for carrying this data
19:50:14 <sipa> but it's ridiculous that you currently can't do multisig stuff without also having payments to individual keys as balance in your wallet
19:50:35 <jonasschnelli> sipa: though that is rarely used, right?
19:50:45 <sipa> jonasschnelli: "used" ?
19:50:47 <sipa> it's an attack
19:51:10 <sipa> you can send funds to a individual key in a multisig, and the user may think it's paid to the multisig
19:51:16 <luke-jr> so we need a way to have can-sign non-ismine descriptors
19:51:22 <jonasschnelli> Kida. Yes. I see. Agree that it is a flaw/ridiculous
19:51:31 <sipa> luke-jr: descriptor wallets already do that
19:51:35 <achow101> luke-jr: we already do that. it's a question of the scripts
19:51:58 <sipa> we just need a good way to import a multisig descriptor + individual key into a descriptor wallet
19:51:59 <instagibbs> IsMine implementation in descriptor is a relative beauty :P
19:52:25 <sipa> if that works, signraw* and PSBT* will function just as before
19:52:34 <sipa> if that doesn't work, it's going to be shitty to use for both
19:52:39 <jonasschnelli> Now I see why it's a UX issue. We have set our own limitations which IMO could be worked around by creating the right structures on the fly for the signraw* commands when using desc.-wallets
19:53:10 <instagibbs> achow101, if you do this you're also going to have to get rid of the "private keys disabled" hack we've been using
19:53:25 <instagibbs> to detect if we want to do PSBT stuff or try to sign the transaction
19:54:19 <achow101> instagibbs: I don't think so. but I'll experiment
19:54:40 <achow101> jonasschnelli: which "right structures"
19:54:43 <instagibbs> achow101, well, if there exists a private key, it will try to sign and fail unexpectedly, I think, but you can test yes :)
19:56:04 <jeremyrubin> BTW: if you have thoughts on BIP-119 Next Steps please submit them in this survey; want to collect feedback from everyone https://forms.gle/rT3v4JjHbdn3RMnL6
19:57:03 <instagibbs> The wallet should just probably know explicitly that it should try to auto-sign or return a PSBT, but that's yet another UX q.
19:57:37 <wumpus> 3 minutes to go
19:57:57 <jonasschnelli> achow101: maybe I get it wrong. But if someone invokes a signraw, depending on the input, you could create either a watch-only-ms wallet or a single-privkey wallet in the background and use the PSBT workflow
19:58:36 <achow101> but where does the multisig script come from?
19:59:01 <sipa> jonasschnelli: the question is not about signraw* or PSBT*; the question is constructing a wallet that has the right information
19:59:08 <achow101> it's not hard to wrap signrawtx around psbts so it uses psbt internally. but it just doesn't have all of the data there
19:59:24 <luke-jr> descriptor wallets can or can't have multiple descriptors?
19:59:36 <jonasschnelli> provide it manually or provide it via a second wallet
19:59:55 <sipa> luke-jr: can; change and payments in general will come from distinct descriptors
20:00:12 <instagibbs> luke-jr, you can import any number of descriptors, there are 6 "Active" ones, aka keypool, by default
20:00:30 <achow101> luke-jr: can, but having a descriptor wallet contain both multisig and single key descriptors goes back to the mixed watchonly wallet thing
20:00:39 <instagibbs> legacy/p2sh-segwit/bech32 x internal/external
20:00:50 <luke-jr> achow101: not if you flag the single-keys descriptor as non-ismine?
20:00:54 <wumpus> sorry, time to wrap up the meeting
20:00:56 <achow101> jonasschnelli: yes, and that's the shitty ux sipa was talking about
20:01:06 <wumpus> #endmeeting