< bitcoin-git>
[bitcoin] practicalswift opened pull request #10923: Use -Wthread-safety-analysis if available (+ -Werror=thread-safety-analysis if --enable-werror) (master...thread-safety-analysis) https://github.com/bitcoin/bitcoin/pull/10923
< jonasschnelli>
If you request blocks B, C, D while your tip is A, then all three blocks (B, C, D, regardless of the order) would pass and be trigger the BlockChecked() signal
< jonasschnelli>
sipa: regarding the CChain object for headers, how to efficiently replace the GetDepthInMainChain in SPV mode?
< jonasschnelli>
There is a chainActive->Contains(pindex) check in GetDepthInMainChain
< jonasschnelli>
(which basically is a vector::[] access)
< jonasschnelli>
without a CChain object this seems to be very inefficient
< sipa>
jonasschnelli: GetAncestor is log(n)
< jonasschnelli>
sipa: I see, GetAncestor also uses the pskip
< sipa>
yes
< jonasschnelli>
So replacing Contains() with GetAncestor() seems okayish?
< sipa>
i believe so
< jonasschnelli>
Okay. Let me benchmark then. Thanks!
< jonasschnelli>
sipa: is then the assumption 'chainActive.Tip().GetAncestor(otherBlockIndex.nHeight()) == chainActive.Contains(otherBlockIndex)' correct?
< promag>
could we have some convention regarding commit messages and PR titles? for instance, prefix with "foo: " or "[foo] " and so on
< promag>
I know it's not that important, but consistency is nice
< sipa>
it's a balance between consistency and having people be annoyed at having too many rules :)
< luke-jr>
promag: I always use "foo: "
< wumpus>
I also use foo:, but I don't care what you use
< wumpus>
(foo: is the same as kernel/mesa so I can't forget that)
< jonasschnelli>
sipa If use [foo]... but obviously foo: is more pure text conform.
< jonasschnelli>
(sorry for the wrong highlight)
< jonasschnelli>
* I use
< sipa>
we should encourage html3 tags in commit titles
< sipa>
improve readability on <blink>800x600</blink> resolution!
< wumpus>
commit stylesheet extensions
< instagibbs>
commit messages should always be rendered in comic sans
< wumpus>
color: #ff0000;
< wumpus>
instagibbs: at the least in large, friendly letters
< * sipa>
mentally sees a "DON'T PANIC" now
< wumpus>
sipa: seems your latest change to 10526 made things better, repeated the test 3 times, FWIW all three times it had done the cleanup
< wumpus>
will let it run a few more times just to be sure
< sipa>
wumpus: great... aybe we shouldn't bother running the intermediate compactions?
< sipa>
the hooe was that during the uograde process the disk usage wouldn't double
< sipa>
but it does not seem to have that effect
< wumpus>
I guess trying doesn't hurt either
< wumpus>
maybe it sometimes works, sometimes doesn't, depending on how the records happen to be distributed on disk
< promag>
lol ok
< wumpus>
though I don't know the effect on performance
< wumpus>
bleh, this time it was 4.5G
< wumpus>
so no, it's still not entirely reliable
< wumpus>
(and this is starting with exactly the same database every time!)
< sipa>
but it does shrink after restart?
< wumpus>
yes
< sipa>
that's something at least, i guess
< wumpus>
it eventually catches on and compacts, but it doesn't seem to be deterministic
< wumpus>
(well, it always has done so at the next run, up until now)
< wumpus>
monitoring disk usage *during* the process might be interesting too, to see when it does the compaction, but meh
< sipa>
yeah
< luke-jr>
[11:39:32] * sipa mentally sees a "DON'T PANIC" now <-- I noticed Android likes to capitalise "Don't Panic" when the words appear in that order.
< bitcoin-git>
[bitcoin] laanwj opened pull request #10927: test: Make sure wallet.backup is created in temp path (master...2017_07_wallet_backup_temp_path) https://github.com/bitcoin/bitcoin/pull/10927
< BlueMatt>
jonasschnelli: I believe thats still "correct", no?
< BlueMatt>
jonasschnelli: though the comment may only be correct after its moved to a background thread
< gmaxwell>
wumpus: because you commented on it on the original pr, and I hadn't seen the second.
< wumpus>
it introduces a "passed with warnings" result if a test succeeds but prints to stderr
< BlueMatt>
gmaxwell: it still prints "Passed with warnings"
< wumpus>
seems good enough to me
< wumpus>
no need to panic
< BlueMatt>
so you can see it
< gmaxwell>
that wasn't a panic.
< gmaxwell>
it was a question, seems you've answered it. though I'm concerned that passed with warnings will become the normal state since travis won't reject it. Can someone explain the need for this to me?
< BlueMatt>
no, what my gpu makes my kernel do is a panic :(
< BlueMatt>
gmaxwell: i have to assume cause the keypool topup thing prints warnings that you need to top up your keypool? dunno, havent had a chance to review that one yet
< gmaxwell>
litterally the PR says "not sure how much people want this" and the original PR has no description at all beyond the title.
< wumpus>
<wumpus> apparently #10882 (which has 0.15 milestone) was depending on it
< ryanofsky>
i'm actually not sure the reason for the dependency
< BlueMatt>
heh, really shit time for everyone to be in japan
< wumpus>
anyhow if tests don't succeed they shouldn't return success
< gmaxwell>
BlueMatt: if so, then does that mean it'll always print that.. in which case, the warnings notice is not useful.
< BlueMatt>
gmaxwell: ok, lets wait till jnewbery can come back and answer :)
< ryanofsky>
yeah, that seems reasonable. maybe just revert the change and we can ask john about it to see if there's a better solution
< wumpus>
revert it then?
< ryanofsky>
sound good to me
< BlueMatt>
or just wait till we get an answer
< BlueMatt>
or...whatever, i dont care
< wumpus>
I don't, either
< ryanofsky>
me either
< gmaxwell>
wumpus: in terms of the test passing, the tests still pass the the sanitizers print important notices that things have gone wrong... they've helped us find a lot of serious bugs, if there is a good reason for making changes that indirectly hobble them; okay. But I am completely clueless as to why this change is desirable, and the PRs did not justify it, AFAICT (though it's fully possibly I'm mis
< wumpus>
can we now go back to discussing things before they are merged, instead of after?
< gmaxwell>
sing something).
< wumpus>
gmaxwell: I don't feel like defending the change at all
< wumpus>
afaik I didn't even comment on it
< wumpus>
oh I did on the previous one, months ago
< gmaxwell>
wumpus: you commented on the earlier on, first comment, raising the sanitizer compatiblity concern.
< wumpus>
yep
< wumpus>
ok, reverting it then, seems clearly a misunderstanding
< bitcoin-git>
bitcoin/master 88af227 Wladimir J. van der Laan: test: Make sure wallet.backup is created in temp path...
< bitcoin-git>
bitcoin/master 9d5e8f9 Wladimir J. van der Laan: Merge #10927: test: Make sure wallet.backup is created in temp path...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10927: test: Make sure wallet.backup is created in temp path (master...2017_07_wallet_backup_temp_path) https://github.com/bitcoin/bitcoin/pull/10927
< bitcoin-git>
bitcoin/master 99c7fc3 Matt Corallo: Prevent user from specifying conflicting parameters to fundrawtx...
< bitcoin-git>
bitcoin/master 8537187 Wladimir J. van der Laan: Merge #10799: Prevent user from specifying conflicting parameters to fundrawtx...
< bitcoin-git>
[bitcoin] laanwj closed pull request #10799: Prevent user from specifying conflicting parameters to fundrawtx (master...2017-07-no-fundraw-conflicts) https://github.com/bitcoin/bitcoin/pull/10799
< MarcoFalke>
Sorry, I missed the discussion about the stderr thing. Agree that it should be reverted.
< MarcoFalke>
Best is to apply it specifically only where needed
< MarcoFalke>
Instead of per default for all tests
< MarcoFalke>
The exact error message ("Number of keys in keypool is below critical minimum") can even be checked when piped into a SpooledTemporaryFile
< promag>
should we disallow wallet files being symlinks?
< gmaxwell>
MarcoFalke: yea, I think we should have a specific test for that error, since its intentional functionality.
< MarcoFalke>
We already had those issues that the specific error was not tested against. And run into issues...
< MarcoFalke>
Not worth repeating
< MarcoFalke>
Thanks for raising the concern quickly, gmaxwell
< gmaxwell>
No problem, I think I didn't see the new PR because I confused it for the old one or something.
< MarcoFalke>
too much going. I only keep up with pulls after they are merged atm
< MarcoFalke>
jnewbery: Let me know if there are any test pulls I should look at with priority
< instagibbs>
assuming no priority, how does the miner select between two identical feerate transactions if it has to choose between the two?
< gmaxwell>
instagibbs: whatever comes up first out of the index.
< instagibbs>
well, yeah :) I guess the answer is "no particular ordering"