< morcos>
achow101: i'm not sure estimatesmartfee is really designed for that kind of stable lower bound.. I agree it shoudl be possible to hack something together though
< morcos>
I'm not sure what you mean about it returning a rate of 0
< achow101>
it returns CFeeRate(0) on failure
< morcos>
That shouldn't happen beyond the very first few blocks after you turn on a new node
< morcos>
Why are you getting a failure
< achow101>
confirmation target is 1008
< achow101>
(It seems like it fails)
< morcos>
Because it returns 0?
< achow101>
yes? maybe I'm wrong.
< morcos>
What do you mean yes? Is it returning 0
< morcos>
It should return an estimate for the highest target it can
< morcos>
If for some reason you are getting 0, that is a problem
< achow101>
I'm still writing the code so I don't know
< morcos>
oh
< morcos>
you know there is an rpc call so you can see what estimatesmartfee will give you on a node
< achow101>
right. It gave me -1
< morcos>
ok, that is surprising
< achow101>
ehh, that's actually for a target of 2016, not 1008
< morcos>
how long has it been up?
< morcos>
ok, yeah 2016 is not supported
< gmaxwell>
$ ./bitcoin-cli estimatefee 1008
< gmaxwell>
-1
< morcos>
anything >1008 will always return -1
< morcos>
ARGHHH
< morcos>
i'm going to strangle all of you people
< sipa>
gmaxwell: estimateSMARTfee :)
< morcos>
estimatefee is deprecated
< achow101>
18:13:36
< achow101>

< achow101>
estimatesmartfee 1009
< achow101>
18:13:36
< achow101>

