< sipa>
jnewbery: (can't comment on github right now) i intentionally do not want to print private keys in error messages; they may be logged in places where they shouldn't
< promag>
jnewbery: #13100 branch locally has a pile of changes, maybe I should cut down to the minimum and improve on follow ups
< gwillen>
I ran into an interesting wallet issue with multisig when trying to demo my offline signing work that's still ongoing
< gwillen>
which is that (1) there's no way to stop a wallet that I intend to use as only-watchonly from generating keys, and (2) spending from a multisig automatically sends change to those keys I didn't actually want generated
< gwillen>
I suspect that RPC users of multisig are already aware of this footgun and handle change manually, and I am planning to adopt "manual change address" as a stopgap for GUI use
< gwillen>
until we have descriptors which should hopefully solve this
< gwillen>
but I'm going to push again for the idea that a given wallet ought to be either only-watchonly or non-watchonly
< gwillen>
and that mixing a local keypool into a transaction sending from not the local keypool is scary
< gwillen>
although I guess I asked for this kind of issue by tricking / cajoling the wallet into creating a transaction spending from a "solvable" but not "spendable" coin
< gwillen>
because what else should it do
< sipa>
gwillen: there is a recently introduced feature to make a wallet in no-private-keys mode
< gwillen>
ooh, beautiful, that sounds perfect for preventing this
< gwillen>
do you happen to know, if the wallet is called on to create change in that mode, what will it do? Just give up?
< gwillen>
(and how do I engage this mode)
< sipa>
i don't know; i'm currebtly flying, it's a bit hard to check :)
< gwillen>
(and does it make sense that I imagine descriptors will solve this, since if I'm spending from a given descriptor, that same descriptor or a related descriptor should be used to derive where change goes?)
< gwillen>
np, I'm about to head to dinner anyway, just wanted to start thinking about it
< sipa>
gwillen: no, you'll just have 2 specially designated descriptors in a wallet; one where receive addresses are drawn from, and one where change is drawn from
< sipa>
so yes, that's a solution by making change selection explicit and separate from the wallet's keys
< gwillen>
ok great, thanks for validating my hope that all my problems will be solved in the future
< gwillen>
I know I said that I didn't want to wait for descriptors, but actually the change problem means this is going to kind of suck without them
< gwillen>
it's going to have to be either manual change or address reuse
< gwillen>
do descriptors-as-envisioned have support for HD + multisig in some fashion?
< sipa>
ure
< sipa>
sure
< gwillen>
I assume it would be something like "2 of 3 of keys derived from X/Y/Z" and it automatically increments all three together, or something, when generating a new address?
< gwillen>
(the question also came up in conversation today of whether there is a good way to support multiple people generating addresses from the same keys e.g. the holders of a multisig, and not accidentally generating the same address)
< sipa>
yes... have you ever read the descriptors description (no pun intended)?
< sipa>
doc/descriptors.md in the repo
< gwillen>
(e.g. by letting them generate from different subpaths)
< gwillen>
ha, it's been awhile, I will reread, thanks
< gwillen>
ah yes, I see it specifically addresses this, cool
< sipa>
you can just say mukti(2,xpubpath/*,xpub/path/*,xpub/path/*) and it will expand to equal-last-index entries from the xpubs
< sipa>
*multi
< gwillen>
*nods*
< gwillen>
and I guess supporting multiple derivation paths would be easiest by just using multiple descriptors
< gwillen>
e.g. I have a wallet that has "my" descriptors that it uses to generate addresses, and "your" descriptors that it only uses to watch the chain, and it treats them both as "mine"
< sipa>
right
< gwillen>
(this is purely to prevent collisions if we're independently generating receive addresses to the same wallet)
< achow101>
gwillen: createwallet has a disable private keys parameter so you can make a wallet without any privkeys
< gwillen>
achow101: thanks, I hadn't been using createwallet, I was just adding --wallet= and it created it for me
< meshcollider>
Is there a meeting tomorrow or not due to Christmas?
< meshcollider>
Today* for some of you in incorrect timezones
< sipa>
meetings are cancelled for the holidays i think
< meshcollider>
Ok
< provoostenator>
I still don't like that -wallet automatically creates a new wallet.
< promag>
provoostenator: why?
< provoostenator>
Because of the confusion gwillen described, but also because a typo leads to a new wallet.
< provoostenator>
E.g. forgetting to add ".dat"
< promag>
the same typo can happen in createwallet rpc?
< provoostenator>
The problem is when you're trying to open an existing wallet and instead it creates a new one.
< promag>
ah
< provoostenator>
The behavior made sense before we had dynamic wallet loading support, but I think it could be deprecated now, e.g. initially with a warning "In the future, use createwallet to create a new wallet"
< promag>
provoostenator: another alternative is to create wallet if -wallet is specified on the command line
< promag>
-wallet on the config only loads?
< promag>
otoh, if you want to launch a container and create a wallet xxx how would you do it?
< promag>
for instance, postgres docker image has an entrypoint that initializes the database on the first run, and in that script it waits for the server to be available to execute the init sql
< promag>
deprecate -wallet and add both -createwallet and -loadwallet
< provoostenator>
Getting 13100 in makes sense for the GUI, but creating new wallets is "advanced" enough in my opinion that we can ask users to use the RPC console for that. But let's just get it merged.
< provoostenator>
Not a fan of making commandline behavior different from config file.
< provoostenator>
Interesting point about doing things on first.
< provoostenator>
lad
< provoostenator>
load
< provoostenator>
That's a genertic problem though, most RPC operations don't immediately work. We could add a "wait_for_rpc_to_wake_up" ish call.
< provoostenator>
The last time I had to deal with that issue I just polled until it stopped throwing errors :-)
< gmaxwell>
we have a longstanding request to only create wallets when either told to encrypt OR when asked for an address.
< gmaxwell>
As this would fix the problem of an unencrypted wallet being created first and ending up with a bunch of unused retired keys in it when you encrypt.
< gmaxwell>
also solves the problem where the user starts up, backs up their wallet, then encrypts .... and doesn't backup again, and their backup is no good. (solves it because there wouldn't be a wallet file to backup at that point)
< provoostenator>
There's that issue too. It could be solved by adding a password argument to createwallet.
< provoostenator>
However that doesn't handle the default wallet case.
< gmaxwell>
there shouldn't be a default wallet, is what I'm saying
< gmaxwell>
one should get created on demand.
< gmaxwell>
We tried to make the change previously but prior to dynamic wallet support it wasn't easy to implement, it probably is now.
< provoostenator>
Ah yes, then the entire problem goes away. RPC methods that expect a wallet would just throw "create wallet first", and the GUI would have to be smart enough to create a wallet just in time.
< gmaxwell>
Right.
< gmaxwell>
and it would also improve the problem that developers get where you end up with 50 wallet files and aren't sure which of them you can throw away. :P
< provoostenator>
That indeed seems doable once 13100 is in. promag: when rebase? :-)
< provoostenator>
promag: awesome. I'm fine with the idea of splitting the prerequisite stuff out of it, but I also think the PR is small enough to review as a whole.
< provoostenator>
However, splitting some stuff out keeps the UI bikeshedding seperate.
< gmaxwell>
sipa: ack on openssl.
< promag>
btw, I think all wallets commands should have the passphrase argument
< promag>
i mean, why would a wallet be unencrypted for others for a short period of time?
< wumpus>
agree
< wumpus>
making it possible to have wallet 'born encrypted' was one of the goals of multiwallet
< wumpus>
oh, you mean actually *passing the passphrase argument* for unlocking for every command... neutral on that, I guess with named arguments it's somewhat less of a hassle now than before. I think there's something to be said for the current time-locked functionalit as well as it means not all wallet-using commands have to know the passphrase.
< wumpus>
now it could be some separate process that unlocks periodically, or when necessary; imagine the client code having to pass through the passphrase everywhere
< gmaxwell>
I think it would be interesting to support, but it would not be very usable to use that way normally.
< promag>
wumpus: gmaxwell: I think we could support unlock per connection only
< promag>
so the connection would hold the wallet lock and so no other connections could interact with that wallet
< wumpus>
connection specific state really breaks the http abstraction
< promag>
meaning that the wallet could stay unencrypted
< wumpus>
I also don't see why multiple connections wouldn't be allowed to interact with a wallet, this is something for the external software to coordinate!
< promag>
wumpus: for rest yes, but does that hold for rpc?
< promag>
wumpus: and then there is also batch
< wumpus>
I know, but I don't really like it
< wumpus>
a) connections time out, so there'd need to be some hack to keep them open to prevent locking the wallet b) I remember convention is that a connection is closed when there is *any* http error
< wumpus>
for another RPC protocol it might be acceptable, but http just isn't suited to what you want to do
< wumpus>
bolting it on in a bitcoin specific way seems a bad idea
< promag>
I just think walletpassphrase is bad
< wumpus>
client libraries likely won't have fine-grained control what to send over one conection
< promag>
it's like doing sudo xxx and then everyone on the system has sudo
< wumpus>
well RPC supports multiple users now
< gmaxwell>
promag: being able to send a passphrase that applies to a single call, and only that call, wouldn't have that effect.
< wumpus>
could use that if you want to restrict the scope of an unlock...
< wumpus>
that at least is completely compatible with how authentication is used normall...
< promag>
we could send the passphrase as a query parameter (in the URL)?
< wumpus>
though I don't really like more and more 'business rule' kind of functionality leaking into bitcoin core, it reminds me a bit of the account stuff, it's so specific on your applciation
< wumpus>
nooOOOO
< wumpus>
never embed passwords in URLs
< wumpus>
never
< gmaxwell>
Whats wrong with making walletpassphrase an automatic named argument to _every_ rpc, and if the rpc can make use of the passprase, they use it... without unlocking.
< gmaxwell>
so then you can authenticate single sends without opening anything up.
< gmaxwell>
and without any kind of crazy connection-session state.
< wumpus>
yes
< promag>
gmaxwell: just fine!
< promag>
so we would add customer support to each RPC
< wumpus>
as said I'm neutral on that
< gmaxwell>
I think probably I would use that... provide the phassphrase on signrawtransaction/etc. and not otherwise.
< promag>
s/customer/custom
< gmaxwell>
promag: well I think the way we'd do that is with a utility function for rpcs that need an unlock, that checks if its already unlocked or otherwise gets access to it via the passphrase in the argument.
< gmaxwell>
(and otherwise fails)
< promag>
ok
< promag>
so concept ACK?
< gmaxwell>
there are only a couple RPCs that can make use of unlocking (the send, sign, dump ones).
< wumpus>
you should ask the wallet maintainer :)
< promag>
this way no one would be able to "steal" from a wallet in that short period of time
< wumpus>
agree
< promag>
meshcollider: ^
< gmaxwell>
promag: yes, it would reduce exposure slightly... though god knows we probably have an RCE burried in some RPC. :P
< promag>
RCE?
< gmaxwell>
remote code execution (bug).
< promag>
huh, do you even consider that?
< gmaxwell>
promag: I mean the addition of this would not change that I would recommend against letting the rpc be used by less trusted parties.
< wumpus>
well you're afraid of local users, *with* credentials, and access to the RPC, stealing funds; then you're regarding the RPC as a hardened interface
< promag>
oh right
< gmaxwell>
The thing I like about the passphrase argument approach is that it's also quite compatible with cli usage... so it will be used, not just some vague enterprise feature which may or may not get used and we won't get feedback about it. :)
< promag>
I just think walletpassphrase as a global switch is a bad concept
< gmaxwell>
promag: also txfee is another 'global' thing which could be scoped this same way.
< promag>
agree
< gmaxwell>
I don't believe there are any others re wallet behavior?
< gmaxwell>
FWIW, GUI will also prompt on demand and not leave things unlocked.
< promag>
good point
< gmaxwell>
promag: the main utility of the time based unlock is this use case: You have an online hot wallet, you want if the daemon gets shut off that you have to log in and unlock it again yourself.
< gmaxwell>
promag: it turns out that in a significant percentage of commercial system compromises we've heard reported (maybe a majority), someone gets remote access but causes a reboot in the process. (e.g. by getting into a server admin console, or convincing remote hands to change the root password).
< wumpus>
it allows for some separateion of concerns; in any case your idea wouldn't replace it
< gmaxwell>
So several big users have had their bacon saved by that setup.
< gmaxwell>
but its okay we could continue to support both ways of using it.
< wumpus>
I'm ok with the pass-a-passphrase as an option, but not deprecating the old behavior
< promag>
I see
< gmaxwell>
(thats why I'm explaining that usage)
< wumpus>
it also feels like changing things just to change them, just because you don't like it, all users have to change their usage
< gmaxwell>
In an ideal world, I think it would even be best if that usage had two stages of authorization--- a startup key and a usage key, but I think we shouldn't worry about that for now and leave improvements like that to external key handling processes.
< promag>
one time usage key yes
< promag>
I'm working on #13100 and then maybe I'll try adding the passphrase to some RPC
< gmaxwell>
FWIW, the way I often recommend people use walletpassphrase is with a really dumb key (say like "password") that they could never forget... just so they can feel more confident using their wallet without worrying that they'll accidentally send funds somewhere.
< jnewbery>
sipa: ACK not printing private keys in error messages. You're right that it's a terrible idea.
< jnewbery>
More generally, my review comments about improving error messages were mostly meant as notes for me or anyone else who wants to do a follow-up PR to improve error messages and testing. I don't think they need to change in your PR.
< bitcoin-git>
[bitcoin] jnewbery opened pull request #15010: [wallet] Fix getbalance with minconf (master...fix_getbalance_with_minconf) https://github.com/bitcoin/bitcoin/pull/15010
< provoostenator>
Tangential to adding wallet password to RPC calls, I'd like a way to enter said password in a way that doesn't end up in bash history (there might already be a PR for that).
< provoostenator>
That would be a change at the bitcoin-cli level.
< timothy>
provoostenator: it's not a problem of bitcoin-cli, you cannot disable history in called command
< timothy>
you can do: unset HISTFILE before using a command that includes the password
< timothy>
(bitcoin-cli already have -stdin)
< wumpus>
yes, private keys in error messages is almost as bad as private keys in the log (as the client application will most likely log them)
< provoostenator>
timothy: bitcoin-cli could prompt for a password rather than expecting it as an argument
< timothy>
provoostenator: you can pass it by using -stdin option
< gmaxwell>
provoostenator: you can tell bash to not save any line that begins with a space or has certian strings in it.
< gmaxwell>
(or use -stdin)
< provoostenator>
I didn't know about stdin. That's useful indeed, might be worth mentioning in the walletpassphrase RPC help.
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #14932: ui: Warn the user on file corruption in mempool.dat (master...Mf1812-testFileCorruption) https://github.com/bitcoin/bitcoin/pull/14932
< gmaxwell>
I thought the meetings were all canceled.
< gmaxwell>
like every meeting everwhere. (is this why US markets are at a 52 week low? :P you'd think the end of meetings and their overheads would be a boon to the GDP...)
< moneyball>
ah indeed, i see sipa mentioned that now.
< sipa>
i doubt i'll be awake in 1.5h in any case
< achow101>
gmaxwell: I'm not sure that creating the wallet on use is possible even with multiwallet. however getting rid of the default wallet and forcing the user to just make a wallet themselves certainly is
< achow101>
I'm planning on implementing the latter, as the former looks to be fairly complicated and fragile
< sipa>
achow101, gmaxwell: one possibility is creating a dummy CWallet object for freshly created wallets initially, without file to back it, and the only materializing it on firet use
< Csus>
Hi - I'm trying to setup a full node on Raspberry pi. ./configure CPPFLAGS="-I/usr/local/BerkeleyDB.5.3/include -O2" LDFLAGS="-L/usr/local/BerkeleyDB.5.3/lib" --enable-upnp-default
< Csus>
After inserting this command I get error - "configure: error: C++ compiler cannot create executables"
< Csus>
Any ideas?
< wumpus>
Csus: so, can your c++ compiler create executables? does g++ work?
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Dec 20 19:00:10 2018 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< jonasschnelli>
sipa: for the CNetMessage refactoring, instead of using polymorphism (virtual base class), would you suggest having a global pointer to a instance of something we could call CNetMessageSerializer?
< jonasschnelli>
So move all the ser/deser code there?
< jonasschnelli>
So there could be two serialization instance (legacy p2p and BIP151 p2p) where each node would hold a shared pointer to that instance?
< sipa>
jonasschnelli: well each connection only needs one
< jonasschnelli>
sipa: Yes. But a pointer to a global instance seems okay, right?
< sipa>
and if there is no local state in the deserializer you don't need to instantiate it
< jonasschnelli>
Yes
< sipa>
so no pointers
< sipa>
it depends
< jonasschnelli>
but somewhere you need to keep record of the position you have read the message up to
< jonasschnelli>
so CNetMessage gets do then still contain some minimal serialization state
< jonasschnelli>
Where CNetMessage is more or less a serialisation helper
< jonasschnelli>
so I'm not yet entirely convinced of the split between CNetMessage and CNetMessageSerializer
< sipa>
i'm just saying there is no need to make each CNetMessage know what type of serialization is used
< sipa>
it's a per node thing, not per message
< jonasschnelli>
Yes. I agree with that
< sipa>
it's bith a waste of memory and cpu to push it down so deep
< jonasschnelli>
I think the initial design came from the two step BIP151 handshare where v1 protocol was required for the initial messages
< sipa>
i also don't understand the global pointer question
< jonasschnelli>
*handshake
< sipa>
that still doesn't require every message to know how it was serialized
< sipa>
either the serializer classes hold state, and you'll want one object per node
< sipa>
or it doesn't, and there is no need fo instantiate them at all
< jonasschnelli>
sipa: so each node has either v1 or v2 serialization, each CNode could have a shared pointer to v1 or v2 global serializer instance
< jonasschnelli>
but since there is no need for instantisation, I'm unsure
< sipa>
sure, but why?
< sipa>
maybe all you need is a function to serialize/deserialize that takes an enum for the type of serialization
< jonasschnelli>
i see,.. yes
< sipa>
but if there is a concern we may need state, sure a per-node instance sounds good
< jonasschnelli>
I guess we don't need state on that level
< sipa>
i don't know
< sipa>
it can hold partial checksum data
< jonasschnelli>
sipa: you mean the SHA256 or poly1305 state?
< sipa>
yes
< meshcollider>
promag: 2:10 AM <gmaxwell> Whats wrong with making walletpassphrase an automatic named argument to _every_ rpc, and if the rpc can make use of the passprase, they use it... without unlocking.