< luke-jr>
sdaftuar: yes, about the dbcrash test: I don't understand why after restarting the node that crashes on submitblock, it's okay to wait for the block to be the tip? What if it crashed BEFORE the block was processed? (as it seems to be when I run it)
< sdaftuar>
luke-jr: that seems strange to me -- did you perhaps modify the test to run on a pruning node somehow?
< sdaftuar>
other than that, i don't see how FlushStateToDisk could be called before the new block is connected (which is where the crash happens)
< luke-jr>
hmm
< sdaftuar>
oh, maybe there's a race condition somewhere else...
< sdaftuar>
i guess ATMP calls FlushStateToDisk as well, so if the test is generating transactions and then occasionally submitting a block, there could be a race where the FSTD call in transaction acceptance triggers a crash right as submitblock is firing.
< sdaftuar>
i haven't actually looked at the test in-depth enough to know if that might be a plausible explanation though
< sdaftuar>
luke-jr: do you have a commit that the test fails on?
< bitcoin-git>
[bitcoin] fanquake merged pull request #16188: net: Document what happens to getdata of unknown type (master...1906-netGetData) https://github.com/bitcoin/bitcoin/pull/16188
< kallewoof>
With bech32, is MIN_STANDARD_TX_NONWITNESS_SIZE still 82 bytes?
< kallewoof>
I am seeing failure in feature_bip68_sequence.py if I flip the default address type to bech32, cause it runs into a tx that is 62 bytes w/o witness stuff.
< jonasschnelli>
Hmm... compiling depends W64 (minGW64) on Ubuntu 18.04 fails... can't figure out why
< jonasschnelli>
error: ‘mutex’ is not a member of ‘std’ <---- looks like its not compiling with c++11, I tried even to force it with "CXXFLAGS="-std=gnu++11"" ... no success
< jonasschnelli>
Probably missing a package?
< jonasschnelli>
selecting the posix version through sudo update-alternatives --config x86_64-w64-mingw32-g++ fixed it
< gertjaap>
is there anything i should do if the DrahtBot reports a conflicting PR? Or just leave it for reviewers.
< fanquake>
jonasschnelli just need to read the docs :p
< jonasschnelli>
indeed... :)
< jonasschnelli>
gertjaap: just informal,... no need to fix
< fanquake>
gertjaap I wouldn't worry to much
< gertjaap>
ok, cool
< fanquake>
The conflicting PR could be quite possibly be 6 months old, need rebasing itself etc etc.
< meshcollider>
I feel like the conflicting info is more useful for maintainers to decide what order to merge stuff in than it is for anyone else
< bitcoin-git>
[bitcoin] fanquake closed pull request #15572: Add auto select custom fee when smart fee not initialized. (master...dev) https://github.com/bitcoin/bitcoin/pull/15572
< fanquake>
wumpus Would you like to take a look at #15894 ?
< fanquake>
wumpus I was going to ACK #16278 (no longer because it doesn't seem to compile..). Basically because it's tests only, Marco seems ok with it and it doesn't conflict with anything else that is merge ready. However, for the rest of the the 0.19.0 cycle, I feel like we're at the point where we can just start rejecting those kinds of PRs.
< gribble>
https://github.com/bitcoin/bitcoin/issues/16278 | tests: Reduce compilation time and unneccessary recompiles by removing unused includes in tests by practicalswift · Pull Request #16278 · bitcoin/bitcoin · GitHub
< fanquake>
I know you'd be happy to just close them all the time.
< fanquake>
hebasto (are you on IRC, different nick?) also asked how to test these kinds of PRs. I was just going to say using whichever methods wastes the least developer time. It's sort of annoying that a lot of these changes are seemingly hand-rolled, and the PR's don't actually contain any information about how they were done, or point to ways that this can just be automated away and forgotten about in future.
< fanquake>
It's also a bit crazy that the 0.19 branch off is only 3 months away. #15940.
< wumpus>
fanquake: whoa yes that branch-off is pretty soon, thanks for the reminder :)
< wumpus>
fanquake: I don't necessarily want to close them, but yes, messing around with includes just has the lowest priority for me
< fanquake>
yes
< wumpus>
although it's nice they benchmarked it and it increased the build speed, it's not really a metric we generally optimized for, but still
< fanquake>
Sure. Although a bit concerning that they might push a change like that and it not even compile.
< wumpus>
fanquake: silent merge conflict I guess?
< fanquake>
wumpus yea I'd assume so. Looks like it happened right after rebasing.
< wumpus>
(tbh I'm not sure 2% compile speed improvement warrants that much work; most complaints with regard to compile are about compile-time memory usage, not sure how that is affected)
< fanquake>
I think there is another PR open, or possibly got closed that reduced compile time memory by 1% or so by removing a bunch of includes
< fanquake>
Yea #16238. Which was a follow up to #16129 which supposedly reduced compile time memory use by 2%.
< jonasschnelli>
the byte shifts are BE/LE safe AFAIK
< jonasschnelli>
so that one *len24_out = le32toh(*len24_out); is not necessary?
< wumpus>
not as long as you're using the htole64
< wumpus>
so basically you never want to cast bytes to integers and vice versa, so you don't need Xtoh or htoX
< wumpus>
access bytes as bytes and integers as integers and use endian-explicit poke/peek functions where needed to convert between them
< jonasschnelli>
wumpus: and we are using htole64 since this is network protocol serialisation... so you agree it's safe to remove that le32toh?
< wumpus>
I'll have to review the code to be sure, but I think it's unnecessary, unless you're type-punning somethere
< wumpus>
in which case that's what you'd want to avoid instead
< jonasschnelli>
That part is not in the AEAD pull,... since nothing is really creating encrypted streams from internal structures (the testvectors are given)
< wumpus>
jonasschnelli:yes, the " *len24_out = le32toh(*len24_out);" should be unnecessary
< jonasschnelli>
So the AEAD PR tests against test vectors where the integer (there is only one for the AAD len) is encoded LE
< wumpus>
"// encode to host endianness 32bit integer", it's an integer: it's always host endianness in memory, but you don't need to care, treat it as an abstract integer
< wumpus>
only when you convert the integer from/to bytes, endian starts to be something you need to be concerned about
< jonasschnelli>
Yes. Thanks for clearing that up...
< jonasschnelli>
The current code that writes the 3 byte length field is...:
< wumpus>
right, would change that to ((int24>>16)&0xff), ((int24>>8)&0xff), (int24&0xff) (assuming you want it BE) instead of type-punning the int
< wumpus>
(other way around if you want LE)
< jonasschnelli>
if we would use a standard int32 for the size (instead of 3 bytes) we could just pass the int into CVectorWriter since it uses ser_writedata32 under the hood, right?
< wumpus>
yes
< jonasschnelli>
But since there is no ser_writedata24 (obviously), we need to use our own punning
< wumpus>
you have to handle your own serialization / deserialization for the byte stream from/to an integer, yes
< wumpus>
but not with type punning (which is explicitly defined as casting one type of pointer to another and suspicable to various kinds of UB and IBD), rather, use explicit shifts and logical operations to extract/concatentate the bytes
< bitcoin-git>
[bitcoin] promag opened pull request #16285: rpc: Improve scantxoutset response and help message (master...2019-06-scantxoutset-nits) https://github.com/bitcoin/bitcoin/pull/16285
< wumpus>
its funny that no one can answer my question in #16183
< wumpus>
gwillen dongcarl: okay, thanks, makes sense, the thing that made me confused is that if it's not even a library, how do we depend on it?
< wumpus>
I guess I'd still rather depend on some obscure X dep than... expat
< gwillen>
.... that's a good question
< bitcoin-git>
[bitcoin] rrybarczyk opened pull request #16287: Remove extra CBlockIndex declaration (master...remove-extra-cblockindex) https://github.com/bitcoin/bitcoin/pull/16287
< hebasto>
Hi! Could someone point me to the adopted practice or discussion about translation of the "Bitcoin Core" brand name to other languages?
< ossifrage>
Some how I lost 7342 blocks in an out of space shutdown
< ossifrage>
The node had been down since jan, it synced up to the tip and then many hours later it ran out of disk.
< ossifrage>
When I restarted the node the tip went from 582102 back to 574747
< ossifrage>
574747 was the first UpdateTip line from the previous run of bitcoin-qt, this seems like an excessive amount of state to loose
< achow101>
is it project policy that all commits must compile and pass all tests?
< gwillen>
achow101: I think it's like, polite at least?
< gwillen>
I've not heard of it being enforced, I do not believe Travis checks anything but the tip
< gwillen>
but I always try to preserve it, bisection is more annoying if you don't
< achow101>
gwillen: my situation is either make people review a commit that's something like +/- 5000 LOC but compiles and passes tests, or break that up but none of those commits will pass tests (they should compile though)
< achow101>
for my own sanity, I'm making the separate commits, but I can squash them all together if people want commits that pass tests
< gwillen>
you could just disable the broken tests in the intermediate commits
< gwillen>
I don't know if that's actually a good idea but it fixes the bisection issue
< achow101>
.. there's a lot of broken tests. basically anything that involves the wallet
< achow101>
which is almost all of them
< gwillen>
*nods*
< luke-jr>
bisect should only ever go inside a merge when it's related to the issue
< promag>
achow101: fwiw I prefer green commits
< meshcollider>
Squash them locally then ;)
< jb55>
in general I think you should structure commits so that each one passes tests, obviously that's not always possible, but it's good practice