< achow101>
{
< achow101>
"feerate": -1,
< achow101>
"blocks": 1009
< achow101>
}
< achow101>
-1 for anything greater than 1008. is that supposed to happen?
< morcos>
yes
< morcos>
that is supposed to happen
< gmaxwell>
estimatesmartfee gave me -1 too but I was calling it with 2016 because some reason I thought that was the maximum.
< achow101>
why doesn't it just use the highest available like it does for 1008?
< morcos>
achow101: because it is telling you that you are using it wrong
< achow101>
oh
< morcos>
it will never be able to give you an answer for 1009 or 2016
< morcos>
but for 1008 you are using it correctly, so it gives you the best answer it can and tells you what target that answer was for
< morcos>
the design is so that if you do estimatesmartfee N for 1 <= N <= 1008 it should very very rarely give you a -1.
< morcos>
This will happen only if you have a brand new node you just started up, or if your node has been down for > 6 weeks
< achow101>
ok
< morcos>
And then it'll only last for a couple of blocks after your node is fully synced
< morcos>
For your purposes I would recommend that you ask estimatesmartfee 1008
< gmaxwell>
achow101: so your issue is just that you weren't using estimator.estimateSmartFee ?
< achow101>
I went off your word and used 2016 originally :p
< morcos>
Look at the blocks it returns, and if it returns blocks < 144 for example, then perhaps you want to take the min with the fallback fee or something
< gmaxwell>
morcos: for clarity, he's changing CreateTransaction code in wallet.cpp not using the RPC. :)
< gmaxwell>
achow101: oh see, don't listen to me.
< morcos>
yeah makes sense, but the answer should be the same as the RPC gives you
< achow101>
ok
< morcos>
i'd also see if you get use the GetMinimumFee function (if thats what its called) which will do smart things like max with the mempool min fee
< gmaxwell>
morcos: looks like he was doing the right thing but I'd quipped call it with 2016 blocks or something. Which he did, which then resulted in "wtf is it using CFeeRate(0) as its return value for error cases" then also saw that estimatefee -1ing on 1008 and sooo.
< morcos>
one thing that might be missing is documentation of the 1008 upper bound
< morcos>
is that in the rpc help?
< achow101>
morcos: nope
< morcos>
yeah that should get added
< gmaxwell>
why doesn't estimatesmartfee 2016 just reutrn the 1008 result (it gives blocks in the result, so if its telling you a lower number, you know)
< achow101>
I was looking at the code for estimatesmartfee too, but not a lot of comments there either :(
< gmaxwell>
fortunately dumb code that just uses the answer w/ a negative feerate will only manage to produce an invalid txn. :)
< morcos>
gmaxwell: well i mean the real answer is this all just built on prior version which always returned -1 if you put something outside the allowable range
< gmaxwell>
doesn't sound very smart.
< gmaxwell>
:P
< gmaxwell>
morcos: than you for the cluestick.
< morcos>
but i do think it is somewhat reasonable for it to indicate to you that you used an argument outside the supported range
< morcos>
I want to implement this for fee esitmation. But sounds like it would also be good for what you are doing
< morcos>
I guess you'll get it automatically if its just inside of estimatesmartfee
< morcos>
oops slight typo fixed in that issue
< gmaxwell>
well two issues: I think that you should get a best effort answer if you give too big a value-- indicating with blocks would be fine, but also if we can't give an answer we should omit the field, not use -1.
< gmaxwell>
(mentioning it for the future, not blaming anyone (esp not here)-- often magic values for errors results in failures)
< morcos>
Yes I agree
< morcos>
But does that mean we should change it?
< gmaxwell>
I dunno. We could create an estimatesmarterfee with the new interface.
< morcos>
I suppose I should have changed it when I made estimatesmartfee since that was a new AO
< * gmaxwell>
ducks
< morcos>
API all together
< gmaxwell>
yea, sorry I didn't catch that then.
< sipa>
gmaxwell: saneestimatesmartfee
< sipa>
:p
< gmaxwell>
footgunlessfees (perhaps an overstatement)
< gmaxwell>
It doesn't help that C/C++/and friends don't have option types like rust and haskell, so overloaded error-return types are in people's blood.
< gmaxwell>
Pieter pointed out earlier that C++17 has one, but without looking I'm kind of dubious about its utility because the rest of the language (and libraries) weren't built around it being there.
< sipa>
well, C++17 does not technically exist, but it is with near certain probability expected that it will have it :)
< instagibbs>
gmaxwell, so thinking ahead, if we replace IsDust change checks with something more reasonable, is there any reason to try again rather than simply prune/keep the change and exit?
< instagibbs>
I feel like I'm writing extra logic here just to protect our ridiculous IsDust logic :)
< gmaxwell>
Are you asking if there is any reason to loo kat all?
< gmaxwell>
gah, loop at all.
< sipa>
what is looing?
< sipa>
ah
< instagibbs>
correct
< instagibbs>
specifically looping for change, not for fee(different issue we can tackle later)
< gmaxwell>
obviously for fee that is unaviodable without effective rates.
< gmaxwell>
Gonna have to explain what behavior you think we're protecting on the change part.
< * gmaxwell>
brings up the code now
< instagibbs>
Ok so right now it's 1) < IsDust, dump 2) < MIN_FINAL_CHANGE : loop 3) < MIN_F_C keep
< * instagibbs>
heading off for now
< gmaxwell>
right if we get a viabile solution and the change is under dust, turn the dust to fee and call it done.
< gmaxwell>
otherwise, if its under min_final_change loop with a bigger target to try to get enough coins to meet that criteria.
< gmaxwell>
otherwise we're done.
< gmaxwell>
and you're asking if that dust were smarter could we skip test 2?
< instagibbs>
yes
< gmaxwell>
I _think_ that even if dust is smart, there is s range of sizes where we would rather take the change than discard to fees, but would prefer our outputs be larger than that.
< morcos>
I think thats the tricky part of the logic
< gmaxwell>
Basically at the sane dust level the output is worth nothing. So obviously better to not create it. But above that it's worth something, but we'd still rather not have a bunch of almost nothing outputs in our wallet.
< morcos>
It depends on both the set of outputs you have (available and are currently using) and the fee rate you are using on this tx as opposed to what you might use on future txs
< gmaxwell>
certantly if you think fees now are lower than average you should be especially eager to aggregate.
< morcos>
I had written up a heuristic previously that did different things depending on what your tx confirm target is
< morcos>
yeah so how "smart" you want to get about this in an automated fashion is tricky
< instagibbs>
hm yes the "utxo management" aspect, ok ill keep it around for Future Work
< instagibbs>
So in essence we have aspirational utxo amounts/types that the wallet can strive for, and values that are considered worthless
< instagibbs>
former we have no real mechanism in place so min change is what we're using
< phantomcircuit>
i still think that have a score for a set of utxo's and then just randomly selecting utxo's to try is a reasonable approach
< bitcoin-git>
[bitcoin] RHavar opened pull request #10655: Properly document target_confirmations in listsinceblock (master...listsinceblock) https://github.com/bitcoin/bitcoin/pull/10655
< bitcoin-git>
[bitcoin] str4d opened pull request #10657: Utils: Improvements to ECDSA key-handling code (master...libsecp256k1-patches) https://github.com/bitcoin/bitcoin/pull/10657
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #10659: [qa] blockchain: Pass on closed connection during generate call (master...Mf1706-qaStopPass) https://github.com/bitcoin/bitcoin/pull/10659
< bitcoin-git>
[bitcoin] jonasschnelli opened pull request #10660: Allow to cancel the txdb upgrade via splashscreen keypress 's' (master...2017/06/chainstate_update_prog) https://github.com/bitcoin/bitcoin/pull/10660
< luke-jr>
blah, now there's an extra place to change "Core" :x
< jonasschnelli>
luke-jr: heh.. oh yes. But I guess you have already different pixmaps for Knots?
< luke-jr>
jonasschnelli: that one used to just be "bitcoin", so didn't need to change it I think
< jonasschnelli>
Branding!
< luke-jr>
I suppose there's no way to make the string part just a string? XD
< luke-jr>
(FWIW, patch files can't touch binary files, so I do all pixmap changes via SVG rendering)
< jonasschnelli>
Not worth... just replace the bmp for Knots...
< jonasschnelli>
Or remove that commit in knots
< luke-jr>
jonasschnelli: can't
< luke-jr>
replacing bmp, even to undo that commit, changes a binary file => breaks patch files
< jonasschnelli>
luke-jr: you can add a commit that restores the old pixmaps
< jonasschnelli>
However you do it. :)
< wumpus>
git patch can support binary files IIRC (--binary)
< luke-jr>
wumpus: yes, but not normal patch (IIRC there was some practical issues to using git-patch also, but I forget what)
< wumpus>
and otherwise just copy the binary files over, separately from the patch, 'patching' isn't very interesting in general for binary files unless there's some specific diff/mutation tool for that specific kind of file
< wumpus>
(which I guess is true for images! you could use python+PIL to render a different logo over the old one lol)
< * wumpus>
really likes c++17 std::optional, what took them so long
< wumpus>
I'm surprised complex features such as lambdas made it into c++11, but not something as straightforward as an optional value
< sipa>
wumpus: with lambdas you can emulate optional using church encoding :)