< kallewoof>
jnewbery: haha, thanks! I really need to learn how to push my own PRs though. I realize it's my own fault, I just don't really know how to do it without sounding demanding.
< fanquake>
Have thrown some numbers up into #15751, the speedup is so significant it might be worth putting into 0.18 given another rc.
< gmaxwell>
10,000 isn't absurd in any case... esp since it doesn't auto expand as addresses are used.
< gwillen>
yeah that's fair
< gwillen>
that's sort of why I hit 10k in the first place (the fact that importmulti will give me a static pool and that's it)
< gwillen>
of course real descriptor wallets will obviate all of this in the Glorius Future
< fanquake>
The node used to generate those numbers has finally finished shutting down after a 2hr 3m wait.
< gwillen>
heh!
< gwillen>
achow101: can you provide insight into why the Updater and Signer roles in BIP 174 got combined into the single 'walletprocesspsbt' call in Core?
< gwillen>
it's becoming slightly annoying for some blockstream (Elements) stuff I'm working on, and I am wondering why it wasn't "walletfillpsbt" and "walletsignpsbt" separately
< harding>
If I use sendtoaddress to attempt to pay a bech32 address with a witness version >0, the RPC succeeds (surprising me). However, the tx isn't added to the mempool and debug.log says "[default wallet] CommitTransaction(): Transaction cannot be broadcast immediately, scriptpubkey (code 64)". If I try sending the same tx via sendrawtransaction, that fails with the same error code (which I expected). Is it intentional that
< harding>
sendtoaddress returns success despite other commands failing and the transaction not making it to the local node's mempool? Tested on latest master.
< sdaftuar>
harding: that sounds right to me. mempool policy rejecting such transactions is intentional; we don't want to start accepting version1 transactions until a future softfork is designed.
< sdaftuar>
because the first step to deploying a new softfork is to ensure that policy rules will protect (eg) miners running old software
< sdaftuar>
wallet support also makes sense, so that we're not gated on the whole ecosystem upgrading to support sending to new address formats every time new features are deployed in new segwit versions
< luke-jr>
Miners should never be running old software.
< sdaftuar>
luke-jr: fine, users running old software
< sdaftuar>
if coordination of software changes were free, this would be much easier
< luke-jr>
mempool policies don't strictly need adjustment for non-miners
< luke-jr>
I mean, it's nice to have, but I don't think it's essential
< sdaftuar>
oh wait, my reasoning is for not being able to spend a v1 output, not for not being able to send to a v1 output
< sdaftuar>
the reason not to allow sending to a v1 output is the same i guess as not being able to send to any other anyone can spend output
< sdaftuar>
?
< sdaftuar>
which i guess is just footgun protection
< sipa>
yeah
< sipa>
not being able to spend a v1 output is a script execution rule, designed for upgrada safety
< sipa>
i guess there is a separate standardness rule about sending to v1
< harding>
Yeah, that's what I assumed. And testmempoolaccept and sendrawtransaction do prevent me from sending to a witness v1 address; it's just sendtoaddress (and, I'm guessing, sendmany) that succeed as calls unexpectedly. They add the tx to the wallet, but it doesn't get added to the mempool.
< sipa>
that's a surprising inconsistency
< sdaftuar>
sipa: i'm surprised you are saying it's surprising -- i thought you wanted it to work that way for the reason i gave above?
< sdaftuar>
perhaps i misunderstood the deployment goals
< sipa>
sdaftuar: well the not-sending-to-v1-mempool rule should be consistent with the wallet, as they have similar goals
< sipa>
i think?
< harding>
Third-party software I've investigated (e.g. Electrum) that implement bech32 sending support currently require that the witness version be 0 so that they don't create policy-invalid transactions.
< sdaftuar>
sipa: if you put aside the issue that we have no user-friendly way to hand a transaction to someone else except via the p2p network, then i think making the wallet refuse to send creates deployment issues in the future
< sdaftuar>
so that when v1 comes out down the road, users will have to p2sh-wrap it in order to use
< sdaftuar>
which is sort of terrible?
< sdaftuar>
if other software projects are already disabling sending to v1 though, then i guess we're in trouble on that front regardless of what we do
< sipa>
sdaftuar: i'm trying to balance my gut feeling "permitting sendkng to v1 feels like such a footgun!" with the fact that we can't do this for p2sh embedded regardless
< sipa>
and there are plenty of other ways to construct an anyonecanspend address
< harding>
I take that back; I just re-checked the Electrum code. It just contains a redundant check that the witness version is 0-16; I remembered that wrongly because it's a redundancy over the reference library code. Sorry.
< sipa>
really the protection ought to be in code that creates addresses rather than the code sending to it
< achow101>
gwillen: to reduce the need to use an excessive number of RPC calls just do to make a transaction. it needed to be less than or equal to the number of step the *rawtransaction RPCs did otherwise people probably wouldn't use them
< achow101>
also the logic for updating and signing in core are almost exactly the same.
< achow101>
you can have walletprocesspsbt not sign, there's a parameter to set sign to false.
< luke-jr>
wait, people can't even *send* to unknown witness versions? that seems like a bug and defeats the whole point of Bech32's extensibility in that regard, no?
< luke-jr>
sure, it's anyonecanspend for now, but so is a version 0 witness transaction that's simply an OP_TRUE!
< sipa>
achow101, gwillen: in fact making it sign but not update would be kind of hard
< achow101>
luke-jr: it's nonstandard though. we don't allow sending to nonstandard because nonstandard isn't relayed
< sipa>
achow101: right, but arguably sending to it should be allowed, and allowing it should help upgrading when v1 is actually introduced
< sipa>
spending definitely needs to be nonstandard, but that's an independent check
< luke-jr>
yeah, I can't think of any reason not to allow it to be policy-accepted and even mined
< luke-jr>
otoh, accepting it into the wallet and policy-rejecting from mempool doesn't seem too terrible either
< luke-jr>
it would just sit unconfirmed until some future point where a softfork enables it
< luke-jr>
and IIRC the GUI displays a warning about it when you send
< harding>
luke-jr: just tested, there's no warning in the GUI.
< luke-jr>
hmm, there used to be a generic one for mempool rejecting it
< harding>
luke-jr: just a debug.log entry. 2019-04-13T16:59:27Z [default wallet] CommitTransaction(): Transaction cannot be broadcast immediately, scriptpubkey (code 64)
< instagibbs>
I remember asking for v1+ test in functional suite, guess the only checks that were a createraw/decoderaw round-trip
< luke-jr>
this level of hand-holding really belongs in GUI only and not in RPC
< sipa>
luke-jr: the first question is whether it shohld be relay policy
< harding>
I think that, if you want wallets and services to unconditionally accept bech32 addresses even if they use future witness versions, they need to be accepted into the mempool and mined by default. Otherwise some griefer is going to initiate 0.0001 withdrawals from various services, get their transactions stuck, and then they're going to either refuse to send to bech32 addresses altogether or just >0 witness versions.
< bitcoin-git>
[bitcoin] crackercracked opened pull request #15812: not generate coverage report on test failures (master...fix-issue-15648-test-coverage-report) https://github.com/bitcoin/bitcoin/pull/15812
< jnewbery>
harding: to answer your original question (why does sendtoaddress return success, even if the tx is not added to the mempool?), that was changed here: #9302
< jnewbery>
wallet RPCs that use CommitTransaction() will return true even if AcceptToMemoryPool() fails
< jnewbery>
(that also includes GUI sends)
< sipa>
i guess we need to distinguish between permanent and temporary failure
< jnewbery>
The PR doesn't contain a description/motivation, but I believe the reason is that the transaction exists in the wallet, so the RPC should return the txid so the user can find it
< jnewbery>
sipa: yes. It'd be nice if all RPCs returned an object so fields could be added later. In this case a "accepted_to_mempool" boolean would be helpful, for example
< harding>
jnewbery: thanks. From the discussion, it looks like the purpose was to allow the wallet to create chains of transactions beyond the default limits and then trickle those into the mempool later as their ancestors got confirmed. The non-standard witness version error, OTOH, isn't something that's going to resolve itself, so I think the wallet should probably reject what the mempool rejects. (Distinguishing between temp_fail and
< harding>
perm_fail, if not more specific classes, seems like a good idea to me.)
< luke-jr>
temp_fail might not be a good fit for future witness versions though
< luke-jr>
since it's potentially a long term
< luke-jr>
condition
< instagibbs>
policy updates usually aren't activated while running the same software, right?
< luke-jr>
I guess it's a tristate too: "used to be okay, no longer is", "currently okay", "currently not okay, but will be in the future"
< instagibbs>
problem is here previously nothing non-sendable had an address to decode
< harding>
luke-jr: the main thing being distinguished in this discussion is unconfirmed tx chains longer than 25 (or larger than 100,000 vbytes), which is a temporary issue, versus future segwit versions which is a long-term issue (months, years, maybe never). You're right that the second category isn't a perm_fail, but a trying_again_later_wont_help_fail.
< harding>
probably_wont_help*
< instagibbs>
it can also happen with a plain old CreateTransaction making a tx that isn't relay-valid