< jamesob> I'm no great fan of azure but maybe we should consider using some of this free compute time for things like per-commit build validation: https://azure.microsoft.com/en-us/blog/announcing-azure-pipelines-with-unlimited-ci-cd-minutes-for-open-source/
< sipa> wumpus, achow101: so today i've used the PSBT implementation in 0.17.0rc2 for actual transactions, and noticed a bug or at least weird behaviour that seems fixed in master
< sipa> the workflow was using createrawtransaction + converttopsbt + walletprocesspsbt on an online system (which just had an importaddress of a P2SH segwit address, but not the full key), which as expected produced a PSBT with no scripts, and a non-witness UTXO (as it couldn't know whether the address was segwit or not)
< sipa> and then on an offline system i used walletprocesspsbt, which added scripts signed the transaction (as expected), but then produced a PSBT with still a non-witness UTXO is... which finalizepsbt *failed to deserialize*
< achow101> maybe one of the fixes wasn't backported?
< sipa> the workaround was to use walletprocesspsbt on the offline system with sign=false, move it back to an online system (which had master running), which converted the non-witness UTXO to a witness UTXO, then move it back to the online system for signing + finalizing
< sipa> i think this may have been inadvertently fixed by https://github.com/bitcoin/bitcoin/pull/13723
< sipa> i won't have time the coming week to look into this more, however
< achow101> sipa: I'll take a look at it
< achow101> sipa: I think it may have to do with the checking for witness utxo stuff you did in 13723
< sipa> yes, that seems plausible
< achow101> sipa: I see why it doesn't replace the non-witness psbt, but you said finalizepsbt couldn't deserialize it?
< sipa> achow101: sanity check in the deserializer fails
< achow101> ah
< sipa> i assume it's non-witness utxo with witness final signature?
< achow101> yeah
< achow101> sipa: does master replace it with the non-witness utxo with the witness one?
< sipa> yup
< sipa> i ran just walletprocesspsbt on master, and it converted the non-witness utxo to a witness utxo
< sipa> (after the scripts were filled in)
< achow101> was that with a wallet with the utxo?
< achow101> (i.e. online wallet)
< sipa> yes, i believe
< sipa> ah, so it may not have converted it, but instead replaced it
< achow101> yes
< sipa> hmm, so maybe it wasn't related to 0.17/master
< achow101> afaik, there is no conversion from non-witness to witness utxo that happens
< achow101> I think I know how to fix this, both in master and 0.17
< achow101> sipa: #14196 should fix that for the 0.17 branch
< gribble> https://github.com/bitcoin/bitcoin/issues/14196 | [0.17][psbt] always drop the unnecessary utxo and convert non-witness utxo to witness when necessary by achow101 · Pull Request #14196 · bitcoin/bitcoin · GitHubAsset 1Asset 1
< kallewoof> Several people on https://github.com/bitcoin/bips/pull/725 (Generic Signed Message Format) are suggesting I use a fake tx that the prover simply signs. I'm not sure what the benefits of doing this are, though..
< kallewoof> My approach: custom sighash. Suggested: custom sighash with transaction stuff. But why?
< achow101> kallewoof: easier to implement
< achow101> also hacking message signing into the transaction format is something that has been discussed in the past
< kallewoof> achow101: is it, really? (easier to implement) In bitcoin core, I would add a BaseSignatureChecker that took a sighash and that's all. Just call VerifyScript with the inputs from the SignatureProof container.
< kallewoof> achow101: with a tx, you would have to create the two transactions, do the custom tweakeries to ensure it is not actually sendable, then sign it
< Jmabsd> What is the structure of Bitcoin Core's HD wallet now (derivation paths); it's not going with BIP 44/49 today is it?
< Jmabsd> does Bitcoin Core do BIP 44 "m / purpose' / coin_type' / account' / change / address_index" form at all?
< achow101> kallewoof: I'm not sure. I haven't really been following the discussion there
< achow101> Jmabsd: it uses m/0'/0'/i'
< achow101> Jmabsd: and m/0'/1'/i'
< Jmabsd> achow101: err, err, so purpose = 0 - or does it even call it purpose?
< Jmabsd> achow101: and.. second level is change or account?
< achow101> first level has no name. second is change
< achow101> well 'internal' (change) and 'external' (receiving)
< Jmabsd> achow101: aha so it's an ultrareduced form of BIP 44 ha
< achow101> no, bip44 uses unhardened derivation. Core uses all hardened
< Jmabsd> achow101: so Bitcoin Core cut away the "coin" and the "account" derivation, and set purpose to 0 - that's pretty much it yes?
< kallewoof> achow101: Gotcha. Thanks for the hints though. Perhaps I am overthinking the added complexity of requiring tx creation capabilities when you're just out to sign something using a privkey.
< Jmabsd> achow101: in other words, Bitcoin Core rolled it own thing.
< achow101> Jmabsd: or really bip44 is bip32 with stuff added onto it. the original description of suggested derivations paths is very similar to core: https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#the-default-wallet-layout
< Jmabsd> achow101: did Bitcoin Core have any particular point with _not_ implementing an accounts abstraction - it's just Bitcoin Core wants to provide one single wallet balance, yes?
< achow101> Jmabsd: accounts wouldn't have worked with Core's wallet structure. also, the existing accounts system (which could hmaybe ave been used) was being removed and naming would have been confusing
< achow101> in general, accounts don't fit into core's wallet model very well
< sipa> kallewoof: advantage of hacking tx format: hw wallets may not need any changes to produce a signature. downside: hw wallets wouldn't know that they're actually signing a message rather than a tx
< kallewoof> sipa: right. Yeah, maaku convinced me in PR comments
< sipa> Jmabsd: the "accounts" feature as imagined by bip32/bip44 is more like what we now call multiwallet
< sipa> kallewoof: i'm not sure whether that's a good or a bad thing
< kallewoof> I think it's an OK downside that HW wallets don't realize they're not signing a tx
< kallewoof> given the benefits.
< sipa> kallewoof: i agree with luke as well that the signer actually ought to be aware that they're signing a message rather than a tx
< kallewoof> The HW firmware you mean?
< sipa> well, the human
< sipa> but the only way to guarantee that is having the HW be aware, so it can say
< kallewoof> *nods*
< kallewoof> I guess the question then becomes: is it bad to make it possible for people to HW sign a message even before HW wallets support it?
< kallewoof> maaku's point about proof of reserve and such sound useful
< sipa> oh he is suggesting to actually make it a full transaction, not just a signature
< sipa> that gosles even further
< sipa> *goes
< kallewoof> Yeah
< kallewoof> 2 transactions. One input tx whose output takes the scriptPubKey and one output tx spending it
< sipa> right, but the first is constructed by the verifiedlr i suppose and not actually included in the proof
< sipa> it's true that it's a very straightforward way of building a forward compatible signing mechanism... but it also sounds scary that someone could be tricked into thinking they're creating a transaction, but they're really signing a message they don't want to
< kallewoof> I think the idea is that you provide the scriptPubKey, sigScript etc and then construct the two txs, and sign or verify them.
< kallewoof> Wouldn't the opposite be more scary? (Think you are signing a msg but creating a tx)
< sipa> oh absolutely, but there is no risk for that in either idea
< kallewoof> Right
< kallewoof> What if a tx version 0xffffffff was reserved for "message transactions"?
< kallewoof> Should be pretty straightforward to check and point out for a user.
< sipa> that's irrelevant
< sipa> there are many ways you can modify a tx to indicate it's really something else
< sipa> the question whethee you want it to be signable by a non-aware piece of software or hardware
< kallewoof> I have a hard time envisioning the situation where someone is fooled into signing a msg when they meant to sign a tx
< sipa> how so?
< sipa> your online machine is hacked, but you trust your hw devicw
< sipa> that is exactly the scenario it is designdd for
< kallewoof> Right, but what would they do with me proving I own funds X?
< kallewoof> Or proving that I am behind "msg=Hello World"
< sipa> signing a message can be far more than proving you have funds
< sipa> if you don't know what you're signing, you shouldn't sign it
< kallewoof> Fair enough. So you're saying, don't make it possible for HW wallets to sign these things unless the HW firmware gets an update to explicitly support it.
< sipa> right
< sipa> i also understand the argument that actually using a full tx structure gives a lot of compatibility options
< sipa> it also makes it bigger
< kallewoof> Well, not the proof per se.
< kallewoof> The temporary memory foot print, yes.
< sipa> well i believe maaku's idea is to actually use a tx as proof
< sipa> so that it automatically includes whatever future structures get added to it
< kallewoof> Maybe I misunderstood yeah. His talk about 'deterministic' made me assume it would recreate the tx both when signing and when verifying.
< sipa> perhaps
< sipa> it's unclear to me
< sipa> in any case, i agree it is very forward compatible, but i'd prefer something where the signers need to be aware
< sipa> i'm sure you can combine the two, by using a tx like structure, but changing the sighash algorithm, for example, so it can never be a valid transaction
< sipa> though than you also lose the ability for unaware signers to create message sigs... which i think is inevitable
< kallewoof> The whole idea behind using the transaction format is so you can drop it in as is, without doing custom stuff. Making a non-tx tx sounds like it defeats the purpose.
< sipa> it still maintains all forward compatibility
< sipa> just not backward compatibility
< kallewoof> Hm..
< sipa> my preference is still something simple that's just a single scriptSig/witness though
< wumpus> good to catch this in the rc phase at least!
< sipa> wumpus: it's a weird case, and not exactly following the intended workflow... but still, ending up with a non-deserializable psbt string is definitely not acceptable
< wumpus> yes it shouldn't really happen
< fanquake> Since gitian-builder started supporting docker containers, has anyone been using it directly on macOS, rather than having to run inside a VM? i.e macOS -> Debian VM -> gitian-builder?
< wumpus> I haven't, sounds like a cool idea though
< fanquake> I played around with getting it working today, only one issue with the macOS date command. Otherwise, seems to work ok. I've built 0.17.0rc3 sigs that match.
< wumpus> moving to less layers of containers means less moving parts and hopefully more reliability
< wumpus> great !
< fanquake> wumpus Yep, one of the complaints about gitian building was always that it was hard to do, and having to essentially run multiple VMs never helped.
< wumpus> if there's anything we've learned from years of maintainership it's that big redesigned always take much longer than expected and these kind of incremental improvements are worth doing, even if the long-term plan it to throw out gitian completely
< warren> fanquake: what does "date -u" return on macos?
< fanquake> warren Tue 11 Sep 2018 07:23:21 UTC
< warren> fanquake: date +%s ?
< fanquake> warren 1536650642
< warren> k th
< wumpus> fanquake: I suppose that also depends on the locale?
< fanquake> wumpus passing -u Should show the date in UTC. Otherwise if I just do 'date' I'll see "Tue 11 Sep 2018 15:25:52 AWST", which does use my locale.
< wumpus> timezone, yes, but I mean the YYYY/MM/DD ordering still might be locale dependent
< wumpus> or the language of the months
< fanquake> wumpus yes probably. One problem I saw with gitian-builder was using "date +"%F %T" -u -f -", on macOS that'll give you "date: illegal time format"
< fanquake> However, given that you have to install coreutils for sha256sum (which macOS doesn't have), I just swapped to using the coreutils supplied date as well.
< fanquake> macs seem to be pretty bad all over for locale dependance issues. Have seen them before with sed as well.
< wumpus> shell script is a bad language if you want locale independence, or even architecture independence
< wumpus> it relies on so many external exectubles
< fanquake> wumpus Are you proposing re-writing Core in Rust at Core-Dev?
< wumpus> noooooo :)
< mryandao> isnt blue matt already working on one?
< dongcarl> mryandao: rust-bitcoin isn't a rewrite yet, and BlueMatt isn't the only developer
< mryandao> my apologies for inappropriate attribution :(
< wumpus> rust-bitcoin is a library that implements some bitcoin primitives in rust, it's not meant as a replacement for bitcoin core, but something like python-bitcoinlib
< wumpus> (it could be used as part of a node implementation of course)
< warren> fanquake: locale in unix terms is more language than timezone which is a different name
< kallewoof> I very recently discovered it, following andrew poelstra's github trace, actually. he seems to be very active!
< mryandao> anyone balsy enough to run parity-bitcoin?
< fanquake> warren thanks
< dongcarl> wumpus and I talked about perhaps writing a standalone P2P bitcoin server in Rust that can FFI with bitcoind
< dongcarl> The main benefit being memory safety, and zero-cost abstractions for managing all the connections
< wumpus> right, bitcoind as 'consensus engine' with the network-facing parts in rust
< warren> YES!!!
< mryandao> any chance for a libconsensus rewrite in rust?
< wumpus> mryandao: that's a very bad idea
< dongcarl> I don't think everything needs to be rewritten in Rust
< dongcarl> libconsensus should be in C
< dongcarl> as FFI to C is well-implemented for almost all languages
< wumpus> the whole point is that rewriting the consensus code in differnet language
< wumpus> is EXTREMELY DANGEROUS
< wumpus> and has a very high fork risk
< fanquake> ^
< warren> mryandao: Good idea only if you can faithfully reproduce all bugs, even unknown bugs
< wumpus> don't do this
< wumpus> all the non-concensus parts are fair game though
< warren> +1
< mryandao> sorry, i just couldnt resist when i hear re-write
< mryandao> kek
< warren> mryandao: multiple people here started years ago on the side of "This is crazy! We need to write a specification of what is Bitcoin." and a year or two of study later realize reimplmentations are too dangerous.
< dongcarl> I'd like to eventually get to a point where bitcoind is just a consensus engine, but I think the reasonable first step is to start with just the p2p parts
< wumpus> mryandao: sarcasm doesn't travel well over IRC, sorry
< wumpus> exactly, what warren says, you'd not be the first to propose it's seriously
< dongcarl> I believe that it would be wise to revisit libconsensus, as I know that most PRs related to it have become stale
< mryandao> what if a future C++ compiler tweaks the behavior of libconsensus though
< warren> Yes it would be really nice to be able to use most of bitcoin core as libraries in other projects.
< mryandao> could happen?
< dongcarl> revisit as in see if the API need changes, and organize an effort to actually get it done
< warren> It sucks that that drags in libstdc++ as a dependency
< wumpus> mryandao: yes, even different compiler versions or CPU architectures have a slight hardfork risk - that's a really bad reason to increase the risk even more
< wumpus> warren: at least that tends to be installed everywhere, I'd say boost is much worse
< fanquake> dongcarl, are you going to Tokyo? Maybe add that to the list of discussion topics?
< dongcarl> fanquake: Yes, I was proposing that in IRC yesterday.
< warren> mryandao: the bigger risk is undiscovered bugs in the dependent libraries, this blew up several times in the past. Bitcoin devs found/reported/fixed many bugs in openssl and spent years to carefully write a replacement to get rid of openssl.
< wumpus> mryandao: e.g. there has been a case in the past (in OpenSSL) where 32/64 bit architectures had different behavior and could be forked
< dongcarl> fanquake: where do I add to the list?
< fanquake> dongcarl, ah sorry
< fanquake> I think the easiest way is to email the organisers. They are maintaining a list of proposed topics.
< wumpus> mryandao: these are not exactly new concerns FWIW
< fanquake> At least good progress has been made over the last 3-4 years continually dropping dependencies.
< wumpus> yes
< warren> mryandao: other implmenetations exist, they might be good, but they would be better if you could swap out the consensus parts of their code for libconsensus from Core
< fanquake> I saw soneone talking (again) about fulling ridding the codebase of OpenSSL a few days ago.
< wumpus> I think the current situation where OpenSSL is only used for randomness is ok
< warren> "they might be good" is being polite. People here argue that they are dangerous to rely upon without also checking for consistency with a Core node.
< warren> wumpus: huh, thought folks here wrote a replacement for that too years ago?
< warren> maybe they don't trust themselves
< wumpus> (sure, dropping the dependency completely would be great, but at least it doesn't touch consensus code anymore, and replacing randomness is a huge risk)
< wumpus> warren: yes, but it's hard to get enough confidence in it imo
< warren> ah, I hadn't heard an update on that for a while.
< wumpus> I don't think it's actively being worked on at the moment, someone could pick it up, but it doesn't seem like that much of a priority, certainly compared to the risks
< fanquake> There has been #5885 and #10299
< gribble> https://github.com/bitcoin/bitcoin/issues/5885 | [WIP] Replace OpenSSL PRNG with built-in Fortuna implementation by sipa · Pull Request #5885 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/10299 | Remove OpenSSL by sipa · Pull Request #10299 · bitcoin/bitcoin · GitHub
< warren> My handler says to just rely on the hardware RNG directly without mixing other entropy. There's a new kernel option for that.
< wumpus> thanks, yes what fanquake says, the code is there if someone wants to pick it up
< wumpus> warren: HAH
< dongcarl> wumpus: pick it up as in rebase it?
< wumpus> proprietary, secret sauce random, with no-backdoor guarantee!
< fanquake> warren maybe you can write us something like BIP42 for that
< wumpus> dongcarl: rebasing would be the first step, I'm not sure what other work is involved
< fanquake> Would also need to carve openssl out of the depends system if that isn't a part of the PRs already. See how that affects Qt/building etc.
< wumpus> you can't remove it from the depends system as long as qt still relies on it for the payment request shit
< dongcarl> Looks like provoostenator tried working on it in his branches?
< wumpus> (which I was trying to depecrate, just when bitpay got the *genius* idea to require it)
< wumpus> #11622
< gribble> https://github.com/bitcoin/bitcoin/issues/11622 | build: Add --disable-bip70 configure option by laanwj · Pull Request #11622 · bitcoin/bitcoin · GitHub
< wumpus> of all the PRs I tried to do and didn't go through, I think that one hurt the most
< fanquake> :'(
< dongcarl> ooof...
< wumpus> the whole move has caused bitpay to lose a lot of merchants and users, I hope they'll be reduced enough at some point that they don't carry much weight anymore
< wumpus> but for now, it's realistic to drop the dependency of bitcoind on openssl, but not the bitcoin-qt one
< wumpus> (in -qt it's used in *two* ways, directly to validate the payment messages, and indirectly through qt to fetch https:// URLs)
< * dongcarl> facepalm
< warren> wumpus: I think there is still strong enough reason to begin the deprecation process, without beginning the process we'll NEVER get rid of openssl
< wumpus> warren: I think most scenarios that care about dependencies (e.g., on servers) already don't involve building the GUI
< wumpus> so dropping the direct dependency for randomness will help most, there
< wumpus> but if you really want to take on deprecating BIP70 with bitpay against you, I'll support you, but it's not something I want to take up, I don't have the motivation nor energy
< dongcarl> From https://github.com/bitcoin/bitcoin/pull/10299#issuecomment-298785082 it seems like sipa was going to "propose some other RNG changes first"
< dongcarl> Did that end up happening?
< wumpus> no, sipa kind of dropped the project to persue other things (this was talked about at last meeting AFAIK)
< dongcarl> pinging fanquake, keeper of the PRs, adder of the labels
< dongcarl> I see
< dongcarl> wumpus: are there other dependencies that could be dropped that aren't yet?
< warren> We will be able to drop boost entirely at some point?
< fanquake> Eventually, yes, I think it's possible. Cory has various work in process to rid out the "harder" to replace Boost.
< fanquake> *rip
< wumpus> there's a lot of work in that direction, which I think would be pointless if the eventual goal is not to drop boost. It's not close, though.
< wumpus> the toughest hurdle is the multi-index strcuture used in mempool
< fanquake> However I have been overly optimistic about removing Boost in the past, hah. https://github.com/bitcoin/bitcoin/issues/8875#issuecomment-280325296
< wumpus> there's no current or future c++ standard library support planned for that as far as I know, and reimplementing it is difficult, as well as risky
< wumpus> the other things are more or less straightforward work, although some have to wait for a future c++ standard (like filesystem)
< fanquake> I'd like to do any dependency bumping pretty early on for 0.18, it got left (especially qt) until pretty late in the 0.17.0 cycle, so would prefer to do it earlier this time.
< fanquake> Also have to pull in that c++14 requirement :p
< wumpus> I agree, it's better to do such things early-on in the cycle
< wumpus> also want to get the windows unicode patches in asap, the more time is left for noticing problems with them
< dongcarl> wumpus: what is the multi-index structure? a collection that can be indexed into by multiple types or have multiple indices, or both?
< wumpus> dongcarl: multiple indices, like an sql database: https://github.com/bitcoin/bitcoin/blob/master/src/txmempool.h#L461
< fanquake> wumpus I think #14184 can go in. Unrelated, but I seem to have been getting some bad Qt mirrors lately, really slow downloads.
< gribble> https://github.com/bitcoin/bitcoin/issues/14184 | Scripts and tools: increased timeout downloading by cisba · Pull Request #14184 · bitcoin/bitcoin · GitHub
< wumpus> fanquake: yes, why not
< dongcarl> gh down for anyone else?
< wumpus> works for me
< kallewoof> works for me as well
< wumpus> github is sluggish here as well now
< wumpus> at least the website
< wumpus> https://github.com/zw/bitcoin-gh-meta stopped updating?
< wumpus> ok contacted iwilcox, they're not on IRC but managed to find an email
< echeveria> gentle reminder that it's all on github archive as well.
< wumpus> echeveria: the problem is that my release not generation script relies on this specific layout, but sure, I could adapt it if we don't get this running again
< wumpus> echeveria: do you know how to request issue/PR metadata from a specific project from it?
< wumpus> nah, I guess I could request it from github itself through the issue API, the useful thing was having an automatic local mirror of issue data
< echeveria> wumpus: you can grep the data from it if needed, it's just blobs of JSON data straight from the firehose.
< echeveria> same place the other mirror came from actually.
< wumpus> github is completely AWOL here
< EucOcVrFfr2D> Hi does anyone know a tool to compute P2SH from a redeem script? I tried github.com/kallewoof/btcdeb but doesn't seem to offer that feature.
< EucOcVrFfr2D> I'm asking because i can't wrap my head around the correctness of the test in the PSBT (rpc_psbt.json)
< EucOcVrFfr2D> Oh i might just have fund the answer myself
< EucOcVrFfr2D> https://github.com/bitcoin/bitcoin/blob/v0.17.0rc3/test/functional/data/rpc_psbt.json#L90 the PSBT contains a PartiallySignedInput where RedeemScript=[OP_2, 029583bf39ae0a609747ad199addd634fa6108559d6c5cd39b4c2183f1ab96e07f, 02dab61ff49a14db6a7d02b0cd1fbb78fc4b18312b5b4e54dae4dba2fbfef536d7, OP_2, OP_CHECKMULTISIGVERIFY] and the ScriptPubKey=[OP_HASH160, 0fb9463421696b82c833af241c78c17ddbde4934, OP_EQUAL]
< EucOcVrFfr2D> Now, HASH160(redeemScript) != 0fb9463421696b82c833af241c78c17ddbde4934 , but interestingly if i change the OP_CHECKMULTISIGVERIFY into OP_CHECKMULTISIG ... the hash is correct!
< EucOcVrFfr2D> The expected result in that test is equal to the input, the author @achow101 wanted to make sure bitcoind doesn't 'crash' on that scenario but it silently moves on. The scenario is when we're trying to sign a PSBT input but one of the requirement fails -> https://github.com/bitcoin/bips/blame/master/bip-0174.mediawiki#L342
< EucOcVrFfr2D> It's still a bit confusing why bitcoind does not fail when calling 'walletprocesspsbt' with such an ill-formed PartiallySignedInput.
< EucOcVrFfr2D> /fin
< EucOcVrFfr2D> any clue/help/feedback/comment highly appreciated :)
< achow101> EucOcVrFfr2D: the not failing is to aid with a future RPC analyzepsbt which tells you what is wrong with your psbt
< EucOcVrFfr2D> achow101: Oh good to know!
< EucOcVrFfr2D> achow101: By the way, i found a duplicate test for the SIGNER role (still in functional/data/rpc_psbt.json), i opened a PR to remove it https://github.com/bitcoin/bitcoin/pull/14199
< BlueMatt> wumpus: there's a helluvalot more in rust-bitcoin than just primitives, but, ok, it *includes* primitives :p
< wumpus> BlueMatt: yes 'building blocks', I didn't mean 'primitives' in any insulting sense
< wumpus> I just meant it's not intended to be a self-contained node implementation but as a library to use from other software
< jnewbery> What do people think about having bitcoind/-qt not create a default wallet on startup if one doesn't already exist? That was one of my motivations for #13059.
< gribble> https://github.com/bitcoin/bitcoin/issues/13059 | Dynamic wallet load / create / unload · Issue #13059 · bitcoin/bitcoin · GitHub
< jnewbery> The idea is that a fresh start-up of bitcoind/qt wuoldn't crete a wallet until a user tried to carry out a task that required wallet, at which point it would prompt them to create.
< sipa> jnewbery: that sounds great to me
< jnewbery> creating the wallet at run time instead of startup allows the user to specify wallet features like disableprivatekeys (#9662) and avoidreuse (#13756)
< gribble> https://github.com/bitcoin/bitcoin/issues/9662 | Add createwallet "disableprivatekeys" option: a sane mode for watchonly-wallets by jonasschnelli · Pull Request #9662 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/13756 | wallet: -avoidreuse feature for improved privacy by kallewoof · Pull Request #13756 · bitcoin/bitcoin · GitHub
< gmaxwell> jnewbery: been recommended several times before. achow tried to do it, prior to multwallet and there were some dumb roadblocks for running without a wallet and loading it later that should be gone.
< jnewbery> I think that #13100 is really the last piece in the puzzle before we can actually do this
< gribble> https://github.com/bitcoin/bitcoin/issues/13100 | gui: Add dynamic wallets support by promag · Pull Request #13100 · bitcoin/bitcoin · GitHub
< gmaxwell> It'll also allow a wallet to be 'born encrypted' and not end up with a bunch of stupid never used retired keys in it from before encryption.
< gmaxwell> jnewbery: 'promt to create' -- why prompt?
< sipa> gmaxwell: if you want it born encrypted you at least need to prompt for a password
< sipa> but it could also prompt for things like do you have a hw wallet you want integrated
< jnewbery> gmaxwell: I guess maybe that's the wrong wording. The user would need to create a wallet before doing any wallet activities
< gmaxwell> sipa: no, you just choose 'encrypt wallet'.. and that prompts for the password and creates it.
< andytoshi> gmaxwell: if you're restoring a wallet and forget to load the old wallet (say), having Core silently create a new one is annoying and potentially confusing
< gmaxwell> andytoshi: okay, fair enough but the effect of that is that RPCs that currently wont ever fail gain a new failure condition (no wallet yet)
< jnewbery> Those RPCs do fail when no wallet is loaded (eg start with -nowallet)
< gmaxwell> Do they even show up in the rpc list when no wallet is loaded?
< andytoshi> that's frustrating from an API-stability standpoint but in some contexts "succeeding" with a wallet the user didn't intend to create is even worse
< gmaxwell> I'm convinced. I'd just imagined it another way.
< gmaxwell> As soon as you want to use the oppturnity to take more settings then obviously you need to run an explicit create command.
< andytoshi> it may be worth adding a command-line option to automatically create a wallet, for people with automated systems (eg integration tests) that depend on the old behaviour
< andytoshi> or not, those systems can just add an extra "createwallet" RPC call to their scripts
< * andytoshi> stops bikeshedding
< jnewbery> when no wallet is loaded, the wallet RPCs don't show up in the help list. I think that's probably not what we want
< jnewbery> Actually, only the wallet 'management' methods are shown: `createwallet`, `loadwallet`, `unloadwallet` and `listwallets`
< jnewbery> I think that's ok
< wumpus> I think that's fine
< wumpus> if the wallet module is not loaded, wallet RPCs don't show up
< wumpus> you can't use them so why would they show up
< wumpus> just like if you build with --disable-wallet, the wallet options shouldn't show up
< wumpus> (although, yeah, if you build with wallet but have no wallet loaded, there's no wallet to apply the wallet calls to, that's kind of a strange case)
< wumpus> (I think that behavior was inherited from running with -disable-wallet option)
< wumpus> at run time, not compile time
< sipa> well bitcoind can't prompt for anything
< sipa> so either bitcoind or "bitcoin-qt -server" still need to automatically create a wallet
< sipa> or they should require configuring one
< wumpus> or-pass a password to the creation call
< jnewbery> wumpus: there's a slight distinction here. If the wallet *component* is not compiled in, then calls to wallet RPCs fail with "method not found". If the wallet component is compiled in, but no wallet is loaded (either with -nowallet at startup or unloadwallet RPC), then calls to wallet RPCs fail with "Method not found (wallet method is disabled because no wallet is loaded)"
< wumpus> jnewbery: yes you're right
< wumpus> jnewbery: run-time -disablewallet is not really a thing anymore, it just means "0 wallets loaded"
< jnewbery> oh, I forgot about that option
< wumpus> the user could always decide to load a wallet later
< wumpus> or, create one
< jnewbery> That would fail with "method not found"
< jnewbery> (same as if wallet had not been compiled in)
< wumpus> oh it does?
< wumpus> I thought that simply meant "load no wallets"
< jnewbery> yes - -disablewallet means "run without the wallet component" -nowallet means "run with the wallet component but no wallet loaded at startup"
< wumpus> I'm confused as usual
< jnewbery> it's confusing because until multiwallet was implemented, -nowallet didn't make sense
< wumpus> right
< wumpus> I didn't know those things were seperate
< wumpus> it makes sense, though
< jnewbery> -nowallet is kind of a hidden feature. It's not documented and relies on the automatic `-no<option>` conversion.
< wumpus> disablewallet is really "I have no wallet and don't want any to be created either"
< wumpus> would make sense to document that one, unless it becomes the default :)
< wumpus> well... I don't know, you'd still want to load the default wallet by default
< wumpus> if it exists
< jnewbery> Yes, -disablewallet really does completely disable the wallet component (eg, the RPC methods are not registered). It's documented.
< jnewbery> yeah, it's slightly different from making -nowallet default. Like you say, if the wallet already exists, then it should be loaded. The proposed change is just if the wallet does not exist in the wallets directory.
< wumpus> I like that idea, to make creation of the wallet a separate step
< jnewbery> Great. Thanks all for the input.
< wumpus> in the GUI it could be some user friendly wizard that runs by default, in bitcoind it'd have to be a commmand, but it's no different from RDBM software where there's no database by default
< wumpus> this call might have a lot of optional arguments
< wumpus> (though most of the options can be changed later, setting the encryption on first run makes sense, to avoid creating and discarding a keypool)
< jnewbery> Yes, I much prefer an RPC with lots of optional arguments (that can be omitted when using named args) to having a multitude of command-line arguments when calling bitcoind
< wumpus> absolutely
< gmaxwell> 10:48:20 < sipa> so either bitcoind or "bitcoin-qt -server" still need to automatically create a wallet
< gmaxwell> please no
< gmaxwell> just make the wallet needing rpcs fail until one is created.
< wumpus> yes, like -nowallet
< jnewbery> yes, that's the plan
< gmaxwell> Right now I have something like 50 wallet files, 99% of them are just never used dummy wallets. Where I managed to run bitcoind without a wallet, it created one.. then later I wasn't _sure_ if that wallet was ever actually used or not, so I moved it out of the way and backed it up.
< gmaxwell> if not creating by default were a QT only feature, it wouldn't help me at all.
< wumpus> agree gmaxwell
< jnewbery> sounds like #13926 would also be helpful for you
< gribble> https://github.com/bitcoin/bitcoin/issues/13926 | [Tools] bitcoin-wallet-tool by jnewbery · Pull Request #13926 · bitcoin/bitcoin · GitHub
< jnewbery> </shill>
< wumpus> I'm very careful to *usually* build bitcoind without wallet support when I don't need a wallet, or at least run with disablewallet, still I have the same problem in lesser magnitude
< gmaxwell> jnewbery: I don't think anything helps here, no tool could tell if I ever yanked a key out of a wallet and used the key for something.
< jnewbery> oh, I can't help you with that
< gmaxwell> yea, I normally run with disablewallet, but it turns out that if I do 1000 runs, in 45 of them I manage to fail to provide the argument. :)
< wumpus> you mean, yank a key from the keypool without marking it as used?
< wumpus> right
< gmaxwell> I could have gotten keys out from a plaintext dump, unfortunately. So 'used' isn't sufficient.
< phantomcircuit> gmaxwell, yeah i too have a bajillion wallets
< phantomcircuit> 99.9999% of which have never been used
< phantomcircuit> MarcoFalke, updated #14147 to fix that issue and some of the style violations
< gribble> https://github.com/bitcoin/bitcoin/issues/14147 | net: Refactor ThreadSocketHandler by pstratem · Pull Request #14147 · bitcoin/bitcoin · GitHubAsset 1Asset 1
< MarcoFalke> phantomcircuit: thx looks good now
< ken2812221> MarcoFalke: Is #14007 ready for merge or it needs more ACK?
< gribble> https://github.com/bitcoin/bitcoin/issues/14007 | tests: Run functional test on Windows and enable it on Appveyor by ken2812221 · Pull Request #14007 · bitcoin/bitcoin · GitHub
< MarcoFalke> Id like to have someon else review the test changes as well
< ken2812221> Fine. But it seems bitcoinack shows 0 ack on that PR. It has 2 ack for sure.
< wumpus> it is a bit fiddly with spelling of acks sometimes
< phantomcircuit> can i use #define for poll / select er selection?
< gmaxwell> I would expect you to have a define and a configure thing that detects the availablity of poll.
< gmaxwell> ideally copied out of some other project that does both and has been around for a long time.
< gmaxwell> maybe squid cache
< phantomcircuit> gmaxwell, it's kind of awkward cause i want that... but not on windows
< jarthur> libevent uses defines to figure out what's available, final decision on what to use made at runtime. Python does something similar.
< phantomcircuit> jarthur, good info
< phantomcircuit> right now i have have an #ifndef WIN32 #define HAVE_POLL
< phantomcircuit> but that's obviously not exactly right..