< 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
< 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 :-)
< 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.
< 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>
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>
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.
< 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
< 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. :)