< jb55>
I screwed up my first psbt, I didn't realize core created a sh(wpkh()) change address to m/84' which would have been a funds loss to a regular trezor user. hopefully future wallet devs don't make that mistake.
< jb55>
not really "loss" but would require some manual recovery with sign-tx which I just did
< sipa>
jb55: core doesn't care about derivation paths
< jb55>
yeah
< sipa>
it creates whatever you imported as change
< jb55>
I'm just thinking about people building hardware wallet stuff, since core only cares about addresses and not paths, would be a common source of dev errors I predict
< jb55>
afaik native descriptor wallets don't help here either if I understand that PR correctly
< sipa>
i don't see how it would be related, no
< jb55>
sipa: with importmulti I imported wpkh addresses, but when I dumpwallet I see sh(wpkh()) addresses. I'm still a bit confused as to what is actually stored on disk
< jb55>
just pubkeys?
< sipa>
jb55: keys and scripts
< sipa>
and what is treated as "mine" is a based om complex historically grown rules that make little sense
< jb55>
it's possible I screwed something up, but I find it weird that it would generate sh(wpkh()) change addresses after a wpkh import. I guess that descriptor info isn't retained and it just does p2sh-segwit by default
< sipa>
jb55: importing something (for now) in now restricts it to be importing just that
< sipa>
it does make sure the result is "policy compatible"; e.g. importing a multisig will never result in your wallet treating payments to individual keys in it as your
< sipa>
but other than that... the current representation simply has no way to represent one exact type of imported things... it's just keys and script
< sipa>
that's actually the entire reason why i started working on descriptors
< sipa>
*in now -> in no way
< jb55>
sipa: cool. in the meantime I'll be more careful with psbts...
< sipa>
with getrawchangeaddress you can control what type of address you create
< jb55>
yeah and there's a change_type option in createwalletfundedpsbt I just forgot to set it to bech32 :[
< achow101>
jb55: native descriptor wallets will help as you would import one descriptor for change sh(wpkh()) and a different for wpkh() and so on
< achow101>
but the wallet currently treats all keys the same and any key can be used for p2sh-segwit, legacy, or bech32
< achow101>
jb55: if you're using Core + HWI, it shouldn't matter to you whether the path is the type the trezor expects. it will still sign regardless, just complain when the path isn't what it expects
< jb55>
achow101: that means that combo multiimports should do the same thing as any other multiimport on the same range?
< achow101>
right now it does
< achow101>
in theory
< jb55>
achow101: it prompted the change address on the trezor display, because I guess it thought it was someone else's address.
< jb55>
Makes sense in hindsight, thought it was just some bug
< achow101>
IIRC change detection is disabled for trezor by HWI
< achow101>
so that should be expected behavior regardless of derivation path
< achow101>
I don't think we give it the derivation path for outputs (yet). This is intentional behavior because change detection isn't very smart
< jb55>
ah ok yeah I read about, and assumed that what was happening the first time around
< jb55>
achow101: one thing I couldn't get working was a walletcreatefundedpsbt that was flexible enough to express a sh(wpkh(m/84'/..)) -> wpkh(m/84'/...) tx. I tried specifying a specific change address with no outputs but that wasn't allowed
< jb55>
so I ended up just doing an interactive trezorctl sign-tx
< jb55>
perhaps createpsbt could do this but I'm not sure how to get that to work with hwi...
< achow101>
jb55: huh? was it not filling in the input, or did it not like the output? what about itwasn't working?
< jb55>
achow101: input was fine, but I only wanted one output which was the exact same derivation path but as a wpkh output instead of sh(wpkh). I couldn't figure out how to do that with walletcreatefundedpsbt.
< achow101>
just get a new address, set that as the output, and use the option "subtractFeeFromOutputs":[0]
< jb55>
that's the ticket
< achow101>
and send the full input amount to that output
< achow101>
it shouldn't care
< jb55>
that's good to know, I haven't used that option before but that makes sense!
< hebasto>
MarcoFalke: hi! if #16362 has been marked with 0.19.0 milestone, could #16224 be marked 0.19.0 milestone as well?
< bitcoin-git>
bitcoin/master 172213b Andrew Chow: Add GetNewDestination to CWallet to fetch new destinations
< bitcoin-git>
bitcoin/master 33d13ed Andrew Chow: Replace CReserveKey with ReserveDestinatoin
< bitcoin-git>
bitcoin/master 8e7f930 Andrew Chow: Add GetNewChangeDestination for getting new change Destinations
< bitcoin-git>
[bitcoin] laanwj merged pull request #16237: Have the wallet give out destinations instead of keys (master...cwallet-getnewaddr) https://github.com/bitcoin/bitcoin/pull/16237
< bitcoin-git>
bitcoin/master 48bcb2a qmma: Disable other targets when enable-fuzz is set
< bitcoin-git>
bitcoin/master 84edfc7 qmma: Update doc and CI config
< bitcoin-git>
bitcoin/master 5859b7d Wladimir J. van der Laan: Merge #16338: test: Disable other targets when enable-fuzz is set
< bitcoin-git>
[bitcoin] laanwj merged pull request #16338: test: Disable other targets when enable-fuzz is set (master...enable-fuzz) https://github.com/bitcoin/bitcoin/pull/16338
< bitcoin-git>
bitcoin/master 0d101a3 MarcoFalke: test: Add test for maxtxfee option
< bitcoin-git>
[bitcoin] laanwj merged pull request #16322: wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction (master...2019-07-fix-16257) https://github.com/bitcoin/bitcoin/pull/16322
< provoostenator>
Is there a way to run only a subset of functional tests, e.g. just the wallet tests? "python test/functional/wallet*" does not do the trick.
< luke-jr>
provoostenator: for test in test/functional/wallet*; do $test; done
< luke-jr>
or if you want to get fancy: parallel ::: test/functional/wallet*
< provoostenator>
That works, but it's much more verbose than test_runner and it doesn't stop if any test fails.
< luke-jr>
--halt now,fail=1
< wumpus>
you can pass a filter argument to test_runner afaik
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #16366: init: Use InitError for all errors in bitcoind/qt (master...1907-initErrorGui) https://github.com/bitcoin/bitcoin/pull/16366
< provoostenator>
wumpus: only --exclude
< promag>
MarcoFalke: to backport #16322 I'll have to include #15638 -> #15778 -> #16257
< gribble>
https://github.com/bitcoin/bitcoin/issues/16322 | wallet: Fix -maxtxfee check by moving it to CWallet::CreateTransaction by promag · Pull Request #16322 · bitcoin/bitcoin · GitHub
< ariard>
IMO, I think we need to preserve lock order (cs_main - cs_wallet) until we are done on Chain::Lock refactoring and swap it at once
< provoostenator>
sdaftuar: bingo! That only works from within the directory itself weirdly enough.
< sdaftuar>
provoostenator: i think it's just because we don't strip the path when test_runner compares its arguments to what tests it knows about? i didn't investigate though
< instagibbs>
oh that's cool. test prefix naming makes even more sense now
< elichai2>
If I want to add manual psbt constructions support, does it makes sense to have it in `bitcoin-tx`? regular bitcoin core rpc or even a new `bitcoin-psbt`?
< MarcoFalke>
Isn't there already psbt support in the rpc?
< instagibbs>
should be quite robust already in master
< elichai2>
MarcoFalke: only commands that constructs it through the wallet (as far as I understand)
< instagibbs>
`createpsbt`?
< instagibbs>
there are a number of non-wallet ones
< elichai2>
instagibbs: it's just the creator role, not the updater, meaning it doesn't add any inputs/outputs to the psbt.
< elichai2>
it creates a raw transaction and then add empty inputs outputs
< elichai2>
(so maybe a better thing would be just to extend that command to also accept inputs/outputs?)
< instagibbs>
might be #bitcoin chat, achow101 would know
< elichai2>
altough there's `utxoupdatepsbt` which will do the updates but using descriptors.
< elichai2>
k, i'll talk with him
< sipa>
t
< sipa>
there is createpsbt and walletcreatefundedpsbt
< sipa>
and i think we need a bitcoin-psbt tool that can sign/update a psbt with keys and descriptors
< sipa>
elichai2: but a generic updater is hard, as it needs access to the outputs/transactions being spent
< sipa>
so you can't really have all creation/updating done in an offline tool
< elichai2>
sipa: you can make the user provide everything that's needed, like createrawtransaction
< elichai2>
(or signrawtransactionwithkey )
< sipa>
elichai2: right
< elichai2>
sipa: i'm mostly trying to figure out if there's a way to integrate p2c and taproot fields into PSBT without adding full wallet support yet(which is a lot more work)
< sipa>
elichai2: you can if you don't support it
< sipa>
not sure what "full wallet support" means otherwise
< elichai2>
adding classes and types for witness v1, adding descriptors to save the full taproot tree, constructing of taproot addresses.. etc
< elichai2>
I want to add it to the PSBT standard so that other wallets can start working on support this even before it's in bitcoin core's wallet (assuming of course taproot will get activated)
< sipa>
but a standalone tool which you give a psbt and a bunch of descriptors, private keys, xpubs, previous txn, utxos, ... and just updates everything and optionally signs would be really cool
< elichai2>
so you think it should be separate from bitcoin-tx
< sipa>
i think so
< elichai2>
I think i'll start with a standalone RPC method and hopefully divide it into a separate tool (I want to have some PoC working to start a mailing list conversation around the new PSBT fields)
< sipa>
bitcoin-tx is really "transformation based" thing which you give an input and then specify some transformations to apply
< sipa>
for bitcoin-psbt i'm more thinking something which you give a bunch of information and uses it where useful
< sipa>
elichai2: not everyone likes utility RPCs that could be separate tools instead
< sipa>
(because then they require a running bitcoind, which is overkill for things that don't need access to blockchain/utxoset/wallet/...)
< sipa>
though we already have a fair share of those
< sipa>
elichai2: i think it's also premature to discuss psbt extension for taproot before taproot is final
< elichai2>
Yeah I think we're already in a point where we have a lot of different RPC methods that do different kinds of things already
< elichai2>
sipa: why? do you think it's gonna change in any meaningful way?
< sipa>
elichai2: probably
< sipa>
there hasn't been that much discussion
< sipa>
and for wallet support there is a huge amount of time anyway
< elichai2>
really? I really hope that taproot can be activated in less than 6 months
< sipa>
lol
< sipa>
i'm glad to see you're so confident
< luke-jr>
elichai2: a large amount of the network hasn't even patched the inflation bug yet :<
< elichai2>
luke-jr: that's a different thing. for taproot we need to: 1. Have an open PR. 2. Have consensus for merging. 3. have it in 0.19. 4. have BIP9 activation by miners
< elichai2>
you're talking about regular full nodes
< luke-jr>
elichai2: no. we need community support and deployment to a significant percent of user full nodes.
< luke-jr>
miners do not decide network rules
< luke-jr>
and BIP 9 is dead.
< sipa>
really there is no rush for any of this
< nijak_>
almost like we didn't learn anything from the UASF movement
< nijak_>
When
< nijak_>
when is 0.19 meant to be RC'ed? October?
< bitcoin-git>
bitcoin/master 1aecdf2 Andrew Chow: Move wallet creation out of the createwallet rpc into its own function
< bitcoin-git>
bitcoin/master 4fcccda MarcoFalke: Merge #16244: Move wallet creation out of the createwallet rpc into its ow...
< bitcoin-git>
[bitcoin] MarcoFalke merged pull request #16244: Move wallet creation out of the createwallet rpc into its own function (master...mv-createwallet) https://github.com/bitcoin/bitcoin/pull/16244
< achow101>
do we have some way to check if a CTxDestination matches a particular OutputType?
< sipa>
achow101: there is a .which() function
< sipa>
which gives a number corresponding to the various positions in the type declaratio
< achow101>
oh, nice
< achow101>
sipa: actually, I'm now wondering whether it makes sense to keep the combo() descriptor. it doesn't really fit well into this ScriptPubKeyMan model
< achow101>
The main thing is making DescriptorScriptPubKeyMan consistent with the address types that we expect to get from it. For all non-combo descriptors, this is easy. but for combo, it isn't since combo can give all 3 address types
< sipa>
i guess you could add an IsSingleType function to it or so, which is true fot everything but combo
< sipa>
and then require descriptors that satisfy that propery in native desceiptor wallets
< achow101>
if that's the case, what's the point of keeping combo around?
< luke-jr>
it's probably better to do a cast than use .which?
< sipa>
achow101: it's pretty useful for scantxoutset
< sipa>
and would probably be useful to when converting old wallets into new ones... at least at the time that seemed like a useful thing
< jb55>
sipa: I played with the idea of some type of union syntax for descriptors since combo seemed a bit hardcoded, but it is probably not that useful