< gmaxwell>
promag: does that actually make a measurable difference? I would assume very few transactions are fromme, so the early termination would never fire.
< gmaxwell>
I'd expect a lot bigger gain from things like hoisting the locking cs_wallet out of the inner loop or similar.
< promag>
GetDebit(tx) is called from a lot of places
< promag>
it also avoid unnecessary IsMine calls
< promag>
not saying it's a big performance improvement
< promag>
regarding locks, IMO only after 0.16 is branched
< wumpus>
we really need benchmarking for wallet things
< wumpus>
without that, these kind of changes are really hard to evaluate, whether it's worth complicating the code more
< promag>
anyway, in that case, I'd say it's a pretty obvious improvement
< gmaxwell>
I don't agree. It turns one line of simple code into five lines of less clear code. I don't think it's bad, but if its not actually faster for any realistic case, why would we want it?
< gmaxwell>
maybe it is-- e.g. perhaps we call IsFromMe on masses of wallet txn all the time, where its true on the first input... but I'm not aware of that.
< gmaxwell>
Certantly to the extent that the function is run on transactions in the chain the early termination won't make it faster-- I doubt it would make it slower either, but not faster.
< promag>
gmaxwell: I think the semantic of IsFromMe it better explained in that pr
< promag>
"if from me if at least one input is debit"
< promag>
s/if/is
< gmaxwell>
I know what the semantics of the functions are. But early termination is not a win for a function where 99.999% of the time it will never fire, it can even be a performance loss (though I am not saying I expect that here).
< promag>
also, for big wallets, with thousands of transactions, thousands of keys, avoiding IsMine is important
< gmaxwell>
I'm only commenting on the IsFromMe early termination.
< promag>
gmaxwell: sure, for the worst case the performance is the same (although it avoids summing)
< promag>
also not sure about the average case
< gmaxwell>
And, as far as I know, for the average case. If not, we're probably calling it where we shouldn't be at all.
< gmaxwell>
(at least off the top of my head I only know that function being called on all blockchain/relay txn, where it'll return false almost all the time for even the largest users. I think we also call it on some unconfirmed stuff.)
< gmaxwell>
ha
< gmaxwell>
see this is silly, for example:
< gmaxwell>
int nDepth = pcoin->GetDepthInMainChain();
< gmaxwell>
if (nDepth < (pcoin->IsFromMe(ISMINE_ALL) ? 0 : 1))
< gmaxwell>
continue;
< gmaxwell>
so it looks like there are a couple other places it's called, but none strike me as that interesting at a glance, though some could be optimized.
< gmaxwell>
(actually that code looks redundant with the efficient IsTrusted check above)
< gmaxwell>
but don't listen to me, I should be asleep. :)
< wumpus>
otherhwise I'm going to branch off 0.16 without them
< meshcollider>
I'd like to review the fd one to help but its beyond my skill level I'm afraid
< wumpus>
you're not alone in that
< wumpus>
I'm confused by it too
< wumpus>
please do review BlueMatt's though
< wumpus>
that one is not so hard to grasp as I thought initially
< wumpus>
oh you did!
< wumpus>
thanks
< gmaxwell>
ut-sleepy-ACK from me, I'll try to setup some kind of shutdown loop to test it.
< gmaxwell>
I'd bias toward including since we have known easily triggered shutdown deadlock, if users report that on rc1 without this we learn nothing, if they report it or a shutdown crash on an rc1 with this we learn something.
< wumpus>
yes I'd also vote for including #12266, but I think it's quite harmless to hold up on #12274 for now
< wumpus>
it's a lot of logic to work around an upstream issue, and according to test reports there it doesn't even fully solve the issue yet
< wumpus>
also it's regression in 0.15 and it only comes up under rare conditions
< gmaxwell>
when I looked at the limiter I had questions about concurrency of the limiter function and didn't know where to start looking.
< wumpus>
sorry I mean it's *not* regression in 0.16
< wumpus>
it's an issue that has existed since we started using libevent for http, and very hard to trigger
< wumpus>
I don't mean it should not be solved, but I wonder if we shouldn't take a higher level approach and try to fix upstream, instead of hack on hack on hack :(
< gmaxwell>
right. we could also cut down our FD usage in some other way to give it more safty margin, I suppose?
< gmaxwell>
e.g. raise MIN_CORE_FILEDESCRIPTORS
< wumpus>
right - maybe the accounting of fds (how much is reserved for what) is also incorrect
< wumpus>
anyhow I think this is a post-0.16 issue
< bitcoin-git>
bitcoin/master 10847fe Wladimir J. van der Laan: qt: Periodic translations update...
< xabbix>
Can someone please explain how the mempool is filled for a new node and relayed? If I just finished syncing up my node, is it possible that I'll get a tx into my mempool that was sent a week ago (and still haven't confirmed)? Does my node request the mempool from the 8 peers it is connected to? Or does it just start collecting what is relayed to it from the point it was launched?
< gmaxwell>
xabbix: the last thing.
< xabbix>
gmaxwell: thanks, are there cases that a new node will get an 'old' transaction? what are those cases?
< wumpus>
it will start collecting transactuins once it leaves initial sync. Normally these are new transactions, though there's nothing preventing people from re-relaying old transactions.
< gmaxwell>
someone relays it.
< gmaxwell>
the sender and reciever of the txn will rebroadcast every once in a while, normally...
< gmaxwell>
and random other people might do so for whatever reasons they feel like.
< xabbix>
gmaxwell, wumpus: ok, that makes a lot more sense now. thanks.
< gmaxwell>
it won't cross peers that already have it, however.
< xabbix>
As I understand from an answer that jnewbery wrote to me a few days ago, the `time` value in the mempool is saved and relayed by other nodes, so even though my node is 'new' I can get txs with `time` values of before I ever started my node, but the `height` value is not stored and therefore not very reliable. Is there an easy way of calculating what the block height was at a certain time?
< gmaxwell>
it's not relayed.
< xabbix>
it's = height?
< wumpus>
transactions generated by modern wallets will usually be time-locked to the current (or current-1) block
< gmaxwell>
xabbix: no time value is relayed.
< wumpus>
if you really want to compute the height at a certain time there's no efficient way to do that because block times are not strictly increasing and also not accurate
< wumpus>
(the latter making it impossible to know for sure)
< gmaxwell>
xabbix: if you were asking about why you had times before your startup time, that would be because the mempool and its timestamps are saved across restarts.
< bitcoin-git>
bitcoin/master 4602dc7 Wladimir J. van der Laan: build: Bump version to 0.16.99...
< wumpus>
I guess that should not be possible, at least if you didn't install a mempool.dat somehow
< gmaxwell>
I don't think it's possible, the only places in the code that is set is with the output of GetTime() (gets the current time) and when reading it from the mempool.dat file.
< gmaxwell>
if you had an older mempool.dat or something then that would do it.
< wumpus>
is it perhaps an ARM board without realtime clock? I've seen times all over the place with those, if they fail the ntpdate at startup
< bitcoin-git>
bitcoin/master 9cb2309 Wladimir J. van der Laan: doc: Update manpages to 0.16.99...
< wumpus>
going to tag 0.16.0rc1 in a bit on the 0.16 branch
< wumpus>
* [new tag] v0.16.0rc1 -> v0.16.0rc1
< jnewbery>
\o/
< jnewbery>
xabbix: not sure how you interpreted "Both are set by your own node" as time is relayed by peers! Time is set by your own node when it receives the tx, and is saved in mempool.dat on shutdown/loaded on startup.
< jnewbery>
you'll want to look at the AcceptToMemoryPoolWithTime() function in validation.cpp and the places where it's called
< sdaftuar>
luke-jr: I'm trying to restore a document I authored to its original text. I don't know why this needs a discussion on bitcoin-dev?
< luke-jr>
sdaftuar: the Layer field was added by BIP 123; the only reason BIP 90 was a BIP at all, is because it was a hardfork..
< sdaftuar>
BIP 123 came after BIP 90
< sdaftuar>
it was applied to that doc without my approval!
< sdaftuar>
doesn't the author have the final say on changes?
< luke-jr>
nobody objected before BIP 123 became activate..?
< sdaftuar>
no one tagged me to let me know my document was being changed either
< luke-jr>
the document wasn't really changed, just the headers, which was based on the content
< wumpus>
the author is supposed to be asked in case of changes
< luke-jr>
maybe I don't understand the problem - all it does is describe the BIP
< luke-jr>
wumpus: to the BIP itself, yes
< wumpus>
yes, I don't know the details
< sdaftuar>
the point of the BIP is to explain why merely labeling things as "sfot forks" or "hard forks" is deficient
< sdaftuar>
so applying BIP 123's labels to it doesn't make sense
< luke-jr>
the point of the BIP was that changes were being made in Core that were objectively a hardfork, and as such should be documented as a hardfork
< sdaftuar>
no
< sdaftuar>
i'm at a loss as to what the process here is. i wrote a document, you changed it, and you seem to think i can't change it back without... what exactly?
< luke-jr>
the headers aren't part of the document
< sdaftuar>
ah. we should move them out then
< luke-jr>
having them in separate files would just be needlessly confusing to readers IMO
< sdaftuar>
so i don't have editorial rights to the headers?
< luke-jr>
"The BIP editors monitor BIP changes, and update BIP headers as appropriate."
< luke-jr>
I don't see why you would want to make the headers inaccurate anyway
< sdaftuar>
i think bip 123 is misguided, in its naive labeling of consensus changes as soft forks or hard forks
< sdaftuar>
bip 90 demonstrates this
< sdaftuar>
did you not see the quote i pasted from bip 123's author?
< sdaftuar>
i think that quote reflects a view held by many
< luke-jr>
this is a complaint that should have been brought up before BIP 123 was activated, or a new Process BIP to revise/change it
< luke-jr>
bbiab
< provoostenator>
I see it's Gitian o'clock?
< achow101>
hmm, did I screw up my gitian build? The files produced are 0.15.99 not 0.16
< sdaftuar>
luke-jr: it seems to me that BIP123's activation should have required agreement from the BIP-authors that it was applied to
< BlueMatt>
has anyone ever seen a case when compiling qt where flags arent being passed through when compiling the qt stuff properly?
< BlueMatt>
eg I get "No such slot" on all the slots that are inside #ifdef ENABLE_WALLET in bitcoingui.h, and some icons/images arent being compiled in (eg QImage::scaled: Image is a null image) properly
< BlueMatt>
obviously wiped ccache/git clean'd
< BlueMatt>
maybe cfields has some autotools-debug tips?
< cfields>
BlueMatt: you forget the include, maybe?
< cfields>
(config/bitcoin-config.h)
< BlueMatt>
this is 0.15.1, no modifications
< cfields>
BlueMatt: i'm not sure what you mean. You get "no such slot" at runtime, or build time?
< BlueMatt>
runtime
< BlueMatt>
and like cant click the top tabs to switch views cause of missing slot
< provoostenator>
Looks like they got rid of iso images for the 8.9.0 release. 9.2.1 still has them. I'll try if that produces the same build and make a PR.
< achow101>
cfields: I fucked up
< cfields>
BlueMatt: you've verified that the ENABLE_WALLET stuff is actually being built?
< BlueMatt>
it loads the wallet, lists transactions, shows a balance, etc
< BlueMatt>
sooo...yes?
< cfields>
BlueMatt: oh (prepare for possible red herring), kinda sounds like a define is missing when the forms are built
< BlueMatt>
thats what I'm thinking, kinda, but i know almost nothing about qt/autotools
< BlueMatt>
so I have a #ifndef ENABLE_WALLET #error in bitcoingui.h, but I dont know if its being compiled or if qt is doing some C++ parsing?
< BlueMatt>
only change was upgrading packages....is it possible qt is now mis-parsing the .h?
< cfields>
BlueMatt: all qt-generated files are bound to the versions that created them
< cfields>
so moc 1.2.3 will create headers meant to be linked against qt headers/libs v1.2.3
< BlueMatt>
git clean -f -x -d'd :p
< cfields>
provoostenator: thanks :)
< BlueMatt>
uh huh, yes, so moc-qt5 is now broken for us - in normal compile the #ifdef ENABLE_WALLETs are being ignored, if I remove the ifdefs things work (eg gotoOverviewPage will or wont show up in moc_bitcoingui.cpp)
< cfields>
BlueMatt: the defines are passed along. Somehow for you they're not...
< achow101>
provoostenator: the OS you build on doesn't really matter since the actual build happens within another vm/container
< achow101>
that vm/container will be the same OS for everyone
< provoostenator>
achow101 someone needs to draw a picture of that inception / VM (Virtual Matroesjka) situation
< BlueMatt>
cfields: -DHAVE_CONFIG_H is passed to moc-qt5 (make claims it is, at least), and if I put an ENABLE_WALLET inside the if defined(HAVE_CONFIG_H) it works fine, but somehow bitcoin-config.h isnt getting included
< cfields>
BlueMatt: i wonder if you somehow got messed up by the #include "foo.h" -> #include <foo.h>
< cfields>
BlueMatt: out-of-tree ?
< cfields>
ooh, i bet that's it...
< BlueMatt>
in tree
< BlueMatt>
and this is 0.15.1
< BlueMatt>
so pre <foo.h>
< cfields>
BlueMatt: got a stale bitcoin-config.h around somewhere?
< BlueMatt>
i mean clearly qt broke our build, question is if we need to work around it or what
< BlueMatt>
git clean -f -x -d :p
< cfields>
damn, right, i was banking on you saying "out-of-tree"
< cfields>
achow101: I'm betting you just ran out of mem
< cfields>
achow101: I haven't built linux yet though
< achow101>
cfields: probably. I forgot to allocate more memory to kvm when I did this build
< cfields>
BlueMatt: i'm not sure what to tell you. Can you give it a shot with 0.16 so we can start there?
< BlueMatt>
yea, I still had the test_bitcoin-qt failure on 16 so i figured bug was the same but didnt actually try to run it
< BlueMatt>
yes, same bug
< BlueMatt>
gotoOverviewPage (etc) doesn't show up in src/qt/moc_bitcoingui.cpp post-build
< provoostenator>
" run it with the --setup command" should have a specific example, e.g. assuming Debian. I remember that confused me for hours half a year ago
< wumpus>
sdaftuar: cfields: yep that's it, I'm going to remove the check for a specific number
< provoostenator>
I'll take a snapshot of my VM before doing that...
< wumpus>
can reproduce it locally now, too, I was testing with master
< cfields>
wumpus: makes sense. I vaguely remember a conversation about using exit codes rather than testing strings. Guess that didn't pan out.
< wumpus>
well it makes sense that it checks the error message, it just doesn't have to be so specific
< cfields>
BlueMatt: i'm not seeing anything obvious :\
< bitcoin-git>
[bitcoin] laanwj opened pull request #12302: test: Make ua_comment test pass on 0.16.0 (master...2017_01_uacomment_test_fix) https://github.com/bitcoin/bitcoin/pull/12302
< arubi>
daemon stopped, I mkdir 'wallet/' in datadir, moved all my wallet .dats in there, added a new 'wallet=30_01_28_btc_segwit.dat' line in bitcoin.conf, started the daemon, and the new wallet file with this name was created in the datadir root. somewhat surprising, I'm able to getwalletinfo for both this new wallet and old wallets that are now in wallet/ using the -rpcwallet= set
< arubi>
oh this is re. 0.16.0rc1
< arubi>
ohh it says 'wallets' in th release notes, but 'wallet/' in the rpc call help. my bad. it just created a bunch of new wallets :)
< bitcoin-git>
[bitcoin] laanwj closed pull request #12302: test: Make ua_comment test pass on 0.16.0 (master...2017_01_uacomment_test_fix) https://github.com/bitcoin/bitcoin/pull/12302
< jnewbery>
wumpus: I'm happy to open a new PR if you think that's better, but I'm also happy for you to just force push the one-line fix in your PR
< jnewbery>
whichever you prefer
< wumpus>
I'm just tired of this, wanted to quickly fix the test then everyone wants to push their own solution
< wumpus>
I think a regexp is a good idea for message matching, and could be useful in other cases too in the future where there's data embedded in the message we want to ignore
< wumpus>
but I really don't feel like eternally arguing even about things like this, of course you can solve it in 20 different ways too
< jnewbery>
seems over-engineered for fixing a single case. I think here it just makes sense to match on the substring
< jnewbery>
Sorry - I seem to have touched a raw nerve
< jnewbery>
It wasn't a NACK, I just think it's neater to update the individual case than change the util function for this one case
< jnewbery>
but if you prefer your way, that obviously also achieves the same
< wumpus>
no, I don't think matching a partial message only is better than ignoring the few characters we don't care about
< jnewbery>
ok, in that case re-open the PR
< wumpus>
I thought about that too but thought this would cover it better
< sdaftuar>
let's merge it and move on... 0.16.0 awais
< sdaftuar>
awaits*
< jnewbery>
many of the test cases match on substring. I agree that regex matching would be better in the general case
< wumpus>
0.16.0 is not blocked on this, we already tagged rc1
< sdaftuar>
fair, but even if we're not immediately blocked on it, i think we make our lives easier by quickly closing out these kinds of minor issues when we can
< jnewbery>
I agree. I'd argue that implementing a minimal fix that only fixes the broken test case is more appropriate than trying to expand the functionality of the test framework, but really either fix is fine.
< wumpus>
yes, sorry for overreacting
< wumpus>
though I don't think it was unreasonable to try to expand the test framework to do something more than it does
< wumpus>
I understand the consensus code for bitcoin is set in stone, but surely the interface for the test framework isn't
< jnewbery>
I agree that the framework can be improved, but I think that can be done in a separate PR from a quick fix to get the tests running again
< jnewbery>
But we don't need to make this a big thing. I can ACK your PR :)
< provoostenator>
And of course, Linux being Linux, it just silently fails and let the user shoot themselves in the foot
< goatpig>
does Core use zeromq?
< achow101>
goatpig: yes
< goatpig>
isn't Core under the MIT license?
< luke-jr>
Core *offers* a zeromq service, but "using" it is ill-defined in this case
< luke-jr>
goatpig: yes, and?
< goatpig>
isn't LGPL3 (zmq) incompatible with MIT?
< gmaxwell>
No, not at all.
< luke-jr>
goatpig: LGPL is Lesser GPL. It specifically allows non-GPL stuff to use it.
< goatpig>
with version of GPL is incompatible with MIT then? AGPL?
< goatpig>
ah that's what got me confused I guess
< luke-jr>
GPL and AGPL are incompatible with non-free software, but still compatible with MIT
< gmaxwell>
Nothing is incompatible with MIT, some things are more restrictive and we prefer to not use them.
< luke-jr>
they're NOT compatible with OpenSSL, however, which is another dependency of ours..
< gmaxwell>
(luke's They is GPL and AGPL)
< goatpig>
so a MIT project can make use a LGPL dependency just fine?
< gmaxwell>
Yes.
< goatpig>
ok thanks
< luke-jr>
only thing to keep in mind is, you still need to release source for the dependency per the terms
< goatpig>
due noted
< cfields>
yes. We could pull in a gplv3 lib and we'd be fine to distribute it as usual. Problem is that some companies would want to modify that code without publishing it, so it's unlikely that we'd add any dependencies like that
< cfields>
jonasschnelli: ping
< luke-jr>
cfields: well, I think as long as we depend on OpenSSL, the conflict would prevent distribution
< provoostenator>
Still not seeing the br0 interface though.
< cfields>
hmm?
< gmaxwell>
cfields: openssl has a GPL incompatible license (it's advertising clause bsd)
< cfields>
huh
< achow101>
in the test framework, how can I make it so that all of the nodes in a test are connected to each other and stay synced together without having to use sync_all?
< achow101>
sync_all is very slow and it would be better if they were properly connected over the p2p network and sending data between themselves
< bitcoin-git>
bitcoin/master aac6bce Wladimir J. van der Laan: test: Make ua_comment test pass on 0.16.0...
< bitcoin-git>
bitcoin/master f0295be MarcoFalke: Merge #12302: test: Make ua_comment test pass on 0.16.0...
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #12302: test: Make ua_comment test pass on 0.16.0 (master...2017_01_uacomment_test_fix) https://github.com/bitcoin/bitcoin/pull/12302
< provoostenator>
I ended up deleting the file /home/debian/gitian-builder/target-trusty-amd64 and replacing it with a directory, including /proc and /sys subdirs. That at least got me to the next error...
< provoostenator>
lxc-execute: conf.c: setup_pts: 1407 No such file or directory - failed to create '/dev/pts'
< provoostenator>
These are different issues than I ran into half a year ago. I opened some issues on the docs repo. Will try again later.
< wumpus>
you definitely shouldn't have to replace target-something with a directory
< wumpus>
that file should be auto-generated
< wumpus>
as I understand base-trusty-amd64 is the file that will be created by installing gitian / building the initial image, target-trusty-amd64 should be re-generated on every run of gitian when building something, it's a working copy of the base image
< provoostenator>
Mmm, so maybe something silently failed when creating the base image?
< provoostenator>
Ah I see, that's why it mounting that file.
< provoostenator>
I deleted that directory again. Now it seems to be doing a bit more. But I get frequent "init.lxc: failed to mount /dev/shm" error, as well as "sudo: unable to resolve host gitian".
< wumpus>
the sudo resolve warning is harmless
< provoostenator>
Yeah, I think it's actually building now. Hooray!
< sdaftuar>
luke-jr: could you explain your "+1" on evoskuil's last comment? i think i've spent quite a bit of time now on that PR explaining why i find the "soft fork"/"hard fork" language deficient.
< sdaftuar>
is there something you still don't understand about my view?
< jamesob>
paths like "./bitcoin.conf" are interpreted as being relative to datadir as well; is this behavior we want to retain?
< denis2342>
hi
< denis2342>
I just tested bitcoin 0.16.0rc1 on freebsd and it runs fine so far
< promag>
ah sorry maybe that's not possible, because of ShowProgress
< ryanofsky>
actually seems like there is another unnecessary lock below that
< promag>
how about avoid duplicate calls to GuessVerificationProgress(chainParams.TxData(), pindex) ?
< promag>
just do it once where the lock exists and use that value?
< promag>
right after pindex = chainActive.Next(pindex); on the loop end
< promag>
and also before the loop starts
< ryanofsky>
probably something like that is fine. the whole function is a mess, there's a bunch of unnecessary nesting and immediate unlocking/relocking
< Dizzle>
sipa
< Dizzle>
Sorry, miss-typed.
< luke-jr>
sdaftuar: deficient or not, it's the language currently used for the Layer header..
< gmaxwell>
Then the layer header should be removed.
< gmaxwell>
It has very little value at best, and clearly is just resulting in needless arguments.
< luke-jr>
I don't agree that it's deficient. It is working fine in this case as well.
< luke-jr>
I don't understand why people are desperate to deny the reality that BIP 90 is a hardfork.
< gmaxwell>
luke-jr: you're acting inappropriately, your ability to update documents shouldn't be used as a lever to advance arguments you want to make.
< luke-jr>
…
< gmaxwell>
So you disagree about some description stuff, fine. But editing the documents to fit your world view when other people, particularly the authors of the document disagree isn't kosher.
< luke-jr>
it's a change proposed nearly 2 years ago and implemented at least many months ago, without any dispute until this week, not used to advance any arguments at all, and in any case completely objective
< gmaxwell>
The end result will only be that other people will simply walk away from the mechenism.
< gmaxwell>
It's clearly not completely objective, because the decription doesn't fit the conventional definition of a hardfork.
< luke-jr>
the documents are not edited, only the preamble header
< luke-jr>
it does
< gmaxwell>
You can argue that, but not via special privledges to edit the repository.
< luke-jr>
unless you're trying to push a redefinition of hardfork, which is not the fault of a change made a long time ago
< gmaxwell>
I didn't notice that you stuck a hardfork label on BIP 90 until today.
< gmaxwell>
Had I noticed it I would have disagreed at any point, and you should have known that because voskuil tried arguing that on the mailing list previously and almost everyone disagreed with him.
< luke-jr>
the whole reason it was made a BIP at all, was because it was a hardfork..
< gmaxwell>
That is not true.
< luke-jr>
then why would there be a BIP?
< gmaxwell>
because there is a BIP for any protocol change with potential interoperability implications... even just documentary ones.
< luke-jr>
if it is a protocol change, it is a hardfork by definition
< gmaxwell>
And there has never been a hardfork performed via a BIP.
< luke-jr>
the only way to argue it isn't a hardfork, is to argue that it isn't a protocol change
< gmaxwell>
you are now arguing that all changes to the protocol are hardforks?
< luke-jr>
no
< gmaxwell>
so segwit is a hardfork? p2sh is a hardfork?
< gmaxwell>
Fee filter.. hardfork?
< gmaxwell>
compact blocks hardfork
< luke-jr>
Segwit and P2SH constrain the protocol, they don't loosen it
< gmaxwell>
BIP32 ... totally hardfork there.
< luke-jr>
Fee filter and compact blocks aren't protocol changes at all
< gmaxwell>
15:28:57 < luke-jr> the whole reason it was made a BIP at all, was because it was a hardfork..
< gmaxwell>
15:29:35 < luke-jr> then why would there be a BIP?
< luke-jr>
as a protocol change, BIP 90 loosens the rules, and is therefore a hardfork. if it is not a protocol change, then it is an implementation detail that doesn't need standardization
< luke-jr>
there is no reason for standardization of this kind of change, except if we see it as a protocol change
< gmaxwell>
BIP 90 doesn't losen the rules. It changes implementation details about things burried in the past, what in the past is in the past. If there were some other chain they would possibly accept that something else would reject then there would be a distinction there. But there isn't.
< gmaxwell>
it's like arguing that jtimon's changes to not validate the genesis block were a hardfork.
< luke-jr>
if it's an implementation detail, then there's nothing to standardize
< gmaxwell>
it turns "hardfork" into some jibberish technical defintion that doesn't map usefully to system behavior, which serves no productive goal other than making it so that only you can decide what is and isn't one.
< gmaxwell>
luke-jr: Things aren't so black and white, most of BIP90 had been done for something like a year before the BIP was written. But it's better to communicate more instead of less.
< luke-jr>
where's the grey? I don't see it in this case.
< gmaxwell>
Writing documentation doesn't turn something into a hardfork that had existed for a long time.
< gmaxwell>
luke-jr: BIP90 can't fork off nodes in any plausable situation; thats not even grey: it's not a hardfork in any meaningful sense.
< luke-jr>
maybe the Layer header should just be deleted for BIP 90 (seeing as it's optional and not always applicable)
< gmaxwell>
That would be an okay way to handle a dispute, I think.
< gmaxwell>
The BIP describes itself, I think accurately and in detail.
< luke-jr>
well, the description in the BIP is clearly a hardfork, IMO
< gmaxwell>
Lets not create bad incentives for people to describe things using anything less than the most conservative language.
< luke-jr>
it's a header. there is no incentive..
< gmaxwell>
esp since bips can pretty much say whatever nonsense they want already.
< gmaxwell>
Sure there is, it's an incentive to not be as conservative (e.g. spelling out any possible way anyone could call it a hardfork, and als possible risks) if its going to result in a misleading header.
< BlueMatt>
can we just define a new term here
< luke-jr>
is there some context I'm missing? to me, it looks like someone just up and randomly decided to whine about the layer header
< BlueMatt>
call it "Buried Fork"
< BlueMatt>
or something
< BlueMatt>
then no one has to be upset
< gmaxwell>
The word fork should probably not be used, because there is no fork.
< gmaxwell>
there is no split in consensus state.
< gmaxwell>
"Silver spoon" :P
< luke-jr>
it's essentially the same thing as adding/removing checkpoints
< gmaxwell>
I think I agree with that, which we also never have BIPed.
< BlueMatt>
"Buried Spoon"
< BlueMatt>
that seems appropriate
< gmaxwell>
we talked about this when the bip was written and thought there was technically no need to BIP it, but more documentation is better than less.
< gmaxwell>
I think thats a good practice, but this showing up as some "hardfork enforce by core devs" crap would be a profound disincentive to ever do that again.
< contrapumpkin>
(a BIP on the need for the layer header in BIPs?)
< jtimon>
luke-jr: of course it's good to make bip90 or sdaftuar's pr whether they're hfs, sfs or none of that. I honestly don't care if bip90 is a hf or not, it's still a good thing and should be documented in a bip. I don't think it's good to edit bips without the author's permission unless it's for typos or tiny things
< jtimon>
BlueMatt: I like more "Buried deployment", but bikeshedding...
< contrapumpkin>
jtimon: it seems he got disconnected
< gmaxwell>
well I think a general editoral principle is that you can be liberal with changes that no one complaints about, and conservative with things that cause a fuss.
< jtimon>
yeah, nobody will ever complain for a typo being corrected
< luke-jr>
if we're looking at BIP90-like changes as implementation details, then it should probably go in a Core-specific doc repo rather than BIPs since it isn't a coordinated thing
< luke-jr>
(I read the IRC log to catch up)
< luke-jr>
having a Core-specific place for implementation detail documentation would encourage more documentation of Core-specific stuff too
< luke-jr>
?
< jtimon>
luke-jr: it's not core specific, everybody is welcomed to do bip90 too
< gmaxwell>
you can look at as as we did the analysis to determine that this was an okay thing to do and are publishing the details.
< jtimon>
was bip39 whatever-wallet-implemented-it-first-specific?