< phantomcircuit>
wumpus, fyi the two prs for wallet stuff dont merge cleanly against each other because of a trivial conflict
< phantomcircuit>
if one gets merged i will rebase the other and/or the merge can fix it
< wumpus>
damnit, trying to copy the transifex resource for 0.13 but looks like my script to clone transifex resources no longer works, transifex API PUTs take forever now
< sipa_>
#8149 is 25% consensus/p2p/node changes, 10% wallet/signing changes, and 65% tests
< sipa_>
(the tests mostly due to sdaftuar)
< sipa_>
well, mostly is exaggerated, but more than half of it is from his p2p test
< * sipa_>
wishes that git/github had a way to organize commits in a PR hierarchically
< instagibbs>
yes, those tests are very comprehensive, thanks sdaftuar :)
< kanzure>
sipa_: could you add some text to the top-level comment in pull request 7910 to refer to 8149 (otherwise folks will have to read all the way to the bottom to learn about 8149)
< cfields_>
luke-jr: ping. I'm not seeing your "-dirty" behavior
< MarcoFalke>
sipa_: How do you plan to handle "segnet is not intended to be merged into Core"
< MarcoFalke>
is it going to be merged to result in the same tree hash?
< sipa_>
MarcoFalke: in 7910 i will add a commit to remove it; in 8149 is will be removed from history
< sipa_>
i'll keep updating both PRs now, as i don't expect many changes anymore
< kanzure>
i wonder if git can revert a (given) patch post-rebase
< sipa_>
but there are still a few (GBT, dns seeds, segnet, ...)
< sipa_>
kanzure: pretty sure it can, if it doesn't conflict
< kanzure>
alright then the check is "post-rebase, revert the remove-segnet patch, then check to see if the tree matches" (or you can compare 7910 latest tree hash with 8149 latest tree hash)
< sipa_>
i'll just keep both in sync
< MarcoFalke>
Also, I think GitHub orders by commit date and not author date.
< MarcoFalke>
If that is something to care about
< sipa_>
MarcoFalke: that's why i provide my own list of commits...
< sipa_>
i want them to be dependency-ordered, not time
< sdaftuar>
sipa_: what's your preference for which PR we should post additional review comments?
< sipa_>
sdaftuar: 7910
< MarcoFalke>
Better keep it in 7910, so it is not all over the place?
< sdaftuar>
sounds good
< instagibbs>
might want to note that directly in both PRs
< MarcoFalke>
For the general direction, I agree with what sipa commented on the pull. We should probably rework the wallet fee behavior... But I don't see that happen in 0.13.0
< wumpus>
yes, the wording sounds good to me
< wumpus>
and indeed an option for confirm target would be nice too
< wumpus>
but that's something else, no need to do it there
< wumpus>
I think being consistent with amounts is pretty important, there is already one place where satoshis values leaked in the RPC API (the fee delta in mining.cpp), we were too late with changing there as that version was already deployed
< MarcoFalke>
Making it confirm target would change the behavior and users seem to be very sensitive to such wallet behavior changes
< MarcoFalke>
as we noticed in 0.12
< helo>
sorry to bikeshed... i know a confrimation target can't be guaranteed, but can it have confidence intervals to temper expectations?
< wumpus>
don't we already have a confirm target?
< wumpus>
this would just override the global steting
< MarcoFalke>
not for fundrawtx?
< wumpus>
ok, then only use it when the option is specified
< wumpus>
I assumed it'd just use the global default confirmtarget unles specified otherwise
< MarcoFalke>
And if the users didn't set anything, it will also happen to "fall back" to txconfirmtarget
< MarcoFalke>
jup
< wumpus>
that's what I mean
< wumpus>
helo: if you find a way to compute those, sure
< helo>
i'll give it a shot, we'll see
< sipa_>
helo: it aims for 95% confidence, iirc
< phantomcircuit>
wumpus, rebased #8142
< MarcoFalke>
based on the observations from the past
< wumpus>
phantomcircuit: thanks
< MarcoFalke>
But I don't think you can figure out confidence intervals for the future
< sipa_>
MarcoFalke: of course
< helo>
yeah, you can't... just bayesian i'd assum
< MarcoFalke>
Yet another 32-bit 64-bit issue...
< MarcoFalke>
Can we forbid floating point multiplication for CAmount?
< sipa_>
MarcoFalke: where?
< MarcoFalke>
In the tests
< MarcoFalke>
behavior is not the same on 32bit arch and 64
< wumpus>
floating point for CAmounts? heresy!
< MarcoFalke>
.6 * 100000 sat will give 59999 sat on 32bit under some circumstances
< wumpus>
(and if you do you at least use correct rounding instead of truncation, but please don't use floating point for monetary amounts)
< MarcoFalke>
Haven't yet figured out why it is working right now, but I was doing some refactor and triggered it.
< GitHub154>
bitcoin/master 152ab23 Patrick Strateman: Improve CWallet API with new GetAccountPubkey function....
< GitHub154>
bitcoin/master 52c3f34 Wladimir J. van der Laan: Merge #8142: Improve CWallet API with new GetAccountPubkey function....
< GitHub33>
[bitcoin] laanwj closed pull request #8142: Improve CWallet API with new GetAccountPubkey function. (master...2016-06-02-cwallet-getaccountpubkey) https://github.com/bitcoin/bitcoin/pull/8142
< sipa_>
i'd like to see some P2P tests for the eviction logic being touched in #8084 etc
< sipa_>
but i don't know how to do that easily, as you'd need to spoof ip addresses
< kanzure>
maybe te network refactor stuff could make that easier?
< kanzure>
*the network
< sipa_>
i expect it to, indeed
< luke-jr>
cfields_: pong
< GitHub72>
[bitcoin] MarcoFalke opened pull request #8151: [init] Make feefilter option debug option (master...Mf1606-feefilterDebug) https://github.com/bitcoin/bitcoin/pull/8151
< MarcoFalke>
wumpus, I think the issue is "JSONRPC error: Expected type number for feeRate, got string"
< MarcoFalke>
ups
< MarcoFalke>
forget what I said
< hello_>
Could someone explain the most recent issue raised on github ""Tell other nodes to filter invs to us by our mempool min fee" message is too technical"
< hello_>
What is "invs" ... and what is the context here?
< sipa_>
helo: if you need to ask what invs are, then it's an indication that the messages is indeed too technical
< sipa_>
and that is the issue
< gmaxwell>
it's not even a setting an ordinary user should ever touch.
< gmaxwell>
-makemynodeworkworse
< instagibbs>
jl2012, have you adapted the BIP/PR for relaxing witness program size yet?
< sipa_>
instagibbs: yes, it's already included
< sipa_>
(to length 40)
< instagibbs>
ah thanks
< hello_>
Well Im trying to contribute to bitcoin... So I would be happy if you could just give a line of explanation. :) Cheers..
< hello_>
sipa_: I already understand about 50% of the software so ....
< sipa_>
hello_: i'll gladly explain things, but i don't think this is the place to explain the basic p2p protocol :)
< MarcoFalke>
hello_: There is a bitcoin wiki with the basic description
< sipa_>
for breaking changes (not csv, but for segwit it would) it requires that explicit support be indicated in the GBT call request
< sipa_>
(which makes sense, as they'd construct bogus blocks without, but i wonder if that's not a problem for downstream software)
< sipa_>
perhaps a command-line flag saying "yes, my mining client supports feature X, don't require it to tell you every time" is more convenient
< luke-jr>
and then have downstream software fail to implement it?..
< luke-jr>
easier to just tell you every time
< luke-jr>
especially considering this is GBT so there's no bandwidth savings from omitting it
< gmaxwell>
commandline flag seems error prone, you don't upgrade one piece of downstream software and 20% of your infrastructure is producing invalid blocks.
< sipa_>
luke-jr: that seems reasonable, but i haven't seen anyone comment on what changes this would require in their stack
< luke-jr>
adding a key to the JSON object is trivial in every stack I know of
< Lightsword>
sipa_, since stratum server requries a patch anyways can easially include the explicit support in the patch
< sipa_>
ok, thanks
< Lightsword>
sipa_, are there any potential issues with making a request indicating SW support to a non-SW capable node?
< Lightsword>
it’s common to upgrade nodes separately from the stratum server
< sipa_>
i do not believe that's a problem... you can always indicate support for more things than the server
< luke-jr>
it shouldn't be a problem, GBT has always required the Object, and while bitcoind tolerates it being omitted, it also tolerates it being provided as spec'd
< sipa_>
i think the PR always does what you'd expect; if you upgrade bitcoind before the client, it will not set the relevant bit by default during the started phase, and fail after activation (as you're unable to construct a valid block anymore)
< sipa_>
if you upgrade the client before the server, no effect
< GitHub189>
[bitcoin] pstratem opened pull request #8152: Remove CWalletDB* parameter from CWallet::AddToWallet (master...2016-06-06-cwallet-addtowallet-remove-cwalletdb) https://github.com/bitcoin/bitcoin/pull/8152
< GitHub142>
[bitcoin] MarcoFalke opened pull request #8153: [rpc] fundrawtransaction feeRate: Use BTC/kB (master...Mf1606-univalAny) https://github.com/bitcoin/bitcoin/pull/8153
< MarcoFalke>
jonasschnelli: I think to make it BTC/kB, you'd need to modify univalue.
< MarcoFalke>
(see pull)
< jonasschnelli>
MarcoFalke: hmm.. i think you have added the VANY only because of the RPCTypeCheckObj check? Right?
< MarcoFalke>
jup
< MarcoFalke>
it is ugly
< jonasschnelli>
Can you not add a third parameter to RPCTypeCheckObj's array?
< jonasschnelli>
mandatory, or something...
< luke-jr>
everyone is using satoshis per byte these days, which is much more logical in this case
< luke-jr>
we haven't counted fees per kB in a long time
< sipa>
i think consistency is more important
< luke-jr>
I suppose
< jonasschnelli>
sipa: agree with sipa
< luke-jr>
it should probably be documented that it's not *actually* BTC/kB though
< sipa>
how do you mean?
< luke-jr>
in the RPC help or whatever
< jonasschnelli>
If you use "estimatefee 10", you should be able to take the output as a fundrawtransaction feerate input
< jonasschnelli>
so, BTC/kb
< luke-jr>
mBTC/B
< MarcoFalke>
luke-jr: Would work but you'd have to replace it in every doc/rpc help
< MarcoFalke>
and I don't think it's less confusing
< jonasschnelli>
If we should change the API, we would use satoshis everywhere.
< luke-jr>
we switched from BTC/kB to mBTC/B in 0.10
< luke-jr>
yeah, policy/fees.cpp uses CFeeRate, so estimatefee is almost certainly mBTC/B in practice
< jonasschnelli>
maxtxfee: /** Absolute maximum transaction fee (in satoshis)
< luke-jr>
anyway, this seems to be a deeper documentation issue than expected, so perhaps out of scope for MarcoFalke's PR
< cfields_>
luke-jr: could you please specify the case where you see "-dirty" and shouldn't? I
< cfields_>
I'm missing some detail to reproduce
< sipa>
luke-jr: BTC/kB or mBTC/B is the same thing; i assume you're talking about whether it's counted per full byte or not... but fuel prices are also advertized per gallon, but counted at much smaller increments
< luke-jr>
cfields_: it seems the chown is important
< luke-jr>
sipa: fair enough, but historically we have counted per full kB, so it's confusing
< cfields_>
luke-jr: interesting. thanks.
< cfields_>
luke-jr: just as an aside, you know about git new-workdir/git worktree, right?
< sipa>
luke-jr: agree, but i guess we can add a "counted per byte" somewhere
< luke-jr>
cfields_: no, I don't..
< sipa>
worktree is awesome
< cfields_>
luke-jr: ah, you'll want to google that then. sounds like you may be making your life harder if this test represents your workflow at all
< luke-jr>
how is it different than git-clone?
< sipa>
it doesn't create a new repository
< sipa>
so not duplicate storage of the entire history
< cfields_>
luke-jr: clone once, then have as many local checkouts in separate dirs as you want
< cfields_>
^^ what he said
< sipa>
this creates multiple workdirs with the same repo
< luke-jr>
git-clone uses hardlinks..
< sipa>
ah, didn't know that
< sipa>
well, it also has the advantage of not needing to push/pull between them
< cfields_>
luke-jr: iirc the hardlinks are optional if you have fs restrictions
< luke-jr>
ah
< luke-jr>
that might be handy
< cfields_>
sorry for the tangent, just thought it may be useful. I'll try to reproduce the case above.
< luke-jr>
on another note, I wouldn't suggest using it with the test case I just gave, and might even want --no-hardlinks.. who knows what the chown will do :P