< Lightsword>
I would assume if there were any issues there someone would have hit them by now
< morcos>
Lightsword: For what purpose? Do we think its important to have block creation happen 100ms faster? I'd be hesitant to do it unless someone could make a good argument that it's really beneficial to save the time
< gmaxwell>
morcos: I think it could move into the background with basically no hit on reliablity... and then get rid of the 100ms concern.
< Lightsword>
morcos, to speed up GBT and potentially reduce the need for validationless mining
< morcos>
I suppose after a tip change, it makes sense to get a new block 100ms faster... I wonder if we could just skip it in that case...
< Lightsword>
gmaxwell, I think we couldn’t do that due to cs_main locks
< morcos>
gmaxwell: i agree. we could move it to the background... but i think you just want to be a tad careful that we don't view that as free, since it still locks cs_main for 100ms
< Lightsword>
I’m pretty sure the cs_main lock itself is the problem
< gmaxwell>
the lock is irrelevant from the perspective of time to response for the response itself.
< morcos>
anyway... this late in the game i'd view that as a bit aggressive for 0.14...
< morcos>
(any of it)
< gmaxwell>
Lightsword: in 0.12 we made a number of fairly agressive changes to how block construction trusts the integrity of the mempool, with the argument that the test would catch any corruption that happened.
< gmaxwell>
We've had mempool corruption bugs in the past. So totally running without ever having a test seems risky-- turns random mempool optimizations into possible quiet consensus bugs.
< Lightsword>
well…we haven’t had anyone get a failure of TBV yet right?
< gmaxwell>
No but we had failures of the internal mempool tests that we've since removed.
< gmaxwell>
In any case, what I'd prefer to do is just get the TBV out of the critical path rather than never run it.
< gmaxwell>
and it would also be fine as far as I'm concerned if it ran less often.
< Lightsword>
gmaxwell, can we get the cs_main lock out of the critical path as well?
< Lightsword>
I removed the TBV call first without removing CValidationState state; but that had minimal effect, removing CValidationState state; however seems to have made a pretty decent difference
< gmaxwell>
how much time are you talking about?
< Lightsword>
it seems vary a lot unless I remove CValidationState state;
< Lightsword>
over 100ms gbt call time differences
< Lightsword>
just between successive calls
< Lightsword>
vs close to 20 with it removed
< morcos>
I'm not sure what you mean about CValidationState, the only line doing anything is the TBV, but yes I think that sounds about right.... CreateNewBlock = 20ms + 100ms for TBV where there is a lot of variance in the 100ms part
< Lightsword>
hmm, maybe I’m mixing that up
< Lightsword>
anyways direct comparing def is much more consistant
< bitcoin-git>
[bitcoin] goku1997 opened pull request #9339: Revert segwit. Increase block size to 8MB for Bitcoin Ocho. Bitcoin Ocho is the future. (master...master) https://github.com/bitcoin/bitcoin/pull/9339
< rabidus_>
:E
< bitcoin-git>
[bitcoin] jonasschnelli closed pull request #9339: Revert segwit. Increase block size to 8MB for Bitcoin Ocho. Bitcoin Ocho is the future. (master...master) https://github.com/bitcoin/bitcoin/pull/9339
< luke-jr>
sigh
< Lauda>
You should listen ti maxwell's advice regarding that one.
< bitcoin-git>
bitcoin/master b3a7410 Pieter Wuille: Return txid even if ATMP fails for new transaction
< bitcoin-git>
bitcoin/master b6abdc7 Wladimir J. van der Laan: Merge #9302: Return txid even if ATMP fails for new transaction...
< bitcoin-git>
[bitcoin] laanwj closed pull request #9302: Return txid even if ATMP fails for new transaction (master...failedtxid) https://github.com/bitcoin/bitcoin/pull/9302
< jonasschnelli>
luke-jr: the question is, if we really want to support commands like cmd(arg,,) resulting in `cmd arg "" ""`
< jonasschnelli>
IMO the later is fine... but not sure about cmd(arg,,)
< jonasschnelli>
Empty arguments in a bracket-like syntax looks strange to me.
< luke-jr>
jonasschnelli: IMO either it should be supported, or an error ;)
< jonasschnelli>
luke-jr: Yes. Agree. IMO it should thrown an error... I'll fix that.
< phantomcircuit>
jonasschnelli, is this for nested calls?
< jtimon>
sorry, can't access github now, but rebased https://github.com/bitcoin/bitcoin/pull/9279 and the windows builds keep failing in a way I cannot understand or reproduce, it says "int64_t does not name a type". Any help welcomed
< ryanofsky_>
jtimon: maybe it needs an #include <stdint.h>
< jtimon>
ryanofsky_: it has an #include <stdlib.h> but it may be just that, thanks, I'll try that
< gmaxwell>
wumpus: thanks for the merges!
< morcos>
I'm working on a PR to properly distinguish all the different uses of minRelayTxFee. It is my opinion that the dust threshold should not be arbitrarily changed by a desire to increase your minimum relay fee. Changing the definition of dust has a negative effect on the network by causing some txs to be non-standard, and potentially get stuck in your mempool.
< sipa>
morcos: i'm working on a per-txout chainstate... this simplifies so much
< morcos>
My question is, does it makes sense to be able to control the dust limit with a command line option... Or do we think it's only something that needs a hard-coded constant, that could be changed in a future release if we've had several releases that already use a higher value for tx creation
< morcos>
sipa: ah, i had some ideas about maybe how to do that.. i'll be curious to see what you come up with! i agree. it'll be good
< sipa>
morcos: doing it a very naive way, hoping that leveldb properly deduplicates the txids
< morcos>
won't take long to figure that out.. ha
< sipa>
but no need for CCoinsModifier etc anymore... just an AddCoins and SpendCoins function, which never need to call GetCoins on the lower-level, just HaveCoins to know whether to mark something as fresh
< gmaxwell>
morcos: I think our practice has been to make things configurable, so thats what you should probably do. (though I think this is often a mistake and it's not really here)
< sipa>
morcos: regarding dust... it's my opinion that a rational wallet should never create dust outputs by such a wide margin that the actual value of the dust threshold does not matter
< morcos>
ok already adding -incrementalrelayfee and -blockmintxfee , was hoping to avoid so many new command line options that wumpus reached through my screen to choke me
< gmaxwell>
(it's not really needed here, loast a word)
< gmaxwell>
sipa: you could put down the eng work on that for a moment, and make a quick change to adjust how the records work in leveldb. to see if it's a non-starter.
< morcos>
sipa: yes, thats goign to be among the changes i make.. we actually do an OK job of that now because of MIN_CHANGE, but we still have soem safety checks against IsDust which should be against a higher limit I think...
< morcos>
Unfortunately.. you can't put too much of a safety margin, otherwise you start having non-trivial amounts , which is espeically a problem when you are subtracting fees from recipients.. (where if your change would have been dust, then you take more money from the recipients to make your change higher!)
< morcos>
sipa: so aside from change, it makes sense for our wallet to refuse to pay someone an amount between dust and safetyMargin*dust as well?
< sipa>
morcos: i don't know about outputs... but for change, we shouldn't make anything between changeOutputWeight * effectiveRelayFee
< sipa>
s/between/below/
< morcos>
sipa: my idea is that what is important is that the txs your wallet creates will likely be considered standard for sometime
< morcos>
so minChange takes care of making a best-efforts basis to not produce anything uneconomical
< morcos>
but it would makes sense to have some dustCreationTheshold = SafetyMargin * dustThreshold
< morcos>
that applies to everything...
< morcos>
then if ever need to change dustThreshold.. we're not causing old wallets to create unrelayable/unminable txs by accident
< morcos>
Can we add one of those checkboxes that lets bitcoin-qt send back diagnostics to us about how it's actually used so we can improve it? :)
< jtimon>
btw ryanofsky_ thanks was that
< jtimon>
morcos: mhmm, of -incrementalrelayfee and -blockmintxfee which one is the one for the dust? I assume the second, what about calling it -dusttxfee ?
< morcos>
jtimon: ha. neither! thats why i was hoping to avoid a third.
< jtimon>
oh, mhmm, I lack the context on why you need 2 already
< morcos>
will explain in the PR
< gmaxwell>
morcos: be sure to include private keys.
< jtimon>
sure, no hurry
< gmaxwell>
morcos: so long as what the wallet targets and what the network enforces are seperate settings, then we don't have any problem with changing it in the future... (wallet must be changed amply ahead of the network, if the network will become more restrictive)
< gmaxwell>
I hope there is no reason to make the network more restrictive of dust in the future.
< paveljanik>
gmaxwell, ;-) I expect it will be rbtced in 5, 4, 3, ... ;-)
< morcos>
jtimon: yes, certainly don't object to moving it out of consensus, but I think it's actually useful that you can pass in a feerate to use. For instance what we were talkign about with gmaxwell , you might want to use a higher threshold for creation than the policy dust limit.
< jtimon>
mhmm, I hadn't though about that, maybe a reason to modify #9279 ...
< jtimon>
I was hoping to encapsulate the variable behind a policy class at some point...
< jtimon>
I guess another option would be to have 2 functions/methods that call the same one internally
< morcos>
jtimon: yeah i think that would be fine and an easy change: GDT(txout) { return GDT(txout, ::minRelayTxFee); }
< jtimon>
I mean, if we plan to encapsulate the value later that's 2 rounds of disruption instead of one
< morcos>
and could even just make that change later if you don't want to now.. maybe i will wait on my dust change until yours get merged, but i'd really like to have dust stop changing every time you change the minrelaytxfee
< jtimon>
I mean, didn't mean to stop you
< morcos>
well i like your change.. and i have to potentially change all the IsDust call sites too, so no reason to do it multiple times. I think the change you made to them makes sense, and then maybe some of them will later take an optional feerate argument if we want to use a higher dust creation threshold
< jtimon>
my point about was we could have policy.IsDust(txOut) and policy.IsDustCreate(txOut) if we want ot have 2 dust rates, but I'm not sure I understand the purpose of the create one
< jtimon>
or that
< jtimon>
optionally taking another one if we maybe want to have more than 2 rates
< gmaxwell>
jtimon: any time there is a limit on relay behavior there really should be two limits-- one used for relay and one equal or more conservative one for the wallet.
< gmaxwell>
Otherwise it is impossible to change the limit without causing the creation of limit violating transactions.
< morcos>
gmaxwell: ^ just a first step to avoid having to deal with a complicated edge case... but maybe it won't be acceptable to people. it seems odd to me that this would be a case that is hit very often, as i expect subtractFeeFromAmount is used when you want to send whole balances or coins.
< morcos>
but i admit i don't know how its used....