< promag> wumpus: ping re #18064
< gribble> https://github.com/bitcoin/bitcoin/issues/18064 | gui: Drop WalletModel dependency to RecentRequestsTableModel by promag · Pull Request #18064 · bitcoin/bitcoin · GitHub
< wumpus> promag: will take a look thanks
< bitcoin-git> [bitcoin] practicalswift closed pull request #18199: build: Enable AddressSanitizer as part of --enable-debug (master...enable-debug-asan) https://github.com/bitcoin/bitcoin/pull/18199
< bitcoin-git> [bitcoin] zengyiheng opened pull request #18200: Sebible v0.18.1 prow (master...sebible-v0.18.1-prow) https://github.com/bitcoin/bitcoin/pull/18200
< bitcoin-git> [bitcoin] fanquake closed pull request #18200: Sebible v0.18.1 prow (master...sebible-v0.18.1-prow) https://github.com/bitcoin/bitcoin/pull/18200
< 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] MarcoFalke pushed 2 commits to master: https://github.com/bitcoin/bitcoin/compare/ab9de435880c...225aa5d6d519
< bitcoin-git> bitcoin/master bca8665 Luke Dashjr: scripted-diff: Wallet: Rename incorrectly named *UsedDestination
< bitcoin-git> bitcoin/master 225aa5d MarcoFalke: Merge #18193: scripted-diff: Wallet: Rename incorrectly named *UsedDestina...
< bitcoin-git> [bitcoin] MarcoFalke merged pull request #18193: scripted-diff: Wallet: Rename incorrectly named *UsedDestination (master...rename_useddest) https://github.com/bitcoin/bitcoin/pull/18193
< 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> we'ver already violated this
< luke-jr> see #18192
< gribble> https://github.com/bitcoin/bitcoin/issues/18192 | Bugfix: Wallet: Safely deal with change in the address book by luke-jr · Pull Request #18192 · bitcoin/bitcoin · GitHub
< instagibbs> Ok, doesn't change my point :)
< 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?
< provoostenator> I could revive #16378
< gribble> https://github.com/bitcoin/bitcoin/issues/16378 | [WIP] The ultimate send RPC by Sjors · Pull Request #16378 · bitcoin/bitcoin · GitHub
< luke-jr> provoostenator: I'd prefer that (if there are actual new use cases)
< provoostenator> (which I closed in favor of this approach :-)
< provoostenator> Should I still make a PR for the first commit that refactors sendmany & sendtoaddress?
< provoostenator> Not useful on its own though.
< bitcoin-git> [bitcoin] Sjors closed 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
< instagibbs> provoostenator, yes please at least for separate consideration
< bitcoin-git> [bitcoin] Sjors opened pull request #18202: refactor: consolidate sendmany and sendtoaddress code (master...2020/02/refactor_sendmany_sendtoaddress) https://github.com/bitcoin/bitcoin/pull/18202
< provoostenator> instagibbs: done
< achow101> instagibbs: I prefer luke-jr's IsChange fix rather than figuring out change based on scriptPubKeyMan
< achow101> if solely based on the spkman, we would lose changeness when an internal spkman is changed
< instagibbs> :/
< bitcoin-git> [bitcoin] Sjors reopened pull request #16378: [WIP] The ultimate send RPC (master...2019/07/send) https://github.com/bitcoin/bitcoin/pull/16378
< 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