< GitHub1>
[bitcoin] EthanHeilman opened pull request #7696: Fix de-serialization bug where AddrMan is left corrupted (master...bug) https://github.com/bitcoin/bitcoin/pull/7696
< achow101>
will there be a full write up of the details of the segwit changes so that wallet developers can work on its implementation?
< achow101>
preferably before the release of the client with segwit?
< GitHub25>
[bitcoin] sdaftuar opened pull request #7697: Tests: make prioritise_transaction.py more robust (master...fix-prioritise-transaction) https://github.com/bitcoin/bitcoin/pull/7697
< Chris_Stewart_5>
Hi guys, I know this isn't the correct channel, but I figured some one in this channel might be able to answer my question - i've already tried #bitcoin-dev
< Chris_Stewart_5>
I'm looking at this test case from script_valid.json that is suppose to pass in core
< Chris_Stewart_5>
["0x4a 0x0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000", "0 CHECKSIG NOT", "", "Overly long signature is correctly encoded"]
< Chris_Stewart_5>
however the signature 0x00...000 isn't a valid DER signature - so why is this test case suppose to pass?
< Chris_Stewart_5>
interestingly enough, that same test case is also included in script_invalid.json as well
< sipa>
that test doesn't specify DERSIG as validation flag
< Chris_Stewart_5>
sipa: So this is a historical test case pre BIP-66?
< sipa>
or STRICTENC or LOW_S
< wumpus>
intersting
< wumpus>
I think he got confused by ["Increase test coverage for DERSIG"]
< sipa>
Chris_Stewart_5: in a way...
< wumpus>
after which none of the test cases supply the DERSIG flag :-)
< sipa>
Chris_Stewart_5: the consensus rules are still well defined for historical cases
< wumpus>
(some do, later on, but not under that heading)
< sipa>
wumpus: compare that section to the corresponding section in script_invalid; the tests verify the difference in validity by setting/unsetting that flag
< wumpus>
sipa: I believe you that it's correct, it just looks funny
< sipa>
that's the typical way these tests are constructed: find the minimal set of flag difference that cause validity to change, and put one side in valid and one side in invalid
< sipa>
probably deserves a comment!
< wumpus>
right
< Chris_Stewart_5>
thanks guys, If I am understanding this correctly, we would have to deploy another soft fork to make this signature valid again for bitcoin core nodes? Are the flags in the test case used ONLY for these test cases or is there similar flags used in bitcoin core's interpreter?
< sipa>
Chris_Stewart_5: it would require a hard fork to make them valid again
< sipa>
Chris_Stewart_5: that specific test tests the script evaluation engine, which does not know anything about consensus rules or transactions or blocks even
< sipa>
so it specifies the flags for evaluation with each test
< sipa>
other, higher-level tests exist that actually check whether the consensus logic evaluates things correctly
< Chris_Stewart_5>
sipa: Is that right though? Obviously OP_CHECKSIG has to know about txs because they are needed for hashing to compare against the sigs?
< Chris_Stewart_5>
or OP_CHECKMULTISIG
< sipa>
Chris_Stewart_5: nope, it's abstracted through BaseSignatureChecker
< sipa>
and then there is an implementation for that for CTransaction, but it can be used to verify signatures on other things than transactions
< Chris_Stewart_5>
interesting, I"ve been trying to model something similar in Scala, i'll have to take a closer look
< Chris_Stewart_5>
I think i was just running into this problem of who knows about what (does the interpreter needs to know about txs etc..)
< sipa>
there were some plans of introducing a new message signing feature based on this, so that you can do multisigs on a message, for example
< MarcoFalke>
wumpus, I was wondering what you think about the patches toGetFee()
< wumpus>
haven't really looked at that yet
< wumpus>
most of the fee logic honestly confuses me
< MarcoFalke>
Jup the current logic even has a bug
< MarcoFalke>
GetFee() is not monotonic in the size
< MarcoFalke>
Would be trivial to fix
< wumpus>
I''m not surprised - I think we should first document what we want, then try to achieve that in the code
< MarcoFalke>
So I'd prefer to keep the integer logic (truncate)
< wumpus>
currently it's impossible to say if behavior is desirable or not
< MarcoFalke>
morcos and jtimon suggested to make it always ceil
< wumpus>
well at the very least please don't use doubles for monetary values
< wumpus>
I don't have an opinion on ceil versus floor, that'd at most make a satoshi difference I guess?
< MarcoFalke>
sure
< MarcoFalke>
but ceil at least needs a double for the time of calculation
< wumpus>
so go with whatever results in the simplest code
< wumpus>
you don't ever *need* doubles
< jtimon>
I think avoiding the optional param as suggested by morcos certainly simplifies things
< morcos>
MarcoFalke: Why do we NEED to change something?
< MarcoFalke>
we need at least make getFee() monotonic
< morcos>
ehh
< wumpus>
morcos: yes, that's what first needs to be decided
< morcos>
MarcoFalke: btw, i'm totally fine with just truncating all the time too, but you were against truncating to 0
< morcos>
what i dont' want is more complexity
< wumpus>
if we don't need to change something that'd be preferable, so we can catch up to document the current behavior
< jtimon>
to be honest I'm still not sure what this is all about, even after reading the 2 PRs
< MarcoFalke>
ok, I will submit a minimal code change patch this evening
< wumpus>
if we could describe things in human understandable terms that'd help devs too
< wumpus>
as said, I've been maintaining this code for years and the fee code freaks me out
< jtimon>
what about always truncating but instead of ever returning 0, just return 1 satoshi?
< wumpus>
at least if there's a large change in behavior we should document it in the release notes next time :)
< MarcoFalke>
jtimon, that's what I am going to do
< morcos>
wumpus: i know you say you don't like doubles, but the mining code and the fee estimation code which are the things that use fee rates, both use them as doubles
< MarcoFalke>
will result in least code and diff
< wumpus>
morcos: that's awful
< jtimon>
MarcoFalke: oh, nice, what's the problem with returning 0 in the first place anywa?
< MarcoFalke>
we still use it in the wallet to "detect" what the user selected
< MarcoFalke>
paytxfee or confirmtarget
< jtimon>
and now the user can't select 0 fee?
< wumpus>
doubles don't have exactly the same behavior on all platforms, which make it dangerous to use them for monetary values, we've had some strange reports of behavior while using doubles in the RPC layer
< morcos>
wumpus: i mean i said that kind of for effect, its not really awful the way its used, and i'm fine keeping CFeeRate to not use doubles, but its worth keeping in mind that we're losing precision by converting it to some weird integer
< wumpus>
now we've got rid of them there
< jtimon>
just use int satoshis everywhere
< morcos>
but its satoshis per size
< wumpus>
morcos: if you lose precision with integer arithmetic at least in a predictable, deterministic, platform independent way :)
< wumpus>
but it should be possible not to lost precision, if you use the right amount of fixed point precision
< MarcoFalke>
Using doubles for bitcoin should be fine right now, as a double can hold all possible values with exact precision IIRC.
< MarcoFalke>
But we should avoid it for consistency
< morcos>
wumpus: well for instance in the mining (actually mempool sorting) code its calculation using integers but they are converted to doubles to ignore overflow risk
< wumpus>
'should be fine' but please don't
< wumpus>
they said the same about using doubles in RPC, and there's been some strange rounding errors
< MarcoFalke>
Jup, people will look at the code and think this is best practise
< morcos>
anywya, we're not talking about using doubles for an amount, i think everyone would 100% agree, that no bitcoin amount should ever be represented as a double (bitcoins.satoshis)
< jtimon>
I guess CFeeRate's ```/ 1000``` simplifies some parts of the code, but I still dislike the whole concept (and much more that this code is included in current libconsensus)
< sdaftuar>
CFeeRate is in libconsensus?
< MarcoFalke>
jtimon, you mean IsDust()?
< wumpus>
I'd really prefer to stick to the point that doubles and floating point arithmetic dosn't belong in financial software
< jtimon>
yes, from the beginning, I've been trying to move it out ever since without luck, but if someone else wants to rewrite any of my attempts I'm more than happy to review
< wumpus>
I'm sure it is not used by any actual consensus code and could be moved out of libconsensus in due time
< jtimon>
MarcoFalke: IsDust and dustThereshold too, that's the reason why it cannot simply be moved out (amount.h would remain without .cpp or all its code can be simply moved back to primitives/transaction.h but then policy/feerate.o needs to depend on the whole consensus/transaction.o instead of only amount.h)
< jtimon>
wumpus: it can be moved out of consensus at any time
< wumpus>
jtimon: right
< jtimon>
for the first version that was small enough to just leave it for later, but we're leaving it for later ever since
< jtimon>
s/we're/we've been
< wumpus>
anyhow, re; fee, I think we should first have a plan what we really want, what is the current behavior, what is the intended behavior, before we just start changing code again and surpise users
< morcos>
wumpus: to summarize what i think the CFeeRate current problem is , we definited it as satoshis/kB which doesn't have enough precision as an integer to not be a bit annoying (both close to 0 and too many ties in mempool sorting). not super annoying, but it was a bit of a silly definition
< wumpus>
morcos: yes I fully agree the current implementation of CFeeRate is somewhat silly
< jtimon>
I guess the advantage of using KB instead of just bytes it's to contemplate fees below 1 satoshi per byte, which I have no idea if currently makes sense
< morcos>
yes, jtimon from a human communication format satoshis/B makes more sense, but for precision, you really want satoshis/MB
< jtimon>
so any more generic replacement to contemplate lower fees would do it, I think
< wumpus>
well the internal representation doesn't have to depend on human communication format at all
< wumpus>
it cen be presented in whatever way makes sense to the user
< morcos>
well CFeeRate is almost only for human communication format
< morcos>
most things internally store fees and size separately
< morcos>
and do the calculations as necessary
< jtimon>
alright, so we need more precission and 1000 happens to be too small of a divider some times. what about an optional bytes param that defaults to 1000 ?
< wumpus>
human communication depends on the formatting function only
< morcos>
or like the estimation code use doubles b/c they're doing all kinds of crazy exponential decays and stuff
< wumpus>
jtimon: you mean make it a rational?
< jtimon>
I think that would open the door to some more simplifications for cases where you multiply by 1000 and divide by tx_size right after dividing by 1000 in the exnapsulated stuff
< wumpus>
well it's better than a double :)
< jtimon>
no, wait I've probably said something stupid, let me think more while looking at code
< morcos>
well, its not causing me any issues right now, so i prefer no changes to anything that isn't an obvious improvement
< wumpus>
morcos: absolutely
< wumpus>
that's my whole point about the fee logic, really
< wumpus>
first consider what is the current case, and what we want, so we can define what is an improvement
< morcos>
the problem MarcoFalke is trying to solve (or at least part of it) isnt' a problem with CFeeRate, but with a shortcut in how we detect whether the user has selected to paytxfee
< morcos>
it would make more sense to find out whether the paytxfee option has been set, and not whether it = 0 or rounds to 0 in terms of decidign whether to use dynamic fee estimation
< morcos>
thats would be the ideal fix, but that would be a change of behavior potentially for users
< wumpus>
I've been kind of caught by surprise by the fPayAtLeastCustomFee stuff, for example
< morcos>
right now you can't select paytxfee=0 , that just seems silly, you should be able to set whatever fee you want, and if you select a fee, you don't use fee estimation, even if your fee rounds/truncates/ceilings/doubles/or trampolines to 0
< wumpus>
"how we detect whether the user has selected to paytxfee" let's just follow the straightforward route and add a boolean then
< wumpus>
second guessing the user sucks
< wumpus>
sure - we can change the API, it's not prohobitive to change the behavior at all, just be sure to mention it in the release notes :)
< MarcoFalke>
I should have done the release notes as part of the pull probably. Still, no one yelled at me all the time before 0.12 was released. Even though my original pull was from September.
< wumpus>
and we shoudl really write a document what the various fee options do, how they interact, what is the result in practice
< wumpus>
it's soo flexible that no one understands really :)
< wumpus>
I'm not blaming you at all MarcoFalke, it's a strange coincidence of circumstances
< MarcoFalke>
you suggested to add a .md in /doc? I'd rather not do this taking considering that you need a pull every time someone wants to edit the file.
< wumpus>
I don't care where
< MarcoFalke>
bitcoin wiki it?
< MarcoFalke>
or the website?
< MarcoFalke>
Do we have infrastructure on the website for documentation yet?
< wumpus>
fine with me, I usually put stuff in gists but that's hard to find I suppose
< wumpus>
well on the website you need a pull for every change too
< MarcoFalke>
but bitcoin/bitcoin commits are read by somewhat more people than the website commits
< wumpus>
wikis have the problem that there's no change control at all
< wumpus>
that's why we moved the BIPs to git
< wumpus>
anyhow this isn't imporant, if you write it you get to decide where to put it
< wumpus>
bitcoin.org has extensive infrastructure for documentation, don't think bitcoincore.org has
< morcos>
what do you think about changing the names of those options. i mean i know thats annoying. but calling them paytxfeerate instead of paytxfee would make a whole lot more sense
< morcos>
i don't know if its easy enough to do that in a compatible way, i guess we'd have to consolidate where the arguments are looked at
< wumpus>
why not just document them better instead of change the name :)
< morcos>
yeah i guess there are a ton of them
< wumpus>
I mean for a clean slate, in retrospect, it would be awesome to name them differently, and as in any program there are plenty of awkwarly named options... but yes there's a strong backward compatibiltiy versus sensibility for new users compromise
< GitHub37>
[bitcoin] MarcoFalke closed pull request #7660: [amount] Extend GetFee() by optional flag ceil (master...Mf1603-amountCeil) https://github.com/bitcoin/bitcoin/pull/7660
< GitHub110>
[bitcoin] MarcoFalke closed pull request #7661: [wallet] Round up to the next satoshi on odd fee rates (master...Mf1603-walletCeil) https://github.com/bitcoin/bitcoin/pull/7661
< wumpus>
and it's possible to argue that the name of the options themselves is just arbitrary, it's just a name, as long as it's not completely deceptive what matters is how they're documented
< MarcoFalke>
Would be great to have different option names map to the same option to be used as synonyms... Which brings us back to the rewrite of the arg parser...
< wumpus>
well we do have some options that just throw an error if you provide them, and mention that they're either removed or have a new name
< wumpus>
but there's got to be a good motivation to do that, it shouldn't look like just sending the user on a wild goose chase :)
< MarcoFalke>
paytxfee is still widely used. Would be nasty to require everyone change their conf
< morcos>
wumpus: btw, i made your change to 7187, should i go ahead and squash all the commits, not sure if you thought review was done
< MarcoFalke>
wumpus any update on "wumpus: Would it be possible for us to arrange open source licenses of the Jetbrains IDEs for C++ and Python? They offer free licenses to projects like this"
< wumpus>
e.g. there's an issue to rename '-rescan' becuase many people use it out of confusion... well, yes, but you can't really keep the new name secret to clueless users :)
< wumpus>
morcos: I think review was good for that, we should merge it
< wumpus>
MarcoFalke: yea will do that, haven't got around to it
< wumpus>
morcos: (and squash, yes)
< MarcoFalke>
morcos (and others) Make sure to have the commit id somewhere in the GitHub discussion before you squash. Otherwise it's less transparent and harder to re-review if the reviewer hasn't pulled your repo yet.
< wumpus>
morcos: I think changing the semantics in potentially dangerous way would be a good reason to rename them, and error when the old name is used
< morcos>
wumpus: like maxsigcachesize :)
< morcos>
ok done
< wumpus>
morcos: ehh that changed from entries to MiB didn't it
< wumpus>
morcos: yes, luckily it's an option no one knows about :)
< morcos>
yeah, i mean the damage is probably not that large, just an unlimited sigcache
< wumpus>
but yes it'd have made sense to change the name
< btcdrak>
The mempool behaviours for BIP68/112 have been backported by cherry-pick to 0.11 and 0.12 in PRs #7543, #7695 and #7693. Can I ask a few eyes you verify they are correct.