< sipa>
i saw 5% in cpu reduction in the disk flushing code
< fanquake>
Ok, I'll post some numbers shortly.
< sipa>
i would estimate that this effect is most visible if you havr fast disk, slow cou, and low dbcache
< sipa>
*slow cpu
< sipa>
but yes, please post numbers
< sipa>
and thank you
< fanquake>
If anyone want's to chime in on #8639 with any minimum required versions/more info it would be appreciated. When I get a chance I'll turn the tables into some proper documentation. Which will partially fix #8923
< MarcoFalke>
I feel like we should change the releases to only contain a link to the release notes and not the full release notes copied: https://github.com/bitcoin/bitcoin/releases
<@wumpus>
why? redundancy is pretty good for distributed projects? :-)
< MarcoFalke>
There is no redundancy when the link point to github as well.
<@wumpus>
ah you mean linking to the release notes on github - yes that'd make sense
<@wumpus>
true
< MarcoFalke>
It takes 10 minutes to scroll though the releases page
<@wumpus>
I'm ok with replacing them with a link, on the other hand sipa did all this work to put them in in the first place, I'm not sure it's worth the trouble re-doing it another way
< MarcoFalke>
Also, no one will notice if they are changed by someone. They are not signed and can not be verified
< MarcoFalke>
Should be a matter of less than 10 minutes. I am happy to do it if sipa is ok with it.
<@wumpus>
also updating that page should probably be part of the release process
< GitHub68>
[bitcoin] MarcoFalke opened pull request #9093: [doc] release-process: Mention GitHub release and archived release notes (master...Mf1611-docRel) https://github.com/bitcoin/bitcoin/pull/9093
< GitHub10>
[bitcoin] laanwj opened pull request #9094: qt: Use correct conversion function for boost::path datadir (master...2016_11_datadir_in_console) https://github.com/bitcoin/bitcoin/pull/9094
< GitHub126>
bitcoin/master 1ee6f91 nomnombtc: new var DIST_CONTRIB adds useful things for packagers from contrib/ to EXTRA_DIST
< GitHub126>
bitcoin/master 078900d Wladimir J. van der Laan: Merge #8568: new var DIST_CONTRIB adds useful things for packagers from contrib...
< bitcoin-git>
[bitcoin] laanwj closed pull request #8568: new var DIST_CONTRIB adds useful things for packagers from contrib (master...DIST_CONTRIB) https://github.com/bitcoin/bitcoin/pull/8568
< gribble>
https://github.com/bitcoin/bitcoin/issues/8568 | new var DIST_CONTRIB adds useful things for packagers from contrib by nomnombtc · Pull Request #8568 · bitcoin/bitcoin · GitHub
< bitcoin-git>
[bitcoin] laanwj closed pull request #8981: Wshadow: Do not shadow argument with a local variable (master...20161020_Wshadow_rpcdump) https://github.com/bitcoin/bitcoin/pull/8981
< sipa>
wumpus, MarcoFalke: it wasn't much work, and i was a bit surprised there was no way to collapse the notes in the releases page, so of anyone cares, i'm fine with changing it to links (but i hope it doesn't cause mass mails again)
<@wumpus>
looks like you're safe, or maybe gh is queing them all up to send them at once once you finish editing all the tags :)
< jonasschnelli>
MarcoFalke: compiling 9095 over gitian still gives me: test/coins_tests.cpp:10:30: fatal error: test/test_random.h: No such file or directory
< MarcoFalke>
:/
< MarcoFalke>
Trying local gitian build
< jonasschnelli>
I don't understand why we don't have the test headers in the Makefile.test.include
< jonasschnelli>
I know it can't be the reason.. but still
<@wumpus>
I don't understand it either
<@wumpus>
the .h files should be in SOURCES too
<@wumpus>
maybe just try adding them?
< sdaftuar>
MarcoFalke: regarding the wallet-dump.py test issues i've been seeing, here's an example of a test failure on the machine in question: https://0bin.net/paste/iL+SYGPlmsW0E4DH#
< sdaftuar>
i don't really have any idea why it's so slow on this machine that i use to run all the tests. i agree with your observation that on other hardware, like the machine i do most of my work on, the test is very fast.
< MarcoFalke>
jtimon: Wouldn't it make sense to create a separate class for parsing instead.
< MarcoFalke>
I mean not passing the global is a trivial improvement, but it won't help us in the long term goal
< jtimon>
not sure, maybe, so far it would be just those 3 getarg methods, maybe getAmountArg too
< jtimon>
I don't really see much diferrence on passing the map as it is as const or pass it as a class (also as const)
< MarcoFalke>
I mean why would the wallet need to know that mapArgs is of such type? Why would the wallet need to know mapArgs exists at all?
< jtimon>
why would it need to know about the new class? it's the same, no?
< MarcoFalke>
Imo you are exposing too much internal details with the new interface.
< MarcoFalke>
It is similar, but not the same. I think a new class could help to rework the arg handling.
< MarcoFalke>
First, you instanciate CArgParse with the args provided by each module. Then you can parse the args and finally each module can ask the instance for the value of each arg...
< MarcoFalke>
If there are args present in the map which are not provided by any module you can warn the user
< * jtimon>
notes MANDATORY_SCRIPT_VERIFY_FLAGS is outdated
< jtimon>
MarcoFalke: oh, I see, yeah I guess for extra checks it may make sense, not sure it's worth it to parse a different mapArgs for each module and sounds like nasty work
< jtimon>
regarding "exposing too much details" maybe a typedef for the map will help? or you just want to encapsulate the map? in that case you will definitely need more methods, like getAmountArg
< MarcoFalke>
It is a single mapArgs internally and each module provides all valid parameter names to the arg parse class somwehere in init.cpp
< MarcoFalke>
In a second step you actually parse. And thereafter, the modules can query their values by providing a the corresponding parameter name
< MarcoFalke>
jonasschnelli: amended your suggestion. #9095 should pass now
< jonasschnelli>
MarcoFalke: still got: test/coins_tests.cpp:10:30: fatal error: test/test_random.h: No such file or directory
< jonasschnelli>
Do i need to flush/remove something?
< jonasschnelli>
Ah.. shit..
< jonasschnelli>
checked wrong log... still compiling. False alarm.
< MarcoFalke>
Heh, the fact that it didn't fail until now probably means it will pass.
< jonasschnelli>
Yes. Seems to be passing... qt stuff is now compiling
< * jtimon>
doesn't like BlockAssembler::CreateNewBlock uses global chainActive, could just take CBlockIndex* pindexPrev = chainActive.Tip() as param...
< sdaftuar>
MarcoFalke: i tried adding some instrumentation to GenerateNewKey. It looks like the disk-related calls (WriteHDChain, AddKeyPubKey) are what can be slow, but aren't always
< sdaftuar>
MarcoFalke: i'm guessing it's just an issue with my disk being a bottleneck when the machine gets loaded. it's a spinning disk, and i have a lot of jobs running on this machine potentially at the same time
< sdaftuar>
MarcoFalke: i'll probably migrate my test-running to a new machine that's less loaded but i think bumping timeouts to avoid transient errors is still something we might as well do, not really any downside right?
< MarcoFalke>
Yeah, a minute should do it, though. No need to wait for a quarter hour to see something timed out.
< sdaftuar>
yeah
< andytoshi>
is there a simple or linkable reason that segwit blocks need to commit to witness data at all? this is hard to google for, seems like this is always a background assumption, but it's coming up in context of mimblewimble..
< sipa>
the most important reason is DoS protection
< sipa>
usually, we can assume that a high frequency of incoming blocks (which pass PoW) is impossible, because PoW prevents that
< sipa>
but if witness data is not committed to, you cannot distinguish between the case where a miner simply created an invalid block, or your relay peer damaged the witness in flight, and thus you can no longer make validation failure result in the block being marked permanently invalid
< andytoshi>
ok, cool, that's what i had thought
< andytoshi>
there is a related DoS vector with in-flight transactions though, and they don't have PoW
< andytoshi>
i guess that is dealt with by just banning peers?
< sipa>
right
< sipa>
the issue does exist for transactions
< sipa>
but the cost of a transaction for validation is much lower
< andytoshi>
ok, great, thanks
< sipa>
and failure to process a transaction in a short time does not result in network convergence being hurt
< andytoshi>
yeah, that just occured to me
< sipa>
... though after compact blocks, they sort of do, in a weaker way
< andytoshi>
well, peer banning on bad blocks (that aren't actually bad) could lead to a sybil attack being amplified to a permanent fork i suspect
< andytoshi>
the risk with CB is that things will be slower
< instagibbs>
So I noticed that AcceptBlock attempts to validate various amounts of the genesisblock upon reindexing. Is this desired behavior? For example, it checks for height in coinbase if bip34height is set to <= 0
< instagibbs>
I don't know the validation flow very well, so perhaps this is ok, but I've been told Core doesn't validate the genesis block.
< BlueMatt>
reminder: the first round of public 33c3 tickets go on sale in ~40 minutes
< BlueMatt>
wumpus: ^ :p
< instagibbs>
I now understand that this won't fork anyone off, but it might make sense to special case the genesis block again for this case.
< instagibbs>
(this meaning current behavior)
< sipa>
instagibbs: imho, we should not apply any tests to genssis
< instagibbs>
Ok, I can PR something
< instagibbs>
sipa, ConnectBlock calls CheckBlock on genesis right before it short-circuits the rest of the logic. Comment above said CheckBlock is "// Check it again in case a previous version let a bad block in"
< sipa>
instagibbs: that's a very old comment
< instagibbs>
Could you elaborate on what a newer comment explaining the line would entail :)
< instagibbs>
From a naive reviewer I'd say it's a bunch of cheap checks to run before loading coins, validating everything else.
< cfields>
sipa / wumpus: thanks for helping out on #8708. Just catching up after being out of town for the weekend. I was expecting to fix it up today, but finding it already merged is even better :)
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #9097: [qa] Rework sync_* and preciousblock.py (master...Mf1611-qaSyncAndPrecious) https://github.com/bitcoin/bitcoin/pull/9097
< jtimon>
how can the same test pass when make check but not with test_bitcoin --run_test=MyTest ?
< sipa>
it means you're leaving some state around and not cleaning it up
< jtimon>
within the test?
< MarcoFalke>
In a previous test. So MyTest depends on a previous test, which it should not.
< jtimon>
oh, I see
< jtimon>
may be, let me comment the previous test
< jtimon>
thanks
< sipa>
or _any_ previous test
< jtimon>
yes, it is my own previous test in the same file, but I still don't understand why, thanks again
< satosh-777-xl_>
I asked this on bitcoin-dev and was told to check the developer notes and then post on here if I couldn't find anything.. Anyways as core is moving away from boost and BOOST_FOREACH loops in favor of standard C++ (11), is the following for loop syntax acceptable?
< satosh-777-xl_>
for (Class c : vClass) { c.nonce++ }
< sipa>
that won't work. that syntax will make c into a copy of the item being iterator over
< satosh-777-xl_>
I'm sorry, that loop is broken. I was just wondering if that code style (if it didn't have the error of copying c) is okay or if for (size_t i = 0; i < 10; i++) should be the only style used
< sipa>
(Class c& : vClass) {
< sipa>
for (Class c& : vClass) {
< satosh-777-xl_>
Okay, so if used correctly that style of for loop is okay to use in a new pull request?
< sipa>
yes
< satosh-777-xl_>
Thanks you I was curious, thats all :)
< satosh-777-xl_>
*thank you
< cfields>
morcos: re yesterday's ping, it should be fixed in master now. The fix (8708) was PR'd a long time ago, it just took a while to get merged.
< morcos>
cfields: thanks! i did try scanning open PR's but not thoroughly enough I guess and missed it.
< btcdrak>
wumpus: looks like jgarzik merged that JSON whitespace issue upstream, what is the next step - once that is merged into core we'll need to remove the white space from a couple of fixtures in src/test/data/*.json files.
< bitcoin-git>
[bitcoin] MarcoFalke opened pull request #9098: [qa] Handle zombies and cluttered tmpdirs (master...Mf1611-qaZombies) https://github.com/bitcoin/bitcoin/pull/9098
< bitcoin-git>
bitcoin/master fe1dc62 Matt Corallo: Hash P2P messages as they are received instead of at process-time
< bitcoin-git>
bitcoin/master 9f554e0 Pieter Wuille: Merge #9045: Hash P2P messages as they are received instead of at process-time...
< bitcoin-git>
[bitcoin] sipa closed pull request #9045: Hash P2P messages as they are received instead of at process-time (master...2016-10-p2p-hash) https://github.com/bitcoin/bitcoin/pull/9045
< BlueMatt>
lol, sipa, did you go through all my pulls just to tell me that i need to rebase all of them? :p
< BlueMatt>
fucking cfields and his refactors....
< cfields>
BlueMatt: i could say the same about you :)
< BlueMatt>
heh, true
< sipa>
BlueMatt: yes
< sipa>
BlueMatt: i didn't expect they'd all need rebasing
< BlueMatt>
heh, all good
< cfields>
BlueMatt: I'm getting ready to PR the layer violation fix you requested (avoiding having CConnman do the serializing). It'll touch all PushMessage one last time. Would you like me to hold off until some of yours go in?
< cfields>
not sure if that disrupts you at all
< BlueMatt>
naa, all good, those are trivial rebaes
< BlueMatt>
rebases
< cfields>
ok
< gmaxwell>
sipa: "an unintentionally strong preference for witness peers" < well it was as strong as I intend it to be, after SW activates I was planning on merging the NODE_WITNESS preference into the node-network one and making it absolute and not a preference. In releases post sw I think we shouldn't connect out at all to non sw peers.
< cfields>
sipa: i think i've decided against the refcounted messages altogether. I just can't see a clean way to make it happen. And you pretty much convinced me it wouldn't be effective anyway.
< cfields>
gmaxwell: heh, sorry, apparently i type too slowly :)
< sipa>
gmaxwell: it seems arbitrary to base it on the number of connections, and not the number of witness connections
< sipa>
maybe it was what you intended, but then i misunderstood the intention
< gmaxwell>
sipa: the intention was "don't leave ourselves totally disconnected because we're insisting on witness peers"-- which is a reasonable thing to do before segwit is activated and when the number of witness peers is low.
< gmaxwell>
I would much rather prefer that all of a node's outbound were witness peers, even now. But that was unrealistic in 0.13.1 due to the chicken/egg problem. :)