< kanzure>
#proposedmeetingtopic more topic collection for upcoming physical meeting
< instagibbs>
achow101, descriptor wallet PR is a *great* place to redefine "IsChange" to be something much more meaningful than "address isn't in address book"
< instagibbs>
since we're adhering to BIP44/49/84(throwing this out there in case someone thinks this is a bad idea)
< bitcoin-git>
[bitcoin] Sjors opened pull request #18201: rpc: sendmany and sendtoaddress return PSBT for wallets without private keys (master...2020/02/sendmany_sendtoaddress) https://github.com/bitcoin/bitcoin/pull/18201
< achow101>
instagibbs: I'm not sure how that would fit in with LegacyScriptPubKeyMan though
< achow101>
we could have the check be whether a destination belongs to a spkman in m_internal_spk_managers, but for legacy, that doesn't work
< sipa>
achow101: have a IsChange(destination, label) function in spkmanagers, which is implemented in legacy as checking if label is empty, and has sane logic in descriptor spkmans?
< achow101>
spkmans internally don't really have a concept of being change/not-change
< achow101>
but I guess we can just have IsChange have more conditionals based on spkman type as we already do in a few places
< sipa>
in an envisioned native descriptor wallet, you'd have separate spkmans for change and non-change, no?
< achow101>
yes
< sipa>
but it's not the spkman itself that knows it is for change or not?
< achow101>
yes
< achow101>
we would have to check whether a scriptPubKey belongs to a spkman in m_internal_spk_mans
< sipa>
that makes sense
< sipa>
but it also.sounds like there shoukd be an easy solution
< sipa>
right, so the knowledge is in the wallet, not the spkmans
< instagibbs>
either way my point is that we should change it *now* before we're stuck with behavior again
< instagibbs>
legacy spkm has to keep old behavior
< instagibbs>
anything newer should do something better imo
< instagibbs>
well, any wallet that employs a non-legacy spkm*
< luke-jr>
[14:40:54] <instagibbs> achow101, descriptor wallet PR is a *great* place to redefine "IsChange" to be something much more meaningful than "address isn't in address book"
< luke-jr>
instagibbs: we can and should fix it for existing wallets ;)
< instagibbs>
opt-in damage :P
< instagibbs>
according to your PR at least
< luke-jr>
instagibbs: that's why I rename anything that could be misused
< luke-jr>
any IsChange is inherently opt-in
< instagibbs>
achow101, only edge cases i can think of is if a particular script is for some reason in both an internal and external spkm in the same wallet(though that's pretty dumb to do)
< instagibbs>
otherwise yeah, just add a conditional. I wanted to do that previously but hadn't the opportunity
< provoostenator>
luke-jr: "don't we want such cases to call an external signer and behave as normally?" - not sure what you mean there? (#18201)
< gribble>
https://github.com/bitcoin/bitcoin/issues/18201 | rpc: sendmany and sendtoaddress return PSBT for wallets without private keys by Sjors · Pull Request #18201 · bitcoin/bitcoin · GitHub
< provoostenator>
I have a ExternalSignerScriptPubKeyManager subclass of (DescriptorScriptPubManager) which overrides FillPSBT to call an external signer.
< provoostenator>
Which means that CWallet's FillBSBT will return a complete PSBT in that case.
< provoostenator>
Whereas with a legacy watch-only wallet it always returns an incomplete one.
< luke-jr>
provoostenator: I see. Then the problem is that the return signature changes based on the wallet
< luke-jr>
and potentially based on whether someone forgot to pass -signer or whatever we end up with
< provoostenator>
That's the same pattern we have with bumpfee
< provoostenator>
No, ExternalSigner wallets have a flag
< luke-jr>
bumpfee returns an Object IIRC?
< luke-jr>
with different keys in this case
< provoostenator>
Which causes a throw / exception when -signer isn't set
< luke-jr>
sendmany on the other hand returns a string txid, which callers may very well ignore (while checking the return status)
< provoostenator>
Ah yes, bumpfee always returns an object, but I think I've this pattern in other places.
< provoostenator>
sendmany currently just fails if you use it on a watch-only wallet, which all ExternalDescriptor wallets are
< provoostenator>
Perhaps it makes more sense to always return an object for watch-only wallets?
< luke-jr>
I'm thinking more like having an input flag passed for the PSBT mode
< provoostenator>
I hate extra params though.
< luke-jr>
provoostenator: but what if you want to use it with a normal wallet? ;)
< luke-jr>
eg, prepare a PSBT, manipulate it, resign it, then send it
< provoostenator>
You mean a legacy wallet with imported public keys?
< provoostenator>
But not watch-only?
< luke-jr>
sure
< provoostenator>
Because there's no point in producing a PSBT if you have all the keys
< luke-jr>
I just gave an example
< provoostenator>
I think the general idea is to encourage users to not mix private key and watch-only wallets.
< provoostenator>
So in that case either a wallet is watch-only, and returns an object, or it's not, and returns a string.
< provoostenator>
Alternatively, maybe easier, we always return an object for non-legacy, and for watch-only legacy.
< luke-jr>
having two behaviours like that, is going to confuse callers
< luke-jr>
and my example is where you would want a PSBT for a non-watch-only wallet
< luke-jr>
to modify it before sending
< luke-jr>
remember RPC isn't for bitcoin-cli users; it's for scripts, apps, and such
< provoostenator>
It's _also_ for bitcoin-cli users
< luke-jr>
in this context, it doesn't matter
< luke-jr>
point is, have a call signature that changes based on wallet is going to screw up programs
< luke-jr>
also, I don't mean throwign it on params - I mean include it in options :P
< luke-jr>
"options":{"psbt": true}
< sipa>
probably not, as the failures would occur in situations where the call would fail anyway
< provoostenator>
Neither sendmany nor sendtoaddress has an options dictionary
< sipa>
but i do the risk for confusion
< sipa>
*see
< Randolf>
Breaking backward compatibility in an API is a very bad idea. Better not to change the call signature.
< sipa>
it's not breaking compatibility
< sipa>
right?
< provoostenator>
My thinking was to deprecate the non-object response, but without breaking existing automation, which currently only works with private key wallets
< sipa>
it's extending those RPCs to situations where they couldn't be used before
< provoostenator>
Exactly
< Randolf>
Adding options is fine as long as those options aren't made mandatory. Otherwise I would expect it to "screw up programs" as luke-jr pointed out.
< luke-jr>
provoostenator: add one
< luke-jr>
sipa: if you call sendtoaddress(…) right now and it doesn't throw an error, you assume it worked
< luke-jr>
sipa: if it's returning a PSBT, then it didn't do the job usually expected
< luke-jr>
ie, it didn't broadcast
< provoostenator>
luke-jr: that would be a bug, it should neverreturn a PSBT for legacy wallets with private keys enabled.
< luke-jr>
provoostenator: the caller does not know the type of wallet loaded
< luke-jr>
if I write a program expecting a watch-only wallet, it shouldn't break if I run it with a normal wallet
< luke-jr>
(nor require extra handling to detect the different situation)
< provoostenator>
Why not? Having private keys in a wallet is very different from not having them.
< luke-jr>
having keys is a strict superset of not having them
< luke-jr>
also, in this case, you would broadcast a transaction intended to just be a PSBT template made!
< sipa>
i have a weak preference for doing this in a separate RPC too
< sipa>
the places you'd use this seem to be distinct from the sending RPCs
< luke-jr>
actually, doesn't fundrawtransaction kind of already do it?
< instagibbs>
achow101, one large annoyance on today's wallet code is that recovering a wallet will mark every address you didn't "getnewaddress" as change, at least until you "getnewaddress" until you hit that particular address
< instagibbs>
if you don't want to get confused about changeness, then don't blow away spkms I guess?
< achow101>
you lose the changeness because they get removed from m_internal_spk_managers
< achow101>
because they're no longer active
< sipa>
achow101: hmm
< instagibbs>
hmm let's take this offline
< instagibbs>
or not, if sipa is reading
< instagibbs>
when do they become inactive?
< achow101>
if you were to import a new descriptor for change
< achow101>
whatever was there originally gets inactivatated
< instagibbs>
oh
< sipa>
it shouldn't lose change-info when you import a new active one?
< sipa>
(would be the ideal behavior, i mean)
< instagibbs>
So you have 6 spkm, how does it decide which of the 3 changey ones to deactivate?
< achow101>
the one that's occupying that address type spot
< achow101>
if you imported a new change p2sh-segwit descriptor, the existing change p2sh-segwit descriptor becomes inactive
< luke-jr>
maybe IsChange should pay attention to the derivation of HD keys
< achow101>
luke-jr: I'd rather not
< instagibbs>
luke-jr, that was my original attempt years ago, with current API you can have whatever derivation be change when imported at least
< sipa>
achow101: i feel that the changeness in a descriptor world should be metadata associated with the spkman
< sipa>
independent of what it's active for
< instagibbs>
sipa, mhmm
< achow101>
I guess another m_internal_change_spkmans set could be added where you put all the inactive change spkmans
< sipa>
there may be other metadata too at some point (say: which hw device to use for signing)
< instagibbs>
ACK
< luke-jr>
why doesn't C++ have a QFlags equivalent yet? -.-
< sipa>
std::bitset comes close
< sipa>
combined with an enum for naming the bits
< luke-jr>
I mean something compatible with class enums and including proper type checking