< bitcoin-git>
bitcoin/master fdf82ba Graham Krizek: Update all subprocess.check_output functions in CI scripts to be Python 3.4 compatible...
< bitcoin-git>
bitcoin/master 73a6bac Jonas Schnelli: Merge #15196: [test]: Update all subprocess.check_output functions to be Python 3.4 compatible...
< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #15196: [test]: Update all subprocess.check_output functions to be Python 3.4 compatible (master...cron-ci-fix) https://github.com/bitcoin/bitcoin/pull/15196
< gkrizek>
Thanks jonasschnelli
< jonasschnelli>
gkrizek: thanks for the PR
< * luke-jr>
realises the gitian static binaries don't actually work on non-Ubuntu systems -.-
< luke-jr>
bitcoin-0.17.1/bin/bitcoin-qt: relocation error: bitcoin-0.17.1/bin/bitcoin-qt: symbol __chk_fail, version GLIBC_2.3.4 not defined in file libc.so.6 with link time reference
< bitcoin-git>
bitcoin/master 5a5ea93 David A. Harding: Doc: add information about security to the JSON-RPC doc
< bitcoin-git>
bitcoin/master 5eb32d2 Wladimir J. van der Laan: Merge #15223: Doc: add information about security to the JSON-RPC doc...
< bitcoin-git>
[bitcoin] laanwj closed pull request #15223: Doc: add information about security to the JSON-RPC doc (master...2019-01-rpc-security) https://github.com/bitcoin/bitcoin/pull/15223
< promag>
achow101: I'm getting "libc++abi.dylib: terminating with uncaught exception of type std::__1::system_error: mutex lock failed: Invalid argument" in #15225 on startup
< gribble>
https://github.com/bitcoin/bitcoin/issues/15225 | GUI: Change the receive button to respond to keypool state changing by achow101 · Pull Request #15225 · bitcoin/bitcoin · GitHub
< promag>
wumpus: I think that's the case when the commit is garbage
< promag>
and is no longer available
< promag>
there's also the case when 7chars is not enough
< promag>
enough because there's multiple commits with the same 7 initial chars
< wumpus>
promag: I'm suddenly seeing it a lot though, did everyone suddenly start mispasting commit ids?
< wumpus>
of course I know that a garbage commit ID won't be turned into a link
< promag>
idk, it must be a gh problem
< bitcoin-git>
[bitcoin] instagibbs opened pull request #15244: gdb attaching to process during tests has non-sudo solution (master...gdb_ptrace) https://github.com/bitcoin/bitcoin/pull/15244
< achow101>
meeting?
< jnewbery>
hi
< gleb>
hi
< sipa>
hi
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jan 24 19:00:54 2019 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< gribble>
https://github.com/bitcoin/bitcoin/issues/11082 | Add new bitcoin_rw.conf file that is used for settings modified by this software itself by luke-jr · Pull Request #11082 · bitcoin/bitcoin · GitHub
< gribble>
https://github.com/bitcoin/bitcoin/issues/15225 | GUI: Change the receive button to respond to keypool state changing by achow101 · Pull Request #15225 · bitcoin/bitcoin · GitHub
< promag>
regarding open wallet menu - there are concerns regarding blocking GUI - is this something to avoid or can it be improved in 0.18.1?
< achow101>
gwillen: oh yeah, I forgot to update that
< promag>
wumpus: :(
< gwillen>
achow101: lmk if there's any way I can help!
< wumpus>
promag: I don't mean that negatively! if there's issues there's issues no matter *when* you find them, just mean it postpones the merge if there's new review comments
< wumpus>
ok added #15225 #15118
< gribble>
https://github.com/bitcoin/bitcoin/issues/15225 | GUI: Change the receive button to respond to keypool state changing by achow101 · Pull Request #15225 · bitcoin/bitcoin · GitHub
< wumpus>
#topic Globals and initialization order (sipa)
< sipa>
hi!
< sipa>
we currently have a bit of a mix in the codebase for dealing with initialization order of globals
< sipa>
some things are explicitly initialized using init functions, called from main/test startup
< sipa>
some things are initialized just using global initializers
< sipa>
and some things are using once/init-on-first-use-block-scoped-statics
< sipa>
and mixing them is pretty fragile
< wumpus>
I prefer explicit initializer functions unless it's simply setting a value, at least it's perfectly clear what the order is in which things get initialized
< wumpus>
which is very important if things depend on each other
< wumpus>
also things running before main() is quite annoying for debugging
< sipa>
the problem is that with explicit initializer functions, things don't work when called from global initializer
< wumpus>
(and things that take significant time to run certainly shouldn't be called from global initializers, as they'll delay showing even, say, the help message, which doesn't need any initialization at all)
< sipa>
and i think we actually had a long-standing problem with the RNG, which was used possibly before being initialized (since #14955 it uses an init-on-first use construction, which should always be fine)
< wumpus>
anyhow that's my opinion on it, I'm aware the codebase is quite crazy in that regard, we've had initialation order issues since the first release...
< promag>
sipa: what's your preference?
< sipa>
so i'm getting more and more in favor of using this init-on-first-use construction in more places
< wumpus>
meh
< sipa>
since it's compatible with everything
< wumpus>
it's hard to reason about
< jimpo>
I also like explicit initialization of non-trivial things
< wumpus>
accidentally using something before something else will suddenly change the initialization order
< wumpus>
instead of just fail
< wumpus>
we've seen that with logging, for example
< sipa>
that's because logging using a global initialized
< provoostenator>
I'm in favor of having fewer globals :-) But otherwise haven't really developed a preference in C++
< promag>
I prefer explicit order initialization
< wumpus>
initializing on first use also doesn't really regard initialization dependencies
< luke-jr>
wumpus: in a bad way? the only time I can think of order mattering is when we're pre-config-files-loaded
< sipa>
wumpus: how so?
< wumpus>
like "we need to have the datadirectory set before writing to it"
< luke-jr>
(thinking RNG specifically)
< sipa>
oh, i'm not really talking about application level things
< sipa>
more things like RNG, logging objects (not actually setting the logfile, which happens later), libsecp, ...
< sipa>
syncronization debugging state
< wumpus>
if it's really only setting a value to a data structure I agree it's different, but if there's extensive work that might depend on other or OS things, it gets hairy fast
< wumpus>
only allocating a data structure on first use is fine...
< gmaxwell>
It's also important to avoid undefined behavior over and above just avoiding doing the wrong thing.
< sipa>
basically the RNG right now can't use the SHA module, because the RNG is invoked from global constructors (and it works fine with it), and SHA needs explicit initialzation
< wumpus>
but I think we agree that global constructors are bad
< wumpus>
that's one thing :)
< sipa>
so i think there are two solutions to that... avoid global constructors everywhere, or make everything work fine on first use
< wumpus>
(I here mean global constructors as 'runs before main', not the static initializers that run on first use)
< sipa>
wumpus: right
< sipa>
wumpus: so what's your opinion on solving this particular issue?
< gmaxwell>
Or make SHA module's autodetect get resolved by the linker, using the GCC extension that does that. :P
< gmaxwell>
(doesn't address the general question about dependencies between global constructors)
< wumpus>
sipa: so move to initialization on first use or explicit initialization, whatever makes sense in the case, move away from global initializers that do anything more significant than assigning a constant value
< sipa>
we can get rid of all globals whose construction needs randomness, but making the SHA256 code autodetect on first use seems a simpler change
< wumpus>
I really like how you got rid of CInit, for example
< gmaxwell>
The downside of autodetect on first use is that it would make every call to sha256 slightly slower. :(
< gmaxwell>
One way to compensate for that would be make sure that there is a batch sha256 function that does many of them and only does the detection once, and be sure to use it where possible.
< sipa>
gmaxwell: hmm?
< sipa>
i benchmarked it as a 1.8ns slowdown here to use an on-first-use construction
< gmaxwell>
sipa: I assume 'autodetect on first use' means "Check a synchronized variable every time the function is run".
< sipa>
gmaxwell: nope!
< sipa>
it actually compiles to both a synchronized and unsynchronized variable, and in the initialized case, only the latter is checked
< gmaxwell>
Okay, 1.8ns doesn't sound that terrible. What the runtime of the function on that host?
< wumpus>
nice
< gmaxwell>
sipa: how does it know its initialized?
< wumpus>
though this is a compiler implementation specific thing, clang and gcc might not do it the same way?
< sipa>
gmaxwell: it's just a boolean; when the boolean is false, it means it could be initialized or not (to be checked using synchronized primitives), if it is true, it is guaranteed to be initialized
< sipa>
from cppreference.com:If multiple threads attempt to initialize the same static local variable concurrently, the initialization occurs exactly once (similar behavior can be obtained for arbitrary functions with std::call_once).
< sipa>
Note: usual implementations of this feature use variants of the double-checked locking pattern, which reduces runtime overhead for already-initialized local statics to a single non-atomic boolean comparison.
< wumpus>
good to know
< gmaxwell>
in any case, I think that seems an obviously better way to handle it. Residual performance concerns could be handled by my above batching suggestion (which would be a win regardless because of function call overheads). MOST places where we'd have this concern wouldn't be a big performance bottleneck like sha256 is.
< wumpus>
exactly
< wumpus>
maybe sha256 is just special here
< wumpus>
and we can at least decide for the general case
< sipa>
agree
< sipa>
enough on this topic, i think
< wumpus>
ok!
< gmaxwell>
well I could see the same problem showing up for crc32 and being worse, because 1.8ns would be like a 2x slowdown for it. :P but otherwise I can't come up with much where a 1.8ns slowdown would matter.
< jnewbery>
we'll take care of sourcing residents, paying travel/accommodation/stipend and hosting them in our office
< sipa>
nice!
< provoostenator>
nice!
< jnewbery>
it's 2-3 weeks of seminars followed by 2 months of project work. We're expecting the residents to make really meaningful contributions to Bitcoin Core and other Bitcoin/Lightning projects while they're here
< jnewbery>
I bring this up here because we need help for a couple of things:
< jnewbery>
1. We (Chaincode) will be doing the heavy lifting for the seminar series and hosting, but we need (remote) mentors for the 2 month project period.
< kanzure>
what are the responsibilities of the mentors
< jnewbery>
each resident will be paired with a mentor. We're looking for 1 hour per week video calls with the resident to help guide them in their project
< jnewbery>
obviously chaincoders and their peers will be on hand for incidental questions during the week, and the mentor will be providing overall guidance helping with the project
< meshcollider>
Will the peers be chosen based on areas of knowledge
< instagibbs>
jnewbery, what's the action item here? ping you if interested in mentoring?
< jamesob>
oh don't worry instagibbs, we've already signed you up
< jamesob>
;)
< jnewbery>
We'll try to pair residents with mentors who have overlapping interests obviously
< jnewbery>
instagibbs - I'll be reaching out individually
< instagibbs>
hah
< instagibbs>
ok, so check e-mails DMs
< jnewbery>
2. We're looking for recommendations for residents. If you know anyone who wants to immerse themselves in Bitcoin/Lightning over summer and is excited about making a real contribution to the project, please send them our way
< jnewbery>
Adam Jonas is taking the lead on organizing this one
< jnewbery>
So you can ping him or me if you have any questions
< jnewbery>
that's it!
< wumpus>
ok! thanks for organizing this
< jnewbery>
We're really excited about this one. The longer format means we're expecting to have a lot of great contributions from the residents
< wumpus>
hope so!
< wumpus>
any other topics?
< jnewbery>
one reminder: I'd encourage people to use moneyball's #proposedmeetingtopic to propose meeting topics during the week!
< gmaxwell>
Could I nag for review on #14929 ? it's getting forced to rebase faster than its being reviewed...
< achow101>
does anyone have an opinion on whether encrypting a blank wallet should generate a seed for it?
< luke-jr>
hmm, it might be nice to have all the security-critical stuff generated at once
< luke-jr>
eg, so people who have a no-RNG system can make the wallet easily on one that has a RNG and then backup/restore it
< luke-jr>
but that may be independent of encryption or not
< wumpus>
is it even sensible
< wumpus>
to encrypt a blank wallet? encryption only protects private data, after all
< achow101>
the only thing I can think of is encrypting a blank wallet and then importing privat ekeys
< wumpus>
ah yes
< gmaxwell>
I like the idea that "if you have a wallet file, you can back that up, and your backup is not likely to be surprise invalidated."
< gmaxwell>
under that thinking: wallets should be born encrypted and also born with their master keys.
< gmaxwell>
If you import things, you invalidate backups, sucks to be you. At least the import (docs) can warn you of this.
< gmaxwell>
I dunno if "wallet which is blank except for imported keys" is a case we should care much about supporting.
< gmaxwell>
luke-jr: I'd like us to have wallet loading take the high entropy data from a loaded wallet (e.g. encrypted master key, or first key in non-encrypted wallets) and hash that into the RNG state. That would achieve what you want as a side effect.
< achow101>
since I want blank wallets as a stepping stone to born encrypted wallets, I prefer that encrypting would generate the seed
< gmaxwell>
luke-jr: (similarly, we should also be hashing any rpcauth credentials into the RNG state)
< gmaxwell>
I see.
< achow101>
but other people may want blank wallets for other reasons, which is why I ask
< gmaxwell>
achow101: well I don't think it would be terrible to have blank wallets with no keys in them at all. At least they'd be obviously smaller? Long term though we should try to get it so that under normal workflows if there is a file there, thats the file you should be backing up.
< luke-jr>
achow101: consider that some wallets might never have a seed
< achow101>
luke-jr: we don't make non-hd wallets anymore.
< luke-jr>
achow101: I'm thinking watch-JBOK-only
< achow101>
luke-jr: right. I think that's the only case to not set a seed when encrypting
< luke-jr>
well, watch-only means encryption is meaningless
< luke-jr>
so nevermind that case
< achow101>
oh, i thought you meant importing the privkeys
< luke-jr>
I suppose there might be a use case for that
< achow101>
(that's what we were taling about earlier)
< luke-jr>
but that seems roughly equivalent to non-HD wallets which isn't supported
< gwillen>
I mean, it still makes sense to have support for legacy imported non-HD wallets
< luke-jr>
gwillen: sure
< luke-jr>
but the topic is strictly new wallets AFAIK
< gwillen>
in fact, I think in the multiwallet world, it would make sense to split wallets into like (*) normal HD wallets, (*) watchonly wallets, (*) legacy importonly wallet
< gwillen>
*nods*
< gwillen>
but if you're going to import privkeys then you do want a blank empty wallet with no seed and encryption
< gmaxwell>
I don't think we generally want to support that case so much as to special case it.
< gmaxwell>
Having an extra seed and keys you don't use is harmless.
< gwillen>
well, I'm afraid of it leading to "oopses"
< gmaxwell>
(other than making the wallet bigger, but it's already going to be fairly big if you're importing a bunch of keys)
< luke-jr>
gmaxwell: well, you don't want to accidentally use one of the keys
< gmaxwell>
any manual private key handling already leads to oopses, which is why we already strongly discourage it.
< gwillen>
like, when I was testing my new offline signing stuff, it autogenerated a change address using the wallet seed that I didn't want
< gwillen>
and sent my change to it, oops
< gmaxwell>
And exactly what would an imported keys only wallet do?
< gwillen>
which I realized about 10 minutes before doing a live demo and frantically sent it back ;-)
< gwillen>
I mean, it would let you spend from your legacy keys from your old wallet
< gwillen>
although I guess that's messy if you don't spend all at once
< luke-jr>
gmaxwell: if it has no solution, throw an error
< gwillen>
because you still have to handle change _somehow_
< achow101>
gwillen: that implies address reuse, no?
< gmaxwell>
gwillen: right.
< gmaxwell>
It implies: this is just not a supported case.
< luke-jr>
you might have imported a keypool too
< luke-jr>
not sure if it's possible to mark them as such, but we do allow manually specifying change addresses
< gmaxwell>
If you're going to manually specify change addresses, you don't have gwillen's problem.
< luke-jr>
gmaxwell: if you forget to, it's better to get an exception than potentially lose funds
< achow101>
I think the question is really whether this is a case that happens frequently enough that it is something we should support
< gmaxwell>
There are 1001 other wallet features that deserve love, attention, writing, and review... going through and breaking the design assumption that you can always get a key from a signable wallet just to support a currently unsupported and known risky usage, seems like a really bad idea. Esp when the subject of doing it came up as an excercise in thinking about how some other case is handled,
< gmaxwell>
rather than someone showing up and saying "it should support this and heres why"
< achow101>
to support it requires adding more parameters to encryptwallet and getting a born encrypted wallet needs another step
< luke-jr>
achow101: I can't think of a good reason to support it
< luke-jr>
any case where it *might* make sense can also do a watch-only wallet and provide private keys to signraw as needed I think
< achow101>
agreed
< gmaxwell>
unrelated, it would be kinda neat if there were a backupwallet rpc that made a watching wallet by playing the keypools arbritarily far forward and then writing out a wallet without private keys.
< provoostenator>
gmaxwell: or just use a descriptor to import whatever you want in a watch-only wallet, requires: #14075
< gribble>
https://github.com/bitcoin/bitcoin/issues/14075 | Import watch only pubkeys to the keypool if private keys are disabled by achow101 · Pull Request #14075 · bitcoin/bitcoin · GitHub
< gmaxwell>
Using a single descriptor forces you to use security toxic non-hardened keys. Plus doing that (using the descriptor at all) violates the principle that directly handling keys is risky and fault prone.
< gmaxwell>
Note that descriptors don't have checksums, a simple typo or copy-paste error can result in your funds blasting off into space.
< provoostenator>
achow: as for encrypting an empty wallet, easiest might be to just a bool param where user can decide if a seed is added
< sipa>
gmaxwell: the idea is for descriptor wallets to contained cached pre-expanded public keys
< provoostenator>
gmaxwell: you could use hardened derivation too if you include a private key in the descriptor (which is not saved)
< provoostenator>
(and that currently doesn't work probably)
< sipa>
so you can use hardened bip32 keys as descriptor; they need access to the private key to derive, but not to otherwise use
< gmaxwell>
provoostenator: fair on that point, doesn't address my second point... and that also means you're likely leaving private keys in your shell history.
< gwillen>
hrm, why do descriptors _not_ have checksums?
< gwillen>
given their intended uses it seems like maybe they should
< gmaxwell>
because they weren't intended to be some general construct for transporting keys around?
< sipa>
gwillen: they're human editable
< provoostenator>
gmaxwell: assuming the private keys are on a different device, that device could return an array of descriptors, one for each pubkey, in import format.
< provoostenator>
As for checksums: that sounds solveable.
< gmaxwell>
provoostenator: sweet, then you get a multiplicative blowup in possibilities to corrupt the data and cause funds to go off into space.
< sipa>
gwillen: inside the wallet we could store them with a checksum, or even have a more efficient binary encoding
< gwillen>
*nods*
< sipa>
which would still be human accessible for transport, if they're intended to be treated as a black box
< gmaxwell>
sipa: but its when humans touch them that they're most likely to get corrupted.
< gmaxwell>
(as the now hundreds of millions lost in eth land due to corrupted addresses attest to...)
< gmaxwell>
(humans and application boundaries)
< gmaxwell>
The original descriptor work was largely wallet internal... it's mostly later efforts that have been turning them into a general key import/export thing. This seems ill-advised to me.
< provoostenator>
So we need something to replace xpub that has a better checksum? Or does that still data corruption risk? Because something goes wrong inside of Bitcoin Core during the import?
< gmaxwell>
xpub actually has a checksum, it's sufficient, as it was intended for use as an address.
< provoostenator>
In #14912 I'm using xpubs all the way. The HWI tool (achow101) gets xpub(s) from the device.
< provoostenator>
(though I'm not sure _how_ they are obtained from the device, that's probably worth double checking)
< gmaxwell>
Corruption inside bitcoin is also a concern, but a strictly less serious one than corruption by humans or across application boundaries.
< provoostenator>
If the device spits out a normal public key and the script converts it to an xpub then there's no point.
< achow101>
provoostenator: it depends on the device. some give xpubs, other give pubkeys + chaincode which then become xpubs
< gmaxwell>
For HWI you might want to consider an enrolling process that gets a signed message with one of the imported watch keys, to prove that the device can sign for the keys you're importing.
< provoostenator>
Also there's a feature to show an address on the device display. We could make that mandatory for the receive address functionality to be on the safe side.
< gmaxwell>
Which is analogous to the process bitcoin uses internally when generating keys: it signs a message and verifies, to make sure that the generated address/key actually work.
< achow101>
gmaxwell: currently we have a displayaddress command where the address for a given derivation path is shown on the device's screen (if it has one). you can then compare that to the address the wallet gives you for that path
< provoostenator>
Though perhaps betetr is to 1) import using xpubs and then 2) checking all imported keys with a call to the device
< gmaxwell>
achow101: thats good, but I think thats more effective at checking for correct pairing than checking for corruption.
< gmaxwell>
getting the device to sign something that you can verify is probably the gold standard for corruption checking. I'm not sure about how the devices work, but is it currently possible to import a device that you don't know the pin for, thus cannot actually sign with?
< provoostenator>
You can always forget the pin and lose your backup. But the normal flow is that you unlock the device with the pin before it's willing to give your computer any pubkeys.
< achow101>
i think for all devices you need the pin/passphrase to do anything with it
< gmaxwell>
Okay, thats useful. I'm not sure if there are other ways that you could export keys but turn out not to be able to sign, except a device fault.
< provoostenator>
Ideally we'd ask the device to sign something to prove it really has the keys. I don't know if devices can do that though, especially without user interaction.
< gmaxwell>
ISTM that it wouldn't be too hard for the enrollment process though to ask the device to sign a message.. you're user interacting on the import to enter the pin in any case, no?
< achow101>
signing anything requires user interaction
< achow101>
otherwise it would be a huge security hole
< gmaxwell>
But so does the import?
< provoostenator>
gmaxwell: the problem is that the device probably only signs 1 thing at a time, so checking 1000 addresses would be tedious.
< gmaxwell>
I don't think it's necessary to check all the addresses.
< provoostenator>
So then the next best thing is essentially just importing again and double checking.
< gmaxwell>
But it's a big gain to check at least one.
< gmaxwell>
provoostenator: importing again doesn't tell you that the device actually can sign, however.
< achow101>
for implementation with core, you could definitely make it import then sign a message
< achow101>
right now where using HWI is manual, you do that manually
< gmaxwell>
So for example, imagine a device doing public derrivation, botches the generation of the public key, and it caches that value (since its slow I bet all implementations store it). Any number of imports would give keys, they'd all look fine, but the device would fail to be able to sign for any of its keys.
< provoostenator>
I suspect most devices don't cache public keys, and only store the seed. So if they can derive a valid address they can sign it.
< gmaxwell>
With public derrivation in use, I can't think of any likely issue where it could sign for one key but not another.
< provoostenator>
(until they can't, which is where backups become important)
< gmaxwell>
If they don't cache the master pubkey, then indeed multiple imports would be different under that proposed failure mode.
< achow101>
I don't think they cache the pubkeys, but that could change at any time and be different between devices
< gmaxwell>
Though a 'also try to get a signed message on import' would catch faults in both implementations.
< achow101>
deriving multiple keys with just different indexes is slow as fuck on all devices
< gmaxwell>
Basically getting a signature is a fully general test to make sure signing works.
< gmaxwell>
achow101: I was thinking they'd cache the master, not the child keys. but who knows, as you note it could change at any time.
< provoostenator>
To the degree cache exists, it might also be ephemeral, so in that case asking the user to unplug and replug, might help.
< provoostenator>
But this all varies per device. I agree signing would be better.
< achow101>
gmaxwell: they all require hardened derivation at some point IIRC, so caching the master wouldn't be particularly helpful
< provoostenator>
But devices don't like having code that can sign arbitrary stuff, for risk of an attacking abusing that.
< gmaxwell>
right. Well we can only go so far too. :) But the standard we've set internally to bitcoin core is sign-after-generate.
< provoostenator>
(sign arbitrary stuff without user approving, and checking all the details)
< gmaxwell>
At enrollment time, a user approving one signature seems reasonsable.
< provoostenator>
"internally to bitcoin core is sign-after-generate" - I didn't know we did that, that seems smart.
< gmaxwell>
It's a response to a real failure-- in other wallet software a few years back-- that resulted in an extreme amount funds loss (that unfortunately is non-public).
< provoostenator>
Even devices that don't have an official sign message feature, Bitcoin Core could just have it sign a fake transaction.
< gmaxwell>
Indeed.
< provoostenator>
(provably fake ideally)
< gmaxwell>
0 value, locktimed almost-forever far in the future would be nice.
< provoostenator>
Or with a relative lock time after the heath death of the universe.
< gmaxwell>
I think it would be nice to even do more than what Bitcoin Core currently does.. but priorities, the sign/verify after generate is a pretty general protection at least.
< achow101>
provoostenator: that's actually largely not possible for most devices since they verify inputs
< achow101>
oh, actually if you claim it's segwit that's fine
< provoostenator>
Ah that pesky Segwit thing, but they still sign legacy transactions...
< provoostenator>
Or you mean the other way around?
< achow101>
sign a segwit input. they don't verify those inputs
< provoostenator>
Ah ok, so for legacy they insist on seeing an entire transaction with inputs? That does get tedious.
< achow101>
yeah
< provoostenator>
But that could also be fake right?
< provoostenator>
Spending non-existing coins.
< achow101>
sure, but that's more work
< provoostenator>
Indeed
< provoostenator>
So I'll probably add something to my PR in the signerfetchkeys RPC method, which imports the keys, to call sign message on the device with one of the keys, and then check the result.
< provoostenator>
Although there's no way to undo an import, at the RPC method would fail with a giant error telling the user to delete their new wallet and retry the import.
< provoostenator>
*, at least the RPC...
< bitcoin-git>
[bitcoin] instagibbs opened pull request #15245: remove deprecated mentions of signrawtransaction from fundraw help (master...fundraw_signraw) https://github.com/bitcoin/bitcoin/pull/15245
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #15246: qa: Add tests for invalid message headers (master...Mf1901-qaMsgHeader) https://github.com/bitcoin/bitcoin/pull/15246
< meshcollider>
current behaviour is that all pubkeys are added as watchonly if theyre provided in the pubkeys field of importmulti, even if they appear only in subscripts
< meshcollider>
do we want to keep that behaviuor?
< sipa>
meshcollider: you can only specify pubkeys for importmulti when they're actually used as pubkey hashes
< sipa>
right?
< sipa>
and not for example a pubkey in a multisig
< meshcollider>
yeah but if you use pubkey hashes in a P2SH script is it intentionally that the pubkey itself becomes watched
< sipa>
sure
< sipa>
that's ok
< meshcollider>
ok
< sipa>
(and inevitable for now)
< sipa>
well, it's not ok - but it isn't changing policy
< sipa>
if you want to watch a P2WPKH you shouldn't need to also watch the corresponding P2PK and P2PKH, but for now that's inevitable
< meshcollider>
right
< sipa>
however, if you're able to spend the P2WPKH, you're also able to spend the P2PK/P2PKH
< sipa>
this is not the case for multisig; if you want to watch the multisig, you shouldn't also watch P2PKH for the keys in it
< sipa>
does that make sense?
< meshcollider>
yes
< meshcollider>
I just wanted to check the current behaviour was correct
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #15247: qa: Use wallet to retrieve raw transactions (master...Mf1901-qaWalletRaw) https://github.com/bitcoin/bitcoin/pull/15247