< luke-jr>
achow101: rpc_psbt's "Make sure that a psbt with signatures cannot be converted" doesn't make sense to me. is the comment/description wrong? (also, it's failing if it gets a non-segwit transaction..)\
< bitcoin-git>
[bitcoin] instagibbs opened pull request #14816: Add CScriptNum decode python implementation in functional suite (master...functional_cscriptnum_decode) https://github.com/bitcoin/bitcoin/pull/14816
< luke-jr>
so I'm still not really clear why the test looks for "TX decode failed", but the error is (supposed to be?) "Inputs must not have scriptSigs and scriptWitnesses"
< instagibbs>
Should have filed an issue when it was in my brain, oops
< bitcoin-git>
[bitcoin] hebasto opened pull request #14817: qt: Remove unnecessary columns in Coin Selection window (#11811) (master...20181126-fix-hidden-columns) https://github.com/bitcoin/bitcoin/pull/14817
< gribble>
https://github.com/bitcoin/bitcoin/issues/14602 | Bugfix: Correctly calculate balances when min_conf is used, and for getbalance("*") by luke-jr · Pull Request #14602 · bitcoin/bitcoin · GitHub
< jnewbery>
Do people just need to use getunconfirmedbalance or am I missing some other use case?
< jnewbery>
as far as I can tell, getbalance * wasn't documented as a feature, was marked as deprecated and there was even a warning in the most recent rpc help text saying "it is recommended to avoid passing this argument"
< jnewbery>
Trying to understand what people were using it for
< esotericnonsense>
jnewbery: atomicity perhaps?
< esotericnonsense>
(just a guess)
< esotericnonsense>
if you call getbalance and get x, and getunconfirmedbalance and get y, seperately, you're not guaranteed that the "total unconfirmed balance" is x+y
< esotericnonsense>
(unless there's some getunconfirmedbalance command that includes it, in which case sure that works... or just use getwalletinfo(?))
< sipa>
jnewbery: i think the better questiom is why doesn't getbalancd "" return unconfirmed balance?
< jnewbery>
It's frustrating. There was originally a code comment saying "getbalance and getbalance '*' 0 should return the same number
< jnewbery>
and then people started relying on undocumented behaviour about how they return different things
< instagibbs>
:(
< jnewbery>
sipa: are you suggesting that getbalance should return unconfirmed balance?
< sipa>
i don't see why not
< sipa>
when confirmations=0 is passed
< jnewbery>
ah, yes, when minconf=0
< sipa>
jnewbery: my guess is that more people rely on getbalance * 0 returning unconfirmed balance than there are people relying on it being different from getbalance
< sipa>
but that's just a guess
< luke-jr>
minconf=0 wasn't originally supported without "account"
< luke-jr>
(and never has worked without it, before that PR)
< sipa>
luke-jr: i know
< sipa>
but i don't see why it wouldn't be
< sipa>
my suggestion would be to always support minconf=0, and leave the dummy/account argument alone
< luke-jr>
sipa: and use minconf=null to get the current behaviour?
< sipa>
right, just "getbalance" should keep doing what it did before
< sipa>
but there is no ambiguity in getbalance minconf=0; clearly the caller wants to include 0 conf
< luke-jr>
hmm
< luke-jr>
what if at some point we only include 6 blocks deep in IsTrusted?
< luke-jr>
overloading minconf might bite us then
< sipa>
i think that would either be a new RPC call, or a configuration option
< sipa>
how would it require overloading?
< sipa>
ah, i see
< sipa>
the default is nominally 0 now
< sipa>
but that only has an effect when an account was passed
< luke-jr>
getbalance's behaviour distinction seems to go way, way back
< luke-jr>
to a Satoshi commit even
< sipa>
that doesn't surprise me
< sipa>
i think it's not unreasonable to just change the default minconf to one, bit add a "if dummy=="*", minconf is 0 by default for backward compatibility"
< luke-jr>
ah, converttopsbt fails differently w/ non-segwit inputs because it never tries deserialising as segwit
< luke-jr>
sipa: I'm not sure a default of minconf=1 will produce the same behaviour.
< luke-jr>
sipa: the default behaviour is 0 or 1 depending on the individual TXO
< gmaxwell>
having your balance inexplicably go down due to change would be quite frightening...
< sipa>
gmaxwell: it won't
< sipa>
luke-jr: hmm?
< sipa>
luke-jr: perhaps i misunderstand the issue then
< esotericnonsense>
the weirdness is that a sent transaction is "half included" right.
< sipa>
luke-jr: is the default using IsTrusted, but the accounts-based logic including all 0conf?
< luke-jr>
sipa: the default uses IsTrusted, and the accounts-based logic just checks IsFinal and minconf
< jnewbery>
sipa: I believe that's the case
< sipa>
oooh
< sipa>
ugh, ok
< sipa>
ignore my suggestion in that case
< gmaxwell>
that was my belief. that default is 1/0 depending on istrusted so your blance doesn't go down, and "*" uses minconf.
< sipa>
including non-istrusted balance sounds like a pretty bad idea regardless
< luke-jr>
why?
< sipa>
it can be doublespent
< sipa>
doesn't mean you can't know about it, but including it in a balance RPC by default sounds weird
< sipa>
promag: if the GBT client doesn't support segwit, but the returned template contains segwit tx, the resulting block they produce will be invalid
< promag>
I guess that would be for a short period of time?
< sipa>
no?
< sipa>
they will not produce the segwit commitment
< luke-jr>
it would be until the miner notices on his own :/
< gmaxwell>
assuming he ever does
< promag>
if he upgrades then he has to
< bitcoin-git>
[bitcoin] luke-jr opened pull request #14818: Bugfix: test/functional/rpc_psbt: Remove check for specific error message that depends on uncertain assumptions (master...bugfix_test_rpc_psbt) https://github.com/bitcoin/bitcoin/pull/14818
< sipa>
promag: that's the point
< sipa>
if his setup is not supporting segwit, it should fail immediately
< sipa>
so that it can be fixed
< gmaxwell>
otherwise, how would he even know to upgrade?
< luke-jr>
promag: miners are amazingly ignorant of problems
< sipa>
it shouldn't need to take days... perhaps more until he notices he is producing invalid blocks
< luke-jr>
see the recent lawsuit where a miner is suing Bitmain because he didn't bother to configure his new miners for his own account..
< promag>
ok, sorry my ignorance here
< promag>
what if he upgrades, but wants to keep the upgraded version for some other reason but without this change?
< luke-jr>
huh?
< promag>
I guess this is 0.18?
< sipa>
promag: this is about gbt client
< sipa>
not bitcoim core
< gmaxwell>
I think we're talking past each other with the use of the word 'upgrade', when we say upgrade we mean his mining software.
< sipa>
promag: currently, if the gbt client does not provide segwit flag, bitcoin core will produce a (suboptimal) block template without segwit txn
< sipa>
we want to change that, as segwit is so widely adopted now, it almost certainly means a configuration error rather than an intentional choice
< gmaxwell>
promag: mining software (e.g. pool software) had to be upgraded to be able to include segwit txn because it needs to compute and include the segwit commitment, because GBT doesn't itself make the coinbase transaction. To avoid forcing everyone to upgrade at once, we made it optional-- if you don't send the segwit flag, you don't get segwit txn and your blocks are still valid.
< jnewbery>
I think promag means 'what if the miner wants to take Bitcoin Core 0.18 but doesn't want to upgrade his pool software to support mining segwit blocks?'
< gmaxwell>
So sad for him.
< promag>
lol, thanks jnewbery
< sipa>
oh!
< promag>
so in the future it can be implict by default? like when last N blocks are segwit?
< luke-jr>
once that PR is merged, we just won't support that anymore
< gmaxwell>
If he wants to keep losing money, he can modify the software himself.
< jnewbery>
it seems pretty edge-case. Over 99% of blocks include segwit transactions right now
< luke-jr>
promag: no, implicit = broken
< sipa>
promag: making it implicit is worse than silently mining non-segwit blocks
< gmaxwell>
promag: it can't be implicit, because then if you restart with old mining software or something you'll silently produce invalid blocks.
< sipa>
instead it will be silently not making any blocks at all
< gmaxwell>
We could, in the future, introduce a replacement to that rpc that does it implicitly, for example.
< promag>
gmaxwell: ah right
< jnewbery>
I'm inclined to agree with gmaxwell. The Bitcoin Core project doesn't need to support theoretical miners who for some reason don't want to include a certain class of transaction in their blocks.
< bitcoin-git>
[bitcoin] luke-jr opened pull request #14819: Bugfix: test/functional/mempool_accept: Ensure oversize transaction is actually oversize (master...bugfix_test_mempool_accept) https://github.com/bitcoin/bitcoin/pull/14819
< promag>
does it make sense to add deprecatedrpc=getblocktemplate to have same behavior in 0.18?
< jnewbery>
Yes, in v0.19 or v0.20 we could implicitly assume that rule:segwit is set
< gmaxwell>
If we were really worried about back compat we could have a depricated flag to bring it back, but given the stats, I don't think there is a need to.
< sipa>
promag: why bother?
< gmaxwell>
jnewbery: I would be opposed to that, because there is always a risk of accidental downgrade.
< promag>
ok, just asking
< jnewbery>
even if it was v0.20?
< luke-jr>
[17:57:13] <gmaxwell> We could, in the future, introduce a replacement to that rpc that does it implicitly, for example. <-- breaking all compatibility with the BIPs and existing software? :/
< jnewbery>
ie 1-1.5 years after it was possible to mine a non-segwit block
< gmaxwell>
jnewbery: people sometimes manage to accidentally start up old software/configs.
< gmaxwell>
luke-jr: we're going to replace the rpc eventually... GBT has a lot of problems.
< gmaxwell>
luke-jr: doesn't mean the old one wouldn't continue to work for a long time.
< jnewbery>
but at that point, all pool software will necessarily support segwit blocks
< luke-jr>
jnewbery: bitcoind is not the only GBT server, and pool software is not the only GBT clients
< gmaxwell>
jnewbery: yes, and what happens if you reboot your host and your startup script starts an old version of your pool software?
< sipa>
testing that segwit is set in the call is essentially free
< sipa>
i don't think there is any reason to remove that (1?) line of code
< sipa>
ever
< sipa>
as long as GBT exists
< jnewbery>
I think that scenario is out of the scope of things we can reasonably worry about.
< gmaxwell>
We shouldn't obsessively trying to "clean things up" in ways that introduce real, if somewhat obscure, funds loss avenues while providing NO benefit.
< sipa>
jnewbery: it happened recently
< gmaxwell>
And similar has happened in the past.
< sipa>
a few weeks ago a bunch of blocks were mined without segwit txn as a result of a configuration error
< gmaxwell>
jnewbery: a large miner recently started producing blocks without segwit txn exactly because they accidentally regressed to non-sw supporting mining software.
< jnewbery>
sipa: sure - I don't think it needs to happen, but I wouldn't be opposed to it being implicit in future
< sipa>
jnewbery: i'm not strictly opposed to it either if there was a good reasom
< sipa>
but i see literally zero benefit to making it implicit
< sipa>
apart from saving 1 line of code
< jnewbery>
I'm aware of that. I also don't think there's any way that bitcoind can police all possible misconfigurations in client software.
< sipa>
sure
< jnewbery>
Sure, I don't really care that much :)
< gmaxwell>
This isn't a question about all possible misconfigurations, this is a protocol versioning question.
< gmaxwell>
Essentially GBT is a protocol, it was effectively unversioned originally. The different versions were not compatible and would cause funds loss if mixed.
< gmaxwell>
Setting the flag manually negoiates the version.
< luke-jr>
eh, it wasn't unversioned
< luke-jr>
this is exactly the verisoning it has always had
< gmaxwell>
Requiring the flag drops support for the old version.
< jnewbery>
gmaxwell: yes, that makes sense
< luke-jr>
well, I guess the original had "capabilities"
< luke-jr>
instead of "rules"
< gmaxwell>
But implicitly accepting incompatible requests just creates exposure. To a real issue that has cropped up multiple times, because making sure all your software is running lockstep correct versions is hard. (esp since many of these pools also support non-sw altcoins...)
< jnewbery>
ok, you've convinced me! I won't try to remove it
< gmaxwell>
If we were to make a new GBT-like call, obviously we'd need no segwit flag for it, as it would just be defined to always have it.
< luke-jr>
while we're on the topic, I think we should still merge the fix in #10595 before this, so it can be backported..
< gribble>
https://github.com/bitcoin/bitcoin/issues/10595 | Bugfix: RPC/Mining: Use pre-segwit sigops and limits, when working with non-segwit GBT clients by luke-jr · Pull Request #10595 · bitcoin/bitcoin · GitHub
< luke-jr>
otoh, realistically, I'm not sure anyone's encountering that issue, so maybe not worth the effort, dunno
< sipa>
yeah, i think it's not worth the effort
< gmaxwell>
luke-jr: or backport dropping segwit support? :P (maybe in that case it would be with a depricated flag)
< gmaxwell>
er dropping nonsegwitsupport
< luke-jr>
gmaxwell: might trigger some trolls with that :p (but who cares)
< luke-jr>
(they'll probably be triggered either way)
< gmaxwell>
yea, who cares? I think in a BP we probably would want to make it switchable.
< gmaxwell>
so maybe thats an argument against it.
< sipa>
BP?
< provoostenator>
Would it make sense to have a "disabled_rules" param for GBT? Throw the error if neither rules or disabled_rules contains "segwit"?
< sipa>
ah, backport
< luke-jr>
provoostenator: why?
< jnewbery>
I don't think it's worth the effort of backporting
< gmaxwell>
provoostenator: 09:57:52 < jnewbery> I'm inclined to agree with gmaxwell. The Bitcoin Core project doesn't need to support theoretical miners who for some reason
< gmaxwell>
don't want to include a certain class of transaction in their blocks.
< luke-jr>
the only question is if the GBT client supports segwit or not. there's no reason it would ever be supported but disabled..?
< gmaxwell>
provoostenator: if there were some actual reason known to intentionally run in that config that made sense, then what you suggest is what it should do...
< provoostenator>
The "theoretical miner" argument is reasonable, but has anyone actually asked a bunch of miners why they're not mining SegWit blocks to make sure it's really either an accident or a lack of upgrading other tools?
< gmaxwell>
provoostenator: there aren't "a bunch of miners" who aren't mining segwit blocks
< gmaxwell>
There was a recent incident, and it was an accident.
< provoostenator>
9 out of 1000 blocks accodring to the PR
< provoostenator>
(in that time range)
< gmaxwell>
provoostenator: which may just not have had any segwit txn available at all.
< luke-jr>
provoostenator: if they really don't want to mine blocks with segwit txs, they can just not upgrade
< provoostenator>
Oh ok, so if there's 0 segwit transactions, nothing in the block indicates SegWit?
< provoostenator>
In that case a miner who doesn't want to mine SegWit could just filter those before they reach the node.
< phantomcircuit>
gmaxwell, can you still make blocks without the commitment if there's no segwit transactions in it?
< gmaxwell>
phantomcircuit: yes.
< gmaxwell>
the whole idea for those rules was to avoid creating a flag day where everyone had to upgrade their software at the same time.
< phantomcircuit>
gmaxwell, yeah
< luke-jr>
MarcoFalke: if we wanted to avoid the import, we could just add +1 unconditionally.. but I think using ceil is clearer and no real downside
< bitcoin-git>
[bitcoin] ryanofsky opened pull request #14820: Fix descriptor_tests not checking ToString output of public descriptors (master...pr/descstr) https://github.com/bitcoin/bitcoin/pull/14820
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #14822: bench: Destroy wallet txs instead of leaking their memory (master...Mf1811-benchWalletTxs) https://github.com/bitcoin/bitcoin/pull/14822
< jnewbery>
I'm trying to work out how fAvailableCreditCached in WalletTx is supposed to work. If the spentness of any of the outputs changes, the cache doesn't get cleared and subsequent calls to GetAvailableCredit() will return the cached value. Am I missing something?
< meshcollider>
jnewbery: not sure exactly where you're looking, but does it call MarkDirty?
< meshcollider>
MarkDirty will set fAvailableCreditCached to false so it won't return the cached value