< bitcoin-git>
bitcoin/master faaf3ca MarcoFalke: travis: make distdir before make
< bitcoin-git>
bitcoin/master 9ec1330 MarcoFalke: Merge #9416: travis: make distdir before make...
< bitcoin-git>
[bitcoin] MarcoFalke closed pull request #9416: travis: make distdir before make (master...Mf1612-travisDistDirCheck) https://github.com/bitcoin/bitcoin/pull/9416
< gmaxwell>
I finally figured out why I wasn't getting any github emails.
< midnightmagic>
Whyzzat
< gmaxwell>
I configured an email rule to put them in another folder...
< morcos>
gmaxwell: that sucks.. i only get like half of github notifications as emails.. and i was less bothered by the fact that not only i was affected
< gmaxwell>
well I still may be only getting half.
< gmaxwell>
But I'm not getting none.
< MarcoFalke>
except: pass
< MarcoFalke>
why is this still a thing?
< midnightmagic>
haha
< luke-jr>
gmaxwell: why does #8775 need rebase? O.o
< gribble>
https://github.com/bitcoin/bitcoin/issues/8775 | RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest by luke-jr · Pull Request #8775 · bitcoin/bitcoin · GitHub
< gmaxwell>
why is that linking issues?
< gmaxwell>
luke-jr: ask git, not me, it doesn't apply cleanly to master.
< gmaxwell>
(git am -3 <patch> fails)
< luke-jr>
gmaxwell: I ask only because both git and github merge it cleanly :x
< gmaxwell>
gah whitespace errors
< gmaxwell>
MarcoFalke: does your git diff not show crazy whitespace as wads of red?
< MarcoFalke>
It does
< gmaxwell>
K. #9297 added big wads of end of line whitespace.
< gmaxwell>
Just wanted to make sure you could see it-- used to be that you had to configure it that way, but I think its a default now.
< gmaxwell>
luke-jr: just catches fire here.
< gmaxwell>
fatal: sha1 information is lacking or useless (src/wallet/rpcdump.cpp).
< gmaxwell>
error: could not build fake ancestor
< gmaxwell>
Patch failed at 0007 More tightly couple EnsureWalletIsAvailable with GetWalletForJSONRPCRequest where appropriate
< MarcoFalke>
At some point I feel we need to merge a pull. It can't be always free of µnits...
< MarcoFalke>
We have more pressing issues right now.
< luke-jr>
gmaxwell: what if you use git-merge? could you have an old branch?
< MarcoFalke>
But otoh I understand. Someone will fix it some day...
< gmaxwell>
luke-jr: that is in a current checkout vs the patch from github.
< gmaxwell>
MarcoFalke: I wasn't critizing, just making sure you could see it, so at least you were making the decision rather than just unaware. :)
< MarcoFalke>
ok
< luke-jr>
gmaxwell: looks specific to git-am. merging it normally works
< gmaxwell>
okay, well I'm not testing it unless it merges with git-am.
< luke-jr>
ok, rebased and seems to work now
< gmaxwell>
YUP!
< gmaxwell>
thanks.
< fanquake>
If anyone has some more info they could contribute to #8639, it would be appreciated. There's probably enough there now to turn it into some docs, but filling in a few more gaps would be handy. i.e minimum required openssl.
< sipa>
for non-qt we only need openssl for the PRNG
< sipa>
which i think means very old versions will work
< sipa>
not sure if we should suggest that, though
< gmaxwell>
and our use of it in the rng could darn near be replaced with rand()
< fanquake>
Should we be suggesting use of the 1.0.1 series at all if it's out of support? Or is that irrelevant here.
< fanquake>
We are currently using 1.0.1k in depends.
< gmaxwell>
fanquake: virtually every linux distribution is 1.0.x something, 1.1 changed the API in incompatible ways. We're compatible now, but it would be weird to suggest software that no one has.
< MarcoFalke>
How come that doxygen still has main.cpp?
< MarcoFalke>
^ wumpus can you take a look at doxygen, please?
< gmaxwell>
achow101: The issue is that segwit txn deseralizes as a 'crazy' non-segwit txn-- one with zero input transactions.
< gmaxwell>
achow101: the decodehex tries to decode as non-segwit first and then segwit, but that txn successfully decodes.
< achow101>
is that just bad luck then that it is able to successfully decode as non-segwit first?
< gmaxwell>
well could be bad luck or you could construct ones that way, in the actual protocol sw and non-sw txn are handled distinctly not by trying to decode both ways.
< gmaxwell>
achow101: I believe that an acceptable solution is to just make that first try fail if the number of inputs is zero. But this actually would reduce the functionality of raw transactions slightly, because passing around an inputless raw transaction isn't totally absurd.
< gmaxwell>
But I think that is likely the correct fix.
< gmaxwell>
or most correct.
< sipa>
didn't we fix that already?
< gmaxwell>
sipa: for decoderawtransactions? my description is from reading the current code.
< achow101>
what use case is there for 0 input txns?
< gmaxwell>
an alternative would be to reverse the trial order, so it won't even try nonwit unless witness fails.
< gmaxwell>
achow101: I can use them as a payment request, for example... give you a txn with the outputs I want, leave it up to you to add inputs (and change, if required.)
< sipa>
raw transactions are more often used for unsigned transactions
< sipa>
which by definition aren't segwit
< achow101>
oh
< sipa>
once they're signed, they're usually complete
< sipa>
but it's unfortunate that there is ambiguity
< sipa>
we should propose some 'partially signed' transaction format
< sipa>
that's unambiguous
< sipa>
but has place for metadata etc
< gmaxwell>
well in particular, amounts and scriptpubkeys.
< achow101>
gmaxwell: what about having decoderawtx take a parameter to tell it to decode as segwit or non
< gmaxwell>
achow101: thats a bit ugly.
< achow101>
and have that somehow be related to soft fork activation
< gmaxwell>
achow101: thats more than a bit ugly. :P
< sdaftuar>
sipa: gmaxwell: does decoderawtransactions need to handle the 0 input case? i remember discussing this for fundrawtransaction, where 0 inputs does make sense, but not obvious to me that decode should accept a 0-input tx?
< gmaxwell>
sdaftuar: sure, except for this bug.
< gmaxwell>
just shows now inputs.
< gmaxwell>
I suppose it being unwilling wouldn't be the end of the world.
< sipa>
decoderawtransaction and fundraetransaction both attempt old and new deserialization
< gmaxwell>
it's not like I could sign a zero input transaction.
< sipa>
but they only accept the old serialization if it exactly matches the string
< sipa>
(no undecoded garbage at the end)
< gmaxwell>
So I think it would be okay if decoderaw would not decode zero input. But fundraw needs to read zero input, and this transaction example decodes both ways.
< achow101>
changing decodehextx in core_read.cpp will also affect sendrawtx. but that shouldn't be an issue since a 0 input tx isn't valid
< sipa>
decodehextx gets a bool arg
< sipa>
to indicate if non-extended format decoding should be attempted
< sipa>
only decoderawtx and fundrawtx set that bool to true
< gmaxwell>
that should be false for sendraw.
< sipa>
it iz
< sipa>
it is
< gmaxwell>
iz was okay too.
< sipa>
i zhall rezord do uzing voized vowelz vrom now on
< gmaxwell>
oh dear.
< sdaftuar>
gmaxwell: in #8456, we can replace a bumped transaction, so i don't think we should always mark the replacement as non-replaceable. i agree with your second reason for wanting the ability though.
< morcos>
i think it would make the most sense to have the replacement tx inherit -walletrbf setting , but also add an option to explicitly set it...
< morcos>
we never settled on a way to do that, but now with usings the options argument or named arguments to rpc, its easy enough to just add an rbfflag option to sendtoaddress, sendtomany, and bumpfee
< morcos>
i suppose for starters having it inherit the -walletrbf setting is a very minor change and allows you to do anything you want if you're willing to restart your node
< sdaftuar>
seems reasonable to me
< bitcoin-git>
[bitcoin] achow101 opened pull request #9522: [RPC] Fix decoderawtransaction decoding of segwit txs (master...fix-decoderawtx) https://github.com/bitcoin/bitcoin/pull/9522
< sipa>
achow101: the bool arg to decodehextx should probably be changed to being an enum, with extended_only, prefer_extended, prefer_normal
< sipa>
instead of basing it on hex bytes
< achow101>
it would still have to be based on the hex bytes to know when each one should be done though
< sipa>
no
< sipa>
you've just implemented a "prefer extended" stratwgy
< achow101>
but then how do you know which strategy to choose?
< sipa>
decoderawtransaction would use prefer extended
< sipa>
fundrawtransaction would use prefer olf
< sipa>
all the rest would be extended only
< achow101>
oh.
< sipa>
that's what you have implemented now
< achow101>
I see
< achow101>
I can make that change
< sipa>
actually
< sipa>
what if you just pass false to decodehextx in decoderawtransaction?
< sipa>
i believe the behavior will be the same as what you have now
< achow101>
then it will be decoding all transactions as extended
< sipa>
yes
< sipa>
that's what you want, no?
< sipa>
the extended format for transactions without witness is identical to the old format
< achow101>
right
< achow101>
yes, that should work.
< achow101>
why was it passing in true originally then?
< sipa>
to pick old decoding in case it was ambiguous
< sipa>
which is what you want for fundrawtransaction
< sipa>
but perhaps not for decoderawtransaction
< sipa>
i'm not sure, it's choosing between two suboptimal options
< BlueMatt>
cfields: hum, I owe you an apology - I dont think that issue is gonna go away by changing the threading stuff - I think the "unlock cs_main prior to ActivateBestChain" stuff is going to have to stay because it is the only way to let things which will be quick but need to check something against cs_main and want to run on NewPoW... run
< BlueMatt>
I fixed your specific complaint (the read-block-from-disk one) by simply using the most_recent_block shared_ptr that was already lying around, but I get that thats not really a fix for your general comment of adding complexity
< cfields>
BlueMatt: yea, i almost suggested that, but it would just kinda mask the issue (usually)
< cfields>
BlueMatt: to be clear, i'm not worried about the disk access _at all_
< BlueMatt>
I know
< BlueMatt>
I am, though, so thanks for reporting :p
< cfields>
was just using it as an example
< cfields>
heh
< BlueMatt>
i mean it fixes your specific issue, but I'm really not a fan of the BlockUntilBlockIsWhereIWant in the net code
< BlueMatt>
I know, I know
< cfields>
BlueMatt: well if you're not a fan of that, certainly you're not a fan of ActivateBestChain there either :)
< BlueMatt>
no, I'm specifically not a fan of BlockOnThing() over DoTheThingIWantToBlockOn()
< cfields>
i see
< BlueMatt>
wellllll, hum
< BlueMatt>
now that i say that
< cfields>
no, you're not doing anything in either action. Either way it's being treated as a fence.
< BlueMatt>
well we need a fence primitive for wallet to fix #9148
< cfields>
er, "not doing anything" is a very poor choice of words, that's obviously not what i meant
< BlueMatt>
I mean I feel kinda yucky doing this because what if some braindead caller calls AcceptBlock without ActivateBestChain?
< BlueMatt>
we then call the fence and freeze forever
< BlueMatt>
is my concern
< BlueMatt>
same applies for wallet, though....
< BlueMatt>
hence my comment on prefering FenceButFeelFreeToDoWorkToGetThere
< BlueMatt>
which is what ABC is to me, here, i guess
< cfields>
BlueMatt: i was just picturing: at the top of ABC, working=true. at the bottom, working=false
< cfields>
nasty hack, but that's what you really want to know, right?
< BlueMatt>
no, you'd need to do that in ProcessNewBlock
< cfields>
not for your purposes, i think? It's ABC that would trigger the callback to push out the message, no?
< cfields>
er, nm
< jonasschnelli>
<*highlight>[02:02:36] <gmaxwell:#bitcoin-core-dev> jonasschnelli: did you ever get to producing the change that removes keys from the keypool when they're seen used on the network?
< jonasschnelli>
gmaxwell: No. Didn't started. I had in mind that you said you want to start with that... but I can try something...
< da2ce7>
Just tested 'make deploy' from git-head on latest mac. Works without problem :)
< wumpus>
great!
< fanquake>
wumpus any objection to closing 7149 ?
< wumpus>
fanquake: yeah I don't think it's going to be merged as it is
< fanquake>
wumpus yea, open for > 1yr with only 2 comments.
< fanquake>
I might close a few older/inactive pull requests. Mentioning that they are of course free to reopen if they wish to restart/continue the work.
< bitcoin-git>
[bitcoin] fanquake closed pull request #8339: Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress] (master...blkconsensus) https://github.com/bitcoin/bitcoin/pull/8339
< bitcoin-git>
[bitcoin] fanquake closed pull request #7149: Bugfix: Correctly calculate priority when inputs are mined after dependent transactions enter the memory pool (master...bugfix_priority) https://github.com/bitcoin/bitcoin/pull/7149
< bitcoin-git>
[bitcoin] fanquake closed pull request #8849: print P2WSH redeemScript in getrawtransaction if it s not a pubkey (master...print-p2wsh-redeemscript-in-getrawtransaction) https://github.com/bitcoin/bitcoin/pull/8849
< bitcoin-git>
[bitcoin] fanquake closed pull request #9116: Allow getblocktemlate for not connected regtest node (master...master) https://github.com/bitcoin/bitcoin/pull/9116
< morcos>
wumpus: is your concern with changing -walletrbf this close to 0.14 from a technical safety standpoint or more from a user/community awareness standpoint?
< wumpus>
morcos: both, though mostly the latter
< wumpus>
mostly that people get confused that they're suddenly creating a different kind of transactions with different properties
< wumpus>
without having asked for it
< wumpus>
of course it could be included in the release notes, but the planned release notes for 0.14 will already be huge :)
< morcos>
ok, i agree that maybe it needs more forewarning... but i do think that the problem we'll run into as is, is that people won't have changed the default, they'll have some "stuck" tx and want to bumpfee it and realize they can't
< fanquake>
Yes I'm sure a few things are going to get lost in there already.
< morcos>
so perhaps we should at least make it very clear in the release notes that if you're using a core to send your txs, that you need to change this default in order for bumpfee to be of any use to you
< wumpus>
I think at least in the UI it would e.g. make sense to add a question on the first transaction send after upgrade
< wumpus>
anywhere bumpfee is mentioned it should be made clear
< wumpus>
also in the RPC help for the funcition, thinking of it
< wumpus>
in any case let's not make merging that functionality dependent on the defaults discussion
< morcos>
sure agreed
< morcos>
wumpus: suppose i want to add a new option such as rbfflag to an rpc call like sendtoaddress... what is the right way to do that now we have named arguments? 1) add support for holes in sendtoaddress and 2) add a new argument in the last position for rbfflag ?
< wumpus>
morcos: yes, that sounds like the right approach to me
< morcos>
i guess i'm just trying to reconcile some rpc calls having an options argument like bumpfee and some not, so for bumpfee i wouldn't worry about and just add the rbfflag inside the options object?
< sipa>
we should also make the rpc example code use named rgs
< sipa>
*args
< sipa>
and i wonder if we shouldn't first start converting some methods to natively accept names
< fanquake>
Do we really need a nearly 1100 line script to check/maintain copyright headers
< wumpus>
well right now named arugments are completely backwards compatible, and supporting holes also helps people that use positional arguments. It means they can specify 'null' to use the default.
< wumpus>
so adding holes support to a RPC call is good in every way
< wumpus>
fanquake: well as long as the guy maintains it it's fine I guess
< instagibbs>
morcos, I'm up to 20% RTT saved now, syncing with your number
< fanquake>
wumpus I guess. It just seems like something set to degrade. Checking sub-trees also seems like overkill.
< wumpus>
fanquake: checking sub-trees is absolutely overkill
< fanquake>
I'm not sure what the point of that is, as it's not like we're going to modify them also.
< wumpus>
we don't merge any changes to those
< wumpus>
right
< wumpus>
but I suppose that comes by default - it's *avoiding* subtrees that requires extra logic. Or maybe I'm thinking of it wrong.
< fanquake>
I thought you'd be able to just exclude all the files in the subtree dirs.
< wumpus>
very possible - I don't know what he uses to query the list of files from git
< fanquake>
I am starting to like the idea of adding other types of checking to Travis though. Some discussion in 9452
< fanquake>
Just got to find the balance between not failing on every code change to the point that Travis is never green again, and picking up things like 9487.
< gmaxwell>
Re walletrbf: I could see it going either way. We could write the release note for the bumpfee that just said you have to set walletrbf in advance for it to be useful.
< gmaxwell>
Though I think we can't default to walletrbf unless we have a way to bump to non-rbf. Though I think that would be a trivial change to bumpfee.
< morcos>
gmaxwell: what i said yesterday about using the default in bumpfee doesn't make sense
< morcos>
i don't think you want to be changing the sequence numbers on the tx which may have other meaning unless you specifically indicate that you want to
< morcos>
ryanofsky is going to add an option to bumpfee, and the only time you ever modify the sequence numbers is if the option tells you to specifically change it to not be rbf any more
< gmaxwell>
thats okay with me.
< morcos>
even that though sounds a bit risky to me
< morcos>
what happens when you bump some transaction that was only valid because of its sequence numbers and CSV
< gmaxwell>
well considering we can't sign for such a transaction currently...
< morcos>
we can't?
< gmaxwell>
no, if there is a OP_CSV signrawtransaction won't work, you'd need to be running modified software.
< Chris_Stewart_5>
requires standardness?
< gmaxwell>
jonasschnelli: sorry, darn, must have been a miscommunication. I was nagging before because I was saying I'd start on it if you weren't going to; but if you told me you weren't going to I either didn't get the message or forgot.
< jonasschnelli>
gmaxwell: deadlock. :)
< jonasschnelli>
Should I start to do something or do you want to work on the HD-keypool auto-jump?
< sipa>
Chris_Stewart_5: no, the signing code just doesn't know how to produce a scriptSig for that
< gmaxwell>
jonasschnelli: you should start because I'm going to be travling most of the day and don't know if I'll be productive (e.g. have power).
< gmaxwell>
jonasschnelli: I think it will be simple (famous last words).
< jonasschnelli>
gmaxwell: Okay. Let me try... but I can't start before tomorrow / friday. Not sure if I get something done before the freeze.
< jonasschnelli>
gmaxwell: heh. Okay.
< Chris_Stewart_5>
gotcha. Thanks.
< morcos>
gmaxwell: damn.. i should have known that since i wrote the rpc test... i put the stupid OP_CSV in the scriptSig to test it instead
< wumpus>
fanquake: I'm not against adding checks to travis if they're strongly useful in preventing immediate problems. The only thing I'm sure about is that I don't want copyright header checks in there.
< wumpus>
the problem really is that travis only has two states (or three?) in any case it has no support for severity levels. This means that someone whose tests fails has to skip through screenfuls of logs to get to the place where the error happened. I wish it had a way to produce a summary instead
< fanquake>
wumpus Yea I wish it just posted the error/log output in the GitHub comment bit.
< wumpus>
well please not the entire thing
< wumpus>
just a summary of points, say e.g. what build step failed and the error message
< fanquake>
Yes not the whole lot, just the last 10 lines or so.
< fanquake>
I should be minimized by default too.
< wumpus>
any way to put a custom message in the comment bit would be great, we can handle the rest
< fanquake>
Anything to save looking at 6 different sets of logs.
< fanquake>
Also, are we still manually starting the builds of 7148
< fanquake>
Just noticed Travis now has a Cron Job feature (beta), so maybe we could setup a branch for the extended test suite and have it run by the cron rather than manual trigger.
< cfields>
BlueMatt: i'm reasonably convinced that it's harmless as-is, my only fear is that it leads to bugs in the future. Any thoughts on preventing future side-effects there?
< BlueMatt>
I mean I tend to air on the side of it-shouldnt-have-side-effects just because thats how that function /should/ work
< BlueMatt>
:/
< wumpus>
#startmeeting
< lightningbot>
Meeting started Thu Jan 12 19:00:08 2017 UTC. The chair is wumpus. Information about MeetBot at http://wiki.debian.org/MeetBot.
< luke-jr>
would be nice to get multiwallet in, but it's mostly just waiting on review at this point. if we think we can get it in, I will rebase the final PR(s) as soon as the last pre-MW refactor is merged
< morcos>
+1 BlueMatt's list
< luke-jr>
(but IIRC from last meeting, there were more important things than MW that take precedence)
< cfields>
wumpus: there's qt 5.7 which is probably worth having in.
< jonasschnelli>
cfields: +1... whats missing? Can I help?
< wumpus>
cfields: I don't understand the blocker there
< BlueMatt>
luke-jr's multiwallet would have been nice, but we're at least two prs away...#8775 could probably be merged easily, but the one thereafter hasnt really gotten any review yet :(
< gribble>
https://github.com/bitcoin/bitcoin/issues/8775 | RPC refactoring: Access wallet using new GetWalletForJSONRPCRequest by luke-jr · Pull Request #8775 · bitcoin/bitcoin · GitHub
< wumpus>
yes #9441 should be ready for merge really
< cfields>
wumpus: i did some work on a bump+restructure a while back, and fanquake recently bumped but it's a bit broken. We just need to pull the fixes out of mine and into his. Should be quick/easy, it's just the building that takes a while
< BlueMatt>
btcdrak: my point was I dont think thats gonna make it
< cfields>
(re qt 7.1)
< cfields>
er, 5.7. heh.
< BlueMatt>
would have to turn out to get acks without any comments on the final multiwallet pr, I think
< BlueMatt>
unless we decide we super want it and are willing to let it go in post-feature-freeze
< wumpus>
forget about multwallet for 0.14
< jonasschnelli>
Yes. It's probably to late
< BlueMatt>
yup
< wumpus>
it's a pity but let's just merge that stuff after the 0.14 split
< jonasschnelli>
Yes. No hurry.
< wumpus>
then it has ample time to be tested in master, which it needs
< cfields>
jonasschnelli: happy to have help, sure. Let's discuss after meeting.
< jonasschnelli>
ok
< wumpus>
it's not something that is safe to merge last minute before a release and let people figure it out in a rc :)
< BlueMatt>
havent looked at the diff, but I'd call #9519 a bugfix, so should go in post-monday
< BlueMatt>
wumpus: I mean I'd call code correctness bugfixes even if there are no bugs
< wumpus>
I don't really like the perf hit
< BlueMatt>
wumpus: assuming we can fix the performance hit?
< wumpus>
normally I woudln't mind but hashing is very important to bitcoind performance
< sdaftuar>
regarding 9512, sipa just told me (he's walking out the door) that he can make it a very slight performance improvement... but i guess he hasn't pushed that yet
< BlueMatt>
I know gmaxwell wanted #9484 for 0.14, which I think it should be
< wumpus>
I had hoped to work on platform specific implementations for sha256, but anyhow, that won't make 0.14 and we shouldn't make the default implementation slower than necessary either
< BlueMatt>
wumpus: ok, lets punt on 9512 for now, then
< BlueMatt>
can decide later
< morcos>
Can we discuss briefly the concept of dust being tied to minrelaytxfee
< wumpus>
BlueMatt: yeah unless there's something that introduces an actual bug (I don't even understand all the change in it - e.g. asked about the change from LONG_MAX to 1<<40)
< BlueMatt>
so things that need review before monday: 9484, 9499, 9375, 9441
< wumpus>
morcos: sure, next topic
< sipa>
wumpus: i'm running to catch a bus, but i believe i have a slightly faster-than-master but sanitize-correct version of ReadLE32 etc
< morcos>
I want to motivate why its important to consider #9380 as well. At least one of the commits there has translation strings.. do we translate debug help?
< BlueMatt>
wumpus: ahh, yea, admittedly I havent read the code there yet, beyond very brief skimming
< BlueMatt>
morcos: go ahead?
< cfields>
yes, it's in the CScriptNum tests
< morcos>
Sorry I was confused as to whether I was waiting for "topic:" or not.. Anyway. The point is that right now if a miner changes the -minrelaytxfee, this already automatically changes their definition of dust. This occasionally leads to txs with high feerates not being minable by some portion of miners
< wumpus>
morcos: oh yes forgot that - current topic is still what to do before the feature freeze
< wumpus>
#topic the concept of dust being tied to minrelaytxfee
< BlueMatt>
morcos: ahh, ok, I'd call that a bugfix that we can do post-feature-freeze? but sounds like something that should be fixed
< BlueMatt>
(I assume code is realatively trivial)
< morcos>
BlueMatt: what about the transaltion strings
< BlueMatt>
wumpus: ?
< MarcoFalke>
morcos: Those are "hidden" features? So no translation string
< MarcoFalke>
I'd recommend against exposing -dustfeerate
< MarcoFalke>
to every user
< MarcoFalke>
Maybe not even at all. Just make it a const in the code.
< wumpus>
yes, translation freeze is at the same time as feature freeze
< morcos>
MarcoFalke: ok, in that PR, -blockmintxfee was not hidden, specifically intended to be used by miners... But yes -dustrelayfee is hidden. And I agree, it shouldn't be changed by anyone.
< wumpus>
but if it's a debug feature it won't have translation strings, ofc
< morcos>
I suppose if we merge it too late, we can start with all features hidden and expose them next version
< BlueMatt>
ehh, I'd say the diff is pretty trivial, lets just target it for monday?
< BlueMatt>
(at the risk of piling on top of the other 4)
< morcos>
Anyhway there are 2 potential problems: 1, a users txs are stuck for reasons they don't understand, but potentially more seriously it hurts fee estimation...
< morcos>
I actually do agree with luke-jr that ideally fee estimation will be more robust to this... but considering we dont' see much value in having different nodes have different definitions of dust. It seems a no-brainer to fix that so it doesn't change anytime someone is trying to avoid spammy low-fee txs
< luke-jr>
morcos: eh, is it based on your own fee, or the relay min fee? I thought the latter
< morcos>
luke-jr: dust is based on minrelayfee, but people change minrelayfee to avoid spam or limit their mempool and inadvently change dust in teh process
< luke-jr>
ah
< morcos>
OK, I'm done.. Just wanted to be sure this was flagged before it was too late... seems like we could merge some version after feature freeze if we had to.. , anyway, someone please tag 9380 for 0.14 as well
< wumpus>
ok
< BlueMatt>
lets add it to the list for monday and if it slips thats ok
< BlueMatt>
wumpus/sdaftuar/morcos should #8456 be un-tagged for 0.14? probably #8501 should be. I dont think we're gonna make #8654 or #8723 or #8755 either
< morcos>
I think 8456 can make it... The others maybe not
< jonasschnelli>
Yes. 9294 must go in!
< BlueMatt>
morcos: its kinda light on ACKs to get merged this weekend, no?
< jonasschnelli>
Needs review. Has only a single utACK
< BlueMatt>
jonasschnelli: looks like it needs rebase?
< jonasschnelli>
We should also tag #9377
< gribble>
https://github.com/bitcoin/bitcoin/issues/9377 | fundrawtransaction: Keep change-output keys by default, make it optional by jonasschnelli · Pull Request #9377 · bitcoin/bitcoin · GitHub
< BlueMatt>
grrr, this list is a bit long for 4 days incl a weekend...
< jonasschnelli>
Oh. Yes. Needs rebase (since today)
< wumpus>
agree on 8456 may still make it, I think the only discussion there is about the default value for walletrbf and that's unnecessary to decide there
< luke-jr>
maybe we should split up the list between different people?
< wumpus>
yes, it's not going to all make it
< luke-jr>
we don't all have to review the same stuff
< wumpus>
as I've said last week we should really make a choice
< wumpus>
instead of trying to cram everything in
< BlueMatt>
luke-jr: we dont have enough reviewers for that to work well enough :(
< jonasschnelli>
9377 is a bugfix and can go in later
< jonasschnelli>
But please review 9294,.. is a sensitive wallet thing
< BlueMatt>
and I think gmaxwell and sipa are on the road, plus I'm avoiding review at least until my cold is a bit better and I can think straight
< luke-jr>
:<
< wumpus>
especiall for the wallet it seems getting reviewers is really hard
< jonasschnelli>
That's why I'd like combining the HD split with other stuff.
< luke-jr>
surely at least HD/split can be upgraded to flex-keypath⁇
< sipa>
breaking backward compatibility in major releases is fine
< instagibbs>
Yeah why can't we upgrade to keypath?
< jonasschnelli>
Okay.
< jonasschnelli>
You can upgrade to keypath, but not downgrade
< * instagibbs>
should have actually reviewed, my bad
< jonasschnelli>
Use a 0.15 wallet in 0.14 as an example
< jonasschnelli>
But maybe its not so bad.
< luke-jr>
upgrade-only is okay. we have -walletupgrade for that
< wumpus>
don't you mean forwards compatible? backwards compatible means that it can use old wallets, which should always be possible
< jonasschnelli>
wumpus: right. My fault.
< sipa>
backward compatible means that old software can use new wallets
< wumpus>
but being able to use a new wallet with an old major version is not
< wumpus>
huh?
< jonasschnelli>
perspetive thing. :)
< wumpus>
I thought the other way around.
< jonasschnelli>
*perspective
< sipa>
forward compatible is what you normally always have
< wumpus>
I don't understand it anymore then
< instagibbs>
wallet.dat vs Core wallet relativity...
< sipa>
oopz
< CodeShark>
Walllet format vs. Wallet app
< luke-jr>
I think we have more cases than normal
< sipa>
maybe i am wrong too
< jonasschnelli>
Looking at the git history tells me, that we took good care about the fact that you can run a newer wallet on an older bitcoin-core version
< sipa>
i will shut up
< jonasschnelli>
And now we break that in 0.13, 0.14 and probably 0.15.
< jonasschnelli>
But fine for me.
< instagibbs>
jonasschnelli, OTOH, these are the kind of upgrades people desperately want...
< instagibbs>
for future work
< wumpus>
jonasschnelli: well in my opinion that doesn't matter too much. What is important is that if you open a wallet once with the new version you should still be able to downgrade
< luke-jr>
jonasschnelli: there have been cases where -walletupgrade is needed, and then the wallet ceases to work in old versions
< CodeShark>
wallet format is forward compatible, wallet app is backward compatible
< wumpus>
jonasschnelli: however, new wallets created with new versions don't need to be open-able with old versions
< jonasschnelli>
Okay. Seems that we all agree. Good.
< morcos>
wumpus: +1 for those standards
< wumpus>
we're just trying to avoid that *touching* a wallet with a new version automatically makes it incompatible, which would happen when upgrading berkeleydb for example
< jonasschnelli>
Flexible keypath is nice. But we don't want to support BIP44 anyways thats why it's not urgent
< jtimon>
all these backards and forwards compatibility is confusing, softfork and hardforks are much more clear :p
< petertodd>
jtimon: +1
< luke-jr>
we should also avoid making it incompatible unintentionally; only if -walletupgrade is enabled should we bump the feature requirements of a wallet
< luke-jr>
jtimon: lol
< luke-jr>
eg, if someone tries to use the flex-keypath, throw an error instead of upgrading the wallet (unless option is enabled0
< BlueMatt>
ok, list for monday: 9380 (if it slips that ok, but prefer monday), net-related: 9441, 9375, 9499 (can someone tag 9499), 9486. wallet related: 9294, 8456 (are you sure that can make it sdaftuar/morcos?)
< BlueMatt>
well, ok, 9486 is whatever, its trivial
< sdaftuar>
BlueMatt: i think 8456 ought to be done, though it is true that gmaxwell keeps finding small issues
< CodeShark>
Please no use of the terms "evil compatibility" or "backward-forward compatibility"
< BlueMatt>
everything else tagged looks like a bugfix (#9490 is, right?)
< gribble>
https://github.com/bitcoin/bitcoin/issues/9490 | Replace FindLatestBefore used by importmuti with FindEarliestAtLeast. by gmaxwell · Pull Request #9490 · bitcoin/bitcoin · GitHub
< gribble>
https://github.com/bitcoin/bitcoin/issues/9461 | [Qt] Improve progress display during headers-sync and peer-finding by jonasschnelli · Pull Request #9461 · bitcoin/bitcoin · GitHub
< jtimon>
lol evil compatibility
< BlueMatt>
sipa: oops, yes, add that to the list
< jonasschnelli>
BlueMatt: simple change. Hope we can get it into 0.14
< jonasschnelli>
Needs review
< BlueMatt>
ok, list for monday: 9380 (if it slips that ok, but prefer monday), 9484, net-related: 9441, 9375, 9499 (can someone tag 9499), 9486. wallet related: 9294, 8456 (are you sure that can make it sdaftuar/morcos?)
< luke-jr>
BlueMatt: do an `action` thing with the final list?
< instagibbs>
someone with the meeting hammer has to do that i think
< luke-jr>
O.o
< BlueMatt>
that list is much too long :(
< wumpus>
I've tagged 9499. Though we should stop tagging more stuff for monday.
< morcos>
BlueMatt: nah... we can do all those
< BlueMatt>
lol
< wumpus>
we're not going to make the current list
< morcos>
I think they are almost all ready for merge except perhaps 9294
< luke-jr>
maybe we should sort the list by priority
< luke-jr>
otherwise we're liable to work on opposite ones first and get none done
< BlueMatt>
i dont think 8456 is gonna make that, either
< morcos>
in 2 hours it'll have 2 ACK's, but if it doesn't make it, thats fine
< cfields>
BlueMatt: what do you think about pulling out your net lock change from #9419 for inclusion? I've been assuming that would make it in in one way or another
< * sipa>
imagines creating a few sockpuppet accounts on github now
< sipa>
*morcos
< jonasschnelli>
heh
< BlueMatt>
wumpus: 9499 was deliberately written to be as easy to review as possible (for 0.14)...there are tons of things to make it better, but they were all left
< luke-jr>
sipa: that'd violate github tos :P
< cfields>
(it belongs in 9441, i just left it out because you already had one)
< jonasschnelli>
luke-jr: depends how many kids you have
< BlueMatt>
cfields: oh fuck I forgot about the cs_vSend split there
< jtimon>
9380 seems easy to review, more about deciding what to expose now and what to leave for later
< BlueMatt>
argh, well I can open it in its own pr, but that one's gonna be a rush if we want it
< BlueMatt>
it is a huge win, though :/
< luke-jr>
jonasschnelli: account terms #1 You must be 13 years or older to use this Service.
< BlueMatt>
no more releases, just keep putting things in :)
< luke-jr>
nobody did a #action with the PR list
< luke-jr>
did anyone sort it?
< BlueMatt>
ehh, whatever
< luke-jr>
if nobody sorts it for me, I'm just going to review them in random order <.<
< jtimon>
luke-jr: they're ordered by appearence in the link above ^
< BlueMatt>
cfields: to finish our discussion from pre-meeting: I'm not sure what to do here...I dont think having a WaitForSync() barrier is sufficient, I can add lots of comments everywhere noting potential issues for reviewers to see on future prs?
< cfields>
BlueMatt: deal.
< cfields>
it'll get solved as part of parallel processing
< BlueMatt>
not really?
< BlueMatt>
i mean its not going away
< cfields>
BlueMatt: i mean as part of SendMessages dissolving into more event-based sends
< BlueMatt>
how does that fix this?
< cfields>
BlueMatt: for ex, BlockChecked could unblock all sends waiting on full verification
< luke-jr>
(FWIW, my prioritised list I will be using is: 9294, 8456, 9499, 9375, 8441, 9486, 9484, 9380)
< cfields>
(not thought through, just an off-the-cuff approach)
< BlueMatt>
cfields: yes, possibly
< instagibbs>
luke-jr, 8441 is some merged thing from august..
< luke-jr>
oops *goes to find what he meant to put there*
< cfields>
luke-jr: 9441 i assume
< luke-jr>
yep
< instagibbs>
crisis averted
< * luke-jr>
wishes 9294 was multiple commits :x oh well
< BlueMatt>
cfields: is that sufficient for #9375?
< sipa>
wumpus: i wonder if you should choose randomized feature freeze dates, and not announce them in advance
< BlueMatt>
lol
< BlueMatt>
probably
< sipa>
BlueMatt: going to look over 9375 again soon
< luke-jr>
it kinda scares me that some failure cases of ReserveKeyFromKeyPool are non-obvious and requires explicit checking. would anyone mind if I made it throw an exception instead? jonasschnelli BlueMatt
< BlueMatt>
luke-jr: i havent looked at that code in a long time, happy to review a pr, though :;
< BlueMatt>
:p
< bitcoin-git>
[bitcoin] luke-jr opened pull request #9537: Wallet: Refactor ReserveKeyFromKeyPool for safety (master...refactor_wallet_RKFKP) https://github.com/bitcoin/bitcoin/pull/9537
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9538: [trivial] Remove redundant call to get() on smart pointer (thread_specific_ptr) (master...remove-redundant-get-on-smartptr) https://github.com/bitcoin/bitcoin/pull/9538
< BlueMatt>
cfields: sorry, found two more issues in 9441
< BlueMatt>
:(
< cfields>
BlueMatt: yep, looking them over now
< cfields>
BlueMatt: iirc i was unhappy about the placement of setting fPauseSend, but left it alone for the sake of not being a moving target. happy to fix that.
< BlueMatt>
well right now its technically wrong
< BlueMatt>
soooo
< BlueMatt>
if someone set a stupid low nSendBufferMaxSize it might actually be triggerable, though maybe not
< jeremyru1in>
is there a good reason why validation.h should not include consensus/consensus.h?
< BlueMatt>
no?
< jeremyru1in>
(ok, was making some derived constants that belong in validation.h)
< instagibbs>
morcos, what kind of performance boost does the caching give? (asking the obvious re:relay cmpct)
< morcos>
the act of looping through your peers and announcing blocks to them is now much faster that the block is cached, as well as responding to getdata's for the cmpctblock or blocktxn messages
< morcos>
it greatly decreases the processing time for relay
< BlueMatt>
massively so, in fact
< BlueMatt>
luckily sipa's last round on 9375 was all pretty minor stuff
< bitcoin-git>
[bitcoin] practicalswift opened pull request #9539: [trivial] Avoid initialization to a value that is never read (master...avoid-initialization-to-a-value-that-is-never-read) https://github.com/bitcoin/bitcoin/pull/9539
< cfields>
BlueMatt: yep, you're right.
< cfields>
lol, "git commit --amen" worked
< cfields>
praise be.
< BlueMatt>
wut
< cfields>
typo'd 'git commit --amend'
< BlueMatt>
lol, someone left that in as a easteregg
< BlueMatt>
not in man-page
< gmaxwell>
yep, git commit --amen works.
< BlueMatt>
someone go file a bug and ruin their fun :P
< BlueMatt>
confirmed: easter egg works here too
< gmaxwell>
I don't know if its an easter egg or just 'shortest unambigious prefix is accepted for maximum forward incompatiblity'
< sdaftuar>
cfields: in CConnMan::Interrupt(), why is there a lock protecting flagInterruptMsgProc? it's an atmoic that doesn't seem to be protected by a lock anywhere else (unless i'm just badly confused about how all this works)
< cfields>
seems to be that. --ame works too.
< BlueMatt>
i could totally see git pulling shit like that :(
< BlueMatt>
ffs
< sipa>
BlueMatt, gmaxwell: pretty sure any unambiguous prefix is intentionally accepted
< sipa>
sdaftuar: you need a lock for the condition variable anyway
< cfields>
right, it's for the condvar
< cfields>
sdaftuar: I was thinking the same thing as you. BlueMatt and i debated that. I lost hard.
< sdaftuar>
but we release the lock before calling condMsgProc.notifyAll(), don't we?
< BlueMatt>
sdaftuar: you cannot use a condvar without a lock.
< cfields>
sdaftuar: need to lock regardless, may as well stick the wake condition in it. Notifying while locked would be a pessimism, though.
< BlueMatt>
said lock has to be released after updating the wakeup condition, and either held during, or released before the wakeup call. (it can, optionally, be taken entirely after updating the wakeup condition)
< sdaftuar>
if i understand right, we acquire lock, set the variable, release the lock, then do the notify_all() (which internally is acquiring and releasing as well)?
< sdaftuar>
is that correct?
< BlueMatt>
gmaxwell: my reasoning for requesting a month timeout on 9484 instead of a week was that if software were to be shipped with a bad hash I'd feel much better if it required the attacker create a month of blocks on top of the invalid one and stay ahead of the longest chain than only a week
< BlueMatt>
sdaftuar: notify_all does not acquire or release a lock, or, it does not need to per the spec, it might very well internally
< BlueMatt>
std::condition_variable_any definitely does
< sipa>
sdaftuar: *waiting* on a condvar requires a lock
< sdaftuar>
ok, i remain deeply puzzled, but i will worry about this when i'm at my desk again
< sipa>
signalling can happen by anyone, anytime
< BlueMatt>
sdaftuar: specifically, whie technically implementing the "unlock+wait is a single atomic action" will require a lock, it will require a lock that is intimately connected with the kernel's thread_waiting stuff in order to not have weird corner cases that are really non-performant
< gmaxwell>
BlueMatt: anyone who can create more than a day of bad blocks is already reversing third party transations and stealing coins from arbritary people.
< BlueMatt>
gmaxwell: I absolutely am confident you could easily get a massive chunk of the network hashpower for a day or two
< BlueMatt>
I am much less confident you could hold it for a week...and if you give it time to reorg back to a valid chain a month makes me feel better :p
< gmaxwell>
BlueMatt: great 7 days is 7 times a day.
< BlueMatt>
if you have like 75% for two days, then it trails off for a few days as folks recover, you may end up staying ahead for a week or so
< gmaxwell>
keep in mind this also requires the vendor to ship a bad hash, seriously, you're arguing against removing the feature.
< gmaxwell>
the argument PT gave is that its harmless because the hash is much easier to audit/review than virtually any of the other code.
< BlueMatt>
huh? I mean if a month is a massive sync-time feature then I'm fine with leaving it, but I dont think it is?
< BlueMatt>
I could absolutely see the hash getting changed in the middle of an rc, everyone saying "meh, i guess thats ok", building happening, and then it getting discovered mid-gitian-phase
< BlueMatt>
(ie how much of that is non-sig-checking)
< BlueMatt>
I mean even with -assumevalid we're gonna take an hour or five to sync on some machines like that...doing the spv-initial sync (especially with -assumevalid) is gonna be the savior if you want lightning-fast sync...I'm ok eating an extra 30 minutes or hour, especially if its not something thats gonna grow ad infinitum forever
< gmaxwell>
^ thats with a 3GB db cache.. I think it's about 30 minutes for the same span with assume valid.
< gmaxwell>
you're missing my point. If you earnestly believe that auditing the hash is harder then the software then you should believe that we MUST NOT ship this feature at all.
< BlueMatt>
hilariously, setting it to a month is gonna mean constant performance for a month after release, as the chain grows on top of the assumevalid setting, whereas setting it to a week means it'll start growing again after a week :p
< luke-jr>
auditing the hash is as trivial as running without it, no?
< BlueMatt>
no, i get your point, but I also dont think we have enough people looking at code, plus there are how many instructions for "how to run a node" that people might figure out if it said "download from my server" but might not catch if it says "set assumevalid to this hash, it makes sync happen instantly"
< gmaxwell>
luke-jr: yes and checking if that block is in your chain.
< BlueMatt>
i mean the recommended method to audit the hash takes a day for many folks :p
< BlueMatt>
while the recommended way to set the hash is right before release
< sipa>
i'm fine with either a week or a month. </bikeshed>
< gmaxwell>
BlueMatt: no, it's part of the procedure that happens at the start of RCs.
< * instagibbs>
re-opens <bikeshed>
< gmaxwell>
BlueMatt: the procedure is actually checking two things: that the release didn't introduce any consensus reguressions in previously validated blocks, and that the new hash is acceptable. To only check the latter you need simply check that it's in your chain from a node not yet running that value. 2 seconds, and thats it.
< gmaxwell>
The longer procedure is an omission in our current process that we should already be addressing due to the checkpoint skipping. (though I do a checkpointless resync of every release myself)
< BlueMatt>
gmaxwell: look at it this way - shipping a massive footgun which we think might plausibly trigger without considering the entire bitcoin network worthless seems questionable, shipping a massive footgun which would likely only ever trigger if someone had persistent control of 60% of hashpower....maybe less so
< gmaxwell>
If it is a massive footgun it must not be shipped.
< BlueMatt>
if its a massive footgun that cant go off without bitcoin being completely broken I think thats fine :p
< BlueMatt>
gmaxwell: how long does your sync take on your laptop start-to-finish with -assumevalid set to a week?
< sipa>
i think the footgun (wth is that?) is minimal
< luke-jr>
sipa: footgun is a gun designed for shooting one's own foot
< cfields>
sipa: a device one uses to shoot one's self in the foot.
< TD-Linux>
it's a footgun that only triggers when you've already lost your legs.
< sipa>
to exploit it, the attacker would need to have an invalid majority branch already mined at the time of software relwase
< BlueMatt>
(if this were being proposed with zero additional checks I would be arguing against it wholesale, fwiw)
< sipa>
or is the issue not the hardcoded value, but people convincing others to run with a certain settings value?
< gmaxwell>
BlueMatt: with my large db cache, about 5 hours in a chainstate reindex. I can do more recent numbers later this week.
< luke-jr>
sipa: or trick the user into configuring a majority invalid branch
< BlueMatt>
so its 5 hours -> 7.5 hours if we set it to a month?
< BlueMatt>
or 5 -> 7.25
< sipa>
luke-jr: but they'd need to already have that invalid branch mined, and ahead by a week?
< gmaxwell>
If I never stuck that check in none of you would have suggested it.
< BlueMatt>
gmaxwell: (if this were being proposed with zero additional checks I would be arguing against it wholesale, fwiw)
< jtimon>
luke-jr: right, that's much better justification for the one week thing than the "artificially setting it 'too earlly' in releases" IMO
< gmaxwell>
I think you're wrong.
< luke-jr>
sipa: not necessarily
< BlueMatt>
I'm not sure I would have suggested said check, but if weren't there I'd be saying uhhh, lets not
< luke-jr>
ahead by a week, yes, but not already-mined
< sipa>
BlueMatt: what specific attack scenario are you worried about?
< sipa>
(honest question)
< gmaxwell>
BlueMatt: (my blunt assumption is based on that you haven't been contributing to removing the existing checkpoints, no insult intended)
< BlueMatt>
sipa: no, eg you start mining an invalid chain with your small hashpower, tell people to set the assumevalid setting in some blog post in russian that none of us see, when you find one guy who is gonna be your target, pwn a bunch of hahspower for a few days (I'm pretty confident you could get 75% for a few days, at least, if not more), and stay ahead for a week
< BlueMatt>
its kinda weird, but definitely not impossible
< BlueMatt>
gmaxwell: so, to confirm, it would take chain sync on your laptop from roughly 5.5 hours to 7.25 hours to set it to a month?
< sipa>
BlueMatt: but the assumevalid setting value is only known after creating that branch
< gmaxwell>
BlueMatt: I believe so.
< BlueMatt>
gmaxwell: if thats the case, I might be convinced that a month is too long and we should go with 2 weeks
< BlueMatt>
sipa: creating the start of the branch, not actually having to actively keep it up to sync all that well
< luke-jr>
sipa: you'd make the transactions after your assumevalid all be valid
< BlueMatt>
luke-jr: huh? no, you wouldnt
< luke-jr>
BlueMatt: why not?
< BlueMatt>
you would make all the txn before your assumevalid be valid
< BlueMatt>
not after
< BlueMatt>
well, and including, i think
< luke-jr>
before it, they won't check if it's valid
< * gmaxwell>
flies
< jtimon>
BlueMatt: he means as an attacker
< luke-jr>
so that's where you'd want to hide the invalid stuff
< sipa>
BlueMatt: if at any point your attacker branch is overtaken by the honest branch, assumevalid has no more effect
< BlueMatt>
oh, i see your point, ok
< BlueMatt>
sipa: I'm aware, and I'm pretty confident you could manage to get an attack chain longer than the honest chain for a week
< BlueMatt>
but maybe only like 5/6 days, and probably not much longer
< BlueMatt>
eg start by pwning 75% of the network for 2 full days, then a tail while folks take a while to fix their shit
< BlueMatt>
if you're really clever you'll pwn the pool servers, have some knowledge of where they'll fall back to, and be prepared to do a bgp attack against their (hopefully-less-bw-flood-intensive) fallback asn
< luke-jr>
anyhow, this all requires getting the user to manually set the block hash
< luke-jr>
IMO just hide it as a debug option and we're fixed
< BlueMatt>
luke-jr: I think you missed part, above
< luke-jr>
?
< BlueMatt>
eg you're an attacker, you maintain a russian-language (or some other language none of us would find) blog on "how to set up a bitcoin node"...you keep some small amount of hashpower mining invalid chains based on the best chain all the time, so you always have some hash in your page that isnt too far back from the tip, maybe a few hours, you very actively monitor users and try to find when someone who is a real target uses it
< BlueMatt>
then you reveal that you've pwned all the pool servers and start mining invalid garbage for a few days
< BlueMatt>
its kinda a strange scenario, and seems pretty highly unlikely, but I do not believe it is impossible
< luke-jr>
BlueMatt: how will your invalid chain get accepted?
< BlueMatt>
it wont be accepted by anyone except your target?
< luke-jr>
why would it be accepted by your target, though?
< luke-jr>
he'd need to set the config option to your malicious block hash
< jtimon>
huhm, what does mining invalid garbage on top of your fake invalid assumevalid give you?
< BlueMatt>
because you got them to set the assumevalid setting?
< BlueMatt>
luke-jr: yes, did you miss the part where they set up their bitcoin node based on your instructions?
< jtimon>
right, the ate the invalid stuff belowe your fake assumevalid, but not above it
< jtimon>
s/the/they s/belowe/below
< luke-jr>
ok, so the user is an idiot who will set the option without investigating what it does
< BlueMatt>
as luke pointed out, you could print (by stealing) on the assumevalid block, and then spend it to your victim in a later bnlock
< BlueMatt>
luke-jr: most users do shit like that....
< luke-jr>
what if we rename it to assumevalid_this_can_compromise_your_node?
< BlueMatt>
luke-jr: fuck, many users use the ppa
< jtimon>
Ok, so precisely this attack is what the 1 week thing serves for, no?
< luke-jr>
can't blame them. using the PPA makes a lot of sense considering they already trust Canonical.
< BlueMatt>
jtimon: my claim is that I'm not at all convinced you could not have a longer chain than the honest one for a week
< jtimon>
BlueMatt: thus you propose to change it to 2 weeks ? (sorry, I should have read all the logs before intervening)
< BlueMatt>
gmaxwell: does 2 weeks meet your performance target? its more like an extra half hour
< BlueMatt>
jtimon: yes, I prefer a mont but gmaxwell pointed out that that increases sync time on his laptop with massive dbcache by something like 1.5 hours
< BlueMatt>
which does seem like a big cost to pay
< jtimon>
regarding the 5.5 vs 7.5 h benchmark, that's only for fresh releases or people setting the arg manually right before the limit, right?
< BlueMatt>
yes
< BlueMatt>
thats only if you sync within the first week of the release
< BlueMatt>
well, first week after the hash was selected/set
< jtimon>
I don't oppose changing it to 2 weeks, it's performance vs a more cautious protection against a possible attack
< jtimon>
I mean, even with a month it's a great improvement over the checkpoint way
< BlueMatt>
oh, totally I think we should do this, just prefer a tiny inch more conservatism
< cfields>
BlueMatt: it seems a bit unrealistic to argue what time-period to pick in that scenario. Assuming I'm an attacker in the above conditions, I'd send the victim an "optimized binary that syncs the chain faster" (and it would, the wrong one :P) rather than trying to get them to fiddle with config settings.
< jtimon>
and I'm happy with both 2 or 1 week, so I don't have anything to convince you about
< BlueMatt>
cfields: given an assumevalid setting, I'm pretty sure it'll become common-enough practice to tell people to set it to make chainsync faster
< BlueMatt>
whereas "download from this random site" might be a bit more alarming to folks
< jtimon>
cfields: well, if you can send them an "evil binary" the whole assumevalid thing is irrelevant: you can change the rules!
< cfields>
jtimon: that was exactly my point.
< jtimon>
a bad bitcoin.conf seems more realistic
< BlueMatt>
as for why a month instead of a week, well its about human-scale response to attacks....how long would it take for pools to escalate a large-scale hack against their infrastructure up to their hosting provider...what about if there's a bgp-based attack? how long for global NOCs to respond? etc
< jtimon>
"here's my bitcoin.conf, copy it, it syncs super-fast"
< BlueMatt>
then double because the honest chain has to catch back up
< cfields>
jtimon: given the example above, undermining the majority of hashpower, surely getting them to run my binary is trivial in comparison
< BlueMatt>
cfields: you massively underestimate how trivial it is to pwn most pool servers' hosting providers
< BlueMatt>
also, remember that there is no auth on stratum
< BlueMatt>
so even those who get off big centralized providers and host themselves for security will possibly get fucked due to mitm/bgp/etc/etc
< jtimon>
cfields: maybe it's that I underestimate the loss of using 1 month instead of 1 week...
< cfields>
BlueMatt: i'm not arguing with you about that. I'm arguing the relative ease of undermining your victim (who is already gullible enough to set assumevalid)
< BlueMatt>
ehh, I'm not sure about that, though I absolutely agree the example attack given above is not the most robust example
< cfields>
BlueMatt: updated 9441. Now going over 9375 one last time.