< sipa> it gives an example that include a one-line unbraced indendented conditional
< sipa> -den
< BlueMatt> gmaxwell: I thoguht the same, but sipa corrected me
< BlueMatt> oh, wait, prohibited on single line statements? no
< BlueMatt> i knew that one
< BlueMatt> i thought we had a style guide (or someone had a preference) for the open-brace /always/ being on the line after the if
< gmaxwell> BlueMatt: thank you so much for commenting on #9484
< gribble> https://github.com/bitcoin/bitcoin/issues/9484 | Introduce assumevalid setting to skip validation presumed valid scripts. by gmaxwell · Pull Request #9484 · bitcoin/bitcoin · GitHub
< gmaxwell> sipa: yea, I think that should be prohibited. But even if it's just permitted to not do that, I'd be much happier.
< gmaxwell> I'm fine with "if (!something) return false;" too; but we've accepted patches that do nothing but add line breaks to code like that.
< gmaxwell> but from working on software with that as a coding style, I recall that there was no way to get any of the existing formatters to enforce that rule.
< gmaxwell> it would be neat if someone made a formater autoconfiguration tool that finds the formater settings that minimize the changes to your code base, then prompts you for all the settings that have no effect.
< gmaxwell> hm actually clang-format has AllowShortIfStatementsOnASingleLine so it can do that, I was mistaken.
< Chris_Stewart_5> It is possible to have a tx with no outputs right? It would just pay all inputs to miner fees?
< luke-jr> Chris_Stewart_5: i don't think so
< Chris_Stewart_5> Trying to get my generators right on #8469 it is important that they allow every possible value
< gribble> https://github.com/bitcoin/bitcoin/issues/8469 | [POC] Introducing property based testing to Core by Christewart · Pull Request #8469 · bitcoin/bitcoin · GitHub
< gmaxwell> it is not possible.
< gmaxwell> (I mean you can create such an encoding, but the transaction could never be valid.)
< Chris_Stewart_5> gmaxwell: Valid in the policy sense?
< gmaxwell> no, in the consensus rules sense.
< gmaxwell> the word 'never' should have tipped you off. :)
< Chris_Stewart_5> gmaxwell: I'm not sure i'm ready to believe you yet, why isn't it valid? Some where in validation.cpp there is a check for vout.size() > 0?
< gmaxwell> yes.
< Chris_Stewart_5> like you said, enocding/script wise it should be fine I think
< Chris_Stewart_5> and is it just a thing satoshi did, or is there an actual attack that would be possible
< gmaxwell> well it does simplify validation code when it doesn't have to check the count before accessing the first one... but no there isn't any special attack.
< Chris_Stewart_5> gmaxwell: Thanks for the explanation :-)
< bitcoin-git> [bitcoin] TheBlueMatt opened pull request #9488: Parallel ThreadMessageHandler (master...2017-01-parallel-processmessages) https://github.com/bitcoin/bitcoin/pull/9488
< gmaxwell> #7871 is sometihng I probably should have mentioned for a 0.14 target in the last meeting, it is highly complementary with importmulti.
< gribble> https://github.com/bitcoin/bitcoin/issues/7871 | Manual block file pruning. by mrbandrews · Pull Request #7871 · bitcoin/bitcoin · GitHub
< gmaxwell> though the fact that one works based on time and the other based on height is a little awkward.
< gmaxwell> oh weee. thats a bug.
< bitcoin-git> [bitcoin] gmaxwell opened pull request #9489: Make FindLatestBefore use GetMedianTimePast instead of GetBlockTime. (master...fix_find_latest_before) https://github.com/bitcoin/bitcoin/pull/9489
< luke-jr> Chris_Stewart_5: you *could* have a single output with zero value, note
< Chris_Stewart_5> luke-jr: Or even negative value
< Chris_Stewart_5> I think?
< Chris_Stewart_5> BuildCreditingTransaction uses that inside of transaction_test.cpp any way...
< sipa> wut?
< sipa> negative value outputs?
< bitcoin-git> [bitcoin] gmaxwell closed pull request #9489: Make FindLatestBefore use GetMedianTimePast instead of GetBlockTime. (master...fix_find_latest_before) https://github.com/bitcoin/bitcoin/pull/9489
< gmaxwell> sipa: will I be hated if I make the entries in CBlockIndex 8 bytes larger? :(
< gmaxwell> I need a 'highest timestamp seen so far in this chain' for this FindLatestBefore.
< gmaxwell> obvious way to do that is to just have a maxTime in the CBlockindex which = max(nTime,prev->maxtime). sorry 4 bytes, I guess our time there is only 32bits.
< luke-jr> Chris_Stewart_5: I am relatively certain that negative value outputs are always invalid.
< BlueMatt> gmaxwell: IIRC there are a good chunk more than 8 bytes available in CBlockIndex just from better packing
< BlueMatt> so.....
< kallewoof> I always thought bitcoin was supposed to pick the chain with the most work, not the longest chain. I did some testing, and with a split network bitcoin picked the longest chain, even though the shorter-by-one chain had a much lower hash.
< kallewoof> I.e. (1) 5b6aab3f > 3d697a37 > 4ea2f338. (2) 5b6aab3f > 0000000b. On connecting nodes, result was (1) not (2).
< kadoban> kallewoof: Wouldn't the difficulty at that point matter, not the actual hashes generated? Or did I make that up?
< kallewoof> You mean nBits?
< gmaxwell> kadoban: correct.
< kadoban> I don't know the block format amazingly well, I only remember some parts at a conceptual level. nBits sounds possibly right though
< kadoban> Ah good
< gmaxwell> kadoban: other than sufficiency, the hash value doesn't have anything to do with the work that went into it.
< gmaxwell> though nbits doesn't tell you the total work, it tells you how much work was required for a particular block.
< gmaxwell> the total work is represented as nChainWork in the bitcoin codebase, it isn't seralized as part of the block.
< kallewoof> So what is the point with the whole 'most work not longest chain' talk? Nbits is network-widely defined so there will not be a difference ever, will there?
< gmaxwell> kallewoof: difficulty changes over time.
< kallewoof> I mean, it's retargeted, but that's it
< kallewoof> One client will not have one nbits value while another has a different one, hardly ever.
< gmaxwell> ...
< gmaxwell> nbits is a property of the _chain_ not the client.
< kallewoof> I mean... the work done to a chain is dependent on the accumulated difficulties (which are the nbits values, right?). I just can't think of a case where you would have a shorter chain being chosen due to more work.
< gmaxwell> kallewoof: I can trivially go fork at block 100,000 and then hand you a chain with way more blocks then the current chain but massively less work.
< gmaxwell> it's a trivial attack, and utterly devistating to most-blocks.
< kallewoof> Ahh.. so that's what the whole 'most work' thing is about. I get it. Thanks.
< gmaxwell> absent attacks, whenever there is a chain fork around a retarget, the two sides can have different amounts of work; which makes a difference though not a terribly critical one.
< gmaxwell> no problem.
< gmaxwell> kallewoof: lots of people miss that one, the bitcoin software was originally most blocks.
< kallewoof> It is super obvious now that you've described it. I wish someone had worded it that way somewhere.
< gmaxwell> it's the sort of thing that would have been described in the whitepaper, if it were known at the time. :)
< gmaxwell> also, consider yourself fortunate: the worst position to be in is having never been surprised by anything; from that vantage you can't tell how subtle this stuff is... because it's all obvious when presented clearly.
< kallewoof> gmaxwell: True enough. :)
< kallewoof> Here's the log of my fine-but-failed experiment in case anyone wants to see it, btw: https://gist.github.com/kallewoof/9b0e541366941e08e41ab1bf3e03f017
< bitcoin-git> [bitcoin] gmaxwell opened pull request #9490: Replace FindLatestBefore used by importmuti with FindEarliestAtLeast. (master...fix_find_latest_before) https://github.com/bitcoin/bitcoin/pull/9490
< gmaxwell> wump: I think #9404 could be merged.
< gribble> https://github.com/bitcoin/bitcoin/issues/9404 | Smarter coordination of change and fee in CreateTransaction. by morcos · Pull Request #9404 · bitcoin/bitcoin · GitHub
< gmaxwell> BlueMatt: I with the currently list of network messages in your parallel message handler list-- whats the point? I also don't see how you can handle a GETBLOCKTXN without cs_main. Do you intend on making it peek at the message further to decide if it will be able to use the cache?
< fanquake> paveljanik what do you mean by diff-only related changes in 9469? The changes are required to make the patches apply cleanly..
< paveljanik> fanquake, and this was my question :-) So they are to make the diff apply cleanly. OK. Np with that.
< paveljanik> So we somewhere require the patch to apply cleanly?
< fanquake> The patches are applied before building the dependancies, https://github.com/bitcoin/bitcoin/blob/master/depends/packages/libevent.mk#L8. There isn't a hard requirement that they apply perfectly, but it seems logical to update them to do so.
< paveljanik> I do not think so ;-)
< paveljanik> fanquake, ad config.* files - github shows then without final newlines at the end of them...
< fanquake> paveljanik if you look at the source of them, http://git.savannah.gnu.org/gitweb/?p=config.git;a=blob_plain;f=config.guess, I can't see any new lines. Maybe I'm missing something?
< paveljanik> wget -O config.sub 'http://git.savannah.gnu.org/gitweb/?p=config.git;a=blob_plain;f=config.sub;hb=HEAD'; && hexdump -C config.sub | tail -2
< paveljanik> shows 0x0a at the end.
< paveljanik> the same for the other file.
< fanquake> heh I can change it back, will investigate the OS X issue.
< fanquake> paveljanik It'd be good if you could also review the contents/changelogs of the dependancy updates/hashes of said updates also.
< paveljanik> I'm in the middle of it ;-)
< fanquake> Newlines will be the least of our worries if we've got a borked boost version etc
< paveljanik> Hmm, but we should not change the upstream files...
< paveljanik> and these two are upstream files.
< fanquake> paveljanik which files?
< paveljanik> config.sub and config.guess
< fanquake> Why would we not update those?
< paveljanik> we should fetch them and store as-is in the depends, not modify them after fetching.
< paveljanik> update: yes!
< paveljanik> modify: no!
< paveljanik> You updated (good!) and modified (wrong!).
< paveljanik> but maybe the modification was not intentional...
< paveljanik> some tooling or so...
< fanquake> We're not patching/modifying them though?
< fanquake> oh your talking about the newlines? Yes that can be fixed.
< paveljanik> You did modify the file by deleting the final newline!
< paveljanik> yes
< paveljanik> BTW - why don't we fetch them too?
< fanquake> Probably something you'd have to ask theuni.
< fanquake> gmaxwell I think I must be missing something in #9484. Started a -reindex-chainstate with -assumevalid=<default hash>, and it's working far slower than just current master.
< gribble> https://github.com/bitcoin/bitcoin/issues/9484 | Introduce assumevalid setting to skip validation presumed valid scripts. by gmaxwell · Pull Request #9484 · bitcoin/bitcoin · GitHub
< BlueMatt> gmaxwell: I mean it could only respond to getblocktxn if its the top-block (ie cached one)...I'm perfectly ok with it having a chance at blocking if it happens to be a request for the wrong block
< BlueMatt> gmaxwell: in the common case it'll work super well, in the rare case (or if your peer is mean) it might block
< BlueMatt> gmaxwell: in the future we could look deeper at the message to see if we will block
< sipa> gmaxwell: what does 'timestamp seen in a chain' mean?
< BlueMatt> gmaxwell: for 0.15 I may try to push a read/upgrade/write lock model again, which would also make this useful
< BlueMatt> gmaxwell: with real State() fixes, we could also support FILTERLOAD/FILTERADD/FILTERCLEAR/VERACK/ADDR/SENDHEADERS/
< BlueMatt> which, btw, is all of our non-block/tx-download non-version messages
< BlueMatt> and I think we could get version too
< BlueMatt> but it would take a tiny bit more work
< BlueMatt> maybe for 15 we could even get the mempool-can-be-behind-chainActive stuff like we have for wallet now
< BlueMatt> which would make this really awesome
< BlueMatt> sipa: you were asking about easy merges? #9480, #9353
< gribble> https://github.com/bitcoin/bitcoin/issues/9480 | De-duplicate SignatureCacheHasher by JeremyRubin · Pull Request #9480 · bitcoin/bitcoin · GitHub
< gribble> https://github.com/bitcoin/bitcoin/issues/9353 | Add data() method to CDataStream (and use it) by laanwj · Pull Request #9353 · bitcoin/bitcoin · GitHub
< BlueMatt> sipa: oh, and the one I actually meant to refer to : #9404
< gribble> https://github.com/bitcoin/bitcoin/issues/9404 | Smarter coordination of change and fee in CreateTransaction. by morcos · Pull Request #9404 · bitcoin/bitcoin · GitHub
< jm111t> just wanted to mention that about 2 months ago I reported that I could not import a key and then by rebuilding the blockchain it worked again.
< jm111t> well I had the problem again and found out why. The reason is the file system, the problem arises when your drive is formated with msdos.
< jm111t> just in case it might help someone.
< luke-jr> BlueMatt: I don't think I can fix most of your nits without amending past commits..
< BlueMatt> luke-jr: thats fine...up to you to do that if its required (and no one has published partial reviews)...alternatively you could just add SQUASHME commits on top
< gmaxwell> sipa: I'm not sure of the context of your question, but I presume nTime.
< luke-jr> BlueMatt: that doesn't work for moving changes from commit X to X-1
< BlueMatt> ehh, i dont care if you fix that one
< BlueMatt> but if you get around to doing another rebase, might as well do it then
< luke-jr> BlueMatt: any suggestions for removing the pointer from the debug log? there's kinda nothing else unique..
< luke-jr> maybe it'd be okay with lockwallet(0x%x) instead of a decimal number?
< BlueMatt> yea, I dont have any great insight into that one...what do you use for wallet ids in the final multiwallet pr?
< BlueMatt> no, the point is to not print a pointer into debug.log, since I could contort it into an issue for ASLR
< luke-jr> BlueMatt: final multiwallet gives the wallets names, but there is no strong guarantee they won't have the same name
< luke-jr> I suppose I could use the filename if I move some parts of that into this
< BlueMatt> huh? if they have the same name how would you index them? dont you check name-uniqueness on load?
< luke-jr> they aren't indexed.
< BlueMatt> i mean for this pr you could just have a second id field in the scheduler thinggy that gets appended to the string after print
< BlueMatt> how do you specify the wallet?
< luke-jr> there's a dropdown box in the GUI, and for RPC by filename
< BlueMatt> oh, hum....
< luke-jr> hm, I suppose strWalletFile is already there
< luke-jr> and I guess we don't support files outside of .bitcoin or in subdirs
< BlueMatt> yea, I'd be more comfortable with just using that
< luke-jr> it's not a strong guarantee, but maybe good enough
< BlueMatt> well you certainly cant load two wallets with the same name???
< BlueMatt> oh, wait, no, you also cant load two wallets in the same dir, can you?
< BlueMatt> does bdb barf on that?
< luke-jr> you can only load two wallets in the same dir
< BlueMatt> oh? i guess its all the same bdb context thinggy?
< luke-jr> yes
< luke-jr> we don't actually enforce same-dir, but it's going to use the same bdb database dir I think
< * luke-jr> wonders if we should be enforcing that
< BlueMatt> yes, probably
< BlueMatt> when in doubt, enforce, I'd say here
< BlueMatt> given bdb.........
< luke-jr> not really sure what to do with 989e352f7931f6ab9212e821e2d00e4aa0106635. Someone wanted it. :/
< BlueMatt> i mean the ones inside ifdef ENABLE_WALLET could be moved, and then you could #include inside an ENABLE_WALLET
< BlueMatt> but the include should only be inside an ENABLE_WALLET
< luke-jr> BlueMatt: okay, I think I got everything
< luke-jr> oh fun, master doesn't build :x
< * luke-jr> hopes his rebase went smoothly
< BlueMatt> huh?
< BlueMatt> under what settings?
< luke-jr> test/raii_event_tests.cpp:39:58: error: ‘event_set_mem_functions’ was not declared in this scope
< gmaxwell> fanquake: doesn't bode well for reviewing the constants; when I updated for opening the pull request, I updated the constants, and managed to put a testnet block in the mainnet default. :)
< gmaxwell> fengling: it's fixed now.